feat(auth): require Bearer token auth on /api/* endpoints (ENG-1671)

[limaronaldo]

Summary

• Adds packages/api/src/core/auth.ts: Bearer-token authentication middleware for all /api/* endpoints
• Tokens validated against MULTIPLAI_API_KEYS (CSV) or MULTIPLAI_API_KEY, using constant-time comparison (SHA-256 digest + crypto.timingSafeEqual) to avoid timing side channels
• Fail-closed in production: returns 503 when no API key is configured; in non-production logs a single warning and allows requests (dev ergonomics)
• Public paths preserved: /api/health, /webhooks/github, /docs, /redoc, /openapi.json
• SSE /api/logs/stream and WS /api/ws/tasks may also authenticate via ?token= query param (EventSource/WebSocket cannot set headers)
• Wired into handleRequest in router.ts before route dispatch; 401 responses get CORS headers

Testing

• bun test on core/auth.test.ts: 21 pass, 0 fail
• bun run typecheck: only pre-existing error at router.ts (Property 'count' does not exist on type 'any[]') — confirmed present on main via git stash before/after comparison; no new type errors introduced

Notes

• Closes ENG-1671
• No merge performed (per session mandate: merge is prohibited)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 01c861fe46

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Authenticate upgrades before calling server.upgrade

The actual Bun request flow upgrades /api/ws/tasks in src/index.ts before calling handleRequest; when server.upgrade succeeds, it returns immediately and never reaches this middleware. Consequently, a client with no token or an invalid token can still open the production WebSocket and receive task events. Apply the authentication check in the pre-upgrade branch or ensure upgrades pass through the authenticated router first.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Provide credentials for the first-party web client

When either API-key variable is configured, this rejection path makes the existing first-party web application unusable: its requests in packages/web/src use plain fetch without an Authorization header, and services/sse.service.ts opens /api/logs/stream without the supported token query parameter. Dashboard reads, task actions, settings, plans, and live updates therefore receive 401 responses. Add a secure client authentication/session or proxy mechanism and wire both fetch and SSE requests through it rather than enabling the guard without a credential source.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Declare bearer authentication in the OpenAPI document

This global authentication requirement is not reflected in core/openapi.ts: the published description still says most endpoints are public, and the only declared security scheme is webhookSignature. With API keys enabled, Swagger UI's try-it requests and clients generated from /openapi.json omit the bearer token and receive 401 responses. Add an HTTP bearer scheme and apply it globally or to protected operations, explicitly exempting the public endpoints.

Useful? React with 👍 / 👎.

[limaronaldo]

Review: PR #425 — ENG-1671, Bearer token auth on /api/*

Verdict: CHANGES-NEEDED (1 BLOCKER, 2 HIGH, 2 MEDIUM, 1 LOW)

Reviewed via gh pr diff/gh api (no clone/checkout), 3 independent passes (Security, Correctness, Tests/Quality) plus an external cross-model review via codex exec -m gpt-5.6-terra --sandbox read-only. Codex independently reproduced the BLOCKER finding and surfaced 2 additional HIGH findings that were folded into this consolidation.

Findings

Sev	File:Line	Finding	Suggested fix
BLOCKER	packages/api/src/index.ts (pre-existing, not in diff) + packages/api/src/router.ts:7780	Bun.serve's fetch handler in index.ts special-cases GET /api/ws/tasks WebSocket upgrades and calls server.upgrade() before handleRequest() runs. authMiddleware() is only invoked inside handleRequest() (router.ts:7780), so it never executes for real WS upgrade traffic. The PR's own claim ("?token= authenticates WS") and its passing test (auth.test.ts, "WS upgrade path accepts ?token=") only exercise authMiddleware() in isolation against a synthetic Request — they never call through index.ts's actual fetch callback, so the bypass is invisible to the test suite. Net effect: /api/ws/tasks is unauthenticated in production despite this PR.	Move the WS-upgrade special case in index.ts to run authMiddleware(req) (or an equivalent check) before calling server.upgrade(), and reject with a close/deny if it fails. Add an integration test that drives the real fetch handler (or a Bun.serve-backed test server) for an unauthenticated upgrade attempt and asserts it is rejected.
HIGH	packages/api/src/core/auth.ts (fail-open branch, ~line 124 in file / diff line 313)	When no API keys are configured (getConfiguredApiKeys().length === 0), the middleware fails closed only when process.env.NODE_ENV === "production"; any other value (unset, "staging", a typo, "prod" instead of "production") falls through to fail-open, allowing all /api/* requests with only a console.warn. Given NODE_ENV is commonly unset or misconfigured in containerized/deploy environments, this is a realistic full-auth-bypass path, not just a dev convenience.	Invert the check to an explicit allowlist: fail-closed by default, and only fail-open when a distinct, explicit dev flag is set (e.g. MULTIPLAI_ALLOW_UNAUTHENTICATED_API=true), independent of NODE_ENV string matching. At minimum, fail-closed on any NODE_ENV other than a small explicit dev/test set.
HIGH	packages/api/src/core/auth.ts:141-142 (diff lines 330-331), QUERY_TOKEN_PATHS	/api/logs/stream and /api/ws/tasks accept the raw, long-lived MULTIPLAI_API_KEY/MULTIPLAI_API_KEYS value via ?token= query string. Query strings are commonly captured in reverse-proxy/access logs, APM/monitoring tools, and browser history — this leaks a reusable, non-expiring credential outside the Authorization header's usual handling.	Issue a short-lived, scope-limited token (e.g. signed, single-purpose, expiring in seconds/minutes) for the SSE/WS handshake instead of accepting the primary API key verbatim in the URL.
MEDIUM	packages/api/src/core/auth.test.ts line ~169 (diff line 175), "WS upgrade path accepts ?token="	This test name/assertion implies WS auth is enforced end-to-end, but it only calls authMiddleware() directly — it does not exercise index.ts's fetch/server.upgrade() path. As written, this test will stay green even after the BLOCKER above is fixed or reintroduced, giving false confidence.	Rename to clarify it tests only the middleware function in isolation (e.g. "authMiddleware accepts ?token= for WS path pattern"), and add the integration-level test recommended in the BLOCKER fix.
MEDIUM	packages/api/src/core/auth.ts:280-288, authNotConfiguredResponse()	Returns HTTP 503 with body {"error":"Service Unavailable","message":"auth not configured"} for the "no keys configured, production" case. Semantically defensible (503 = server not correctly provisioned to serve the request) rather than 403 — this is fine, but flagging for explicit confirmation since the task brief called out 401-vs-403 semantics: no correctness issue found here, 401 is correctly used for actual invalid/missing-token cases and 503 for the misconfiguration case. No action needed; documenting as a verified-correct item.
LOW	packages/api/src/core/auth.test.ts (no line — coverage gap)	No test covers: multiple/duplicate Authorization headers, a header value with only "Bearer" and no token, or Authorization header containing embedded newlines/extra whitespace beyond a single space. Low risk given extractBearerToken's regex (^Bearer\s+(.+)$/i) is reasonably strict, but worth covering given this is new security-critical surface.	Add 2-3 negative tests for malformed/edge-case Authorization header values.

Pass notes

SEGURANÇA (Security). Constant-time comparison is implemented correctly: both sides are SHA-256-hashed to fixed-length digests before timingSafeEqual (auth.ts:238-260), which avoids the common pitfall of timingSafeEqual throwing (or being skipped) on length-mismatched inputs — this specifically prevents a length-based timing oracle. No hardcoded secrets found in the diff; keys are sourced only from MULTIPLAI_API_KEYS/MULTIPLAI_API_KEY env vars. No token values are logged (the "no keys configured" warning does not include any token/key material). /webhooks/github is correctly left outside the /api/* guard (untouched, confirmed via router.ts inspection) since it's a different path prefix and has its own signature-based auth. CORS: addCorsHeaders() is applied to 401/503 auth-failure responses in router.ts (confirmed in full router.ts read, lines ~7776-7801), consistent with the PR's stated behavior; since auth here is Bearer-header/token based (not cookie-based), a permissive CORS origin does not itself create a CSRF/credential-leak vector — no issue found. The two BLOCKER/HIGH findings above (WS bypass, NODE_ENV fail-open default, token-in-URL) are the substantive security gaps.

CORREÇÃO (Correctness). 401 is used for missing/invalid tokens, 503 for "not configured in production" — both semantically appropriate, no 403 usage needed since this is authentication (who you are), not authorization (what you're allowed to do). Edge cases are well handled in the unit-level middleware: case-insensitive Bearer scheme (verified in test + regex /i flag), case-sensitive /api/ path-prefix match (explicit test + code comment), and dot-segment normalization relies on the URL API resolving ../. before matching (verified test: /api/../api/tasks → guarded). /api/health and non-/api paths (/webhooks/github, /docs, /redoc, /openapi.json) are correctly exempted. No regressions found in existing route registration or dispatch order — authMiddleware is inserted after the pre-existing rateLimitMiddleware check and before route dispatch, which is the correct position. The one real correctness/compatibility gap is the WS bypass (BLOCKER above): internal WS clients that expect the new auth to apply will observe it silently not applying.

TESTES/QUALIDADE (Tests/Quality). 21 tests added in auth.test.ts, and they cover the middleware's own logic thoroughly: key sourcing (single vs CSV, trimming, empty), token validation (correct, wrong, wrong-length, CSV membership, no-keys-configured), and middleware behavior (401/503/allow paths, case sensitivity of scheme and path, dot-segment normalization, production fail-closed vs non-production fail-open, SSE/WS query-token accept/reject, query-token NOT accepted on regular API paths). No flakiness concerns — tests are synchronous, deterministic, and properly reset env/global state via beforeEach/afterEach. The critical gap is structural, not a missing assertion: every test calls authMiddleware() directly against a synthetic Request, so none of them exercise the actual Bun.serve fetch callback in index.ts — this is precisely why the WS-bypass BLOCKER survives a fully green test suite (see MEDIUM finding above).

Codex (external cross-model reviewer, gpt-5.6-terra, read-only sandbox). Ran to completion (no timeout). Verdict: FAIL — Production WebSocket upgrades bypass the new authentication gate, and the configuration default can expose every API route. Codex independently identified the same BLOCKER (WS bypass via index.ts/router.ts:7780) without being told the answer — it was given only the factual context that the WS special-case exists and asked to assess impact, and it correctly reasoned through to the same architectural conclusion. It additionally surfaced both HIGH findings above (NODE_ENV fail-open default, API key in query string) independently of my own passes, which were merged into the table (deduplicated — same underlying issues, consistent file:line citations after re-verification against the diff). It confirmed the same positive findings: case-insensitive Bearer, fixed-length constant-time comparison, no hardcoded secrets/token leakage in logs or errors.

Consolidation / validation notes

All findings above were re-confirmed against /tmp/pr425-full.diff (the untruncated diff — an initial gh pr diff fetch was silently compacted by output filtering and had to be re-fetched raw) and, for the BLOCKER, against a direct read of packages/api/src/index.ts (pre-existing file, not part of this diff, confirmed via gh api that it was last touched by an unrelated prior PR #410 and has zero references in this PR's diff). No candidate finding was discarded as a false positive in this round — Codex's independent findings all corroborated or extended (not contradicted) the manual passes.

[limaronaldo]

Addressed the blocker and both HIGHs identified in review, in f615c48:

BLOCKER — WS upgrade unauthenticated (fixed)
authMiddleware() now runs before server.upgrade() in index.ts. Previously the WS upgrade path bypassed handleRequest()/authMiddleware() entirely once server.upgrade() was called, so /api/ws/tasks was reachable without credentials in production. Added createFetchHandler() (extracted from main()) plus a new index.test.ts that drives the real Bun.serve fetch handler — not just authMiddleware() in isolation — covering: 401 with no token, 401 with an invalid token, 503 fail-closed with no key configured, a full successful WS handshake with a valid ?token=, and a rejected handshake with no token (verifies the client actually sees close/error, not open).

HIGH — fail-open when no keys configured (fixed)
No API keys configured now fails closed (503) regardless of NODE_ENV, except when explicitly opted into via the documented dev/test escape hatches. Confirmed via the new integration test's "no key configured" case.

HIGH — ?token= query-string auth (kept, documented, follow-up recommended)
Kept because EventSource/WebSocket clients can't send custom headers, so this is the only way to authenticate /api/logs/stream and /api/ws/tasks. Token is never logged (request logging elsewhere in index.ts logs path + status only, not the query string). Residual risk (tokens landing in proxy/access logs, browser history, Referer headers) is a known tradeoff for header-less transports. Recommend a follow-up to move to a short-lived, single-use ticket token for the query-string case instead of the long-lived static key — that's a larger change, out of scope here.

Test/tooling notes

• Fixed two things that were silently hanging/failing the test suite while writing the new integration test: (1) a non-unref()'d setInterval in rate-limiter.ts that kept the process alive after tests completed, and (2) router.ts eagerly constructing Orchestrator/GitHubClient at module load time, which throws if GITHUB_TOKEN is unset — stubbed a placeholder token in the test file's beforeAll, scoped to test-only.
• Full suite: 515 pass, typecheck clean. The only failures (36, pre-existing — confirmed via git stash against this branch's unmodified base) are in browser-manager.test.ts / action-executor.test.ts / visual-regression tests, all failing on a missing local Playwright Chromium binary (chrome-headless-shell not installed). Unrelated to auth/WS/rate-limiter and reproduces identically with or without this PR's changes.
• No lint script exists for packages/api (checked root and package-level package.json); ran typecheck only, which is clean.