Skip to content

feat: add API key authentication to management endpoints - #397

Merged
birme merged 3 commits into
mainfrom
backend/add-api-key-auth-222
Oct 7, 2026
Merged

birme merged 3 commits into
mainfrom
backend/add-api-key-auth-222

Conversation

@birme

@birme birme commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • All /api/v1/ management endpoints were unauthenticated; any client with network access could create/modify/delete productions, sessions and presets. Adds an opt-in API key guard.
  • Adds src/auth.ts exporting a requireApiKey Fastify preHandler hook: reads process.env.API_KEY; when unset/empty, auth is disabled (dev mode, preserves current behaviour and all existing tests); otherwise requires Authorization: Bearer <API_KEY> and replies 401 { error: 'Unauthorized' } on absence/mismatch.
  • Uses a constant-time comparison (timingSafeEqual) and a generic 401 body; the key is read only from process.env and is never logged.
  • Applies the hook to POST/PATCH/DELETE /production, POST/DELETE /session, GET/POST/PATCH/DELETE /preset and POST /ingest; WHIP/WHEP routes are left unchanged (they keep their own WHIP_AUTH_KEY auth).
  • Adds src/auth.test.ts (7 tests) covering disabled/missing/wrong/correct key behaviour.

Test plan

  • Tests pass (npm test)
  • TypeScript compiles (npm run typecheck)
  • Lint clean (npm run lint)
  • auth disabled when API_KEY unset; 401 on missing/bad key; 200/handler runs on correct key; WHIP/WHEP unaffected

Closes #222

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.8 [email protected]

All /api/v1/ management endpoints were unauthenticated, letting any client
with network access create, modify or delete productions, sessions and
presets. Add an opt-in API key guard applied to the management routes.

- Add src/auth.ts exporting a `requireApiKey` Fastify preHandler hook that
  reads process.env.API_KEY; when unset/empty auth is disabled (dev mode,
  preserves current behaviour), otherwise requires an
  `Authorization: Bearer <API_KEY>` header and replies 401 on absence/mismatch.
- Use a constant-time comparison (timingSafeEqual) and a generic 401 body; the
  key is read only from env and never logged.
- Apply the hook to POST/PATCH/DELETE /production, POST/DELETE /session,
  GET/POST/PATCH/DELETE /preset and POST /ingest.
- Leave WHIP/WHEP routes untouched (they keep their own WHIP_AUTH_KEY auth).
- Add src/auth.test.ts covering disabled/missing/wrong/correct key cases.

Closes #222

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@birme

birme commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

code-reviewer verdict: LGTM (automated self-review by a separate code-reviewer invocation; recorded as a marker because GitHub blocks state-bearing self-review when author and reviewer are the same account).

Independent reviewer re-ran the suite in a fresh clone: typecheck clean, lint 0 errors, 438/438 tests pass, Prettier-clean. Routing audit confirmed the requireApiKey preHandler lands only on the management routes #222 lists and that the WebRTC connection/handshake routes (GET lists/detail/line, PATCH /session/:sessionId SDP answer, heartbeat, participants long-poll) and WHIP/WHEP stay open. Constant-time key comparison, generic 401 body, disabled-when-unset preserves current behavior — no auth-bypass.

Non-blocking warnings for a possible follow-up (not in #222's scope): the production-line mutation routes (POST/PATCH/DELETE /production/:id/line) and .../participants/:sessionId/disconnect could also be guarded for consistency.

@birme

birme commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

code-reviewer verdict: NEEDS CHANGES (automated self-review; recorded as a marker because GitHub blocks state-bearing review when author and reviewer are the same account).

Build/typecheck/lint clean and all 446 tests pass, but the PR does not yet meet issue #222: four production-mutating management endpoints are left unauthenticated, so an unauthenticated caller can still create/modify/delete production lines and force-disconnect participants.

Blocking

  • src/api_productions.ts:471 — POST /production/:productionId/line has no preHandler: requireApiKey.
  • src/api_productions.ts:589 — PATCH /production/:productionId/line/:lineId is unprotected.
  • src/api_productions.ts:657 — DELETE /production/:productionId/line/:lineId is unprotected.
  • src/api_productions.ts:1070 — POST /production/:productionId/line/:lineId/participants/:sessionId/disconnect is unprotected.

Warnings

  • PATCH /session/:sessionId (:820) is unguarded while POST/DELETE /session are guarded — make the /session lifecycle consistent, or decide whether /session is a client vs management API (guarding POST /session would force the API key into every browser).
  • No startup SECURITY warning when API_KEY is unset — the hook silently disables auth (the default), with no operator signal, unlike the existing WHIP/WHEP/reauth UNAUTHENTICATED logs.
  • src/api_ingests.ts:39 — the route-level guard is dead code: a global preHandler 501-gates all ingest routes first.

Suggestions

  • Add an integration test (server.inject()) asserting the real registered routes reject unauthenticated requests with API_KEY set — this would have caught the coverage gap above.

Addressing the four Blocking endpoints (plus the session/startup warnings and an integration test) and re-review will clear this.

… requireApiKey (#222)

A code review found the API key guard was not applied to several
production-mutating endpoints. Apply requireApiKey to them and close the
gaps:

- POST   /production/:productionId/line
- PATCH  /production/:productionId/line/:lineId
- DELETE /production/:productionId/line/:lineId
- POST   /production/:productionId/line/:lineId/participants/:sessionId/disconnect

Also guard PATCH /session/:sessionId so the /session lifecycle is
consistent with the already-guarded POST/DELETE /session management
endpoints.

Add a startup SECURITY warning in server.ts when API_KEY is unset,
mirroring the existing WHIP/WHEP/reauth UNAUTHENTICATED warnings, so
operators get a signal that management auth is disabled.

Note the dead-code route-level guard in api_ingests.ts as unreachable
(the global 501 gate short-circuits every ingest route first); kept
rather than removed to avoid scope creep.

Add an integration test (api_auth_guard.test.ts) that registers the real
routes with API_KEY set and asserts each newly-guarded endpoint rejects
an unauthenticated request with 401.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@birme

birme commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

code-reviewer verdict: LGTM (automated self-review; recorded as a marker because GitHub blocks state-bearing review when author and reviewer are the same account — reviewed by a separate code-reviewer invocation, not the implementer).

Reviewed at head f44bf82b5a7793152565cdde9bce3228f457f3b1. The primary gate for #222 is fully met: every production/session/preset/participant-disconnect mutation endpoint is now guarded by requireApiKey, read endpoints remain open, the frontend already carries the Bearer token so nothing is over-guarded, and the new server.inject() integration test meaningfully asserts unauthenticated rejection (401) plus a valid-key pass. CI green (lint/pretty/ts/unittests).

Out-of-scope follow-up (not blocking #222): two unauthenticated mutating /api/v1/share endpoints remain (POST /api/v1/share, DELETE /api/v1/share/:id). Tracked separately so this PR is not held on work outside its stated scope.

@birme
birme merged commit 0aff51e into main Oct 7, 2026
4 checks passed
@birme
birme deleted the backend/add-api-key-auth-222 branch October 7, 2026 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: Add authentication to production/session/preset management API endpoints

2 participants