Commit 854d919
authored
feat(auth): store OIDC sessions in Valkey behind a feature flag (#1089)
* feat(auth): store OIDC sessions in Valkey behind a feature flag
Cookie-only sessions grow unbounded as more tokens (impersonation,
API-gateway, crowdfunding, profile) get written onto req.appSession,
risking browser/proxy header limits. Wire express-openid-connect's
native session.store into the existing ValkeyService so the cookie
only carries an opaque signed session id; gated by
SESSION_STORE_ENABLED + VALKEY_URL so it fails soft to the current
cookie-only behavior when unset.
LFXV2-2666
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(auth): guard destroy against an unsafe session cache key
destroyAsync called valkeyService.del() without checking cacheKey()
for null, unlike get/set which both short-circuit on an invalid id.
del() already no-ops on null so behavior was unaffected, but the
inconsistency was flagged in review.
LFXV2-2666
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address PR #1089 review feedback
Address review comments from @copilot-pull-request-reviewer, @coderabbitai:
- valkey.service.ts / valkey-cache.interface.ts: del() now returns a
boolean success signal instead of Promise<void> (per @copilot-pull-request-reviewer)
- session-store.service.ts: destroyAsync escalates to logger.error when
a session delete fails on logout, since the session would otherwise
remain valid in Valkey until it expires via TTL with no visibility
(per @copilot-pull-request-reviewer)
- session-store.service.ts: isSessionPayload now also requires `cookie`,
matching the full SessionStorePayload shape and rejecting a corrupt/legacy
entry missing that field before it reaches ttlSecondsFor (per @coderabbitai)
Resolves 2 review threads. Responded to a third (expires_at type) with
evidence that express-openid-connect's own Session interface types
expires_at as string, so no change was made there. A fourth thread
(rollout/mixed-pod compatibility) is left open as a documented
deployment trade-off, not a code fix.
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address PR #1089 review feedback (iteration 2)
Address review comments from @copilot-pull-request-reviewer:
- session-store.service.ts: setAsync now invalidates the cache key when
the write fails, instead of only logging a warning. setJson is a plain
SET-with-TTL, so a failed write previously left the prior session
value in place — a cleared token (e.g. stopping impersonation) could
be silently reloaded on the next request. The store now fails closed:
a failed write forces the key to be deleted, and a failure to also
invalidate escalates to logger.error (per @copilot-pull-request-reviewer)
Resolves 1 review thread. A second new thread (server.ts:196) restates
the already-discussed mixed-pod rollout risk from server.ts:194 —
replied pointing back to the existing answer, left open as the same
documented deployment trade-off.
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address PR #1089 review feedback
Address review comment from @copilot-pull-request-reviewer:
- apps/lfx-one/src/server/services/session-store.service.ts: setAsync
now throws when a session write (and its fallback invalidation)
fails, instead of swallowing the error. express-openid-connect awaits
store.set() inside its res.end() wrapper and calls next(err) on
rejection rather than completing the response — surfacing the
failure now prevents the OIDC login callback from issuing a cookie
for a session that was never persisted, which previously caused an
indefinite silent login loop whenever Valkey was unreachable.
Resolves 1 review thread (server.ts:175 — Valkey-reachability gate).
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address PR #1089 review feedback
Address review comments from @copilot-pull-request-reviewer:
- session-store.service.ts: updated the class doc comment to distinguish
fail-soft reads (a read fault degrades to a cache miss, forcing re-auth)
from fail-closed writes (a failed persist now throws and propagates as
a request error via express-openid-connect) — the prior wording claimed
writes were also best-effort/fail-soft, which is no longer accurate
after the earlier fail-closed write fix.
- session-store.service.ts: tightened isSessionPayload to validate the
nested SessionStorePayload shape (numeric header.iat/uat/exp, a non-null
data object, numeric cookie.expires/maxAge) instead of only checking
that the header/data/cookie keys are present. A corrupt cached entry
like `{ header: {...}, data: null, cookie: {} }` previously passed this
guard, causing express-openid-connect to crash attaching `null` as
req.appSession instead of degrading to a cache miss.
Resolves 3 review threads.
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address PR #1089 review feedback
Address review comments from @cursor, @copilot-pull-request-reviewer, @audigregorie:
- session-store.service.ts: setAsync now fails closed when cacheKey()
returns null instead of silently no-op'ing, so a rejected session id
can no longer complete the response with a cookie that resolves to
nothing in Valkey (per @cursor).
- session-store.service.ts: setAsync now distinguishes a brand-new
session (header.iat === header.uat) from a rolling refresh of an
already-established one. Since express-openid-connect defaults
session.rolling to true, a write fires on every authenticated request,
not just at login — failing closed on every failed write meant a
Valkey outage would 500 all authenticated traffic for its duration.
A failed write for a new session still fails closed (nothing was ever
persisted for that id); a failed write for an existing session now
fails soft, leaving the prior entry in place until its own TTL
(per @audigregorie).
- server.ts: reworded the session-store gate comment to stop implying
VALKEY_URL proves live reachability, and to point at
SessionStoreService for the fail-soft read / fail-closed write split
(per @copilot-pull-request-reviewer, @audigregorie).
- server.ts: noted that genid's 64 hex chars is load-bearing against
isFilterSafeIdentifier's 64-char cap, so a future change to the id
format doesn't silently fail-closed every session (per @audigregorie).
- PR description: documented the destroyAsync fail-soft trade-off
(a failed logout delete leaves the session valid until TTL) and
updated the write fail-soft/fail-closed description to match the new
split (per @audigregorie).
Resolves 6 review threads.
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address PR #1089 review feedback
Address review comments from @copilot-pull-request-reviewer:
- session-store.service.ts: session writes are now unconditionally
fail-closed — the iat===uat new-session heuristic from the prior
iteration incorrectly treated every rolling-refresh write failure as
safe to leave in place, but a refresh write can carry a real mutation
(e.g. stop-impersonation clearing impersonationToken) that would
otherwise survive stale until TTL. A failed write of any kind now
invalidates the key and throws AuthenticationError (401) instead of a
bare Error, so apiErrorHandler returns a structured re-auth response
and logs at warn — a Valkey outage still degrades to forced re-login
rather than a raw 500, without trading away data integrity.
- server.ts: added a rollout-safety note next to the sessionStoreEnabled
gate pointing to the PR description's documented mitigation for the
mixed old/new-pod cookie-format hazard during a RollingUpdate.
- PR description: documented the rollout-safety trade-off (Recreate /
low-traffic-window mitigation for toggling SESSION_STORE_ENABLED) and
updated the write-failure behavior description to match the
always-fail-closed design above.
Resolves 3 review threads.
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address PR #1089 review feedback (iteration 7)
Address review comments from @cursor (Bugbot), @copilot-pull-request-reviewer:
- session-store.service.ts: retry the fallback `del` once when a failed
session write's invalidation also fails, closing most of the window where
a transient double-fault leaves a stale/mutated entry for a later `get`
to resurrect (per @cursor)
- session-store.service.ts, valkey-cache.constants.ts: clamp a present-but-
non-positive `cookie.maxAge` to a new 1s `SESSION_EXPIRED_TTL_SECONDS`
instead of handing an already-expired session the 7-day fallback TTL
reserved for missing metadata (per @copilot-pull-request-reviewer)
- server.ts: fail startup when `SESSION_STORE_ENABLED` is on in production
with a non-`rediss://` `VALKEY_URL`, since the session payload now
carries the full bearer-token bundle and a plaintext transport would
ship those credentials unencrypted (per @copilot-pull-request-reviewer)
Resolves 3 review threads. A 4th (PR description test-plan wording:
"signed id" -> "opaque id") is addressed via `gh pr edit`, not a commit.
LFXV2-2666
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address PR #1089 review feedback
Address a review comment from @copilot-pull-request-reviewer on
session-store.service.ts:86-89: a failed Valkey write whose fallback
invalidation also fails left a stale session-store entry in place, but
still returned a 401 whose response express-openid-connect processes
through its own cookie-write hook — a hook that fires independently of
how the request settles and would reissue the same session-id cookie,
resurrecting the stale entry on the next request.
- authentication.error.ts: add a `clearSession` option to
AuthenticationError, carried as a public readonly flag (not part of
the JSON response body)
- session-store.service.ts: set `clearSession: true` on both
AuthenticationError throws in setAsync (unsafe cache key, and write
failure whether or not fallback invalidation succeeds)
- error-handler.middleware.ts: when clearSession is set, set
req.appSession = null before responding — express-openid-connect's
setter treats this as "clear the cookie" instead of reissuing it
- express.d.ts: widen req.appSession to accept null, matching
express-openid-connect's actual setter contract
LFXV2-2666
Resolves 1 review thread.
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address more PR #1089 review feedback
Address minor, zero-logic-risk findings from @copilot-pull-request-reviewer:
- docs/architecture/backend/impersonation.md: correct the stale "cookie-based
(no server-side session store)" statement — impersonation fields live on
req.appSession regardless of backend, cookie-only by default or Valkey-backed
when SESSION_STORE_ENABLED=true
- docs/runtime-configuration.md: add SESSION_STORE_ENABLED to the server-side
cache variable table, with the production rediss:// requirement and the
no-overlap rollout/rollback constraint
- session-store.interface.ts: document that SessionStoreCookieMeta.expires and
maxAge are both milliseconds, matching ttlSecondsFor's existing /1000
- session-store.service.ts: correct the write-failure log message — with the
clearSession fix from the prior commit, the user is logged out on the
current request (401 + cleared cookie), not "on next request"
The session-store.service.ts:74 optimistic-concurrency/race-condition finding
(concurrent writes to the same session id, last-write-wins) is acknowledged as
valid but not addressed in this PR — a correct fix needs per-session versioning
with an atomic compare-and-set, which is a meaningful architectural addition
better scoped and reviewed on its own rather than rushed into this iteration.
Responded on the thread with the reasoning; left open.
LFXV2-2666
Resolves 3 review threads; 1 addressed via response only (deferred).
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(docs): fix prettier table formatting in runtime-configuration.md
Realign markdown table column widths flagged by yarn format:check in
CI for PR #1089.
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address round 3 PR #1089 review feedback
Address review comments from @copilot-pull-request-reviewer:
- server.ts: clear legacy chunked appSession.N cookies on every request
while SESSION_STORE_ENABLED is on — the native custom-store cookie
writer only manages the unchunked appSession cookie, so pre-cutover
chunk cookies could otherwise survive a logout and be silently
resurrected by a later rollback to cookie mode
- charts/lfx-self-serve/values.yaml: corrected the SESSION_STORE_ENABLED
comment — it previously advertised an instant/anytime rollback, which
contradicts the chart's default 3-replica RollingUpdate
- docs/architecture/backend/impersonation.md: updated the Session
Storage section's intro text and diagram, which still asserted
cookie-only, encrypted-chunked storage even after the flag toggles
to Valkey-backed storage
LFXV2-2666
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): improve session write failure error message
Address review comment from @copilot-pull-request-reviewer on PR #1089:
- session-store.service.ts: the AuthenticationError message returned to
the client on a Valkey session write failure said "Session write
failed to persist" (infrastructure jargon, no recovery action). Now
reads "Your session could not be saved — please sign in again."
Resolves 1 review thread.
LFXV2-2666
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address round 4 PR #1089 review feedback
Address review comments from @copilot-pull-request-reviewer and @cursor:
- error-handler.middleware.ts: move the AuthenticationError clearSession
handling before the res.headersSent early-return, so a failed session
write still clears req.appSession (preventing cookie reissue) even on
streaming responses that already flushed headers before the write
failed (per @copilot-pull-request-reviewer, @cursor)
- server.ts: pass matching httpOnly/sameSite/secure attributes to the
legacy appSession.N chunk-cookie clearCookie() calls, mirroring
express-openid-connect's own cookie defaults, so the clear reliably
takes effect in every browser (per @cursor)
- runtime-configuration.md: documented the concrete Helm override needed
to safely flip to a Recreate rollout strategy — strategy.type=Recreate
alone leaves the chart's default strategy.rollingUpdate map in place,
which Kubernetes rejects; operators must also null it out (per
@copilot-pull-request-reviewer)
Resolves 4 review threads.
LFXV2-2666
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address round 5 PR #1089 review feedback
Address review comments from @copilot-pull-request-reviewer, @cursor:
- server.ts: apply the same clearSession-before-headersSent fix inside the
outer global error handler, which has its own headersSent short-circuit
that runs before apiErrorHandler is ever reached — the round-4 fix inside
apiErrorHandler alone never ran for SSR/auth-redirect routes (per @cursor)
- session-store.service.ts: get/set/destroy now return the underlying promise
when called without a callback, so express-openid-connect's minified-source
probe (which can lose the "cb"/"callback" substring match after production
minification renames local parameters) still detects these methods
correctly instead of invoking `callback` as undefined (per
@copilot-pull-request-reviewer)
- session-store.service.ts: cache-key safety-check failures now log at debug
instead of warn — the sid comes from an unsigned, anonymous-controlled
cookie, so a malformed value is expected untrusted input, not a system
fault (per @copilot-pull-request-reviewer)
- session-store.service.ts: replaced the remaining infrastructure-jargon
AuthenticationError message ("cache key failed the safety check") with the
same user-facing re-authentication guidance used elsewhere (per
@copilot-pull-request-reviewer)
LFXV2-2666
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
* fix(review): address round 6 PR #1089 review feedback
Address review comment from @copilot-pull-request-reviewer:
- server.ts: register the legacy appSession.N chunk-cookie cleanup
middleware before auth(authConfig) instead of after. express-openid-
connect's built-in /logout route completes the response inside its own
router without calling next(), so cleanup registered after auth() never
ran on a logout request — the exact request where clearing stale chunks
matters most, since a later rollback to cookie mode could otherwise
silently restore a pre-logout session from an uncleared chunk cookie.
LFXV2-2666
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
---------
Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>1 parent 2407e27 commit 854d919
13 files changed
Lines changed: 413 additions & 34 deletions
File tree
- apps/lfx-one/src
- server
- errors
- middleware
- services
- types
- charts/lfx-self-serve
- docs
- architecture/backend
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
11 | 16 | | |
12 | 17 | | |
13 | 18 | | |
14 | 19 | | |
15 | 20 | | |
16 | 21 | | |
17 | 22 | | |
| 23 | + | |
18 | 24 | | |
19 | 25 | | |
20 | | - | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
21 | 29 | | |
22 | 30 | | |
23 | 31 | | |
| |||
Lines changed: 9 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| |||
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
25 | 33 | | |
26 | 34 | | |
27 | 35 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| 11 | + | |
11 | 12 | | |
12 | 13 | | |
13 | 14 | | |
| |||
18 | 19 | | |
19 | 20 | | |
20 | 21 | | |
| 22 | + | |
21 | 23 | | |
22 | 24 | | |
23 | 25 | | |
| |||
60 | 62 | | |
61 | 63 | | |
62 | 64 | | |
| 65 | + | |
63 | 66 | | |
64 | 67 | | |
65 | 68 | | |
| |||
165 | 168 | | |
166 | 169 | | |
167 | 170 | | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
168 | 197 | | |
169 | 198 | | |
170 | 199 | | |
| |||
182 | 211 | | |
183 | 212 | | |
184 | 213 | | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
185 | 225 | | |
186 | 226 | | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
187 | 260 | | |
188 | 261 | | |
189 | 262 | | |
| |||
414 | 487 | | |
415 | 488 | | |
416 | 489 | | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
417 | 497 | | |
418 | 498 | | |
419 | 499 | | |
| |||
Lines changed: 194 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
0 commit comments