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

AuthCode and Implicit grant don't return state parameter in error cases #597

Closed
pmlt opened this issue Jun 16, 2016 · 3 comments
Closed
Labels

Comments

@pmlt
Copy link

pmlt commented Jun 16, 2016

Hello,

I noticed a flaw in your implementation of AuthCode and Implicit grant. When you redirect back to the client URL with an error code and message, you do not pass back the "state" parameter that was passed to the authorization URL. RFC 6749 clearly states that you should do so in sections 4.1.2.1 and 4.2.2.1 (c.f. https://tools.ietf.org/html/rfc6749#section-4.1.2.1).

Not passing back the "state" parameter means that clients must eitheir implicitely trust your error messages (which can be dangerous) or flag your response as a possible CSRF attack (thus ignoring your error message).

Suggestion: Make the various constructors of OAuthServerException optionally accept a "state" parameter which is then appended to the redirect URI, and pass the appropriate value whenever the AuthCodeGrant and ImplicitGrant classes throw this exception.

@alexbilbie
Copy link
Contributor

Hi @pmlt,

The state parameter is set in the RedirectResponse here https://github.com/thephpleague/oauth2-server/blob/master/src/Grant/AuthCodeGrant.php#L308-L330

I don't suppose you could create a test case for me please?

@pmlt
Copy link
Author

pmlt commented Jun 16, 2016

Well it's very easy, just deny the permissions. Your highlight only runs if the authorization request is approved. If it is not approved, line 336 of the very same file creates an error response that doesn't include the state.

If you want, I can submit a PR to fix the issue, I think I have a pretty good idea of how to fix it.

@alexbilbie alexbilbie added the Bug label Jun 17, 2016
@alexbilbie
Copy link
Contributor

Okay sorry I understand the specific issue now. I will address this in the next patch update.

Thank you for spotting this!

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

No branches or pull requests

2 participants