fix: enforce app-wide browser security headers#2002
Conversation
yuezengwu
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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/**andpackages/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]
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.
Summary
script-srcneeds neitherunsafe-inlinenorunsafe-evalCloses #1541
Validation
pnpm check(exit 0; existing 16 warnings and 1 info remain outside this change)pnpm typecheckpnpm test(attempted; the complete run is not clean in this Node 26/root environment for unrelated baseline reasons listed below)Full-suite environment notes:
NODE_OPTIONS=--no-experimental-webstoragechmod 000remains readable/writable; the file passes 36/38 otherwiseChange Surface
apps/clipublic CLI or help outputNotes
@fastify/helmetto the server and makes the Web package's existing Zod runtime dependency explicit.env.example, CLI ops reference, integration/unit tests, andpackages/qa/cases/cross-surface/browser-security-policy-journeys.md