Repository navigation
Read the oauth handshake token from the cookie only - #329
Merged
Merged
Conversation
The oauth2, oauth1 and Apple callbacks read the handshake token with TokenService.Get, which takes the JWT query parameter and header before the cookie. A callback request could therefore carry a handshake that the browser's own login step never set, and complete a login started elsewhere.
The oauth1 callback read Handshake.State without checking that the claims carry a handshake. A session cookie on the callback, with GET in XSRFIgnoreMethods, made the request panic. It now answers 403 like the oauth2 and Apple callbacks.
The login step gives the handshake a 30 minute expiry, but Get skips the expiry check for cookies, so the callbacks accepted a handshake of any age. The cookie-only lookup now rejects a token whose exp has passed. A token without exp is still accepted, the same in both modules.
Coverage Report for CI Build 42677216081Coverage increased (+0.1%) to 87.062%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The oauth2, oauth1 and Apple callbacks read the handshake token with
TokenService.Get, which takes the JWT query parameter and theX-JWTheader before the cookie. A callback request could carry a handshake that the browser's own login step never set and complete a login started elsewhere, so the browser ended up signed in to an account it did not choose.The callbacks now read the handshake from the JWT cookie only, through
handshakeFromCookieinprovider/service.go. It works on a copy of the request with the query and every header exceptCookieremoved, so theTokenServiceinterface stays the same. Session tokens in the query or header work as before everywhere else.Two related callback fixes in the same PR:
XSRFIgnoreMethods, and the request panicked. It now answers 403 like the oauth2 and Apple callbacks.Getskips the expiry check for cookies, so the 30-minute expiry the login handlers put on the handshake was never enforced. The callback now rejects a handshake whose expiry has passed. A handshake without an expiry is still accepted, the same in both modules.Behavior change: a callback that sends the handshake in
?token=orX-JWTwithout the cookie now fails. No known integration does this: remark42 and_examplefinish OAuth through a browser popup with the cookie, and withSendJWTHeaderthe callback needed the cookie added in #262. A user who leaves the provider's consent page open for more than 30 minutes now has to start the login again.Same changes in the root and v2 modules, with regression tests for each callback. The full request URL in callback error logs is a separate issue, recorded in
docs/backlog/oauth-callback-error-log-keeps-query.md.