fix(csp): enforce the Content-Security-Policy, and fix what enforcing exposed

.claude/references/security-headers.md

@@ -23,28 +23,47 @@ is the modern equivalent for the rest. They are *not* both CSP headers — two
 `Content-Security-Policy` headers on one response are enforced as an
 intersection, which is a miserable thing to debug.
 
-## The CSP ships report-only
+## The CSP enforces
 
-`CSP_ENFORCE=true` switches `src/proxy.ts` from
-`Content-Security-Policy-Report-Only` to `Content-Security-Policy`. Only that
-exact string enforces; anything else, unset included, stays report-only, so a
-typo in the variable cannot take a deploy down.
+`src/proxy.ts` sends `Content-Security-Policy`. `CSP_ENFORCE=false` — and only
+that exact string — drops back to `Content-Security-Policy-Report-Only`.
+Anything else, unset included, enforces, so a typo fails loud (too strict)
+rather than silent (no policy).
 
-A nonce CSP is the one security header that can white-screen an app. Before
-flipping it on, walk the app with devtools open and confirm a clean console:
+It shipped report-only first and was flipped on 2026-09-12 after the policy was
+walked through a real browser against a **production build**, clean:
 
-- sign-in (the redirect out to MP and back)
-- **sign-out** — the likeliest breakage; see `form-action` below
+- sign-in (the redirect out to MP and back), and sign-out to MP's endsession
 - contact photos on the header avatar, search results, and the detail page
-- every Radix surface: dropdown, dialog, select, tooltip, vaul drawer
-- contact search, and contact-log create/edit
+- every Radix surface: dropdown, dialog, the select inside the dialog
+- contact search and the contact-log dialog
+
+**Report-only is not a substitute for that walk.** In the report-only pass the
+console was completely clean; enforcing the same policy immediately blocked a
+runtime-injected `<style>` and broke the contact-log dialog with React error
+#441 (see `style-src` below). If you change the policy, re-walk it enforced,
+against a production build — `next dev` has deliberate relaxations
+(`'unsafe-eval'`, `ws:`) that hide violations.
 
 ## Deliberate loosenings — do not "tighten" these
 
-- **`style-src-attr 'unsafe-inline'`** — Radix and vaul position every popover,
-  dialog, select and drawer by writing inline `style` attributes. A nonce
-  covers `<style>` elements, not attributes. Removing this renders every
-  floating UI surface in the wrong place.
+- **`style-src 'self' 'unsafe-inline'`, with NO nonce** — Radix's dialog pulls
+  in react-remove-scroll, which locks body scroll by **injecting a `<style>`
+  element at runtime**. A nonce cannot cover it (the element is created by
+  script, long after the server picked the nonce) and neither can a hash (the
+  content embeds the computed scrollbar width, so it varies by platform and
+  zoom — two different hashes appeared in one page view).
+
+  The nonce must stay OUT of this directive: CSP3 browsers ignore
+  `'unsafe-inline'` whenever a nonce is present in the same directive, which
+  silently re-blocks every runtime-injected style. `style-src-attr` is gone as
+  redundant — `style-src` covers attributes and elements alike.
+
+  This replaced an earlier `style-src 'self' 'nonce-…'` + `style-src-attr
+  'unsafe-inline'` design that looked right and was wrong. Report-only mode did
+  **not** surface it; only enforcing the policy in a real browser did, where
+  the dialog broke with React error #441. That is the argument for doing the
+  enforced walk rather than trusting a clean report-only run.
 - **`form-action 'self' <MP origin>`** — sign-out is a `<form action={…}>`
   server action that ends in `redirect()` to MP's `/oauth/connect/endsession`.
   Browsers apply `form-action` to the whole redirect chain a form submission

.env.example

@@ -61,18 +61,21 @@ NEXT_PUBLIC_APP_NAME=MPNextApp
 # =============================================================================
 # Security Headers
 # =============================================================================
-# Switches the Content-Security-Policy from report-only to enforcing.
+# Drops the Content-Security-Policy back to report-only. Normally leave blank.
 #
 # The policy in src/lib/security-headers.ts is applied by src/proxy.ts with a
-# fresh per-request nonce. It ships REPORT-ONLY: a nonce-based CSP is the one
-# security header that can white-screen the app (a page Next prerendered at
-# build time carries no nonce, and a missed origin is invisible until a user
-# reaches that exact screen), so violations go to the browser console first.
+# fresh per-request nonce, and it ENFORCES by default. Only the exact string
+# "false" backs off to report-only; anything else, including unset, enforces.
+# The escape hatch is inverted on purpose - a typo here fails loud (too strict)
+# rather than silent (no policy at all).
 #
-# Only the exact string "true" enforces. Anything else, including unset, stays
-# report-only, so a typo here can never take a deploy down.
+# Set this to "false" only to diagnose a violation: the browser then reports
+# what the policy WOULD have blocked, in the console, without breaking the page.
+# Be aware report-only does not catch everything - a runtime-injected <style>
+# that enforcement blocks was NOT reported in report-only mode (2026-09-12).
 #
-# Flip this on only after walking the app with devtools open and seeing a clean
-# console - sign-in, sign-out, contact photos, and every Radix surface
-# (dropdown, dialog, select, tooltip, drawer).
+# If you change the policy, re-walk the app against a PRODUCTION build with
+# devtools open - dev has deliberate relaxations that hide violations. Cover
+# sign-in, sign-out, contact photos, and every Radix surface (dropdown, dialog,
+# select, tooltip, drawer).
 CSP_ENFORCE=

src/lib/security-headers.test.ts

@@ -140,28 +140,39 @@ describe('buildContentSecurityPolicy', () => {
     expect(directive(csp, 'font-src')).toBe("font-src 'self'");
   });
 
-  it('allows inline style attributes', () => {
-    // Deliberate and load-bearing: Radix and vaul position every popover,
-    // dialog, select and drawer with inline `style` attributes, which a nonce
-    // cannot cover. Tightening this renders floating UI in the wrong place.
-    expect(directive(buildContentSecurityPolicy(base), 'style-src-attr')).toBe(
-      "style-src-attr 'unsafe-inline'"
-    );
+  it('allows inline styles, in dev and in production alike', () => {
+    // Deliberate and load-bearing. Proven necessary by ENFORCING the policy in
+    // a real browser (2026-09-12): Radix's dialog pulls in react-remove-scroll,
+    // which locks body scroll by injecting a <style> ELEMENT at runtime. That
+    // is not an attribute, so the old `style-src-attr` did not cover it, and a
+    // nonce cannot cover it either — the element is created by script long
+    // after the server picked the nonce. The browser blocked it and the dialog
+    // died with React error #441.
+    for (const csp of [
+      buildContentSecurityPolicy(base),
+      buildContentSecurityPolicy({ ...base, isDev: true }),
+    ]) {
+      expect(directive(csp, 'style-src')).toBe("style-src 'self' 'unsafe-inline'");
+    }
   });
 
-  it('nonces stylesheets outside dev', () => {
-    expect(directive(buildContentSecurityPolicy(base), 'style-src')).toBe(
-      "style-src 'self' 'nonce-TEST-NONCE'"
-    );
+  it("never puts a nonce in style-src alongside 'unsafe-inline'", () => {
+    // The trap that produced the broken policy: CSP3 browsers IGNORE
+    // 'unsafe-inline' when a nonce is present in the same directive, so a
+    // nonce here silently re-blocks every runtime-injected <style>.
+    expect(directive(buildContentSecurityPolicy(base), 'style-src')).not.toContain('nonce-');
   });
 
-  it('allows unsafe-inline styles in dev only', () => {
-    // The dev server injects stylesheets through JS with no nonce available;
-    // the nonce form would blank the page.
-    const dev = buildContentSecurityPolicy({ ...base, isDev: true });
+  it('no longer emits style-src-attr', () => {
+    // Redundant once style-src allows inline: that covers attributes and
+    // elements alike. Keeping it would imply a distinction that does not hold.
+    expect(directive(buildContentSecurityPolicy(base), 'style-src-attr')).toBeUndefined();
+  });
 
-    expect(directive(dev, 'style-src')).toBe("style-src 'self' 'unsafe-inline'");
-    expect(directive(dev, 'script-src')).toContain("'unsafe-eval'");
+  it('allows unsafe-eval in dev only', () => {
+    expect(directive(buildContentSecurityPolicy({ ...base, isDev: true }), 'script-src')).toContain(
+      "'unsafe-eval'"
+    );
   });
 
   it('does not allow unsafe-eval outside dev', () => {
@@ -185,6 +196,25 @@ describe('buildContentSecurityPolicy', () => {
     ).toBeUndefined();
   });
 
+  it('omits upgrade-insecure-requests in report-only mode', () => {
+    // Browsers refuse to honor it in a report-only policy and log an error
+    // saying so on EVERY page. Observed during the F9 browser walk: it was the
+    // only CSP message in the console, burying the reports report-only mode
+    // exists to surface.
+    expect(
+      directive(buildContentSecurityPolicy({ ...base, reportOnly: true }), 'upgrade-insecure-requests')
+    ).toBeUndefined();
+  });
+
+  it('keeps every other directive identical in report-only mode', () => {
+    // report-only must not quietly weaken the policy being trialled — that
+    // would make the trial meaningless.
+    const enforced = buildContentSecurityPolicy(base).split('; ');
+    const reported = buildContentSecurityPolicy({ ...base, reportOnly: true }).split('; ');
+
+    expect(reported).toEqual(enforced.filter((d) => d !== 'upgrade-insecure-requests'));
+  });
+
   it('adds the MP file origin to img-src when configured', () => {
     const csp = buildContentSecurityPolicy({ ...base, imageOrigin: 'https://files.example.com' });
 
@@ -222,30 +252,35 @@ describe('buildContentSecurityPolicy', () => {
 });
 
 describe('cspHeaderName', () => {
-  it('reports by default', () => {
-    // A nonce CSP is the one security header that can white-screen the app, so
-    // enforcement is opt-in and a typo in the variable cannot take a deploy
-    // down.
-    expect(cspHeaderName(false)).toBe('Content-Security-Policy-Report-Only');
+  it('enforces by default', () => {
+    // Flipped from report-only on 2026-09-12, after the policy was walked
+    // through a real browser against a production build with no violations.
+    expect(cspHeaderName(true)).toBe('Content-Security-Policy');
   });
 
-  it('enforces when asked', () => {
-    expect(cspHeaderName(true)).toBe('Content-Security-Policy');
+  it('reports when explicitly asked', () => {
+    expect(cspHeaderName(false)).toBe('Content-Security-Policy-Report-Only');
   });
 
   it.each([
-    ['true', 'Content-Security-Policy'],
     ['false', 'Content-Security-Policy-Report-Only'],
-    ['1', 'Content-Security-Policy-Report-Only'],
-    ['TRUE', 'Content-Security-Policy-Report-Only'],
+    ['true', 'Content-Security-Policy'],
+    ['0', 'Content-Security-Policy'],
+    ['FALSE', 'Content-Security-Policy'],
+    ['', 'Content-Security-Policy'],
   ])('reads CSP_ENFORCE=%s as %s', (value, expected) => {
+    // Only the exact string "false" backs off. Everything else enforces, so a
+    // typo in this variable can never silently disarm the policy in a deploy —
+    // the failure mode is a too-strict header, which is loud, rather than a
+    // missing one, which is invisible.
     vi.stubEnv('CSP_ENFORCE', value);
     expect(cspHeaderName()).toBe(expected);
     vi.stubEnv('CSP_ENFORCE', undefined);
   });
 
-  it('reports when CSP_ENFORCE is unset', () => {
+  it('enforces when CSP_ENFORCE is unset', () => {
+    // The common case: nobody sets this variable at all.
     vi.stubEnv('CSP_ENFORCE', undefined);
-    expect(cspHeaderName()).toBe('Content-Security-Policy-Report-Only');
+    expect(cspHeaderName()).toBe('Content-Security-Policy');
   });
 });

src/lib/security-headers.ts

@@ -130,6 +130,17 @@ export interface CspOptions {
    * origin in `img-src` every avatar breaks.
    */
   imageOrigin?: string | null;
+  /**
+   * Whether this policy will be sent as `Content-Security-Policy-Report-Only`.
+   *
+   * Only affects `upgrade-insecure-requests`, which browsers refuse to honor in
+   * a report-only policy and complain about in the console on every page:
+   * "The Content Security Policy directive 'upgrade-insecure-requests' is
+   * ignored when delivered in a report-only policy." Emitting it there buys
+   * nothing and trains people to ignore the console, which is the one place
+   * report-only mode does its work.
+   */
+  reportOnly?: boolean;
   /**
    * Origin of the Ministry Platform OAuth server, for `form-action`.
    *
@@ -152,6 +163,7 @@ export interface CspOptions {
 export function buildContentSecurityPolicy({
   nonce,
   isDev = false,
+  reportOnly = false,
   imageOrigin = null,
   formActionOrigin = null,
 }: CspOptions): string {
@@ -169,20 +181,35 @@ export function buildContentSecurityPolicy({
     // in a production build.
     `script-src 'self' 'nonce-${nonce}' 'strict-dynamic'${isDev ? " 'unsafe-eval'" : ''}`,
 
-    // In dev, Next injects stylesheets through JS with no nonce available, so
-    // the nonce form would blank the page. In production, Tailwind and
-    // `next/font` are real files under `/_next/static`, covered by `'self'`.
-    isDev ? "style-src 'self' 'unsafe-inline'" : `style-src 'self' 'nonce-${nonce}'`,
-
-    // Deliberate loosening, do not "tighten" this back. Radix and vaul position
-    // every popover, dialog, select and drawer by writing inline `style`
-    // attributes, which `style-src-attr` governs and which a nonce cannot
-    // cover — a nonce applies to `<style>` elements, not to attributes. Without
-    // this, every floating UI surface in the app renders in the wrong place.
-    // The XSS value of an inline style attribute is low on its own; the
-    // directive exists so that `style-src` above can stay strict for real
-    // stylesheets.
-    "style-src-attr 'unsafe-inline'",
+    // Deliberate loosening, do not "tighten" this back to a nonce.
+    //
+    // This directive originally read `style-src 'self' 'nonce-...'` plus a
+    // separate `style-src-attr 'unsafe-inline'`, on the theory that a nonce
+    // could cover real stylesheets while the attr directive covered Radix's
+    // inline `style` attributes. Enforcing the policy in a browser disproved
+    // it (2026-09-12): Radix's dialog pulls in react-remove-scroll, which locks
+    // body scroll by INJECTING A <style> ELEMENT at runtime. That is an
+    // element, not an attribute, so `style-src-attr` does not apply and it
+    // falls through to `style-src` — where a nonce cannot help, because the
+    // element is created by script long after the server chose the nonce.
+    // Under enforcement the browser blocked it and the dialog broke with
+    // React error #441.
+    //
+    // A hash is not a workable alternative: the blocked content includes the
+    // computed scrollbar width, so it varies by platform and zoom level. Two
+    // different hashes showed up in a single page view.
+    //
+    // `'unsafe-inline'` is therefore the honest answer, and it must appear
+    // WITHOUT a nonce — a nonce in the same directive makes CSP3 browsers
+    // ignore `'unsafe-inline'` entirely, which is the trap that produced the
+    // broken policy above. `style-src-attr` is gone as redundant: this covers
+    // attributes and elements alike.
+    //
+    // The security cost is real but small: inline STYLE injection can do
+    // limited data exfiltration via selectors, but not script execution. The
+    // control that matters, `script-src` with a nonce and `strict-dynamic`,
+    // is untouched.
+    "style-src 'self' 'unsafe-inline'",
 
     // `data:` and `blob:` are next/image's placeholder and preview machinery.
     `img-src 'self' data: blob:${imageOrigin ? ` ${imageOrigin}` : ''}`,
@@ -209,8 +236,10 @@ export function buildContentSecurityPolicy({
   ];
 
   // Production only: the directive rewrites http subresource URLs to https,
-  // which is exactly wrong against a local http dev server.
-  if (!isDev) {
+  // which is exactly wrong against a local http dev server. Also omitted in
+  // report-only mode, where browsers ignore it and log an error saying so on
+  // every page — noise that buries the real violation reports.
+  if (!isDev && !reportOnly) {
     directives.push('upgrade-insecure-requests');
   }
 
@@ -220,19 +249,26 @@ export function buildContentSecurityPolicy({
 /**
  * Which CSP header name to send.
  *
- * Ships report-only. A nonce-based CSP is the one security header that can
- * white-screen an app — a page Next prerendered at build time has no nonce in
- * its inline bootstrap script, and a missed third-party origin is invisible
- * until a user hits that exact screen. Report-only puts the violations in the
- * browser console, and in a report endpoint if one is ever configured, without
- * breaking anything.
+ * ENFORCES by default. `CSP_ENFORCE=false` drops back to report-only as an
+ * escape hatch, and only that exact string does — any other value, including
+ * unset or a typo, enforces.
+ *
+ * This shipped report-only first, deliberately: a nonce-based CSP is the one
+ * security header that can white-screen an app, so the policy was trialled in
+ * a browser before it was allowed to block anything. That trial happened
+ * 2026-09-12 against a production build — sign-in, contact search with MP
+ * photos, contact detail, the Radix dialog, the Select inside it, the user
+ * menu and sign-out to MP — and it earned its keep: enforcement caught a
+ * blocked runtime-injected `<style>` that report-only had NOT reported (see
+ * the style-src comment above). Once that was fixed, the enforced walk came
+ * back clean, so the default flipped.
  *
- * Set `CSP_ENFORCE=true` to switch to enforcement. Any other value, including
- * unset, stays report-only; enforcement is opt-in rather than opt-out so that
- * a typo in this variable can never take down a deploy.
+ * The escape hatch is deliberately inverted from the old default. Report-only
+ * is now the unusual state — something you turn on to diagnose a violation,
+ * not the state a deploy drifts into by forgetting a variable.
  */
 export function cspHeaderName(
-  enforce: boolean = process.env.CSP_ENFORCE === 'true'
+  enforce: boolean = process.env.CSP_ENFORCE !== 'false'
 ): 'Content-Security-Policy' | 'Content-Security-Policy-Report-Only' {
   return enforce ? 'Content-Security-Policy' : 'Content-Security-Policy-Report-Only';
 }