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

Use encodeURIComponent for state in getAuthorizationUri #289

Open
julesair opened this issue Apr 4, 2022 · 3 comments
Open

Use encodeURIComponent for state in getAuthorizationUri #289

julesair opened this issue Apr 4, 2022 · 3 comments

Comments

@julesair
Copy link

julesair commented Apr 4, 2022

Link to PR: #288

Use case: when using a base64 encoded string, the value read from the location path when the user is redirected is not always the same as the one given to getAuthorizationUri.

Like the redirectUri, the state is an user input and as such not safe to be used in an url without escaping certain characters

Can we use encodeURIComponent on the state parameter?

@AbdulrahmanGameel
Copy link

The state should not be user input, it should be created and used by the app only

@julesair
Copy link
Author

julesair commented Apr 8, 2022

The state should not be user input, it should be created and used by the app only

Yes, agree. I did not express myself correctly in the previous message. With user I meant myself, a developer using this library and defining how the state is computed. Not the end user.

There is no protection against a developer using characters in the state string (e.g. a base64 string), that need to be encoded when used as query parameter in the url. Also it is not documented, that the state needs to be encoded before handing it over to this function.
In my opinion it would be a small change, that should not break existing usages of the function, but reduces the probability of unexpected behavior. I patched the library for my use case, but would like to not have to patch it

@acooper4960
Copy link
Contributor

hello @julesair thank you for presenting this to us. I have filed a ticket for our team to look into making this change.

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

No branches or pull requests

3 participants