feat(auth): enforce authentication across the v2 API (#336)

[frankbria]

Summary

Implements #336: authentication enforcement across the v2 API — backend enforcement, SSE token auth, JWT login web UI, and test migration.

• Backend: require_auth (JWT Bearer or X-API-Key) mounted as a router-level dependency on all 22 v2 REST routers + /test/broadcast. Env-gated via CODEFRAME_AUTH_REQUIRED (default ON, read at request time); when disabled, a synthetic local-admin principal preserves current local behavior. Public: /, /health, OpenAPI docs, /auth/*.
• SSE: ?token=<JWT> accepted only on the two EventSource routes (/api/v2/tasks/{id}/stream, /api/v2/prd/stress-test) via a strict path allowlist; rejected everywhere else. WS routers keep their existing ?token= auth (untouched).
• Registration: /auth/register is bootstrap-first-user only — 403 once a real account exists (seeded !DISABLED! admin excluded); check-then-create serialized with an in-process lock (TOCTOU guard).
• Web UI: /login page (sign-in + first-account bootstrap), axios interceptor attaching Authorization: Bearer from the existing auth_token localStorage convention, loop-safe 401 → /login redirect, SSE hooks append the token, logout in sidebar; /auth/* added to the Next.js rewrite proxy.

Acceptance Criteria

• With auth enabled: every /api/v2/* route 401s without credentials; passes with valid JWT and with valid API key (parametrized over all 22 routers, end-to-end X-API-Key test)
• SSE authenticates via ?token= (EventSource-compatible); WS unchanged
• /, /health, docs, /auth/jwt/login, /auth/register remain public; /test/broadcast now requires auth
• Web UI login flow end-to-end; REST/SSE/WS all carry the token; 401 redirects to /login
• Backend suite green (3602 passed; 26 failures are pre-existing on main — verified via stash baseline: 20 stale-v1 /api/projects + 6 environment-dependent e2e/lifecycle); ruff clean
• web-ui: 88 suites / 1000+ tests green; npm run build green
• CLI golden path unaffected (no server required)

Test Plan

• Unit tests written (TDD approach) — 119 new/updated auth + enforcement tests
• All tests passing
• Diff coverage 98% on changed lines (gate ≥85%)
• Linting clean (ruff; web-ui lint errors are pre-existing in untouched files)
• Internal code review (advisory) completed — 2 Major findings fixed (router matrix completeness, e2e API-key path)
• Cross-family review pass: codex (2 rounds). Round 1: P1 (auth proxy gap) and P2 (registration TOCTOU) fixed. Round 2: P2 (global query-token surface) fixed via SSE allowlist; P1 (token in SSE URL) mitigated + documented below.
• Test mutation sanity check completed — 7 mutations across backend/frontend, all detected by tests

Known Limitations / Intentionally Deferred

• No JWT refresh flow — 7-day token (JWT_LIFETIME_SECONDS); users re-login on expiry.
• JWT appears in SSE/WS URLs (browser EventSource/WebSocket cannot send headers). Scope is limited to the 2 SSE routes + 2 WS routes; the pre-existing WS pattern is unchanged. Proper fix (short-lived single-purpose stream tokens) is deferred — follow-up candidate.
• Registration race lock is in-process — multi-worker deployments retain a narrow first-registration race window.
• Auth on by default requires first-run account creation for the web UI (codeframe serve → login page → "create first account"). CLI is unaffected. Local opt-out: CODEFRAME_AUTH_REQUIRED=false.
• Pre-existing web-ui lint errors (6) and the 26 pre-existing backend test failures are untouched — out of scope.

Implementation Notes

• WS auth (issue workstream B) was already implemented on both WS routers; this PR only added the SSE side.
• streaming_v2.py holds SSE utilities; the actual SSE route lives under the tasks prefix and is covered by tasks_v2 router auth.
• Frontend 401-driven redirect means no auth-status probe endpoint is needed: with auth disabled, no 401 ever fires and the login page is simply never visited.

Closes #336

[gitguardian]

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

---

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

[coderabbitai]

Warning

Review limit reached

@frankbria, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 8 minutes and 22 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c81c7ccd-2314-434b-8e9a-014a39687d8f

📥 Commits

Reviewing files that changed from the base of the PR and between 5553157 and 2b795ba.

📒 Files selected for processing (26)

• .env.example
• CLAUDE.md
• codeframe/auth/dependencies.py
• codeframe/auth/router.py
• codeframe/ui/server.py
• tests/auth/test_auth_mode.py
• tests/auth/test_query_param_token.py
• tests/auth/test_registration_bootstrap.py
• tests/conftest.py
• tests/ui/test_v2_auth_enforcement.py
• web-ui/__mocks__/@hugeicons/react.js
• web-ui/next.config.js
• web-ui/src/__tests__/app/login.test.tsx
• web-ui/src/__tests__/components/layout/AppLayout.test.tsx
• web-ui/src/__tests__/components/layout/AppSidebar.test.tsx
• web-ui/src/__tests__/hooks/useStressTestStream.test.ts
• web-ui/src/__tests__/hooks/useTaskStream.test.ts
• web-ui/src/__tests__/lib/api.auth.test.ts
• web-ui/src/__tests__/lib/auth.test.ts
• web-ui/src/app/login/page.tsx
• web-ui/src/components/layout/AppLayout.tsx
• web-ui/src/components/layout/AppSidebar.tsx
• web-ui/src/hooks/useStressTestStream.ts
• web-ui/src/hooks/useTaskStream.ts
• web-ui/src/lib/api.ts
• web-ui/src/lib/auth.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/issue-336-v2-api-auth

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review — feat(auth): enforce authentication across the v2 API (#336)

This is a well-structured, security-conscious implementation. The design choices — router-level enforcement via a shared _AUTH dependency list, SSE allowlist for query-param tokens, bootstrap-first-user registration gating — are all sound. The test coverage (119 tests, parametrized router matrix, TOCTOU concurrent-registration test) is excellent. A few things worth addressing before merge:

---

Findings

1. GitGuardian flag — likely a false positive, but confirm

GitGuardian triggered on web-ui/src/__tests__/lib/auth.test.ts at line ~68. Looking at the file, that region contains mock response data ('LOGIN_BAD_CREDENTIALS', 'pw123', etc.) — no real secrets. Confirm the flagged string is test data and close the incident, or add a GitGuardian inline ignore comment if needed to prevent future noise.

2. allow_registration yields inside async with _register_lock: — review lock scope

The lock is held from dependency entry until the fastapi-users route handler completes. This is intentional per the TOCTOU comment, and it works correctly. One edge case: if fastapi-users performs email-sending or external I/O during registration (some configurations do), the lock will hold for the full duration. This is a bootstrap-only path so the performance impact is negligible — just confirm your fastapi-users config doesn't do anything slow before the INSERT.

3. No server-side logout (JWT stays valid after logout)

logout() in auth.ts clears the localStorage token and hard-redirects to /login, but does not call /auth/jwt/logout to invalidate the token server-side. With a 7-day JWT lifetime, a logged-out token remains valid for up to 7 days. This is documented in the PR as intentional (stateless JWT), but worth ensuring downstream consumers (e.g. CLI tooling or API clients using exported tokens) are aware. If you ever support API key revocation, consider extending the same pattern to JWT revocation.

4. JWT stored in localStorage (XSS exposure)

auth_token in localStorage is readable by any JS running in the same origin. This follows the existing WS convention, which is noted in the PR. Worth calling out explicitly: if the app ever has a stored-XSS or third-party script injection, the JWT can be exfiltrated. For local/self-hosted tooling this risk is acceptable; if this eventually targets a multi-tenant or cloud deployment, consider HttpOnly cookies (which would also eliminate the query-param SSE workaround).

5. JWT in SSE query string logged by proxies/servers

As documented, the ?token= fallback for SSE is scoped to exactly two allowlisted paths and defended via _QUERY_TOKEN_PATHS. The implementation guard is solid. Just ensure production deployments mask or exclude the token= query parameter from access logs at the reverse-proxy layer (nginx: log_format, Caddy: log replace, etc.).

6. importlib.reload(server) in auth_app fixture — potential instability

test_v2_auth_enforcement.py's auth_app fixture reloads the server module to pick up monkeypatched env vars. importlib.reload re-executes module-level code (router registration, middleware setup, etc.) which can cause subtle state pollution if any module-level side effects are non-idempotent (e.g. adding duplicate routes or re-registering signal handlers). If the test suite passes reliably this is probably fine, but if you see flakiness here, consider using app.dependency_overrides to swap just the auth_required callable instead.

7. Minor: _DISABLED_PASSWORD is duplicated across module and tests

router.py defines _DISABLED_PASSWORD = "!DISABLED!" and test files repeat the literal string. If the sentinel ever changes, test files would need separate updating. Consider exporting the constant from router.py and importing it in tests.

8. create_test_jwt_token import in enforcement tests

test_v2_auth_enforcement.py does from tests.conftest import create_test_jwt_token. This function doesn't appear in the conftest.py diff, so it presumably pre-existed. Worth confirming it exists and was not accidentally dropped — if it's missing, the test file will fail to import entirely.

---

What's done well

• Router matrix coverage: parametrizing all 22 routers in V2_GET_ENDPOINTS and asserting exactly 401 (not 404/422) is a robust enforcement contract.
• SSE allowlist design: the path-regex allowlist in _QUERY_TOKEN_PATHS is tight and documented. Rejecting query tokens on all other routes (codex review P2 fix) is correct.
• Auth-disabled synthetic principal: returning a synthetic {type: 'disabled', scopes: [read, write, admin]} instead of raising allows the existing test suite to run unmodified via conftest.py's setdefault. Clean migration path.
• Frontend 401 interceptor loop guard: checking window.location.pathname !== '/login' before redirecting prevents redirect loops on a failed login attempt.
• withTokenParam URL encoding: using encodeURIComponent on the token value and the ?/& separator logic are both correct.
• Bootstrap TOCTOU test: the anyio concurrent-registration test directly validates the in-process lock behavior. Well done.

---

Summary

No blocking issues. Findings 1 (GitGuardian confirm), 7 (constant dedup), and 8 (create_test_jwt_token verification) are the most actionable pre-merge items. Findings 3–5 are intentional/documented tradeoffs worth having on record. Finding 6 is a test-stability advisory.

Overall this is a solid, well-tested auth enforcement implementation. LGTM pending the minor items above.

[claude]

Follow-up Review

Building on my prior review, this pass covers the complete frontend implementation now visible in the diff. The backend work remains solid; the frontend additions are clean and consistent.

auth.ts observations:

• login() correctly uses URLSearchParams for form-encoded POST (fastapi-users requirement)
• withTokenParam() correctly uses encodeURIComponent on the token value
• Both login() and register() use axios directly (not the shared API client) - intentional, since auth endpoints are outside /api/v2 and do not need the Bearer interceptor

api.ts 401 handler:

• The pathname !== '/login' guard correctly prevents redirect loops on failed login
• clearToken() runs before the redirect, so the stale token is cleared even if the nav fails

AppLayout: The BARE_ROUTES = new Set(['/login']) pattern is clean and easy to extend without touching the render tree.

Minor nit: onClick={() => logout()} in AppSidebar.tsx can just be onClick={logout}. The wrapper lambda is a no-op. Not worth a blocking change.

Status of Prior Findings:

• GitGuardian false positive in auth.test.ts: OPEN - still flagged, needs dismissal or ggignore annotation
• allow_registration lock scope: Acceptable - bootstrap-only path
• No server-side logout (JWT valid 7 days post-logout): Intentionally deferred and documented
• JWT in localStorage XSS exposure: Follows existing WS convention, acceptable for current deployment
• JWT in SSE query string / proxy log masking: Deployment note only

The one thing to fix before merge:

The GitGuardian alert is a false positive - the flagged string is test mock data, not a real credential. Add a GitGuardian inline suppression on the flagged line in web-ui/src/tests/lib/auth.test.ts (around line 68):

expect(params.get('password')).toBe('pw123'); // ggignore

The author tried to mitigate by parsing URLSearchParams rather than substring-matching a password=... literal, but GitGuardian still triggers on the asserted value. The inline comment suppresses it cleanly.

Overall: Complete, well-tested implementation. Design is correct, coverage is strong (119 tests, parametrized router matrix, concurrent-registration TOCTOU test), and the deferred items are documented. Recommend merging after resolving the GitGuardian flag.

[claude]

Follow-up Review for PR 610 — feat(auth): enforce authentication across the v2 API (#336)

Reviewing against my prior comment and the 6 commits on this branch.

Prior Findings — Status

1. GitGuardian false positive: Fixed in commit 3b5c985. auth.test.ts now parses URLSearchParams instead of substring-matching a password literal. The prior GitGuardian incident on b3af9e9 stays open (predates fix) but new scans should be clean.

2. allow_registration lock scope: Verified correct. yield sits inside async with _register_lock: so the lock spans the full route handler. The async DB session is closed before yield (no connection held open). The anyio concurrent-registration test validates the TOCTOU guard.

3. No server-side JWT revocation on logout: Known limitation (7-day token post-logout). Documented in PR. Acceptable for this scope.

4. JWT in localStorage: Follows existing WS convention, documented. Acceptable for local/self-hosted use.

5. JWT in SSE query string: Correctly restricted to two allowlisted paths via _QUERY_TOKEN_PATHS. Parametrized tests confirm acceptance on SSE paths and rejection elsewhere.

New Observation

importlib.reload(server) in auth_app fixture: picks up the monkeypatched CODEFRAME_AUTH_REQUIRED=true correctly for serial runs (pytest default), but a module reload mid-run would race with parallel test workers if -n (xdist) is ever used. Not a blocker; worth a comment in the fixture noting the xdist caveat.

Summary

All prior findings addressed. 22/22 v2 routers covered, TOCTOU-safe bootstrap registration, SSE-only query-token allowlist, full frontend login flow, 119 new tests including the parametrized router enforcement matrix and concurrent-registration race test. LGTM.