fix(security): require auth on GET /api/v1/reauth and stop leaking SAT in body

[birme]

Summary

• Add requireReAuth guard to GET /api/v1/reauth (mirrors requireWhipAuth): Bearer header with constant-time timingSafeEqual, 401 + WWW-Authenticate: Bearer realm="reauth". Auth is disabled when no key is configured so existing installs keep working.
• Configurable via REAUTH_AUTH_KEY, falling back to WHIP_AUTH_KEY.
• Defense in depth: stop returning the raw OSC service access token in the JSON response body ({ ok: true }); the httpOnly cookie remains the sole delivery path.
• Warn at startup when /reauth is effectively unauthenticated (incl. whitespace-only key), so auth-off-by-default is never silent.
• Adds test coverage for 401 paths (missing/empty/malformed Bearer) and the no-token-in-body behaviour.

Test plan

• Tests pass (npm test)
• TypeScript compiles (npm run typecheck)
• Lint clean (npm run lint)
• GET /api/v1/reauth with no/invalid Bearer returns 401 when REAUTH_AUTH_KEY/WHIP_AUTH_KEY is set
• Response body no longer contains the token value; sat cookie is still set
• Startup warning logged when a token is configured but no effective reauth key is set

Closes #264

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com

[birme]

Code Review

Verdict: Needs Changes

Summary: The security fix itself is well-executed and faithfully mirrors the established requireWhipAuth pattern (constant-time timingSafeEqual, 401 + WWW-Authenticate, httpOnly cookie as the sole token delivery path, no raw SAT in the JSON body). However, this PR cannot merge as-is because it has a merge conflict with main, and there is one Blocking test-hygiene gap. Rebase and address the log mock before merge.

---

Blocking

• Merge conflict with main — gh pr view reports mergeable=CONFLICTING, mergeStateStatus=DIRTY. The branch must be rebased/merged against main and conflicts resolved before this can land, independent of code quality. This is the primary blocker.
• src/api_re_auth.test.ts — The test file does not include the required jest.mock('./log', ...) mock mandated by the project testing rules for every backend test file. Per criteria, a test file missing this mock pollutes output and risks flaky timing behavior. Add:

  jest.mock('./log', () => ({
    Log: () => ({ info: jest.fn(), error: jest.fn(), debug: jest.fn(), warn: jest.fn() })
  }));

---

Warnings

• src/api_re_auth.ts:24 — requireReAuth(request: any, reply: any) uses bare any for both parameters without a justifying comment. This mirrors the existing requireWhipAuth in api_whip.ts:58, so it is a consistency-preserving copy rather than a new regression, but it defeats TypeScript on a security-critical path. Prefer FastifyRequest / FastifyReply. (If left as-is to match WHIP, add a one-line comment noting the intentional parity.)
• src/server.ts:17-27 — The new startup SECURITY warning (unauthenticated /reauth) has no test coverage. The PR adds a genuinely new behavior (env-driven warning, incl. the whitespace-only-key branch); an assertion that the warning fires when OSC_ACCESS_TOKEN is set but no effective key is present would guard this. Not a hard blocker, but recommended given this is the safety net for auth-off-by-default.

---

Suggestions

• src/api_re_auth.ts:37-40 — The token.length === key.length short-circuit before timingSafeEqual technically leaks key length via timing. This is a negligible, industry-standard tradeoff (and identical to the WHIP implementation), so it is fine to keep — noting only for completeness.
• src/api_re_auth.ts — requireReAuth is async but performs no awaited work; it could be a synchronous helper. Minor; parity with requireWhipAuth justifies leaving it.
• Test coverage is otherwise strong: 401 for missing/empty/malformed/prefix-of-key Bearer, fetch not called on reject, no token in the 200 body, cookie still set, and the unauthenticated-when-no-key path. Good regression coverage for #264. (Verified the 200-path tests are valid because jest.setup.js sets OSC_ACCESS_TOKEN='foo' globally.)

---

Domain Note

This change touches the OSC service-access-token (SAT) reauth lifecycle. The auth mechanics are standard and low-risk, so no intercom-expert consult is required, but confirm downstream frontend callers of /api/v1/reauth no longer read token from the response body (they must now rely on the cookie).

---

CI is green (lint, prettier, typecheck, unittests all SUCCESS), but green CI does not clear the merge conflict.

Next steps: rebase onto main to clear the conflict, then pass Blocking items to bug-fixer → once resolved, use pr-author to update the PR.

[birme]

Code Review

Verdict: Needs Changes

Summary: The security fix itself is well-implemented — it closes #264 by removing the raw SAT from the JSON body and adds a constant-time Bearer auth guard that faithfully mirrors the established requireWhipAuth pattern, with solid regression tests. However, the PR is currently CONFLICTING with main and cannot merge until rebased. That merge conflict is the sole blocker; the remaining items are warnings/suggestions.

---

Blocking

• Merge conflict with base branch (main) — GitHub reports mergeable=CONFLICTING / mergeStateStatus=DIRTY. This must be resolved by rebasing the branch (264-reauth-endpoint-auth) onto the latest main and pushing before the PR can merge. Please re-request review after the rebase so the final merged diff can be re-verified (in particular src/api.ts and src/server.ts, which are the likely conflict sites where the new reAuthKey option is threaded through ApiOptions).

---

Warnings

• src/api_re_auth.test.ts — This test file does not jest.mock('./log', ...). Project convention requires every backend test file to mock ./log. In practice this file does not currently exercise a Log() call path (that lives in server.ts), and the omission is pre-existing rather than introduced by this PR, so it is not a blocker — but since you are already expanding this file substantially, adding the standard mock now would bring it into line with the rest of the suite and avoid future output pollution.
• src/api_re_auth.ts:23 — requireReAuth(request: any, reply: any) uses any for both parameters. This is copied verbatim from requireWhipAuth in api_whip.ts, so it is a consistent (not new) deviation, but the any casts defeat type-checking on the request/reply. Consider typing these as FastifyRequest/FastifyReply. If you keep any to stay symmetric with requireWhipAuth, add a one-line comment noting the intentional parity.

---

Suggestions

• src/api_re_auth.ts:32 — request.headers['Authorization'] (capitalized) is dead code: Node/Fastify normalize all incoming header names to lowercase, so this branch of the || never matches. It is harmless and mirrors requireWhipAuth, but the fallback could be dropped for clarity.
• src/api_re_auth.ts:22 — Good call trimming reAuthKey (opts.reAuthKey?.trim()) so a whitespace-only key is treated as unset; this correctly aligns with the whitespace-only branch of the startup warning in server.ts. Worth a brief inline comment tying the two together.
• src/server.ts:17-27 — The SECURITY: startup warning is a nice defense-in-depth touch and correctly distinguishes the undefined vs. whitespace-only cases. No change needed.
• Tests — Coverage is strong: 401 on missing/empty/malformed/wrong Bearer, the proper-prefix timing case, the success path asserting { success: true } with token absent and the sat cookie present, the unauthenticated-when-unconfigured path, and the 500 token-service-unavailable path. jest.setup.js sets OSC_ACCESS_TOKEN='foo' globally, so the fetch-path tests correctly reach the token service rather than short-circuiting to 405. This exercises the exact failure path from #264.

---

Domain Note

This change touches the /reauth OSC service-access-token lifecycle. It does not alter WHIP/WHEP session handling or audio routing, so no intercom-expert consultation is required.

Next steps: resolve the Blocking merge conflict by rebasing on main, then re-request review. The warning items can be passed to bug-fixer if you choose to address them.

[birme]

daily-backlog-pr: code-reviewer verdict Needs Changes — the PR is currently CONFLICTING with main (must be rebased) plus minor items (missing jest.mock('./log'), any-typed handler params, dead capitalized-header fallback). Moving the tracking issue #264 back to Ready.

[birme]

Code Review

Verdict: Needs Changes

Summary: The core security intent is sound and well-executed: /api/v1/reauth now requires a Bearer token compared with timingSafeEqual, the SAT is no longer leaked in the response body (only set as an httpOnly/secure/sameSite=strict cookie), and there is strong test coverage including timing-prefix and malformed-header cases. However there are two Blocking issues (untyped any handler params against project TS rules, plus this branch currently conflicts with main and cannot merge), and a cluster of warnings. Per the review rules, the merge-conflict Blocking + the warning cluster => Needs Changes.

Note: This branch is in a CONFLICTING merge state with main (conflicts in src/api.ts and src/api_re_auth.ts). This must be resolved before merge. Reviewed on its own merits per the orchestrator's instruction.

---

Blocking

• src/api_re_auth.ts:24 — requireReAuth(request: any, reply: any) uses bare any for both parameters with no justifying comment. Project TS rule (category 1) forbids unjustified any. Use the Fastify types: (request: FastifyRequest, reply: FastifyReply). This also removes the need for the defensive startsWith?. / typeof authHeader !== 'string' guards, since request.headers['authorization'] is typed as string | string[] | undefined.
• Branch conflicts with main — src/api.ts and src/api_re_auth.ts both conflict on merge. Must be rebased/resolved before this can land. (Flagged per orchestrator instruction; resolution handled separately.)

---

Warnings

• src/api_re_auth.ts:30 — request.headers['Authorization'] is dead code. Fastify (via Node's http parser) always lowercases incoming header names, so the || request.headers['Authorization'] branch can never fire. Harmless but misleading; drop it.
• src/api_re_auth.ts:29-35 — Header value can legitimately be string[] (duplicate Authorization headers). With the current any typing, authHeader.startsWith would be undefined for an array and .slice on the raw header is not reached, so it falls through to empty-string token => 401. That is safe, but once properly typed as FastifyRequest you must handle the string[] case explicitly rather than relying on any coercion.
• src/api_re_auth.test.ts:1-20 — This test file does not mock ./log (jest.mock('./log', ...)). Category 5 requires every backend test file to mock ./log to avoid output pollution and flaky timing. This is pre-existing, but this PR substantially rewrites the file and should fix it while here. Every other *.test.ts in src/ mocks it.
• src/api_re_auth.ts:86 — The fetch to the ServiceToken service has no timeout (consistent with existing code, but category 7 flags new/uncovered outbound calls that can block indefinitely if the upstream hangs). Since the surrounding retry loop is being touched, consider adding an AbortSignal.timeout(...) so a hung token service does not tie up the request for the full retry window with no upper bound on each attempt.

---

Suggestions

• src/api_re_auth.ts:22 — Good call trimming reAuthKey and treating empty/whitespace as "auth disabled"; the matching startup warning in server.ts (differentiating undefined vs empty/whitespace) is a nice operator-facing touch.
• src/api_re_auth.ts:41 — Minor: isValid already implies a well-formed string token, so the trailing !authHeader || typeof authHeader !== 'string' in the same condition is redundant once types are correct. Simplify to if (!isValid).
• src/models.ts:341 — The ReAuthResponse schema change to { success: boolean } with a description noting the token is only returned via httpOnly cookie is clear and correct — good documentation of the security posture.
• readme.md — REAUTH_AUTH_KEY doc entry is clear, including the fallback-to-WHIP_AUTH_KEY behavior and the recommendation to set it when OSC_ACCESS_TOKEN is present. No yarn references reintroduced; npm hygiene clean.

---

Domain Note

This change touches auth on the SAT re-issuance path (adjacent to WHIP/WHEP auth via the shared WHIP_AUTH_KEY fallback). No audio routing / PTT / dominant-speaker / data-channel logic is affected. The token cookie name (eyevinn-intercom-manager.sat) and Bearer prefix format are relied on by the frontend/session flow — worth confirming the frontend no longer reads token from the response body. Consider consulting the intercom-expert agent if the SAT cookie is consumed by WHIP/WHEP session establishment.

---

Next steps: pass Blocking items to bug-fixer (fix any typing; resolve the main merge conflict) → once resolved, use pr-author to update the PR.

[birme]

Automated pipeline (Phase 2 conflict-resolution): I attempted to rebase this branch onto main to clear the CONFLICTING/DIRTY state, but the conflict is a premise disagreement, not a mechanical one, so I stopped rather than pick a side.

main now carries an explicit directive in src/api_re_auth.ts and src/api.ts:

Do not add an in-process authentication layer to this route. An extra auth level conflicts with the OSC auth wall and breaks current deploys and installations. See #283 (comment)

That decision (via #283) directly contradicts what this PR / #264 implements (bearer auth on GET /api/v1/reauth). Resolving the conflict either way is a security-design call a maintainer should make, not something to auto-resolve. Routing to a human: please decide whether #264 is still wanted given #283, and either close this as superseded or provide direction on how in-process auth should coexist with the OSC auth wall.

[birme]

Automated daily-backlog-pr run: this PR is CONFLICTING against main, and the conflict is not mechanical. main now carries an explicit, documented decision not to add an in-process auth layer to GET /api/v1/reauth:

"Do not add an in-process authentication layer to this route. An extra auth level conflicts with the OSC auth wall and breaks current deploys and installations." (see src/api_re_auth.ts and PR #283, comment 5231121365)

This PR's premise (issue #264 — require bearer auth on /reauth) directly contradicts that merged decision. This is a disagreement about the issue's premise rather than a rebase I can safely resolve, so I'm not force-resolving it. A human maintainer should decide whether #264 is still wanted given the #283 decision (likely close as won't-fix), or whether the #283 decision should be revisited. Leaving the board item in Ready, unassigned, pending that call.

[birme]

Reviewed by daily-backlog-pr (Phase 2 re-entry). This PR is now conflicting with a documented design decision on main, not just mechanically out of date.

Since this PR was opened, main gained an explicit block comment on the /reauth route (src/api_re_auth.ts) stating:

Do not add an in-process authentication layer to this route. An extra auth level conflicts with the OSC auth wall and breaks current deploys and installations. See #283 (comment)

This PR's whole change — requiring bearer auth on GET /api/v1/reauth — is exactly what that decision rules out. This is a disagreement about the issue's premise, not an actionable rebase, so this automated pass is not force-resolving the conflict or pushing changes.

A human maintainer should decide whether issue #264's premise still holds given the #283 decision, and either close this PR or reconcile the two positions. Leaving the branch untouched.

[birme]

Automated daily-backlog-pr pass: this PR is CONFLICTING against main, but the conflict is not mechanical — it's a disagreement about the issue's premise, so this run is deliberately not auto-rebasing/force-pushing it.

main now carries an explicit block comment in src/api_re_auth.ts (and at the apiReAuth registration in src/api.ts) stating that /api/v1/reauth must not have an in-process authentication layer, because an extra auth level conflicts with the OSC auth wall and "breaks current deploys and installations" (ref: #283 (comment)). This PR does the opposite — it adds requireReAuth bearer-token gating to that same route.

Resolving the conflict either way is a product/security decision (rely on the OSC auth wall vs. add in-process bearer auth), not something this automation should decide by picking a side. Routing to a human maintainer: please decide whether issue #264's premise still stands given #283, and either close this PR or reconcile it with the documented decision. Leaving the board item in Ready and pushing nothing.

[birme]

Picked up by today's daily-backlog-pr run. This PR is CONFLICTING against main, but the conflict is a premise disagreement, not a mechanical one, so this run is deliberately not force-resolving it and is routing it to a human instead.

Since this branch was cut, main gained an explicit, documented decision (see src/api_re_auth.ts block comment referencing #283 / #283 (comment)) stating that authentication is deliberately out of scope for GET /api/v1/reauth:

Do not add an in-process authentication layer to this route. An extra auth level conflicts with the OSC auth wall and breaks current deploys and installations.

PR #293 adds exactly that in-process bearer-auth layer (reAuthKey / requireReAuth), which directly contradicts the decision now recorded on main. Automatically resolving the conflict either way would either silently discard the security change or override a deliberate architectural decision — neither is appropriate for an automated pass.

A maintainer should decide:

1. Whether the OSC auth-wall rationale supersedes issue Security: /reauth endpoint returns raw OSC service access token in JSON response body #264's auth ask (if so, close the auth portion), and
2. Whether the other half of Security: /reauth endpoint returns raw OSC service access token in JSON response body #264 — not returning the raw OSC service access token in the JSON response body — should be split into its own PR, since that hardening is independent of the (contested) in-process-auth change.

Leaving issue #264 in Ready and this PR open for that human decision.