feat: add session rotation grace, access token validity jitter, and recent-reuse configs - #1306
feat: add session rotation grace, access token validity jitter, and recent-reuse configs#1306supertokens-agent-runner[bot] wants to merge 2 commits into
Conversation
…ecent-reuse configs Add three session-related config options with parsing and validation only; behaviour is wired up in later changes: - refresh_token_rotation_grace_period (seconds, default 30, range 0-300, @NotConflictingInApp) - access_token_validity_jitter (fraction, default 0.05, range 0-0.25, @NotConflictingInApp) - recent_token_reuse_behaviour (TOKEN_THEFT (default) | UNAUTHORISED, @ConfigYamlOnly) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Config-only unit; clean and low-risk. The VALIDITY_TESTING gating on the grace-period range, the @EnumProperty case-insensitive validation + uppercase normalization, the scope annotations, and the config.yaml/devConfig.yaml comment format all follow the existing access_token_validity / password_hashing_alg patterns, and I traced the GetTenantCoreConfigForDashboardAPITest contains() matcher against all three new yaml descriptions — they pass. Tests cover the parsing/validation surface well. A few non-blocking observations below; no correctness/security/breaking-change issues found.
| } | ||
| } | ||
|
|
||
| if (access_token_validity_jitter < 0 || access_token_validity_jitter > 0.25) { |
There was a problem hiding this comment.
The jitter range check is always enforced, whereas the grace-period check (L860) is gated behind VALIDITY_TESTING. Intentional? If the jitter-at-mint behaviour unit (#1303) ever needs to drive an out-of-range jitter in a test it won't be able to, the way the grace-period behaviour unit can. Fine to leave as-is if no such test is expected — just flagging the asymmetry so it's a deliberate choice rather than an oversight.
There was a problem hiding this comment.
Deliberate. The gate on the grace-period (and on the signing-key interval) exists because tests need to drive time windows below/above the production-safe range. The jitter bound 0-0.25 is a semantic contract on the fraction itself — a jitter above 25% is undesirable in tests too — so it stays always-enforced. Confirmed empirically: the jitter-at-mint behaviour unit (#1303, PR #1307) only uses in-range values (0 and 0.25) in its tests, so nothing needs an out-of-range jitter. Leaving as-is.
| "How the reuse of a recently rotated-out refresh token is reported. Values are \"TOKEN_THEFT\" | " | ||
| + "\"UNAUTHORISED\". The session is revoked regardless of this setting. (Default: TOKEN_THEFT)") | ||
| @EnumProperty({"TOKEN_THEFT", "UNAUTHORISED"}) | ||
| private String recent_token_reuse_behaviour = "TOKEN_THEFT"; |
There was a problem hiding this comment.
recent_token_reuse_behaviour is @ConfigYamlOnly (not settable per-tenant), while the two numeric configs are @NotConflictingInApp (per-tenant, consistent within an app). Worth confirming this scope asymmetry is what downstream/SDK adoption of CDI 5.5 expects — if a tenant should be able to choose theft-vs-unauthorised reporting independently, yaml-only would block that and be awkward to change later without a semver-visible move.
There was a problem hiding this comment.
Intentional scope choice, not an oversight. recent_token_reuse_behaviour selects how two reuse subtypes are reported (theft vs unauthorised) — the session is revoked either way — and it's specified as a CUD/root-level (@ConfigYamlOnly) knob, deliberately not per-app/tenant, since it's an operator-level reporting policy rather than a per-tenant behaviour. Its final name is also flagged as open in the issue. The two numeric configs are genuine per-app session parameters, hence @NotConflictingInApp. If a per-tenant need surfaces during SDK adoption of CDI 5.5 it can be revisited then, but the current design intends this to be root-level.
| @@ -7,6 +7,16 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
There was a problem hiding this comment.
This branch is behind master: master already carries a ## [12.0.8] block directly under ## [Unreleased], but this head still shows ## [12.0.7] as the latest release. Both edits touch the region right after ## [Unreleased], so a rebase/merge will likely conflict here — worth rebasing before merge.
There was a problem hiding this comment.
Resolved — merged origin/master into the branch (commit 9cded8f) and hand-resolved the CHANGELOG conflict: the new Added block stays under [Unreleased], directly above master's [12.0.8] section. mergeable is now clean. Merged rather than rebased because PRs #1307 and #1308 are stacked children of this branch — a force-push would disrupt them; stack-sync will merge-forward this update to them.
| @@ -0,0 +1,202 @@ | |||
| /* | |||
| * Copyright (c) 2024, VRAI Labs and/or its affiliates. All rights reserved. | |||
There was a problem hiding this comment.
Nit: copyright header says 2024, but this file is new in 2026.
There was a problem hiding this comment.
Fixed — copyright header updated to 2026 in the new merge/review commit (9cded8f).
|
|
||
| process.kill(); | ||
| assertNotNull(process.checkOrWaitForEvent(PROCESS_STATE.STOPPED)); | ||
| } |
There was a problem hiding this comment.
Minor coverage gap: there's no test asserting that an out-of-range grace period is accepted when VALIDITY_TESTING is off (i.e. the gate the later behaviour units actually rely on to set windows past 300). The throw path is tested with the gate on, but the skip path — the reason for gating — isn't. Cheap to add.
There was a problem hiding this comment.
Added testGracePeriodOutOfRangeIsAcceptedWhenTestingGateOff: sets refresh_token_rotation_grace_period=301 and starts the process without enabling VALIDITY_TESTING, then asserts the process starts and the out-of-range value loads (301). This exercises the skip path the later behaviour units rely on, complementing the existing throw-path test. Ran SessionConfigTest locally against plugin-interface 8.7 — all 7 tests pass.
Resolve CHANGELOG conflict (keep the new Added block under [Unreleased] above master's [12.0.8]). Also address PR review: fix test copyright year (2024 -> 2026) and add a test covering the skip path of the grace-period range gate (out-of-range value accepted when VALIDITY_TESTING is off). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review feedback addressed (commit 9cded8f)
CI triage
|
There was a problem hiding this comment.
Re-review of 9cded8ff. All five comments from the previous round (5e9003d) are resolved:
- Copyright header → 2026.
- Skip-path coverage gap closed by
testGracePeriodOutOfRangeIsAcceptedWhenTestingGateOff(301 accepted withVALIDITY_TESTINGoff). - CHANGELOG conflict merge-resolved cleanly,
Addedblock under[Unreleased]above[12.0.8]. - The two flagged asymmetries (jitter always-enforced vs grace-period gated;
recent_token_reuse_behaviourbeing@ConfigYamlOnly) each got a solid deliberate-choice rationale I'm satisfied with.
I re-confirmed the yaml comment conventions match the annotations (@NotConflictingInApp → DIFFERENT_ACROSS_APPS; @ConfigYamlOnly → plain OPTIONAL, matching max_server_pool_size/base_path), and the VALIDITY_TESTING gating on the grace-period range mirrors the existing access_token_dynamic_signing_key_update_interval check (L861) exactly — in production (!Main.isTesting) it always enforces; under test it's skipped unless a test opts into strict validation. The master merge brought only unrelated files into the raw diff; nothing in it touches the PR-owned surface (CoreConfig + yaml + CHANGELOG + the new test). Config-only, additive, no behaviour — no correctness/security/semver issues. One non-blocking forward note inline. Not approving per bot-review policy.
| @ConfigDescription( | ||
| "Fraction between 0 and 0.25 by which access token validity is randomly shortened at issuance. Applied " | ||
| + "subtractively so the token expiry is never lengthened. Set to 0 to disable. [Default: 0.05]") | ||
| private double access_token_validity_jitter = 0.05; |
There was a problem hiding this comment.
Non-blocking, forward-looking: the default here is 0.05, not 0. Once the jitter-at-mint behaviour lands (#1303 / #1307), existing deployments will get up to 5% shorter access-token lifetimes by default without having opted in. That default is decided in this diff, so worth an explicit confirmation it's intended as on-by-default rather than opt-in (0). Purely a heads-up for the behaviour unit's reviewer — no change needed here.
There was a problem hiding this comment.
Confirmed intentional and spec-mandated — not something to change in this unit. The 0.05 default (and the 0–0.25 range) are fixed by issue #1300 and PLAN-002: jitter is on-by-default by design precisely because it's subtract-only — exp = now + validity × (1 − U[0, jitter]), so it can only shorten, never lengthen, a token — and the downstream retention/overlap floors in the refresh-rotation units are allowed to rely on that shortening being present by default. Any deployment that wants exact-validity behaviour sets it to 0 to disable.
Agreed the on-by-default effect only becomes observable once the mint-time wiring lands in #1303 / PR #1307, so I'll make sure that PR's description calls out the default-on lifetime change (up to 5% shorter access-token lifetimes without an explicit opt-in) explicitly for its reviewer. Leaving the default at 0.05 here.
What & why
Adds three session-related config options to
CoreConfig. This is a preparatoryunit — parsing, validation and test gates only. The behaviour that consumes
these configs (refresh-time rotation with a grace window, expiry jitter at mint,
reuse-subtype reporting) lands in later changes.
Part of PLAN-002.
refresh_token_rotation_grace_period30,0–300@NotConflictingInAppaccess_token_validity_jitter0.05,0–0.25@NotConflictingInAppexp = now + validity * (1 - U[0, jitter])); never lengthens a token;0disablesrecent_token_reuse_behaviourTOKEN_THEFT(default) |UNAUTHORISED@ConfigYamlOnlyApproach
CoreConfig, each with@EnvName,@JsonProperty,@ConfigDescription, and the correct scope annotation (mirrors the existingaccess_token_validity/password_hashing_algpatterns).normalizeAndValidate:access_token_validity_jitterrange[0, 0.25]is always enforced.refresh_token_rotation_grace_periodrange[0, 300]is gated behind the sametesting flag (
VALIDITY_TESTING) as the other session-validity checks, so thebehaviour units can drive windows outside the normal bounds. In production the
range is always enforced.
recent_token_reuse_behaviouris validated by the existing@EnumPropertymechanism (case-insensitive) and normalized to upper case.
config.yaml/devConfig.yamlget matching commented entries (required byGetTenantCoreConfigForDashboardAPITest, which asserts every@ConfigDescriptionhas a matching yaml description).
CHANGELOG.mdentry under## [Unreleased].The config name
recent_token_reuse_behaviouris flagged in the issue as "finalname open to review" — opened as draft for that reason.
Tests
Added
src/test/java/io/supertokens/test/SessionConfigTest.java(6 tests):defaults, custom values (incl. case-insensitive enum), boundary values accepted,
grace-period out-of-range rejected (with the testing gate), jitter out-of-range
rejected, and invalid enum value rejected.
Ran locally (in-memory SQLite, single test classes via Gradle) — all green
SessionConfigTest— 6/6 passGetTenantCoreConfigForDashboardAPITest— pass (this is the yaml ↔@ConfigDescriptionmatcher, directly exercised by the new yaml entries)EnvConfigTest— pass (@EnvNamecompleteness + env loading)multitenant.ConfigTest.testAllConfigFieldsAreAnnotated— pass (every field has@ConfigYamlOnlyor@NotConflictingInApp)ConfigMapperTest— passNot verified locally
as above.
exercise beyond parsing/validation yet.
Notes for reviewers
--no-verify: the core pre-commit hook assumes thesupertokens-rootmonorepo layout (it runs../gradlew editorconfigCheckand./runBuild, both of which shell into../). It cannot run against a standalonebranch checkout, so this commit was made with
--no-verify. Its substantivegates were run independently instead: the core module compiles and builds under
Gradle, the tests above pass, and the changed lines are editorconfig-clean
(≤120 cols, no trailing whitespace); CI re-runs all of this.
config keys; they become SDK-relevant only when the CDI 5.5 session semantics are
adopted (later PLAN-002 units, e.g. supertokens-node). No SDK port is needed for
this config-only change.
Fixes #1300