Skip to content

fix: enforce app-wide browser security headers#2002

Open
bestony wants to merge 2 commits into
mainfrom
fix/1541-browser-security-headers
Open

fix: enforce app-wide browser security headers#2002
bestony wants to merge 2 commits into
mainfrom
fix/1541-browser-security-headers

Conversation

@bestony

@bestony bestony commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • enforce one Fastify-owned browser security policy across SPA, static, API, redirect, error, and not-found responses
  • add a strict CSP with exact production Cloud analytics, monitoring, OAuth-avatar, and same-origin WebSocket sources; self-hosted deployments can replace each external-origin list explicitly
  • move theme, GA4, and Clarity startup out of inline HTML and disable Zod runtime code generation so script-src needs neither unsafe-inline nor unsafe-eval
  • cover SPA/API policies, config parsing, external bootstrap behavior, operator documentation, and the live cross-surface QA journey

Closes #1541

Validation

  • pnpm check (exit 0; existing 16 warnings and 1 info remain outside this change)
  • pnpm typecheck
  • pnpm test (attempted; the complete run is not clean in this Node 26/root environment for unrelated baseline reasons listed below)
  • server security-header integration tests: 5/5
  • shared server-config tests: 24/24
  • Web analytics/bootstrap tests: 7/7
  • frozen lockfile install and production Web/server/CLI builds
  • disposable real-browser journey: landing, login, OAuth completion, onboarding, client/agent creation, workspace, authenticated same-origin WebSocket, avatar rendering, and attachment draft; zero CSP violations, console errors, or failed requests

Full-suite environment notes:

  • five Web DOM files failed under Node 26's unavailable experimental global Web Storage; the same files passed 61/61 with NODE_OPTIONS=--no-experimental-webstorage
  • two CLI bootstrap permission tests fail as root because chmod 000 remains readable/writable; the file passes 36/38 otherwise
  • the Context Tree audit eval exceeded its 15-second timeout only under full-suite load; its isolated rerun passed 12/12

Change Surface

  • apps/cli public CLI or help output
  • tree onboarding / binding / inspection behavior
  • shipped or planned skill topology
  • docs or contributor-facing repository metadata
  • CI / packaging / release plumbing

Notes

  • package or install behavior changes: adds @fastify/helmet to the server and makes the Web package's existing Zod runtime dependency explicit
  • docs or tests updated to match: .env.example, CLI ops reference, integration/unit tests, and packages/qa/cases/cross-surface/browser-security-policy-journeys.md
  • formal QA warranted: yes, because this changes auth, WebSocket, and browser behavior across server and Web surfaces; the reusable QA case is included, but no formal QA run was requested
  • follow-up work: none required for the scoped policy; CSP reporting, SRI, COOP/COEP, HSTS preload, and edge-layer changes remain intentionally out of scope

@bestony bestony added the fire_wip GoF: draft PR in progress label Jul 24, 2026
Comment thread packages/web/src/__tests__/analytics.test.ts Fixed

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested

Blocker — the production analytics allowlist is not complete (packages/server/src/security-headers.ts:15-24). The policy permits only e.clarity.ms for connections and c.clarity.ms for images. Microsoft documents that Clarity load-balances production traffic across a.clarity.ms through z.clarity.ms, so a browser that receives any other shard will be blocked by this enforced CSP even though the one recorded QA run happened to use the two listed hosts. GA4 likewise documents additional analytics/tag-manager subdomains and image endpoints beyond the single www.google-analytics.com connection currently allowed. This makes the production telemetry decision and the issue requirement of zero CSP violations dependent on region/timing.

Please derive the exact enumerated defaults from the providers full production CSP requirements (enumerating hosts rather than introducing wildcards), and add coverage for at least a non-c/e Clarity shard plus the GA4 endpoint set. References: Microsoft Clarity CSP and Google GA4 CSP.

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: request changes

  • Rationale: the provider-origin blocker already raised on this head must be resolved before an enforced production CSP is safe; I am commenting rather than duplicating the existing changes-request state.

Risk level: B-high

  • Path baseline: packages/server/** and packages/web/** -> B-low
  • Semantic lift: shared deployment config and package dependencies change the app-wide browser/runtime contract -> B-high

PR summary

  • Author / repo: bestony / agent-team-foundation/first-tree
  • Problem: authenticated Web users currently receive inconsistent or missing browser hardening, leaving the app frameable and external-resource behavior ungoverned.
  • Approach: install one Fastify-owned policy for every response, externalize the Web bootstrap, disable Zod runtime code generation, and make third-party origins deployment-configurable.
  • Impacted modules: Server app/config, Web bootstrap/analytics, operator docs, and cross-surface QA guidance.

Review findings
❌ 1. The existing blocking review on this head is valid: the production analytics origin set is incomplete, so region/load-balanced GA4 and Clarity requests can be blocked by the enforced policy. [R5 / packages/server/src/security-headers.ts]
⚠️ 2. The attachment download route still writes X-Content-Type-Options locally even though this PR makes the global Helmet layer the policy owner. Remove the route-local setter so this invariant has one write path and cannot drift. [R1/R2 / packages/server/src/api/attachments.ts:56]
✅ 3. Registering the policy before all routes and moving theme/analytics bootstrap out of inline HTML are coherent with the app-wide enforcement goal.

Action taken

  • Submitted a comment review; the existing changes-request review remains the active blocker.

@bestony bestony added fire_submitted GoF: PR submitted for maintainer review (stays draft) and removed fire_wip GoF: draft PR in progress labels Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fire_submitted GoF: PR submitted for maintainer review (stays draft)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add app-wide browser security headers for Fastify/SPA

4 participants