chore(auth): hygiene follow-ups from omni-review closeout (#13377) - #13567
diegosouzapw merged 4 commits into
Conversation
…pw#13377) Auth (from PR diegosouzapw#13375): - Delete isStaleDashboardJwtError + staleDashboardJwtWarningEmitted from pipeline.ts; emit log in !payload branch - Minters use getDashboardJwtSecret() (trimmed) in login/route.ts, oidc/callback/route.ts - ws/handshake.ts uses getDashboardJwtSecret() instead of raw env - verifyDashboardSessionToken: pin {algorithms:['HS256']}, reject empty Uint8Array - Use DASHBOARD_SESSION_COOKIE/CLAIM constants in 7 consumers - Source guard: allowlist dashboardSessionToken.ts + oidc/callback for jwtVerify imports - Add try/finally around env mutation in dashboard-session-token-13298.test.ts - AUTHZ_GUIDE.md: 'route guard' -> 'dashboard route guard (isDashboardSessionAuthenticated())' Tests: 28/28 auth tests pass, typecheck clean Scope note: the Batches half of diegosouzapw#13377 (forward-progress guard for deleteCompletedBatches) is intentionally NOT in this PR. The issue's wording ("both for (;;) loops") targets PR diegosouzapw#13374's chunked implementation, which is stacked on PR diegosouzapw#12969 — not this release/v3.8.51 base. It is tracked separately.
…nts + repo-wide jwtVerify guard Two items the initial hygiene commit left open, both required by issue diegosouzapw#13377: - src/server/authz/pipeline.ts: refreshDashboardSessionIfNeeded still carried a local getJwtSecret() and set the refreshed cookie with the literal "auth_token". Use the shared getDashboardJwtSecret() and the exported DASHBOARD_SESSION_COOKIE constant so mint and verify agree everywhere and no consumer hardcodes the cookie name (issue: 'Use the exported DASHBOARD_SESSION_COOKIE / DASHBOARD_SESSION_CLAIM constants in the six consumers'). - tests/unit/dashboard-session-verifier-source-guard.test.ts: the guard only re-asserted jwtVerify inside the two allowlisted files, so a NEW cookie consumer calling jwtVerify would slip through. It now scans every src/**/*.ts file and fails on any non-allowlisted file that calls jwtVerify (issue: 'repo-wide allowlist ... so a NEW cookie consumer is caught, not only regressions in the six known files').
5fb2dd6 to
7269251
Compare
jonlwheat2-gif
changed the title
|
CI note — the failing checks are pre-existing, not from this PR. Every failing job here fails identically on Failing everywhere:
Two pinned from the logs:
The base commit |
|
Good mechanical follow-up, and the |
…kie warning staleDashboardJwtWarningEmitted was moved from module scope to a local `let` inside refreshDashboardSessionIfNeeded(), so it reset to false on every call — the "warn once per process" throttle for the stale/foreign dashboard-cookie log line was defeated, and every request carrying a stale cookie (e.g. a background dashboard tab, or a Cursor CLI token riding along) logged a warning. Restores module scope so the warning fires once per process again, same pattern documented for diegosouzapw#13684 (LEDGER-12). Adds a red-green regression test: reverting the fix reproduces 3 warnings across 3 requests with a foreign cookie; with the fix, 1 warning. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
|
Both review comments addressed; the branch is fixed and verified locally.
Files applied (13, matching PR head 26a6f89): 11 byte-identical to the PR head. Verification: |
Resolve additive conflict in tests/unit/authz/pipeline.test.ts (keep the PR's stale-cookie throttle test and the tip's remote-dashboard session test) and prettier-format the long DASHBOARD_SESSION_COOKIE imports.
2 participants
Closes #13377.
Mechanical auth hygiene follow-ups from the omni-review closeout final reviews (PR #13375 SEC-A). No behaviour change for valid sessions; all covered by existing tests.
12 files changed, 82 insertions(+), 75 deletions(-).
Auth (from #13375)
src/server/authz/pipeline.ts— deleted the now-unreachableisStaleDashboardJwtError()and the module-levelstaleDashboardJwtWarningEmitted; the one-time "Dropped stale dashboard session cookie" warning now fires in the!payloadbranch ofrefreshDashboardSessionIfNeeded(L126-142). The refresh minter now uses the sharedgetDashboardJwtSecret()(L126) instead of a localgetJwtSecret(), and every cookie read/write/delete goes through the exportedDASHBOARD_SESSION_COOKIEconstant (L129 read, L139 delete, L158 set, L171 delete). The catch block just logs (L165) — the stale-error branch is gone.src/app/api/auth/login/route.ts— importgetDashboardJwtSecret(L18); the minter signs withgetDashboardJwtSecret()!(L163), removing the localgetJwtSecret().src/app/api/auth/oidc/callback/route.ts— import (L7); resolveconst secret = getDashboardJwtSecret()(L200) and gate the redirect on it before signing (L214).src/shared/utils/dashboardSessionToken.ts—verifyDashboardSessionTokennow rejects an emptyUint8Array(secret.length === 0, L35) and pins{ algorithms: ["HS256"] }onjwtVerify(L37).src/lib/ws/handshake.ts—hasValidSessionCookieusesgetDashboardJwtSecret()(L39) andDASHBOARD_SESSION_COOKIE(L42) instead of encoding the raw env value, so mint and verify agree.src/server/ws/liveServer.ts— live-dashboard WS cookie read viaDASHBOARD_SESSION_COOKIE(L192).src/shared/utils/apiAuth.ts—isDashboardSessionAuthenticatedreads the cookie viaDASHBOARD_SESSION_COOKIEacross all three fallbacks (L228-242).src/app/api/auth/status/route.ts— cookie read via constant (L13).src/app/api/settings/require-login/route.ts— cookie read via constant (L22).tests/unit/dashboard-session-verifier-source-guard.test.ts— the allowlist guard is now repo-wide: it walks everysrc/**/*.tsand fails if any non-allowlisted file callsjwtVerify(allowlist at L22, scan at L46-52,findTsFilesat L57). A NEW cookie consumer is caught, not only regressions in the six known files.tests/unit/dashboard-session-token-13298.test.ts— env mutation wrapped intry/finally(L38-43) soJWT_SECRETis always restored.docs/architecture/AUTHZ_GUIDE.md— "route guard" → "dashboard route guard (isDashboardSessionAuthenticated())" (L40).Verification
dashboard-session-verifier-source-guard+dashboard-session-token-13298npm run typecheck:coreRelated PRs
deleteCompletedBatches's twofor (;;)loops, stacked on fix(db): owner-scoped file half and chunked instance sweep for DELETE /v1/batches/delete-completed (SEC-C/SEC-D, omni-code-sec on #12969) #13374. This is the Batches half of chore(auth,db,skills): hygiene follow-ups from the omni-review closeout final reviews (#13375, #13374) #13377, in the place the issue actually describes.fix/batches-sweep-file-ownership-chunks: chunked instance sweep + owner-scoped file half (base of fix(db): forward-progress guard in both deleteCompletedBatches chunk loops (#13377) #13570).CONFLICTINGand needs a rebase onto the currentrelease/v3.8.51before the batches stack can land.node:fsin the client chunk graph); unrelated to chore(auth,db,skills): hygiene follow-ups from the omni-review closeout final reviews (#13375, #13374) #13377.Not applicable
The issue's skills bullets (
ledger.mjs,wf-native.mjs,render-jobs.test.mjs,wf-native.test.mjsunder.agents/skills/_shared/omni-review/) belong to a separate skills repository; those files are not present in this repo.