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

Consider dropping me from the GET request that contains the code #167

Closed
aaronpk opened this issue Aug 16, 2017 · 7 comments
Closed

Consider dropping me from the GET request that contains the code #167

aaronpk opened this issue Aug 16, 2017 · 7 comments

Comments

@aaronpk
Copy link
Owner

aaronpk commented Aug 16, 2017

The client should have already established a session or used the state to store things. Removing it from the request will make sure people don't accidentally think this is trusted information at this stage.

@aaronpk
Copy link
Owner Author

aaronpk commented Aug 16, 2017

This would also apply to the IndieAuth spec redirect described here https://indieweb.org/authorization-endpoint#Redirect_to_web_application

@snarfed
Copy link

snarfed commented Aug 16, 2017

hmm. oauth-dropins is stateless and re-fetches me in this GET to rediscover the auth endpoint to use to validate the code and get a token. what should it do instead? store and reuse the site's auth endpoint in the session or state?

@Zegnat
Copy link
Contributor

Zegnat commented Aug 16, 2017

Duplicate of #85? Also see Inklings-io/selfauth#10.

Removing it from the request will make sure people don’t accidentally think this is trusted information at this stage.

This is a good reason. For a login (identification, response_type=id) request you really should be making sure code verification happens against the same endpoint, I feel. If the code request is allowed to overwrite where you go looking for the endpoint for verification that might get you into trouble.

Also note that the returned me value is invalid if it isn’t on the same domain as the previously queried URL. So any implementation needs to store the domain already. @snarfed how did you solve this in oauth-dropins?

@sknebel
Copy link

sknebel commented Aug 16, 2017

It removes the ability of the endpoint to normalize the URL (e.g. http->https, or other redirects), but I'm not sure if that's actually a thing in current implementations. Other than that no reason to keep it.

@aaronpk
Copy link
Owner Author

aaronpk commented Aug 16, 2017

oauth-dropins is stateless and re-fetches me in this GET to rediscover the auth endpoint to use to validate the code and get a token. what should it do instead? store and reuse the site's auth endpoint in the session or state?

@snarfed clients should be using the state parameter to avoid CSRF attacks anyway, and this is a perfect place to store session data if you want to avoid server-side session storage. Relying on the me parameter to re-discover the endpoint actually opens up an attack where someone can swap the me parameter and get the client to send the code to the attacker's server.

It removes the ability of the endpoint to normalize the URL (e.g. http->https, or other redirects), but I'm not sure if that's actually a thing in current implementations. Other than that no reason to keep it.

The response from either a code verification or token exchange includes the final me value, so there is still a chance to normalize the value. (Of course the client should not allow a domain to claim a me value on a different domain, but that's a different issue)

@sknebel
Copy link

sknebel commented Aug 16, 2017

The response from either a code verification or token exchange includes the final me value, so there is still a chance to normalize the value. (Of course the client should not allow a domain to claim a me value on a different domain, but that's a different issue)

Right. Then I agree, no reason to have the parameter in the callback

snarfed added a commit to snarfed/oauth-dropins that referenced this issue Aug 18, 2017
snarfed added a commit to snarfed/oauth-dropins that referenced this issue Aug 18, 2017
@aaronpk
Copy link
Owner Author

aaronpk commented Dec 5, 2017

Duplicate of #85

@aaronpk aaronpk marked this as a duplicate of #85 Dec 5, 2017
@aaronpk aaronpk closed this as completed Dec 5, 2017
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

4 participants