-
Notifications
You must be signed in to change notification settings - Fork 1.7k
OAuth: Allow the authorizeUrl to be a subpath of the callbackUrl #4889
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.
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
base: main
Are you sure you want to change the base?
OAuth: Allow the authorizeUrl to be a subpath of the callbackUrl #4889
Conversation
const matchesCallbackUrl = (url, authorizeUrl, callbackUrl) => { | ||
if (!url) { | ||
return false; | ||
} | ||
|
||
if (url.href.startsWith(authorizeUrl.href)) { | ||
return false; | ||
} | ||
|
||
return url.href.startsWith(callbackUrl.href); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are getting into specific scenarios, this breaks for:
Auth URL: https://apigateway.mycompany.dev/auth/realms/my-realm/protocol/openid-connect/auth
Redirect URL: https://apigateway.mycompany.dev/auth/realms/my-realm/protocol/openid-connect/auth-redirect
It does not seem necessary to include a third parameter in the method. How about just making the test a bit tighter - the normalized URL.origin
and URL.pathName
are expected to match exactly, only the URL.search
part is allowed to match partially (OAuth params must be appended, so startsWith
is ok):
const matchesCallbackUrl = (url, callbackUrl) => {
return url
&& url.origin === callbackUrl.origin
&& url.pathName === callbackUrl.pathName
&& url.search.startsWith(callbackUrl.search);
}
To be extra pedantic, url.username
and url.password
might be checked as well, as they are not part of .origin
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback, I updated the code.
I also added a "remove trailing slash from url.pathname", so that these already given test case still works:
{ url: 'https://callback.url/endpoint/?code=abcd', expected: true },
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
Allow the authorizeUrl to be a subpath of the callbackUrl and callbackUrl to be subpath of the autorizeUrl It now checks for exact matching. Only a trailing slash and additional query params are allowed to be matched as the callbackUrl.
Description
First of all: Thanks for this nice tool :)
This is a little bugfix, to allow an OAuth authorizeUrl to be a subpath of the callbackUrl.
We have the following scenario:
https://apigateway-something.mycompany.dev
https://apigateway-something.mycompany.dev/auth/realms/my-realm/protocol/openid-connect/auth
So the special case is here, that the authorizeUrl is a subpath of the callbackUrl.
In
packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
->window.webContents.on('did-navigate', ...)
this causes the OAuth process to runwindow.close()
to early. It directly closes, when opening authorizeUrl, because it thinks, authorizeUrl is already the callbackUrl.In fact, the user does not even see the window, because it directly closes.
As we see here, authorizeUrl is called, then directly the tokenUrl. Normally there should be another call in between, which is the 302 redirect.
This PR fixes this, by adding another check, that ensures, that the window does not directly close, in that situation.
Contribution Checklist: