Skip to content

fix: enforce app-wide browser security headers#2002

Draft
bestony wants to merge 4 commits into
mainfrom
fix/1541-browser-security-headers
Draft

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

Conversation

@bestony

@bestony bestony commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • register one Fastify-owned security-header policy before API and SPA routes, covering success, redirect, static-asset, error, and not-found responses
  • enforce a least-privilege CSP with no report-only mode, no unsafe-inline/unsafe-eval in script directives, frame protection, HSTS, nosniff, referrer, and permissions policies
  • enumerate the production provider origins required by Google Analytics 4, Microsoft Clarity, Cloudflare Web Analytics, Sentry, and avatar hosts; Clarity collectors cover every exact a.clarity.ms through z.clarity.ms origin
  • keep third-party origins configuration-driven through the existing exact-origin environment fields
  • remove inline HTML scripts while preserving blocking theme initialization, and configure Zod's jitless interpreter before the application module loads
  • remove the attachment route's duplicate nosniff setter so Helmet remains the single app-wide policy owner

Provider references: Google Analytics CSP, Microsoft Clarity CSP, and Cloudflare CSP.

Closes #1541

Validation

  • pnpm install --frozen-lockfile
  • pnpm check (exit 0; the repository reports 16 existing warnings and 1 info outside this change)
  • pnpm typecheck (10/10 tasks)
  • production builds for Shared, Web, and Server
  • Shared tests (63 files, 702 tests)
  • Server tests (246 files, 2,678 tests)
  • Web tests (204 files, 1,739 tests)
  • security-header integration tests (8 tests), including SPA HEAD, static assets, API errors, exact origin coverage, and enforced-policy assertions
  • built server curl -sSI checks against a disposable local instance for SPA, health API, API 404, and the external theme asset; all responses included the same enforced CSP, HSTS, nosniff, referrer, permissions, and frame-protection headers
  • built Web artifact inspection: exactly two external scripts, no inline script tags, and no generated unsafe-eval/new Function usage
  • skill-evals isolated regression run with GIT_CONFIG_GLOBAL=/dev/null and one worker (36 files, 494 tests)
  • full pnpm test in the default local environment: the run is blocked by unrelated skill-evals fixture failures caused by the host global Git ignore (~/.gitignore) and parallel 5-second fixture timeouts; the same skill-evals suite passes with the isolated command above
  • browser functional journey under enforced CSP: the repository's reusable QA case is included, but no browser session was available in this execution environment, so login, chat, WebSocket, avatar, attachment, document-preview, and console-violation checks remain for a browser-enabled QA run

Change Surface

  • public CLI or help output
  • tree onboarding / binding / inspection behavior
  • shipped or planned skill topology
  • server and Web browser security behavior
  • docs and cross-surface QA guidance
  • CI / packaging / release plumbing

Notes

  • package behavior changes: adds the existing @fastify/helmet dependency to the server and makes the Web package's existing Zod runtime dependency explicit
  • the CSP provider defaults follow the providers' documented host requirements; Google Signals and ad-personalization signals are disabled so advertising endpoints are not needed
  • formal QA is warranted because this changes authenticated browser, WebSocket, and attachment behavior; no production deploy or edge configuration was changed

@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
@bestony
bestony marked this pull request as draft July 24, 2026 23:26
@bestony bestony added fire_wip GoF: draft PR in progress and removed fire_submitted GoF: PR submitted for maintainer review (stays draft) labels Jul 24, 2026
@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
@bestony

bestony commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the review blockers on the current draft head:

  • Expanded the exact production Clarity collector allowlist to a.clarity.ms through z.clarity.ms, with regression coverage for t.clarity.ms.
  • Added the documented GA4 analytics/tag-manager connection and image origins, with no wildcards.
  • Disabled Google Signals and ad-personalization signals so advertising endpoints are not required by the policy.
  • Removed the attachment route-local nosniff setter; Helmet is now the single app-wide owner.

The draft remains intentionally unready and is labeled fire_submitted. The reusable browser QA case is updated. A browser session was unavailable in this execution environment, so the full interactive CSP journey is explicitly left for a browser-enabled QA run rather than reported as complete.

@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 — preserve the existing FIRST_TREE_PUBLIC_URL fallback contract (packages/server/src/security-headers.ts:52-60). Both defaultCspOrigins and websocketOrigin call new URL(publicUrl) during buildApp. However, serverConfigSchema.server.publicUrl still accepts any string, and resolvePublicUrl intentionally supports an unparseable configured value by trimming it; packages/server/src/__tests__/connect-token-bootstrap.test.ts explicitly covers first-tree.internal///. A deployment using that supported form now throws before routes are registered, so this hardening change introduces a server boot regression.

Please either make the CSP derivation fail closed for an invalid or non-HTTP(S) public URL (self-only defaults and no derived WebSocket source), or deliberately replace the public URL contract with schema validation plus the corresponding migration, tests, and documentation. The fail-closed helper is the smaller compatibility-preserving fix; add coverage that constructs the app/options with the existing fallback input.

@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 original code findings are resolved, but the binding browser-functional acceptance check is explicitly still incomplete.

Risk level: B-high

  • Path baseline: packages/server/** and packages/web/** -> B-low
  • Semantic lift: app-wide browser enforcement, shared deployment config, and dependency/bootstrap changes -> B-high

PR summary

  • Author / repo: bestony / agent-team-foundation/first-tree
  • Problem: Web users need one enforceable browser-security policy without breaking authentication, realtime chat, previews, avatars, or production telemetry.
  • Approach: Fastify owns the response policy; the Web bootstrap is external and CSP-safe; production provider origins are exact and environment-overridable.
  • Impacted modules: Server policy/config, Web theme/application/analytics bootstrap, operator docs, and cross-surface QA guidance.

Review findings
❌ 1. The newly raised boot-compatibility blocker is valid: CSP derivation calls new URL(publicUrl) even though the existing public-URL contract accepts and trims non-URL strings. The supported fallback fixture first-tree.internal/// now throws during buildApp before any route exists. Preserve the contract with fail-closed CSP derivation, or deliberately migrate the public-URL contract and all consumers. [packages/server/src/security-headers.ts, packages/server/src/utils/public-url.ts]
❌ 2. The browser-functional acceptance criterion remains unmet. The PR now states that no browser session was available, while issue #1541 explicitly requires login, chat send/receive, WebSocket live updates, avatar and attachment rendering, document preview, and zero CSP violations. This head also changes the blocking theme script, dynamic application import, provider requests, and media-src/worker-src; injection tests, artifact inspection, and curl cannot prove those browser-enforced paths. Please provide a browser-enabled QA result tied to the exact head before approval. [packages/qa/cases/cross-surface/browser-security-policy-journeys.md]
✅ 3. The previous static blockers are addressed: Clarity collectors are enumerated from a through z, the core GA4/Tag Manager origins are present with advertising signals disabled, and the attachment route-local nosniff setter is removed. [packages/server/src/security-headers.ts, packages/web/src/bootstrap.ts, packages/server/src/api/attachments.ts]

Action taken

  • Submitted a comment review rather than duplicating the existing changes-request state.

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