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

Handle Authorization URL With Already Existing Query Parameter #193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Handle Authorization URL With Already Existing Query Parameter #193

wants to merge 1 commit into from

Conversation

DavidTPate
Copy link

Hi there! I'm using this library for one of my projects and ran into an instance where I had an authorization end-point similar to the following: http://www.example.com/oauth2/authorize?parameter1=value1 where I was hitting an issue when attempting to build the Authorization URL since it was creating the following: http://www.example.com/oauth2/authorize?parameter1=value1?response_type=code&redirect_uri=REDIRECT_URI&client_id=CLIENT_ID which is an invalid URL (specifically the ?parameter1=value1?response_type=code part.

To address this I've added a simple check for the Authorization URL already having the ? in it which fixed this issue for me. I'm not aware of anywhere else in the OAuth2 spec (specifically the client side) where query parameters already existing on a URL would cause an issue so I've only update the single line.

@DavidTPate
Copy link
Author

@ciaranj Any chance of getting this and some of the other PRs pulled in? If you don't have time for the project I'd love to get involved in working on all of this as this module is widely used and looking through some of these PRs has the need to support more things.

@mirkods
Copy link

mirkods commented Jun 13, 2016

👍 I still needed of this behaviour

@mirkods
Copy link

mirkods commented Jun 13, 2016

@ciaranj can you merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants