@W-24077048: Add pluggable SessionStore provider for embedded OAuth authorization server - #872
@W-24077048: Add pluggable SessionStore provider for embedded OAuth authorization server#872jarhun88 wants to merge 19 commits into
Conversation
Introduces the pluggable SessionStore<V> abstraction (interface, config schema, InMemorySessionStore default, custom-provider loader) mirroring the existing FeatureGateProvider/TelemetryProvider pattern, and rewires EmbeddedOAuthProvider's five in-memory maps onto createNamespacedStore(). Handler call sites still expect raw Map/ExpiringMap and will be converted in a follow-up commit.
The default 30-day OAuth refresh-token TTL exceeds Node setTimeout's 32-bit signed-integer delay cap (~24.86 days), which ExpiringMap enforces by throwing. Add a chunked re-arming scheme in InMemorySessionStore.scheduleSet: TTLs at or under the cap are set directly, longer ones store a capped first chunk and re-arm the remaining TTL via a plain timer once that chunk elapses. Also clamp ttlMs to a 1ms floor before calling ExpiringMap.set, since callers may legitimately pass ttlMs <= 0 (e.g. a test simulating an already-expired token) and rely on their own expiresAt field for the actual expiry check rather than exact store eviction timing.
The .cjs custom-provider test fixtures trip the TS explicit-function-return-type rule despite being plain JS; add an eslint override for src/sessionStore/__fixtures__ mirroring the existing docs/scripts/**/*.mjs exemption. Also picks up lint:fix's formatting fix to a type alias in init.ts.
Add sections mirroring FEATURE_GATE_PROVIDER's, covering the memory default, the custom-provider loader, and the atomicity requirement on consume/rotate for a distributed backend.
Each namespace string is used at exactly one createNamespacedStore call site in provider.ts, so the exported const object added no typo-safety over inlining the literals.
Every set/rotate call for a given namespace always passed the same constant TTL, so threading ttlMs through every call was repetitive and let a call site silently pass the wrong constant. TTL is now configured once per createNamespacedStore call (mirrors how maxSize already works) and custom backends no longer receive it at all, since they own their own expiration policy.
…actor The plan doc still showed set/rotate taking a per-call ttlMs argument; update it to match the implemented design where TTL is configured once at createNamespacedStore construction time instead.
jarhun88
changed the title
| @@ -0,0 +1,114 @@ | |||
| # SessionStore for the Embedded OAuth Authorization Server | |||
There was a problem hiding this comment.
Why do we need to push this?
There was a problem hiding this comment.
im testing out a spec driven development workflow. later i want to try adding a review skill that compares the impl with the plan
There was a problem hiding this comment.
Since it's a public repo pushing plans to it may lead to over sharing.
Let's remove it from the commit and keep it locally or we can maintain a separate repo to share specs and plans.
There was a problem hiding this comment.
separate repo sounds like a good idea
sf-ali-mahmoud
left a comment
sf-ali-mahmoud
left a comment
There was a problem hiding this comment.
Thanks for this — the provider plumbing is clean and mirrors the FeatureGate/Telemetry precedent well, and moving the auth-code path to an atomic consume() is a genuine improvement over the pre-PR delete-on-success behavior.
I do want to flag two things before this backs a multi-replica deployment, because both undercut the PR's stated goal, plus a revocation gap and some interface hardening:
Blocking
- Refresh-token rotation isn't single-use (
token.ts). The refresh grant doesget()→await exchangeRefreshToken()(network) →rotate(), so two concurrent requests with the same token both pass validation and both rotate → two live refresh tokens from one. This defeats OAuth 2.1 rotation + reuse detection (RFC 9700). It's carried over from the Map code, but this PR adds the exact primitive that fixes it (consume) and uses it for auth codes but not here. - Custom-provider load failure fails open to in-memory (
init.ts). If a configured custom store can't load, every replica silently reverts to isolated in-memory state — reintroducing the exact bug this PR exists to fix, while reporting healthy. This should fail closed.
Should-fix
3. Two-store rotation (refreshTokens + refreshTokenIndex) isn't jointly atomic; a partial failure leaves the index stale and access-token-hint revocation silently misses the live refresh token.
4. rotate is optional-in-type / required-in-loader / !-asserted at call sites, and its doc recommends the non-atomic delete+set impl it's meant to prevent.
5. Interface has no lifecycle/health hook, so a distributed backend can't gate startup; maxSize is silently dropped on the custom path.
6. Per-namespace TTL never reaches a custom backend, so a real Redis/DB store can't honor the different expirations (auth code ~5min vs refresh token 30d).
Details inline. Happy to pair on the consume-on-refresh change — it's small. Also: the version bump and docs are checked as deferred, which is fine, but items 1–2 are correctness/security rather than polish.
| // Handle refresh token | ||
| const { refreshToken } = result.data; | ||
| const tokenData = refreshTokens.get(refreshToken); | ||
| const tokenData = await refreshTokens.get(refreshToken); |
There was a problem hiding this comment.
[blocking] Refresh-token rotation is not single-use → replay / double-spend.
This reads with get() (not consume()), then suspends on the network round-trip to Tableau at L197, then rotates at L251/L261. Two requests presenting the same refresh token concurrently both pass the get+expiresAt check before either rotates; the second rotate's delete(old) is a no-op but its set(newId) still lands, so one refresh token yields two valid ones. And if the second Tableau exchange fails, L204-221 falls back to reusing the existing access token and still rotates. This defeats OAuth 2.1 refresh-token rotation and the stolen-token replay detection it provides (RFC 9700 §4.14).
Suggested fix — consume up front so a concurrent second request gets undefined:
| const tokenData = await refreshTokens.get(refreshToken); | |
| const tokenData = await refreshTokens.consume(refreshToken); |
(then re-issue as today). For a real distributed backend this ultimately needs a compare-and-set primitive on the interface, since consume alone doesn't detect reuse of an already-rotated token — worth considering adding reuse detection + family revocation as a follow-up.
There was a problem hiding this comment.
Fixed in 62005e5: swapped refreshTokens.get(refreshToken) for the store's atomic consume() (get-and-delete with no await between them), and removed the now-redundant delete() call in the invalid/expired early-return branch. Added a concurrent-replay regression test in refreshTokenGrant.test.ts that fires two requests sharing one refresh token and asserts exactly one 200/one 400 — confirmed it reproduces the original [200,200] symptom against the old get()-based code before the fix.
| } catch (error) { | ||
| log({ | ||
| message: 'Failed to initialize session store provider', | ||
| level: 'error', | ||
| logger: 'sessionStore', | ||
| data: error, | ||
| }); | ||
| log({ | ||
| message: 'Falling back to in-memory session store provider', | ||
| level: 'info', | ||
| logger: 'sessionStore', | ||
| }); | ||
|
|
||
| // Fallback to in-memory provider on error | ||
| state = { kind: 'memory' }; | ||
| } |
There was a problem hiding this comment.
[blocking] Silent fail-open defeats the feature's purpose.
If SESSION_STORE_PROVIDER=custom but the module throws at load (bad path, missing dep, failed validateSessionStore), we log one line and silently fall back to in-memory. In the multi-replica deployment this PR targets, that means every replica quietly runs isolated in-memory state — cross-replica logins break and revocation is no longer centralized — while the server still reports healthy.
This is inconsistent with config.ts, which hard-throws when custom is set but the config JSON is absent. The FeatureGate/Telemetry precedent degrades gracefully because it's a read-mostly gate; a security-critical session backend that was explicitly configured should fail closed. Suggest: on the custom path, let the error propagate (fatal at boot) rather than falling back to { kind: 'memory' }.
There was a problem hiding this comment.
Fixed in 638b73e: removed the try/catch in initializeSessionStore() so a broken custom provider or malformed config now propagates uncaught and is fatal at boot, via the existing top-level startServer().catch(...) handler — no new scaffolding needed. This now matches config.ts's existing hard-throw for a misconfigured custom provider. Updated the JSDoc and rewrote the 4 affected tests in init.test.ts to assert throwing instead of memory fallback.
| @@ -268,7 +258,11 @@ export function token( | |||
| expiresAt: Math.floor((Date.now() + config.oauth.refreshTokenTimeoutMs) / 1000), | |||
| tableauClientId: tokenData.tableauClientId, | |||
| }); | |||
| refreshTokenIndex.set(tokensToStore.accessToken, refreshTokenId); | |||
| await refreshTokenIndex.rotate!( | |||
| tokenData.tokens.accessToken, | |||
| tokensToStore.accessToken, | |||
| refreshTokenId, | |||
| ); | |||
There was a problem hiding this comment.
[should-fix] Two-store rotation isn't jointly atomic → revocation miss.
refreshTokens.rotate! (L251) and refreshTokenIndex.rotate! (L261) are two separate awaited operations across two namespaces, and the interface has no multi-key transaction. On the in-memory default this is fine (no handler interleaves in the microtask gap), but on a distributed backend a crash/partial failure between them leaves the index stale: refreshTokens updated, but refreshTokenIndex still maps the old (now-deleted) access token with no entry for the new one. revoke.ts:139 uses this index as the sole path for access-token-hint revocation, so "sign out" would silently fail to revoke the live refresh token until its 30-day TTL. The provider.ts:71-72 "harmless if stale" note doesn't hold when it's the only revocation path. Consider folding the index into the primary record, or documenting that both must be updated in one transaction.
| /** | ||
| * Atomic rotate: delete `oldKey` and set `newKey` to `value` in the same logical | ||
| * operation, so there is never a window in which both keys are simultaneously valid | ||
| * (used for OAuth refresh-token rotation). | ||
| * | ||
| * As with `consume`, a distributed backend MUST make this truly atomic (conditional | ||
| * write, Lua script/MULTI-EXEC, or a DB transaction). The in-memory default is atomic | ||
| * because there is no `await` between the delete and the set. | ||
| * | ||
| * `rotate` is declared TypeScript-optional only so that a trivial delete-then-set | ||
| * fallback body is a valid implementation to write. This repo's loader nonetheless | ||
| * treats `rotate` as REQUIRED for custom providers (validation fails if it is absent), | ||
| * because the OAuth refresh-token rotation call sites invoke it directly with no runtime | ||
| * branching on whether it exists. | ||
| */ | ||
| rotate?(oldKey: string, newKey: string, value: V): Promise<void>; |
There was a problem hiding this comment.
[should-fix] rotate contract is incoherent, and the doc recommends the unsafe impl.
rotate? is optional in the type (L57) but the loader requires it for custom providers (init.ts:35-41) and all three call sites force-unwrap with rotate!. So the type lies: any hand-built SessionStore that bypasses the loader (a test double, a future refactor) becomes a silent undefined is not a function with no compile-time signal.
Separately, L51-53 tells implementers "a trivial delete+set fallback body is a valid implementation" — that's exactly the non-atomic window rotate exists to eliminate, contradicting the "MUST be truly atomic" line just above. Suggest making rotate required/non-optional (give InMemorySessionStore + fixtures the impl they already have) and removing the delete+set-is-fine guidance.
| function createPrefixedStore<V>(namespace: string, store: SessionStore<unknown>): SessionStore<V> { | ||
| const prefix = `${namespace}:`; | ||
| const shared = store as SessionStore<V>; | ||
|
|
||
| return { | ||
| get: (key) => shared.get(`${prefix}${key}`), | ||
| set: (key, value) => shared.set(`${prefix}${key}`, value), | ||
| delete: (key) => shared.delete(`${prefix}${key}`), | ||
| consume: (key) => shared.consume(`${prefix}${key}`), | ||
| rotate: (oldKey, newKey, value) => | ||
| shared.rotate!(`${prefix}${oldKey}`, `${prefix}${newKey}`, value), | ||
| }; | ||
| } |
There was a problem hiding this comment.
[should-fix] Custom backends can't honor per-namespace TTL, and maxSize is dropped.
TTL is fixed per-namespace at construction on the memory path, but here it's not threaded through at all — a custom store is never told that auth codes expire in ~5 min while refresh tokens live 30 days and client registrations 24 days. A Redis/DB backend therefore can't reproduce in-memory parity: it either never expires entries (auth codes/refresh tokens live forever) or applies one wrong global TTL. maxSize is likewise dropped, so the custom path grows unbounded while the in-memory clientRegistrations evicts FIFO at 10k. Consider passing a namespace→ttl (and bound) descriptor to the provider, or making TTL a set/rotate argument.
There was a problem hiding this comment.
Fixed in d247b33 — added an optional SessionStore.configureNamespace(namespace, options) hook. createNamespacedStore now forwards the per-namespace { ttlMs, maxSize? } through to the custom-provider wrapper (createPrefixedStore), which calls it once per namespace before returning the prefixed store. A custom provider can implement it to learn which TTL/bound applies to which key prefix (e.g. to set native Redis EX/PEXPIRE), mirroring what the in-memory provider already gets for free at construction. Optional, so existing custom providers that don't implement it are unaffected (safe no-op).
| * infrastructure (S3/blob storage, Redis, a relational DB) so state survives across | ||
| * horizontally-scaled instances. | ||
| */ | ||
| export interface SessionStore<V> { |
There was a problem hiding this comment.
[should-fix] No lifecycle/health hook.
There's no init() / close() / healthCheck(). index.ts opens the port before the session backend is proven reachable, so a down Redis-backed store means the server reports healthy and 500s on OAuth traffic (token.ts:277-284), with no graceful drain. For a store meant to back distributed deployments this is the primitive that matters most. Worth adding an optional async init()/close() the provider factory can await at startup/shutdown.
There was a problem hiding this comment.
Fixed in a964161 — added optional SessionStore.init() / close() hooks. connectSessionStore() is now awaited right after initializeSessionStore() at boot, so a custom provider can prove reachability (e.g. a Redis PING) before the server reports healthy; a rejection propagates uncaught to the existing fatal-boot handler, fail-closed. disconnectSessionStore() is called from minimal SIGTERM/SIGINT handlers so a provider can release its resources on shutdown. Both optional, no-op for the in-memory default and for existing custom providers that don't implement them. Kept deliberately minimal — no general graceful-HTTP-drain system, just the session-store lifecycle.
| function loadCustomProvider(config?: Record<string, unknown>): SessionStore<unknown> { | ||
| if (!config?.module) { | ||
| throw new Error( | ||
| 'Custom session store provider requires "module" in providerConfig. ' + | ||
| 'Example: SESSION_STORE_PROVIDER_CONFIG=\'{"module":"./my-session-store.js"}\'', | ||
| ); | ||
| } | ||
|
|
||
| const modulePath = config.module; | ||
|
|
||
| if (typeof modulePath !== 'string') { | ||
| throw new Error('Custom session store provider requires "module" to be a string'); | ||
| } | ||
|
|
||
| // Determine if it's a file path or npm package name | ||
| let resolvedPath: string; | ||
|
|
||
| if (modulePath.startsWith('.') || modulePath.startsWith('/')) { | ||
| // File path - resolve relative to process working directory (user's project root) | ||
| resolvedPath = resolve(process.cwd(), modulePath); | ||
| } else { | ||
| // npm package name - require as-is | ||
| resolvedPath = modulePath; | ||
| } | ||
|
|
||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports -- Sync load for preload script | ||
| const module = require(resolvedPath); |
There was a problem hiding this comment.
[nit / doc] Loader require()s an env-var-specified path.
SESSION_STORE_PROVIDER_CONFIG.module is resolved against process.cwd() and required, then constructed — arbitrary code execution + path traversal by construction. It's operator-controlled config (same trust model as FEATURE_GATE_PROVIDER/TELEMETRY_PROVIDER), so no new external attack surface, but worth a one-line comment that this is trusted operator input. Note validateSessionStore runs after the module has already loaded and its constructor executed, so it's a usability check, not a security boundary.
|
|
||
| this.map.set(key, value, CHUNK_MS + REFRESH_MARGIN_MS); | ||
| const remaining = ttlMs - CHUNK_MS; | ||
| const timer = setTimeout(() => { |
There was a problem hiding this comment.
[nit] Re-arm chunk timers aren't .unref()'d, so long-lived refresh-token timers keep the event loop alive on shutdown. Also, the map-entry TTL at L81 is exactly MAX_SIGNED_INT32 — correct today, but zero margin against the cap ExpiringMap throws on, so a future REFRESH_MARGIN_MS reduction could push it over silently. A boundary test asserting the entry-TTL value would lock this in.
There was a problem hiding this comment.
Fixed in afbd84f — the chunk re-arm timer now calls .unref() so it doesn't keep the event loop alive on shutdown, plus a boundary test locking in the chunked-path entry TTL at exactly MAX_SIGNED_INT32 and a test asserting the re-arm timer is unref'd.
|
|
||
| The session store provider to use for the embedded OAuth authorization server's session state | ||
| (pending authorizations, authorization codes, refresh tokens, and OAuth client registrations). | ||
| Only relevant when [`AUTH`](#auth) is `oauth` with the embedded authorization server enabled. |
There was a problem hiding this comment.
if this is only usable when AUTH=oauth you should put this in oauth.md, also mention how to enable embedded auth server or the env variable related to it
ExpiringMap's maxSize eviction removed keys without InMemorySessionStore knowing, leaving a stale chunked-TTL rearm timer alive for up to ~24.86 days. Add an optional onDelete hook to ExpiringMap.delete() (the single choke point every removal path already routes through) and wire it to clearRefreshTimer.
Long-lived sessions with TTLs beyond setTimeout's 32-bit cap re-arm a background timer on each chunk boundary; unref it so a pending re-arm doesn't keep the event loop alive and block process shutdown. Adds a test locking in the chunked-path expiration boundary at MAX_SIGNED_INT32 and a test asserting the re-arm timer is unref'd.
refreshTokens.get() followed later by delete()+set() left a window during the Tableau token exchange await where a concurrent replay of the same refresh token could succeed twice. Swap to the store's atomic consume() (get-and-delete with no await between them) to close it.
initializeSessionStore() silently fell back to the in-memory provider on any error, including a broken custom provider or malformed config. In a multi-replica deployment that leaves every replica quietly running isolated state while the server reports healthy. Let errors propagate to the existing fatal-boot handler instead, matching config.ts's existing hard-throw for a misconfigured custom provider.
…roviders Adds an optional SessionStore.configureNamespace(namespace, options) hook, called once per namespace when wrapping a custom provider, so it can learn the TTL/bound the memory provider already gets automatically at construction.
…providers Adds optional SessionStore.init()/close(), awaited at startup/shutdown, so a custom backend can prove reachability before the server reports healthy and release resources on shutdown, instead of silently 500ing on first use.
3 participants
Description
SessionStore<V>interface + in-memory default implementation, mirroring the existingFeatureGateProvider/TelemetryProviderpattern (interface + default impl +SESSION_STORE_PROVIDER/SESSION_STORE_PROVIDER_CONFIGenv-var-driven custom loader)EmbeddedOAuthProvider's five in-memory session maps (pending authorizations, authorization codes, refresh tokens, refresh-token index, client registrations) to the newSessionStoreinterface across all handlers (authorize,authorizeRedirectUri,callback,token,revoke,register)InMemorySessionStoreTTL handling: the 30-day default refresh-token TTL exceeds NodesetTimeout's 32-bit signed-integer delay cap enforced byExpiringMap; add a chunked re-arming scheme inscheduleSetfor TTLs beyond the cap, and clampttlMsto a 1ms floor for callers that legitimately passttlMs <= 0(e.g. tests simulating an already-expired token)Motivation and Context
Session state for the embedded OAuth authorization server currently lives entirely in process-local maps, so a multi-replica deployment (or even a single-instance restart) silently breaks in-flight logins and wipes refresh tokens. This introduces a swappable
SessionStoreso a deployer can bring their own backend (Redis/DB/etc.), while keeping today's in-memory behavior as the zero-dependency default. No concrete external backend implementation ships in this PR — same split as the existingUploadUrlProvider/UploadUrlProviderImplprecedent.Type of Change
How Has This Been Tested?
src/sessionStore/inMemorySessionStore.test.ts,src/sessionStore/init.test.ts(provider factory, custom-loader validation, namespace isolation)tests/oauth/embedded-authz) passes unmodified against the new defaulttsc --noEmitcleanRelated Issues
N/A
Checklist
npm run version— deferred to a follow-up docs/version-bump pass covering this whole featureContributor Agreement