Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Check OAuth2 redirect URL for matching callback URL and authorization code in query parameters #2148

Merged
merged 3 commits into from
May 31, 2024
Merged

fix: Check OAuth2 redirect URL for matching callback URL and authorization code in query parameters #2148

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24753e8
Check OAuth2 redirect URL for matching callback URL and authorization…
dakshin-k Apr 21, 2024
906e4e4
Add test cases for callback URL check
dakshin-k Apr 22, 2024
d01d312
Update check to cover URLs with same host but different endpoints
dakshin-k Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ const authorizeUserInWindow = ({ authorizeUrl, callbackUrl, session }) => {
});

function onWindowRedirect(url) {
// check if the url contains an authorization code
if (new URL(url).searchParams.has('code')) {
finalUrl = url;
if (!url || !finalUrl.includes(callbackUrl)) {
reject(new Error('Invalid Callback Url'));
// check if the redirect is to the callback URL and if it contains an authorization code
if (url && url.includes(callbackUrl)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed a case where this check may not be sufficient (which is the problem in this version as it was in the original):
My keycloak server has a redirect uri set to exactly: https://example.com?param=value. As keycloak expects me to use callback url exactly as configured, I do so in bruno, but after I provide correct credentials, I get redirected to https://example.com/?param=value&code=..., whis does not pass the check url.includes(callbackUrl) (extra / breaks this).
Perhaps the string comparison using includes should be done on URL.href's rather than plain string values, to have a level of normalization?

let strurl1 = 'https://example.com?par=val'
let strurl2 = 'https://example.com/?par=val'
strurl1 == strurl2 // ==>false
new URL(strurl1) == new URL(strurl2); // ==> false
new URL(strurl1).href == new URL(strurl2).href; // ==> true
new URL(strurl1).href // => https://example.com/?par=val 
new URL(strurl2).href // => https://example.com/?par=val

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@helloanoop @lohit-1 Do we have any unit tests on this behaviour? I was not able to find any when creating the PR but it seems like there are several edge cases here so I would like to add some if they are not there

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dakshin-k, there aren't any unit tests in place for this behaviour currently.
Please feel free to add them. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lohit-1 Have added some test cases, please review.

I couldn't write a test for the entire component, including mocking all the window events, so I just extracted that check into a separate function and wrote tests for it.

if (!new URL(url).searchParams.has('code')) {
reject(new Error('Invalid Callback URL: Does not contain an authorization code'));
}
finalUrl = url;
window.close();
}
if (url.match(/(error=).*/) || url.match(/(error_description=).*/) || url.match(/(error_uri=).*/)) {
Expand Down