Skip to content

Conversation

bhavikkumar
Copy link
Contributor

Validates access tokens passed through query parameters in the same manner as if they were passed in Authorization HTTP header as a bearer token.

This resolves #7011

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 17, 2019
@bhavikkumar
Copy link
Contributor Author

bhavikkumar commented Jun 17, 2019

I've made the assumption that the query parameters here have already been decoded and therefore can be validated against a pattern.

If this is not true then just checking for empty query parameter will also resolve the issue.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @bhavikkumar, for the PR! I've left some feedback inline.

@jzheaux
Copy link
Contributor

jzheaux commented Jun 18, 2019

Also, @bhavikkumar, would you please format your commit message like this, including at least the phrase "Fixes: gh-7011"?

@bhavikkumar bhavikkumar force-pushed the gh-7011 branch 2 times, most recently from 2713651 to a7eb0d2 Compare June 19, 2019 19:32
@bhavikkumar
Copy link
Contributor Author

@rwinch Fixed the reactive flow in #7020

@jzheaux
Copy link
Contributor

jzheaux commented Jun 24, 2019

@bhavikkumar, in taking another look at this PR and matching it against the RFC, I think there's a mismatch since the RFC doesn't specify a format for bearer token query parameters. I think it would be better to stick to the RFC, and, in fact, #7020 aligns with this on the reactive side.

So, to better align with the RFC and also the reactive solution of #7011, would you please update your PR to complain when the query parameter is empty (not if it doesn't follow the same pattern as a bearer token header value)?

@bhavikkumar
Copy link
Contributor Author

I just wrote a quick test around this for the non reactive flow, it seems to work as expected. Closing this PR.

@bhavikkumar bhavikkumar deleted the gh-7011 branch June 26, 2019 08:37
@jzheaux
Copy link
Contributor

jzheaux commented Jun 26, 2019

@bhavikkumar, sorry, I'm confused. #7020 only addresses the reactive stack. I think your PR is still necessary for the servlet stack?

@bhavikkumar
Copy link
Contributor Author

I had not verified the servlet stack, but based on the code it looked like it needed the same fix. However, when I implemented it to check, it worked fine without the fix. I received the expected 401 response.

I didn't follow it through to where it was being handled. I can push up the example if you want to verify.

@rwinch rwinch added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 27, 2019
@rwinch rwinch self-assigned this Jun 27, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Jun 28, 2019

I see, @bhavikkumar, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reactive OAuth2 using query parameters for access_token can cause HTTP 500s
4 participants