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.