fix(oauth2): compute expires_in from the server clock - #66
Merged
Conversation
ServerConfig.Now is documented as the clock the server stamps issuance and expiry with, and every other site honoured it. writeTokenResponse did not: it measured the remaining lifetime with time.Until, i.e. against the wall clock, while ExpiresAt came from the injected clock. With a pinned clock the advertised expires_in stopped describing the token it shipped with, and a clock pinned far enough in the past put a negative lifetime on the wire — which RFC 6749 §5.1 does not allow. The stored expiry was right, so a client trusting expires_in refreshed at the wrong moment while introspection still reported the token as valid. serveToken now reads the clock once and passes that instant to both the grant and the response, so expires_in is exactly the configured TTL. The value is rounded to the nearest second — truncation advertised a 1h TTL as 3599 as soon as a millisecond elapsed between two clock reads — and an already-expired token drops the field instead of carrying a negative lifetime. Refs #64.
Coverage Report for CI Build 32661849110Coverage increased (+0.02%) to 91.77%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.
Closes #64.
Problem
ServerConfig.Nowis documented as the clock the server stamps issuance andexpiry with, and every other site honours it.
writeTokenResponsedid not:ExpiresAtcomes from the injected clock (req.Now.Add(cfg.AccessTTL),with
req.Now = s.cfg.Now()), whiletime.Untilmeasures againsttime.Now(). Mixing the two means the advertisedexpires_instopsdescribing the token it ships with: a clock pinned in the past yields a
value smaller than the real lifetime, and one pinned far enough in the past
yields a negative
expires_in, which RFC 6749 §5.1 does not allow. Thestored expiry stays correct, so a client trusting
expires_inrefreshes atthe wrong moment while introspection still reports the token as valid.
The truncation the issue notes as a side effect is real too:
int(d.Seconds())truncates toward zero, so a 1h TTL was advertised as3599as soon as a millisecond elapsed between the two clock reads — onthe wall clock, i.e. in production, not only under a test clock.
Fix
serveTokenreads the clock once and hands that instant to both thegrant and the response:
expires_inis then derived from the same instant the expiry was stampedwith, so it equals the configured TTL exactly. A small helper covers the
two remaining edges:
advertised as itself. This also keeps sub-second TTLs honest for a
custom grant that stamps its own expiry.
0, and the field isomitempty, so it drops off the wire rather than carrying a lifetime theRFC forbids.
expires_inis RECOMMENDED, not REQUIRED, so omitting itstays compliant — sending
-3600does not.The implicit flow already built
expires_infrom the TTL directly(
authorize_endpoint.go:439) and was never affected.Tests
New
oauth2/token_endpoint_test.go:TestTokenExpiresInHonorsServerClock— clock pinned to 2020, TTL 1h:advertises
3600. Against the wall clock this would be a large negativenumber.
TestTokenExpiresInMatchesTTLOnWallClock— the default clock advertisesthe configured TTL exactly, not one second short.
TestTokenExpiresInRoundsToNearestSecond— whole seconds, rounds up,rounds down.
TestTokenExpiresInNeverNegative— already expired and expiring now: thefield is absent, the rest of the body intact.
All 8 subtests fail against the previous implementation and pass with this
one (verified by reverting the helper locally).
Checks
make test— 26 packages pass with-race;oauth2coverage90.9% -> 91.1%.
make lint— 0 issues.make build— OK.Also updated: the
ServerConfig.Nowgodoc (it now says it governsexpires_intoo) and theCHANGELOG.mdFixed section.