From 2f54f1e184824e2d44e369030b4c095af2a2e24e Mon Sep 17 00:00:00 2001 From: frankbria Date: Fri, 24 Jul 2026 19:57:29 -0700 Subject: [PATCH 1/3] fix(web-ui): remove dead nav branch, drop prod unsafe-eval CSP, fail auth guard closed (#783) - AppSidebar: delete the unreachable !enabled 'coming soon' branch (all NAV_ITEMS were enabled:true) - security-headers: ship 'unsafe-eval' only in dev (Next.js React Refresh); document the residual localStorage-JWT + unsafe-inline risk and upgrade path - AppLayout: probe-error path now fails closed to /login instead of rendering the shell; checkAuthAccess docs updated Closes #783 --- CLAUDE.md | 2 +- web-ui/security-headers.js | 18 +++++++--- .../components/layout/AppLayout.test.tsx | 8 +++-- .../lib/verifyAuthAfterStreamFailure.test.ts | 2 +- web-ui/src/__tests__/security-headers.test.js | 11 ++++++ web-ui/src/components/layout/AppLayout.tsx | 14 +++++--- web-ui/src/components/layout/AppSidebar.tsx | 36 ++++++------------- web-ui/src/lib/api.ts | 4 +-- 8 files changed, 53 insertions(+), 42 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 765d0bd1..54454b5a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,7 +54,7 @@ Issue **import + traceability** (#565) is **complete**: `POST /api/v2/integratio **Phase 3.5C is complete** — `CaptureGlitchModal` form (description/markdown, source, scope, gate obligations, severity, expiry) reachable from the PROOF9 page and the persistent sidebar "Capture Glitch" button. REQ detail view (`/proof/[req_id]`) ships markdown description rendering, `ProofScope` metadata display, obligations table with `Latest Run` column, sortable/filterable evidence history, and empty-state CTA. Backend: `ScopeOut` model on `RequirementResponse`. Issues #568, #569. -**v2 API auth enforcement (#336) is complete** — all 22 v2 REST routers require auth (`require_auth`: JWT Bearer or `X-API-Key`) via router-level dependencies in `server.py`; env-gated `CODEFRAME_AUTH_REQUIRED` (default **ON**; set `false` for local dev — read at request time). Streams never carry the JWT in the URL (#745): an authenticated `POST /auth/stream-ticket` (write scope; `has_scope`, so admin implies write) mints a 60s **single-use** ticket (`codeframe/auth/stream_tickets.py`, in-process store — multi-worker caveat documented in the module), redeemed as `?ticket=` **only** on the two SSE routes (allowlist `_QUERY_TICKET_PATHS` in `codeframe/auth/dependencies.py`) and by `authenticate_websocket` for the two WS routes; `?token=` is no longer accepted anywhere. Frontend fetches a fresh ticket per (re)connect via `fetchStreamTicket()`/`withStreamTicket()`; `useEventSource`/`useTerminalSocket` take an async `buildUrl` re-resolved on every retry. `/auth/register` admits only the bootstrap first user (403 after; seeded `!DISABLED!` admin excluded; in-process lock closes the TOCTOU window). Web UI: `/login` page (sign-in + create-first-account), proactive client-side auth guard in `AppLayout` (token-present → allow; no token → `checkAuthAccess` probe → redirect or fail-open; #651), axios Bearer interceptor for reactive 401→`/login` redirect, SSE/WS hooks probe the `require_auth`-gated `/api/v2/settings/keys` (which respects `CODEFRAME_AUTH_REQUIRED`) on stream failure to catch token expiry (#651), SSE hooks append a fresh stream ticket, sidebar logout; `/auth/*` proxied in `next.config.js`. Backend tests run auth-off via root `tests/conftest.py` `setdefault`; `tests/ui/test_v2_auth_enforcement.py` opts back in. +**v2 API auth enforcement (#336) is complete** — all 22 v2 REST routers require auth (`require_auth`: JWT Bearer or `X-API-Key`) via router-level dependencies in `server.py`; env-gated `CODEFRAME_AUTH_REQUIRED` (default **ON**; set `false` for local dev — read at request time). Streams never carry the JWT in the URL (#745): an authenticated `POST /auth/stream-ticket` (write scope; `has_scope`, so admin implies write) mints a 60s **single-use** ticket (`codeframe/auth/stream_tickets.py`, in-process store — multi-worker caveat documented in the module), redeemed as `?ticket=` **only** on the two SSE routes (allowlist `_QUERY_TICKET_PATHS` in `codeframe/auth/dependencies.py`) and by `authenticate_websocket` for the two WS routes; `?token=` is no longer accepted anywhere. Frontend fetches a fresh ticket per (re)connect via `fetchStreamTicket()`/`withStreamTicket()`; `useEventSource`/`useTerminalSocket` take an async `buildUrl` re-resolved on every retry. `/auth/register` admits only the bootstrap first user (403 after; seeded `!DISABLED!` admin excluded; in-process lock closes the TOCTOU window). Web UI: `/login` page (sign-in + create-first-account), proactive client-side auth guard in `AppLayout` (token-present → allow; no token → `checkAuthAccess` probe → allow only on explicit 2xx, else fail closed to `/login`; #651, #783), axios Bearer interceptor for reactive 401→`/login` redirect, SSE/WS hooks probe the `require_auth`-gated `/api/v2/settings/keys` (which respects `CODEFRAME_AUTH_REQUIRED`) on stream failure to catch token expiry (#651), SSE hooks append a fresh stream ticket, sidebar logout; `/auth/*` proxied in `next.config.js`. Backend tests run auth-off via root `tests/conftest.py` `setdefault`; `tests/ui/test_v2_auth_enforcement.py` opts back in. **PROOF9 merge gate is enforced (#731)** — `POST /api/v2/pr/{n}/merge` blocks while open (non-waived) requirements exist (409 with a blocking-requirement summary; ledger failure → explicit 500, never a silent pass-through). Explicit bypass: `override: true` + `override_reason` in `MergePRRequest`; the override is persisted to the `pr_merge_overrides` ledger table (actor from `require_auth`, reason, bypassed requirements, timestamp) and surfaced as `merge_override` on `GET /api/v2/pr/history` items. `cf pr merge` enforces the same gate on the cwd workspace (`--override --reason "..."` to bypass, same audit record; no workspace in cwd → no gate). Scope is workspace-global open requirements — the same condition as the frontend `canMerge` in `PRStatusPanel`. Known follow-ups (not blockers): per-branch scope filtering, `vacuous_pass` distinction (#556), and no frontend override UI yet (API/CLI-only). diff --git a/web-ui/security-headers.js b/web-ui/security-headers.js index 752721f2..74edc36c 100644 --- a/web-ui/security-headers.js +++ b/web-ui/security-headers.js @@ -30,13 +30,21 @@ function buildCsp(env = process.env) { apiUrl: env.NEXT_PUBLIC_API_URL, wsUrl: env.NEXT_PUBLIC_WS_URL, }); + // unsafe-eval is only needed by the Next.js dev runtime (React Refresh / + // eval source maps); production bundles never eval, so it ships dev-only. + const scriptSrc = env.NODE_ENV === 'development' + ? "script-src 'self' 'unsafe-inline' 'unsafe-eval'" + : "script-src 'self' 'unsafe-inline'"; return [ "default-src 'self'", - // ponytail: 'unsafe-inline'/'unsafe-eval' are required by the Next.js App - // Router without a per-request nonce middleware (a much larger change). - // Exfil containment comes from connect-src/img-src/object-src below — not - // script-src — so the token can't be POSTed/GET'd to an attacker origin. - "script-src 'self' 'unsafe-inline' 'unsafe-eval'", + // ponytail: 'unsafe-inline' is required by the Next.js App Router without + // a per-request nonce middleware (a much larger change). Residual risk + // (#783): with the JWT in localStorage and inline scripts allowed, an + // injected inline script could read the token — containment comes from + // connect-src/img-src/object-src below (not script-src), which close off + // the origins the token could be POSTed/GET'd to. Upgrade path: nonce + // middleware and/or an httpOnly-cookie token. + scriptSrc, "style-src 'self' 'unsafe-inline'", `img-src 'self' data: blob: ${AVATAR_HOST}`, "font-src 'self' data:", diff --git a/web-ui/src/__tests__/components/layout/AppLayout.test.tsx b/web-ui/src/__tests__/components/layout/AppLayout.test.tsx index 6bd9a621..6d98aed0 100644 --- a/web-ui/src/__tests__/components/layout/AppLayout.test.tsx +++ b/web-ui/src/__tests__/components/layout/AppLayout.test.tsx @@ -97,7 +97,7 @@ describe('AppLayout', () => { expect(mockReplace).not.toHaveBeenCalled(); }); - it('fails open (renders the shell) when the backend probe errors', async () => { + it('fails closed (redirects to /login, no shell) when the backend probe errors (#783)', async () => { mockIsAuthenticated.mockReturnValue(false); mockCheckAuthAccess.mockResolvedValue('error'); render( @@ -105,7 +105,9 @@ describe('AppLayout', () => {
content
); - expect(await screen.findByTestId('app-sidebar')).toBeInTheDocument(); - expect(mockReplace).not.toHaveBeenCalled(); + await waitFor(() => expect(mockReplace).toHaveBeenCalledWith('/login')); + expect(screen.queryByTestId('app-sidebar')).not.toBeInTheDocument(); + expect(screen.queryByTestId('child')).not.toBeInTheDocument(); + expect(screen.getByRole('status')).toBeInTheDocument(); }); }); diff --git a/web-ui/src/__tests__/lib/verifyAuthAfterStreamFailure.test.ts b/web-ui/src/__tests__/lib/verifyAuthAfterStreamFailure.test.ts index 7d7dfdb7..5a72f4e1 100644 --- a/web-ui/src/__tests__/lib/verifyAuthAfterStreamFailure.test.ts +++ b/web-ui/src/__tests__/lib/verifyAuthAfterStreamFailure.test.ts @@ -80,7 +80,7 @@ describe('checkAuthAccess', () => { await expect(checkAuthAccess()).resolves.toBe('denied'); }); - it('returns "error" on a network/other failure (fail open)', async () => { + it('returns "error" on a network/other failure (caller fails closed, #783)', async () => { mockIsAxiosError = () => true; mockBareGet.mockRejectedValue({ response: { status: 503 } }); await expect(checkAuthAccess()).resolves.toBe('error'); diff --git a/web-ui/src/__tests__/security-headers.test.js b/web-ui/src/__tests__/security-headers.test.js index 930634b5..8deb806f 100644 --- a/web-ui/src/__tests__/security-headers.test.js +++ b/web-ui/src/__tests__/security-headers.test.js @@ -38,6 +38,17 @@ describe('security headers (#657)', () => { expect(cs).toContain('ws://localhost:8000'); }); + test('production CSP does not allow eval (#783)', () => { + // unsafe-eval is only needed by the Next.js dev runtime (React Refresh); + // shipping it in production widens the XSS surface for no benefit. + expect(buildCsp({})).not.toContain("'unsafe-eval'"); + expect(buildCsp({ NODE_ENV: 'production' })).not.toContain("'unsafe-eval'"); + }); + + test('dev CSP allows eval for the Next.js dev runtime', () => { + expect(buildCsp({ NODE_ENV: 'development' })).toContain("'unsafe-eval'"); + }); + test('securityHeaders ships the CSP plus the hardening header set', () => { const keys = securityHeaders({}).map((h) => h.key); expect(keys).toEqual( diff --git a/web-ui/src/components/layout/AppLayout.tsx b/web-ui/src/components/layout/AppLayout.tsx index 9eec529e..2a56501d 100644 --- a/web-ui/src/components/layout/AppLayout.tsx +++ b/web-ui/src/components/layout/AppLayout.tsx @@ -41,7 +41,9 @@ function FullPageLoader() { * permitted before deciding: * - allowed (valid token, or auth disabled) → render the shell; * - denied (auth required, no token) → redirect to /login; - * - error (backend unreachable) → fail open, render the shell. + * - error (backend unreachable) → fail closed, redirect to + * /login (#783) — the shell is useless without a backend anyway, and the + * login page surfaces the connectivity error on submit. * * Throughout, an unauthenticated visitor only ever sees a neutral loader — the * sidebar/shell never renders for them, so there's no "shell → flicker → login" @@ -73,11 +75,13 @@ export function AppLayout({ children }: AppLayoutProps) { let cancelled = false; checkAuthAccess().then((result) => { if (cancelled) return; - if (result === 'denied') { - router.replace('/login'); - } else { - // 'allowed' (auth disabled) or 'error' (fail open) → render the app. + if (result === 'allowed') { + // Auth disabled on the backend → token-less access is fine. setAccess('allowed'); + } else { + // 'denied' (auth required) or 'error' (backend unreachable) → fail + // closed to the login page (#783). + router.replace('/login'); } }); return () => { diff --git a/web-ui/src/components/layout/AppSidebar.tsx b/web-ui/src/components/layout/AppSidebar.tsx index 54cdb8c5..b8d348ea 100644 --- a/web-ui/src/components/layout/AppSidebar.tsx +++ b/web-ui/src/components/layout/AppSidebar.tsx @@ -31,20 +31,19 @@ interface NavItem { href: string; label: string; icon: IconSvgElement; - enabled: boolean; } const NAV_ITEMS: NavItem[] = [ - { href: '/', label: 'Workspace', icon: Home01Icon, enabled: true }, - { href: '/prd', label: 'PRD', icon: FileEditIcon, enabled: true }, - { href: '/tasks', label: 'Tasks', icon: Task01Icon, enabled: true }, - { href: '/execution', label: 'Execution', icon: PlayIcon, enabled: true }, - { href: '/sessions', label: 'Sessions', icon: CommandLineIcon, enabled: true }, - { href: '/blockers', label: 'Blockers', icon: Alert02Icon, enabled: true }, - { href: '/review', label: 'Review', icon: GitBranchIcon, enabled: true }, - { href: '/proof', label: 'Proof', icon: CheckmarkCircle01Icon, enabled: true }, - { href: '/costs', label: 'Costs', icon: ChartLineData01Icon, enabled: true }, - { href: '/settings', label: 'Settings', icon: Settings01Icon, enabled: true }, + { href: '/', label: 'Workspace', icon: Home01Icon }, + { href: '/prd', label: 'PRD', icon: FileEditIcon }, + { href: '/tasks', label: 'Tasks', icon: Task01Icon }, + { href: '/execution', label: 'Execution', icon: PlayIcon }, + { href: '/sessions', label: 'Sessions', icon: CommandLineIcon }, + { href: '/blockers', label: 'Blockers', icon: Alert02Icon }, + { href: '/review', label: 'Review', icon: GitBranchIcon }, + { href: '/proof', label: 'Proof', icon: CheckmarkCircle01Icon }, + { href: '/costs', label: 'Costs', icon: ChartLineData01Icon }, + { href: '/settings', label: 'Settings', icon: Settings01Icon }, ]; export function AppSidebar() { @@ -106,24 +105,11 @@ export function AppSidebar() { {/* Navigation */}