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.
|
Resolved the review blockers on the current draft head:
The draft remains intentionally unready and is labeled |
yuezengwu
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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/**andpackages/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.
Summary
unsafe-inline/unsafe-evalin script directives, frame protection, HSTS, nosniff, referrer, and permissions policiesa.clarity.msthroughz.clarity.msoriginnosniffsetter so Helmet remains the single app-wide policy ownerProvider references: Google Analytics CSP, Microsoft Clarity CSP, and Cloudflare CSP.
Closes #1541
Validation
pnpm install --frozen-lockfilepnpm check(exit 0; the repository reports 16 existing warnings and 1 info outside this change)pnpm typecheck(10/10 tasks)HEAD, static assets, API errors, exact origin coverage, and enforced-policy assertionscurl -sSIchecks 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 headersunsafe-eval/new FunctionusageGIT_CONFIG_GLOBAL=/dev/nulland one worker (36 files, 494 tests)pnpm testin 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 aboveChange Surface
Notes
@fastify/helmetdependency to the server and makes the Web package's existing Zod runtime dependency explicit