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.

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

Add redirectUri to url regex for OAuth2 #396

Merged

Conversation

@starJammer
Copy link
Contributor

starJammer commented Aug 3, 2017

What

Made a change to how the regex used to detect the finalUrl during the OAuth2 flow.

Why

Keycloak is an an OAuth2/OpenId Connect authentication provider. As part of Keycloak's authentication flow it loads several pages in order to prompt the user for different pieces of information. One of these pages is an internal keycloak page with a code=12341234 at the end of the url.

This triggers the regex and Insomnia will extract this invalid code and attempt to get a token. The access token request fails because the code is internal to keycloak and isn't actually an authorization code.

This PR will add the redirect uri, if present, to the regex so that it will only trigger if the url loaded in the window begins with the redirect uri.

Details

Not sure if RegEx is commonly available. Don't normally contribute to node/js based projects.

Notes

Not sure how to add tests regarding this.

Related Issue: (Issue number this PR references, for example #4)

Keycloak, an OAuth2/OpenId Connect authentication server,
has multiple pages it redirects to during the login process
for the user. One such internal url it uses has `code=` as part
of the url. This causes the window to close prematurely and attempt
to get an access token. However, the code is wrong because it is
only an intermediaery step for Keycloak.

This fix will add the redirectUri to the regex. Insomnia will only attempt
to fetch the token AFTER keycloak has finished it's full login flow
and has redirected back to the redirectUri.
@starJammer

This comment has been minimized.

Copy link
Contributor Author

starJammer commented Aug 3, 2017

Why is appveyor failing? :-(

@gschier

This comment has been minimized.

Copy link
Contributor

gschier commented Aug 3, 2017

Hmm, "AppVeyor was unable to build non-mergeable pull request"

Seems like a one-off error.

Copy link
Contributor

gschier left a comment

Great job on finding that bug @starJammer. Just one comment for ya 👍

Also, don't worry about the CI failing. Seems like a one-off problem.

@@ -57,8 +57,9 @@ async function _authorize (url, clientId, redirectUri = '', scope = '', state =
// Add query params to URL
const qs = querystring.buildFromParams(params);
const finalUrl = querystring.joinUrl(url, qs);
const regex = redirectUri ? new RegExp(redirectUri + '.*(code=|error=).*') : /(code=|error=)/;

This comment has been minimized.

Copy link
@gschier

gschier Aug 3, 2017

Contributor

Two things here. redirectUri must first be escaped before being included in the regex constructor. Also, no need to check for redirectUri because if it's an empty string, you still get the correct regex.

Something like this should do the trick.

const escapedUri = redirectUri.replace(/[-[\]/{}()*+?.\\^$|]/g, '\\$&');
const regex = new RegExp(`${escapedUri}.*(code=|error=)`)

This comment has been minimized.

Copy link
@starJammer

starJammer Aug 3, 2017

Author Contributor

Thanks for the improvement @gschier !!

@gschier
gschier approved these changes Aug 3, 2017
Copy link
Contributor

gschier left a comment

Awesome, thanks @starJammer!

@gschier gschier merged commit 43bc074 into Kong:develop Aug 4, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@starJammer starJammer deleted the starJammer:improvement/partition-oauth2-sessions-more branch Aug 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.