fix: Check OAuth2 redirect URL for matching callback URL and authorization code in query parameters #2148
Conversation
… code in query parameters In an Authorization code flow, there may be multiple intermediate redirects before reaching the final one which matches the callback URL and has a code in the query params. We should wait until we see a redirect URI that matches both the conditions. This fixes the issue where, when a redirect contains `code` as a query param but is not the final one (i.e., is not to the callback URL) an error is thrown saying the callback URL is invalid. Fixes usebruno#2147
dakshin-k
force-pushed
the
fix-callback-url-check
branch
from
6cfcfb3 to
24753e8
Compare
dakshin-k
changed the title
|
@lohit-1 Can you review this? |
| 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)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
Hey @dakshin-k, there aren't any unit tests in place for this behaviour currently.
Please feel free to add them. Thanks
There was a problem hiding this comment.
@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.
| const matchesCallbackUrl = (url, callbackUrl) => { | ||
| return url ? new URL(url).host == new URL(callbackUrl).host : false; | ||
| }; |
There was a problem hiding this comment.
Nope: my callback url is pointing to the same server that is auth url. It will pass this check and fail on missing 'code' even before the first redirect.
There was a problem hiding this comment.
Oh man, there are lots of edge cases here 😅
Comparing the href as you suggested earlier will not work as the href includes any query params in the URL, i.e. while it solves the trailing / problem it breaks for the below case:
callbackUrl: https://callback.url/endpoint
testUrl : https://callback.url/endpoint?code=abcd
// testUrl.href = https://callback.url/endpoint?code=abcd
This is the reason I compared against hostname instead of href. I currently don't have a better idea to implement this, will think about it and come back later.
Ideas are welcome if I'm missing something 😇
There was a problem hiding this comment.
The function could be something similar to this @dakshin-k
const matchesCallbackUrl = (url, callbackUrl) => {
let { origin: urlOrigin, pathname: urlPathname } = new URL(url);
let { origin: callbackUrlOrigin, pathname: callbackUrlPathname } = new URL(callbackUrl);
urlPathname = urlPathname?.at(-1) == '/' ? urlPathname?.slice(0, -1) : urlPathname;
callbackUrlPathname = callbackUrlPathname?.at(-1) == '/' ? callbackUrlPathname?.slice(0, -1) : callbackUrlPathname;
const urlWithoutQueryParams = `${urlOrigin}${urlPathname}`;
const callbackUrlWithoutQueryParams = `${callbackUrlOrigin}${callbackUrlPathname}`;
return url ? urlWithoutQueryParams == callbackUrlWithoutQueryParams : false;
};
Edit:
Not recommended, seems like too much of an overkill
The below solution should be enough
There was a problem hiding this comment.
Wouldn't this suffice?
testUrl.href.startswith(callbackUrl.href)
The spec mandates the code param is appended the the query component, so it should be at the end. Also luckily the fragment component (the url part after #) is not allowed, so it does not have to be handled.
There was a problem hiding this comment.
Thanks, that worked! I have pushed the change, would someone mind testing it out once?
I faced a weird problem testing locally, not sure if I introduced a bug or found an existing one 😅
I'm able to generate auth tokens successfully now, but I can't make any requests - When I click the send request button, it just shows me the response from my OAuth server with the access token, it doesn't actually make a request using that token or display that response:
There was a problem hiding this comment.
I wouldn't mind that. That's actually #1999
There was a problem hiding this comment.
Ah okay, thanks!
So I guess that's it on this PR? Let me know if there's anything further to do!
|
I have verified that the changes in this PR allow for using Auth0 with Bruno. Would appreciate for this to get merged and released so I can roll out Bruno in our company :)
|
|
fix: #1003 |
|
Merged! Thanks @dakshin-k for working on this. This will be released in the build scheduled to go out on Tuesday, June 4 2024. |
…ation code in query parameters (usebruno#2148) * Check OAuth2 redirect URL for matching callback URL and authorization code in query parameters In an Authorization code flow, there may be multiple intermediate redirects before reaching the final one which matches the callback URL and has a code in the query params. We should wait until we see a redirect URI that matches both the conditions. This fixes the issue where, when a redirect contains `code` as a query param but is not the final one (i.e., is not to the callback URL) an error is thrown saying the callback URL is invalid. Fixes usebruno#2147 * Add test cases for callback URL check * Update check to cover URLs with same host but different endpoints
Fixes #2147
Description
In an Authorization code flow, there may be multiple intermediate redirects before reaching the final one which matches the callback URL and has a code in the query params.
We should wait until we see a redirect URI that matches both the conditions. This fixes the issue where, when a redirect contains
codeas a query param but is not the final one (i.e., is not to the callback URL) an error is thrown saying the callback URL is invalid.Contribution Checklist: