feat: apply access token validity jitter at session creation and refresh - #1307
Conversation
Wire access_token_validity_jitter into the access token mint: on session creation and refresh, exp = now + validity * (1 - U[0, jitter]). The jitter is subtract-only, so a token is never valid longer than the configured validity; 0 disables it. Token regeneration is exempt (preserves the original expiry) and verify does not re-roll it. Part of PLAN-002. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the full diff against the surrounding mint/refresh/regenerate/verify paths in Session.java and AccessToken.java, plus the parent's config validation. The change is correct and well-tested; no blocking defects.
What I verified:
- Formula is safe at the config bounds.
jitteredValidityreturnsvalidity * (1 - Math.random()*jitter); the parent (#1300) validatesaccess_token_validity_jitter ∈ [0, 0.25](and that bound is not gated behind!Main.isTesting, so it holds in tests too).Math.random() ∈ [0,1)⇒ factor ∈(0.75, 1], so the token is always strictly shortened and never zero/negative — the subtract-only contract holds and there's no risk of a past/zero expiry. - Single
now. expiry and timeCreated derive from oneSystem.currentTimeMillis(), so lifetime == the (jittered) validity with no sub-second skew; thejitterZeroKeepsFullValidityexact-equality assertion is sound. - Regenerate is exempt (lines 263/312 pass non-null
expiryTime→ non-jitter branch) and the@TestOnlymints route through the no-jitter overload. Confirmed against RegenerateTokenTest / SessionRegenerateAPITest2_7 equality assertions. - Blast radius of the new 0.05 default is contained: the only exact minted-lifetime assertions in the tree are in the session / session.api packages (all adjusted or run per the PR body). No exact access-token-lifetime assertion exists outside those packages.
Math.random()(global PRNG) is fine here — jitter isn't security-sensitive and the tests assert statistically, not on a fixed seed.
Two points below are decisions worth a reviewer's explicit sign-off, not defects.
| throws StorageQueryException, StorageTransactionLogicException, InvalidKeyException, | ||
| NoSuchAlgorithmException, TenantOrAppNotFoundException, InvalidKeySpecException, SignatureException, | ||
| AccessTokenPayloadError, UnsupportedJWTSigningAlgorithmException { | ||
| // Default entry point: no validity jitter. Callers that mint fresh tokens on session creation |
There was a problem hiding this comment.
Decision worth confirming: the CDI ≤ 5.4 verify-time promotion (Session.java lines 420/500) is also a fresh mint — it passes expiryTime = null, so it resets expiry to now + validity — yet it stays on this no-jitter path. So legacy clients that get their token replaced at verify still receive fully-synchronized expiries, which is exactly the herd this feature targets. The PR body flags this ambiguity vs. PLAN-002's "all fresh mints" wording and argues it's fine because that path is removed in unit 6. That reasoning is reasonable, but please confirm it matches the plan's intent before merge — it's the one spot where the code's scope ("session creation and refresh") diverges from "all fresh mints".
Minor: the comment says verify-time promotion and regenerate both "keep the configured validity as-is," but they differ — regenerate preserves the original absolute expiry (passes the old expiryTime), whereas verify-promotion resets to now + validity. Slightly misleading to lump them.
There was a problem hiding this comment.
Thanks — addressed both points in f461415.
Minor (misleading comment): fixed. The comment now distinguishes the two no-jitter re-mint callers explicitly — regenerate passes the original (non-null) expiryTime, so it preserves the existing absolute expiry with no validity re-resolution; verify-time promotion (CDI ≤ 5.4) passes a null expiryTime, so it re-mints at the full configured validity (now + validity), deliberately without jitter. They're no longer lumped as "keep the configured validity as-is."
Decision (CDI ≤ 5.4 verify-time promotion staying no-jitter): confirmed against the plan, no change. The jitter unit scopes the mint sites to session creation and refresh, and the "applied at all fresh mints regardless of CDI" wording means those two sites aren't gated behind CDI 5.5 (jitter applies to refresh on 5.4 as well) — it is not a directive to jitter the legacy verify-time promotion path. That path is the implicit payload-propagation-at-verify behavior which a later unit removes entirely under CDI 5.5; the plan never enumerates it as a jitter site. So the code's scope ("session creation and refresh") matches intent, and leaving verify-promotion no-jitter is deliberate. Part of PLAN-002.
|
|
||
| ### Changed | ||
|
|
||
| - `access_token_validity_jitter` is now applied when a fresh access token is minted on session creation and refresh: |
There was a problem hiding this comment.
Semver note for the reviewer: with the parent's default of 0.05, every deployment that upgrades without touching config gets access tokens up to ~5% shorter with no action on their part. It's listed under Changed (not Breaking) and is defensible — subtract-only, and any correct client keys off the exp claim rather than the configured access_token_validity — but it is a silent default-behavior change on upgrade. Worth a conscious call on whether that belongs in a minor bump. (Cross-SDK: node/python/golang inherit it via the response exp, so no port is needed — as the PR body states.)
There was a problem hiding this comment.
Good to flag — this is a deliberate classification call rather than a defect, so I've left it as-is for your explicit sign-off (the PR is draft precisely for decisions like this).
Rationale for Changed over Breaking Changes: the default 0.05 is subtract-only (exp = now + validity × (1 − U[0, jitter])), so a token can only ever be up to ~5% shorter, never longer; every upper-bound guarantee (revocation horizon, the 2× lifetime convention) is preserved; and any correct client keys off the token's exp claim rather than the configured access_token_validity, so no client contract breaks — only the observed default lifetime shifts slightly on upgrade. Cross-SDK: node/python/golang inherit it via the response exp, so no port is needed.
That said, it is a silent default-behavior change on upgrade, so if you'd rather classify it as Breaking (or drop the default to 0), I'm happy to move it — flagging for your call.
…time promotion Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed f461415 in response to the review.
No functional change in this push (comment-only), so the mint/refresh/regenerate/verify behavior and all existing assertions are unchanged. CI note: the heavy test matrix stays |
There was a problem hiding this comment.
Re-review of f4614158 (docs-only follow-up to my prior review on d62be1a8).
The interim commit is exactly and only the comment clarification I asked for: the no-jitter default-entry-point comment now correctly distinguishes the two re-mint callers — regenerate preserves the original absolute expiryTime (non-null, no validity re-resolution), whereas CDI ≤ 5.4 verify-time promotion re-mints at now + validity deliberately without jitter — instead of lumping them as "keep the configured validity as-is." No functional lines changed, so all behavior and assertions I verified last round still stand (confirmed git diff d62be1a8..f4614158 touches only that comment block).
I have no blocking defects. Re-confirmed on this head: the parent's access_token_validity_jitter ∈ [0, 0.25] validation (CoreConfig.java:869) is unconditional — not gated behind !Main.isTesting — so the mint factor (1 - Math.random()*jitter) is always in (0.75, 1] and the subtract-only contract holds, including in tests.
My two prior notes were decisions, not defects, and remain open for your explicit sign-off (the author left them deliberately, which is the right call for a draft):
- CDI ≤ 5.4 verify-time promotion stays no-jitter. Defensible — the plan scopes jitter to session creation + refresh, and that legacy promotion path is removed under CDI 5.5 — but it is the one spot where "session creation and refresh" diverges from the plan's "all fresh mints" phrasing. Confirm it matches intent before merge.
- CHANGELOG classifies the new
0.05default as Changed, not Breaking. Subtract-only and clients key off theexpclaim, so no contract breaks; still, it is a silent up-to-5%-shorter default lifetime on upgrade with no config action. Worth a conscious call on whether that belongs in a minor bump.
Both are yours to decide; the implementation is correct and well-tested as it stands.
Stacked on
agent/issue-1300-session-configs(parent issue #1300) — the diff shown here is only this issue's commit relative to that branch.Problem / root cause
Parent #1300 added the
access_token_validity_jitterconfig (parsing + validation only); nothing consumed it yet, so token expiry was still exactlynow + access_token_validity. This unit wires the config into the mint path so that synchronized token expiries no longer produce fully synchronized refresh herds.Fix approach
At every fresh access token mint on session creation and refresh, the expiry is now:
access_token_validity, so every upper-bound guarantee that leans on token lifetime (revocation horizon, signing-key overlap) is preserved.access_token_validity_jitter = 0disables it. Applied on all CDI versions (config-driven).AccessToken.createNewAccessTokenvia a new overload that takes anapplyValidityJitterflag. Computing the jitter here (rather than at the call site) keeps the token'sexpiryandtimeCreatedderived from a singlenow, avoiding a sub-second skew that would otherwise make exact-lifetime assertions flaky.Session.java(session creation + the two refresh rotation paths) opt in withtrue.regenerateTokenpasses an explicit preservedexpiryTime, which takes the non-jitter branch, so absolute expiry is untouched and jitter is never re-rolled.expiryTime == nullbranch) — flagging the decision explicitly since it's the one ambiguity between the issue text and the plan's "all fresh mints" wording.Tests added
New
AccessTokenValidityJitterTest:jitterZeroKeepsFullValidity— jitter 0 ⇒ lifetime is exactly the configured validity (deterministic, 20 mints).jitterShortensWithinBandAtSessionCreation— jitter 0.25 ⇒ every mint's lifetime ∈[validity*(1-jitter), validity], and the values are shortened and vary (100 mints).jitterAppliedAtRefresh— same guarantees on the refresh path (100 chained refreshes).defaultConfigAppliesJitter— the default0.05is enabled out of the box.Existing tests adjusted for the new default (
0.05):SessionTest5,VerifySessionAPITest3_0,VerifySessionAPITest4_0(encoding tests, default config) — widened the expiry lower bound by the jitter band; the upper bound is unchanged (subtract-only).SessionTest4(exact long-lifetime assertions) — pinnedaccess_token_validity_jitter = 0so the exactexpiry - createdTime == validitychecks stay meaningful.What I ran locally (isolated gradle sandbox, in-memory SQLite, plugin-interface 8.6)
AccessTokenValidityJitterTest,SessionTest4,SessionTest5,AccessTokenTest,RegenerateTokenTest,VerifySessionAPITest3_0/4_0,SessionConfigTest→ 64 passed, 0 failed.io.supertokens.test.session.*+io.supertokens.test.session.api.*→ 226 passed, 0 failed.Committed with
--no-verify(the pre-commit hook is monorepo-coupled and cannot run in a standalone worktree); I independently ran the gradle compile + the tests above and confirmed the changed lines are editorconfig-clean (≤120 cols, no trailing whitespace).Not verified locally
The heavy
Release Branch Testsmatrix (SQLite/PostgreSQL against multiple core versions) is skipped for master-based PRs unlessrun-testsis added — left to CI/reviewer. Thechangelog/Lint PR titlechecks run in CI.Cross-SDK note
Core is the reference for token semantics. The backend SDKs (node/python/golang) mint access tokens driven by core's response
expiry, so they inherit the jitter automatically with no code change. PLAN-002's SDK adoption (unit 10, node) is a separate, later unit; no port is needed for this change.Part of PLAN-002.
Fixes #1303