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

Stripe do not follow RFC6749 OAuth 2 specification in case of rejection by user #948

Open
marco-sacchi opened this issue Feb 21, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@marco-sacchi
Copy link

marco-sacchi commented Feb 21, 2024

As requested in stripe-php#1653 I report the issue here.

Describe the bug

I don't know if this is the right place to open this issue, but since I use the PHP SDK I thought it appropriate to do it here.

Stripe do not follow RFC 6749 section 4.1.2 in the case of authorization rejected by the user. The state parameter is required in the response if the client_id and redirect_uri in the request are both valid (see also section 4.1.2.1), I quote:

If the resource owner denies the access request or if the request
fails for reasons other than a missing or invalid redirection URI,
the authorization server informs the client by adding the following
parameters to the query component of the redirection URI using the
"application/x-www-form-urlencoded" format, per Appendix B:

....

state
REQUIRED if a "state" parameter was present in the client
authorization request. The exact value received from the
client.

....

Consider implementing the specification according the RFC6749, otherwise cross-site request forgery cannot be verified.

To Reproduce
Steps to reproduce the behavior:

  1. paste an authorization url into your browser like: https://marketplace.stripe.com/oauth/v2/authorize?client_id=${clientId}&redirect_uri=${redirectUrl}&state=${state}
  2. click "Cancel" on the Stripe authorization page
  3. the URL to which the browser is redirected does not have the state parameter: https://*********/oauth2?error=access_denied&error_description=The+user+denied+your+request

Expected behavior
state parameter must be returned unchanged.

Screenshots

  • not applicable -

Desktop (please complete the following information):

  • OS: Ubuntu 22.04.4 LTS
  • Browser Google Chrome
  • Version Version 121.0.6167.184 (Official Build) (64-bit)
@marco-sacchi marco-sacchi added bug Something isn't working needs-triage labels Feb 21, 2024
@gabrielhurley-stripe
Copy link
Contributor

Thanks for bringing this to our attention. The eng team is taking a look to ensure we handle this case correctly!

@marco-sacchi
Copy link
Author

marco-sacchi commented Feb 22, 2024

Thank you for your quick answer.

Since it is part of the same area, and I am preparing the data for the review, I saw that your installation links for the app also obviously lack the state parameter since the flow starts from you instead of from a client platform:

https://marketplace.stripe.com/oauth/v2/authorize?client_id=*******&redirect_uri=********

The spec does not mention this type of procedure for the same CSRF security issue, and currently our platform rejects requests without the state parameter.

There is the possibility of chatting with the review team to draw attention to this aspect? Otherwise we cannot send the app for review because I think it will be rejected even if it is not up to us.

@gabrielhurley-stripe
Copy link
Contributor

We do document that the OAuth install link is recommended to include the state parameter in both the example and the callout here: https://docs.stripe.com/stripe-apps/api-authentication/oauth#install-app

Perhaps I'm misunderstanding where you are getting the link and it's lacking that param?

@marco-sacchi
Copy link
Author

@gabrielhurley-stripe Sorry, I forgot to include the link to the documentation I was referring to: https://docs.stripe.com/stripe-apps/publish-app#submit-app-for-review

Point 5 of the "Submit app for review" section indicates that it is mandatory to specify the Marketplace install URL for OAuth apps.

In the URL that I must specify in the form, should I include a value (in this case hardcoded) of the state parameter for the authorization flows triggered by installing the app from your Marketplace?

There is news about:

Thanks for bringing this to our attention. The eng team is taking a look to ensure we handle this case correctly!

@marco-sacchi
Copy link
Author

Is there any news about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants
@marco-sacchi @gabrielhurley-stripe and others