From 400f6f5824332ee8aab7f6d2c839cb02848ba428 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sat, 10 Oct 2026 13:36:52 -0400 Subject: [PATCH 1/3] feat(security): harden headers, proxy matching and CSP sources - next.config: COOP/CORP same-origin, poweredByHeader off, images.unoptimized (no next/image in use), logging.serverFunctions off so `next dev` stops printing server-action arguments. - proxy: /api/auth public check matches the segment, not the prefix; matcher exclusions anchored so look-alike paths stay gated. - CSP: MP origins accepted only over https (loopback http in dev), base-uri 'none', opt-in CSP_REPORT_URI emitting report-uri/report-to plus Reporting-Endpoints. Co-Authored-By: Claude Opus 5.5 (1M context) --- .env.example | 8 ++++ next.config.ts | 22 +++++++++ src/lib/security-headers.test.ts | 77 +++++++++++++++++++++++++++++++- src/lib/security-headers.ts | 76 ++++++++++++++++++++++++++++--- src/proxy.test.ts | 70 ++++++++++++++++++++++++++++- src/proxy.ts | 29 ++++++++++-- 6 files changed, 270 insertions(+), 12 deletions(-) diff --git a/.env.example b/.env.example index baa3324..c29857a 100644 --- a/.env.example +++ b/.env.example @@ -109,6 +109,14 @@ MP_SECURITY_ROLES=* # silent (no policy). Set to "false" only to diagnose a violation. CSP_ENFORCE= +# Optional CSP violation collector (Sentry, report-uri.com, or your own route). +# Unset = no reporting. When set, the policy gains `report-uri` + `report-to` +# and responses carry a matching `Reporting-Endpoints` header. Must be https:// +# (http://localhost is accepted in development only); a value that is not, or +# that contains `;`, `,`, `"` or whitespace, is ignored. When adding a new +# origin, run with CSP_ENFORCE=false (report-only) for one release cycle first. +CSP_REPORT_URI= + # Client IP for sign-in rate limiting (comma-separated header names, tried in # order). Name ONLY a header your edge always OVERWRITES — a header clients can # set themselves lets them dodge the limit or lock someone else out. Never use a diff --git a/next.config.ts b/next.config.ts index 9ac0c6d..466a6d7 100644 --- a/next.config.ts +++ b/next.config.ts @@ -18,6 +18,12 @@ const securityHeaders = [ { key: "X-Frame-Options", value: "DENY" }, { key: "X-Content-Type-Options", value: "nosniff" }, { key: "Referrer-Policy", value: "strict-origin-when-cross-origin" }, + // Cross-origin isolation. COOP severs `window.opener` between this app and + // any page it opens or is opened by; sign-in is a full-page redirect to MP, + // not a popup, so nothing relies on that link. CORP stops other origins from + // embedding our responses (images, JSON, chunks) at all. + { key: "Cross-Origin-Opener-Policy", value: "same-origin" }, + { key: "Cross-Origin-Resource-Policy", value: "same-origin" }, { key: "Permissions-Policy", value: "camera=(), microphone=(), geolocation=(), payment=(), usb=()", @@ -37,6 +43,22 @@ const hstsHeader = { }; const nextConfig: NextConfig = { + // Drops `X-Powered-By: Next.js` — free version fingerprinting for scanners. + poweredByHeader: false, + // Nothing in `src/` uses `next/image`, so the `/_next/image` optimizer is + // pure attack surface (its URL-fetching has carried SSRF advisories). With + // `unoptimized` it never fetches anything; a future `` still renders, + // just served as-is. + images: { + unoptimized: true, + }, + logging: { + // `next dev` otherwise prints every server action's ARGUMENTS to the + // terminal — household lists, contact records — while a developer is often + // pointed at production MP. CLAUDE.md rule 14 forbids logging record + // content; this closes the one path that bypassed it. + serverFunctions: false, + }, experimental: { serverActions: { // Default is 1 MB, which made the address-label mail merge's documented diff --git a/src/lib/security-headers.test.ts b/src/lib/security-headers.test.ts index fa74d37..abc7a0c 100644 --- a/src/lib/security-headers.test.ts +++ b/src/lib/security-headers.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect } from 'vitest'; -import { buildCsp, cspHeaderName, generateNonce } from './security-headers'; +import { + buildCsp, + cspHeaderName, + cspReportUri, + generateNonce, + reportingEndpointsHeader, +} from './security-headers'; /** * Security-header tests. @@ -71,7 +77,7 @@ describe('buildCsp', () => { expect(csp).toContain("frame-ancestors 'none'"); expect(csp).toContain("object-src 'none'"); expect(csp).toContain("frame-src 'none'"); - expect(csp).toContain("base-uri 'self'"); + expect(csp).toContain("base-uri 'none'"); }); it('omits upgrade-insecure-requests when report-only', () => { @@ -104,6 +110,73 @@ describe('buildCsp', () => { expect(csp).not.toContain('undefined'); expect(csp).not.toContain('null'); }); + + it.each([ + ['plain http to a real host', 'http://mp.example.org/ministryplatformapi'], + ['a data: URL (whose origin is the string "null")', 'data:,x'], + ['a javascript: URL', 'javascript:alert(1)'], + ['a credentialed URL', 'https://user:pass@mp.example.org/api'], + ])('drops %s from the policy instead of trusting it', (_label, url) => { + const csp = buildCsp({ nonce: 'n', mpBaseUrl: url, mpFileUrl: url }); + expect(csp).toContain("form-action 'self';"); + expect(csp).toContain("img-src 'self' data: blob:;"); + }); + + it('accepts http://localhost only in development', () => { + const local = 'http://localhost:4000/api'; + expect(buildCsp({ nonce: 'n', isDev: true, mpBaseUrl: local })).toContain( + "form-action 'self' http://localhost:4000", + ); + expect(buildCsp({ nonce: 'n', mpBaseUrl: local })).toContain("form-action 'self';"); + }); + + it('emits no reporting directives by default', () => { + expect(buildCsp(base)).not.toContain('report-'); + }); + + it('emits report-uri and report-to when a collector is configured', () => { + const csp = buildCsp({ ...base, reportUri: 'https://r.example.org/csp' }); + expect(csp).toContain('report-uri https://r.example.org/csp'); + expect(csp).toContain('report-to csp-endpoint'); + }); +}); + +describe('cspReportUri', () => { + it('is off when unset or blank', () => { + expect(cspReportUri(undefined)).toBeNull(); + expect(cspReportUri('')).toBeNull(); + }); + + it('accepts an https collector, keeping its path', () => { + expect(cspReportUri(' https://o1.ingest.sentry.io/api/1/security/?sentry_key=k ')).toBe( + 'https://o1.ingest.sentry.io/api/1/security/?sentry_key=k', + ); + }); + + it('refuses non-https and malformed values', () => { + expect(cspReportUri('http://reports.example.org/csp')).toBeNull(); + expect(cspReportUri('not a url')).toBeNull(); + expect(cspReportUri('http://localhost:3000/csp')).toBeNull(); + expect(cspReportUri('http://localhost:3000/csp', true)).toBe('http://localhost:3000/csp'); + }); + + it('refuses characters that would break out of the directive or header', () => { + // `new URL` percent-encodes spaces and quotes but leaves `;` and `,` alone. + expect(cspReportUri('https://r.example.org/a;script-src *')).toBeNull(); + expect(cspReportUri('https://r.example.org/a,b')).toBeNull(); + }); +}); + +describe('reportingEndpointsHeader', () => { + it('is null when reporting is off', () => { + expect(reportingEndpointsHeader(null)).toBeNull(); + }); + + it('names the same group the CSP reports to', () => { + expect(reportingEndpointsHeader('https://r.example.org/csp')).toBe( + 'csp-endpoint="https://r.example.org/csp"', + ); + }); }); describe('generateNonce', () => { diff --git a/src/lib/security-headers.ts b/src/lib/security-headers.ts index bb738a3..6749bfc 100644 --- a/src/lib/security-headers.ts +++ b/src/lib/security-headers.ts @@ -31,14 +31,65 @@ export function cspHeaderName( : "Content-Security-Policy-Report-Only"; } -/** Reduces a configured URL to a bare scheme+host origin, or null. */ -function originOf(value: string | undefined): string | null { +/** Loopback hosts allowed over plain http, and only in development. */ +const LOOPBACK_HOSTNAMES = new Set(["localhost", "127.0.0.1", "[::1]"]); + +/** + * Parses a configured URL, accepting only `https:` (or `http:` to a loopback + * host in development). Anything else — `http:` to a real host, `javascript:`, + * `data:`, a credentialed URL, garbage — is null, so a bad env value drops the + * source from the policy instead of widening it. `new URL()` alone would + * happily turn `data:,x` into the origin `"null"`, and an `http:` origin would + * let a network attacker serve the "trusted" images. + */ +function safeUrl(value: string | undefined, isDev: boolean): URL | null { if (!value) return null; + let url: URL; try { - return new URL(value).origin; + url = new URL(value.trim()); } catch { return null; } + const loopbackHttp = + isDev && url.protocol === "http:" && LOOPBACK_HOSTNAMES.has(url.hostname); + if (url.protocol !== "https:" && !loopbackHttp) return null; + if (url.username || url.password) return null; + return url; +} + +/** Reduces a configured URL to a bare scheme+host origin, or null. */ +function originOf(value: string | undefined, isDev: boolean): string | null { + return safeUrl(value, isDev)?.origin ?? null; +} + +/** The `report-to` group name, shared by the CSP and `Reporting-Endpoints`. */ +const REPORT_GROUP = "csp-endpoint"; + +/** + * The validated CSP report collector (`CSP_REPORT_URI`), or null when unset or + * not a safe URL. Reporting is OPT-IN: this repo ships no collector, so each + * deployment points it at one it owns (Sentry, report-uri.com, ...). + * + * Characters that would break out of a header value or a directive (`;`, `,`, + * `"`, whitespace) are refused rather than escaped — a legitimate collector URL + * never needs them. + */ +export function cspReportUri( + value: string | undefined = process.env.CSP_REPORT_URI, + isDev: boolean = false, +): string | null { + const url = safeUrl(value, isDev); + if (!url) return null; + const href = url.href; + return /[\s;,"]/.test(href) ? null : href; +} + +/** + * The `Reporting-Endpoints` header value that `report-to` resolves against, or + * null when reporting is off. Sent alongside the CSP by the proxy. + */ +export function reportingEndpointsHeader(reportUri: string | null): string | null { + return reportUri ? `${REPORT_GROUP}="${reportUri}"` : null; } export interface CspOptions { @@ -48,6 +99,8 @@ export interface CspOptions { reportOnly?: boolean; mpBaseUrl?: string; mpFileUrl?: string; + /** Validated collector URL (see `cspReportUri`); null/omitted = no reporting. */ + reportUri?: string | null; } /** @@ -88,9 +141,10 @@ export function buildCsp({ reportOnly = false, mpBaseUrl = process.env.MINISTRY_PLATFORM_BASE_URL, mpFileUrl = process.env.NEXT_PUBLIC_MINISTRY_PLATFORM_FILE_URL, + reportUri = null, }: CspOptions): string { - const mpOrigin = originOf(mpBaseUrl); - const fileOrigin = originOf(mpFileUrl); + const mpOrigin = originOf(mpBaseUrl, isDev); + const fileOrigin = originOf(mpFileUrl, isDev); const imgSources = ["'self'", "data:", "blob:", mpOrigin, fileOrigin].filter( (v, i, a): v is string => Boolean(v) && a.indexOf(v) === i, @@ -111,7 +165,10 @@ export function buildCsp({ `connect-src 'self'${isDev ? " ws: wss:" : ""}`, `object-src 'none'`, `frame-src 'none'`, - `base-uri 'self'`, + // 'none', not 'self': nothing here emits a tag, and 'self' would + // still let an injected silently re-root every relative + // URL on the page. + `base-uri 'none'`, `form-action ${formActions.join(" ")}`, `frame-ancestors 'none'`, ]; @@ -120,6 +177,13 @@ export function buildCsp({ directives.push("upgrade-insecure-requests"); } + // Both spellings: `report-to` (with the `Reporting-Endpoints` header) is the + // current standard; `report-uri` is what Firefox still honours, and browsers + // that support `report-to` ignore `report-uri` when both are present. + if (reportUri) { + directives.push(`report-uri ${reportUri}`, `report-to ${REPORT_GROUP}`); + } + return directives.join("; "); } diff --git a/src/proxy.test.ts b/src/proxy.test.ts index c11efdc..1badd16 100644 --- a/src/proxy.test.ts +++ b/src/proxy.test.ts @@ -107,6 +107,24 @@ describe('proxy', () => { expect(mockNext).not.toHaveBeenCalled(); }); + it.each(['/api/authz', '/api/auth-admin', '/api/authorize'])( + 'does NOT treat %s as public just because it shares the /api/auth prefix', + async (path) => { + mockGetSessionCookie.mockReturnValueOnce(null); + + await proxy(createMockRequest(path)); + + expect(mockGetSessionCookie).toHaveBeenCalled(); + expect(mockRedirect).toHaveBeenCalled(); + }, + ); + + it('treats the bare /api/auth path as public', async () => { + await proxy(createMockRequest('/api/auth')); + + expect(mockGetSessionCookie).not.toHaveBeenCalled(); + }); + it('should allow /signin path without session check', async () => { const request = createMockRequest('/signin'); @@ -230,9 +248,35 @@ describe('proxy', () => { const pattern = config.matcher[0]; expect(pattern).toContain('_next/static'); expect(pattern).toContain('_next/image'); - expect(pattern).toContain('favicon.ico'); + expect(pattern).toContain('favicon\\.ico'); expect(pattern).toContain('assets/'); }); + + // Next compiles the matcher with path-to-regexp, which anchors it at both + // ends; a plain anchored RegExp is a faithful stand-in for the custom group. + const matches = (path: string) => + new RegExp(`^${config.matcher[0]}$`).test(path); + + it.each([ + '/_next/static/chunks/main.js', + '/_next/image', + '/favicon.ico', + '/assets/logo.png', + ])('skips the exempt path %s', (path) => { + expect(matches(path)).toBe(false); + }); + + it.each([ + '/', + '/tools/addresslabels', + '/favicon.ico-admin', + '/faviconXico', + '/_next/imagery', + '/_next/staticky', + '/assets-admin', + ])('still runs on %s, which only shares a prefix with an exempt path', (path) => { + expect(matches(path)).toBe(true); + }); }); }); @@ -292,6 +336,30 @@ describe('proxy - Content Security Policy', () => { expect(nonceOf(a)).not.toBe(nonceOf(b)); }); + it('sends no Reporting-Endpoints header when CSP_REPORT_URI is unset', async () => { + vi.stubEnv('CSP_REPORT_URI', ''); + const result = await proxy(createMockRequest('/a')); + const headers = (result as unknown as { headers: Headers }).headers; + + expect(headers.get('Reporting-Endpoints')).toBeNull(); + expect(headers.get('Content-Security-Policy')).not.toContain('report-'); + vi.unstubAllEnvs(); + }); + + it('wires CSP_REPORT_URI into the policy and the Reporting-Endpoints header', async () => { + vi.stubEnv('CSP_REPORT_URI', 'https://reports.example.org/csp'); + const result = await proxy(createMockRequest('/a')); + const headers = (result as unknown as { headers: Headers }).headers; + + expect(headers.get('Reporting-Endpoints')).toBe( + 'csp-endpoint="https://reports.example.org/csp"', + ); + expect(headers.get('Content-Security-Policy')).toContain( + 'report-uri https://reports.example.org/csp; report-to csp-endpoint', + ); + vi.unstubAllEnvs(); + }); + it('sets the CSP on redirects as well', async () => { mockGetSessionCookie.mockReturnValue(null); diff --git a/src/proxy.ts b/src/proxy.ts index cd16a5d..cc2611d 100644 --- a/src/proxy.ts +++ b/src/proxy.ts @@ -1,6 +1,12 @@ import { NextResponse, NextRequest } from 'next/server'; import { getSessionCookie } from 'better-auth/cookies'; -import { buildCsp, cspHeaderName, generateNonce } from '@/lib/security-headers'; +import { + buildCsp, + cspHeaderName, + cspReportUri, + generateNonce, + reportingEndpointsHeader, +} from '@/lib/security-headers'; /** * Paths reachable without a session. @@ -19,7 +25,10 @@ import { buildCsp, cspHeaderName, generateNonce } from '@/lib/security-headers'; */ function isPublicPath(pathname: string): boolean { return ( - pathname.startsWith('/api/auth') || + // Exact segment match: a bare `startsWith('/api/auth')` would also make a + // future `/api/authz` or `/api/auth-admin` route public by accident. + pathname === '/api/auth' || + pathname.startsWith('/api/auth/') || pathname === '/signin' || pathname === '/auth-error' ); @@ -32,10 +41,13 @@ export async function proxy(request: NextRequest) { const isDev = process.env.NODE_ENV !== 'production'; const headerName = cspHeaderName(); const nonce = generateNonce(); + const reportUri = cspReportUri(process.env.CSP_REPORT_URI, isDev); + const reportingEndpoints = reportingEndpointsHeader(reportUri); const csp = buildCsp({ nonce, isDev, reportOnly: headerName === 'Content-Security-Policy-Report-Only', + reportUri, }); // Forward the original URL (pathname + search) to downstream server @@ -56,6 +68,9 @@ export async function proxy(request: NextRequest) { const withCsp = (response: NextResponse) => { response.headers.set(headerName, csp); + if (reportingEndpoints) { + response.headers.set('Reporting-Endpoints', reportingEndpoints); + } return response; }; @@ -90,8 +105,16 @@ export async function proxy(request: NextRequest) { } } +/** + * Every path except Next's static chunks, the image endpoint, the favicon and + * `public/assets/`. Each exclusion is ANCHORED — to a trailing `/` for the + * directories and to end-of-path (`$`) for the two single files — so a page + * named `/favicon.ico-admin` or `/_next/imagery` cannot slip past the session + * check and the CSP by sharing a prefix with an exempt path. The `.` in + * `favicon.ico` is escaped for the same reason. + */ export const config = { matcher: [ - '/((?!_next/static|_next/image|favicon.ico|assets/).*)', + '/((?!_next/static/|_next/image$|favicon\\.ico$|assets/).*)', ], }; From 5ddfc8eac06b851d077f952dc2b3f24312963a8f Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sat, 10 Oct 2026 13:40:46 -0400 Subject: [PATCH 2/3] feat(build): server-only guards, prerender check, build in CI - `server-only` on every service, auth, env, tool-params.server and the MP client/credentials modules; a client import now fails `next build` (verified by planting one). Vitest aliases it to the no-op entry; the codegen CLIs run tsx with --conditions=react-server. - scripts/check-prerender.mjs fails when any route besides Next's built-in /_not-found and /_global-error is statically prerendered (no CSP nonce). - CI `test` job now runs `npm run build` (placeholder env) and the prerender check. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/test.yml | 21 +++++++ package-lock.json | 7 +++ package.json | 8 ++- scripts/check-prerender.mjs | 55 +++++++++++++++++++ src/lib/auth.ts | 1 + src/lib/env.ts | 2 + .../auth/client-credentials.ts | 1 + src/lib/providers/ministry-platform/client.ts | 1 + src/lib/providers/ministry-platform/helper.ts | 1 + src/lib/tool-params.server.ts | 1 + src/services/addressLabelService.ts | 1 + src/services/authorizationService.ts | 1 + src/services/domainTimezoneService.ts | 1 + src/services/familyService.ts | 1 + src/services/fieldManagementService.ts | 1 + src/services/googlePlacesService.ts | 1 + src/services/groupService.ts | 1 + src/services/toolService.ts | 1 + src/services/userService.ts | 1 + vitest.config.mts | 5 ++ 20 files changed, 109 insertions(+), 3 deletions(-) create mode 100644 scripts/check-prerender.mjs diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1a14124..25170ac 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -57,6 +57,27 @@ jobs: - name: Test with coverage run: npm run test:coverage + # Catches what tests and tsc cannot: a `server-only` module pulled into + # a client component, Turbopack graph errors, and boot-time env guards. + # The values are placeholders that satisfy `src/lib/env.ts` and the + # secret guard in `src/lib/auth.ts`; the build makes no MP call (OIDC + # discovery is lazy), so nothing here needs to be real or secret. + - name: Build + run: npm run build + env: + BETTER_AUTH_SECRET: ci-build-only-placeholder-secret-0123456789abcdef + BETTER_AUTH_URL: http://localhost:3000 + MINISTRY_PLATFORM_BASE_URL: https://mp.example.invalid/ministryplatformapi + MINISTRY_PLATFORM_CLIENT_ID: ci-build + MINISTRY_PLATFORM_CLIENT_SECRET: ci-build-placeholder + MP_SECURITY_ROLES: "*" + NEXT_PUBLIC_MINISTRY_PLATFORM_FILE_URL: https://mp.example.invalid/ministryplatformapi/files + + # A statically prerendered route has no CSP nonce, so it would never + # hydrate under the enforced policy. See scripts/check-prerender.mjs. + - name: Check prerendered routes + run: npm run check:prerender + - uses: codecov/codecov-action@v5 if: always() with: diff --git a/package-lock.json b/package-lock.json index af0029e..77b6851 100644 --- a/package-lock.json +++ b/package-lock.json @@ -41,6 +41,7 @@ "react": "^19.3.0", "react-dom": "^19.3.0", "react-hook-form": "^7.89.0", + "server-only": "^0.0.1", "sonner": "^2.0.8", "tailwind-merge": "^3.7.0", "tsx": "^4.23.15", @@ -10480,6 +10481,12 @@ "semver": "bin/semver.js" } }, + "node_modules/server-only": { + "version": "0.0.1", + "resolved": "https://registry.npmjs.org/server-only/-/server-only-0.0.1.tgz", + "integrity": "sha512-qepMx2JxAa5jjfzxG79yPPq+8BuFToHd1hm7kI+Z4zAq1ftQiP7HcxMhDDItrbtwVeLg/cY2JnKnrcFkmiswNA==", + "license": "MIT" + }, "node_modules/set-cookie-parser": { "version": "3.1.3", "resolved": "https://registry.npmjs.org/set-cookie-parser/-/set-cookie-parser-3.1.3.tgz", diff --git a/package.json b/package.json index 5a269cf..b650fdf 100644 --- a/package.json +++ b/package.json @@ -9,14 +9,15 @@ "dev": "next dev", "build": "tsx src/lib/providers/ministry-platform/scripts/build-sql-install.ts && next build", "start": "next start", + "check:prerender": "node scripts/check-prerender.mjs", "lint": "eslint .", "typecheck": "tsc --noEmit", "test": "vitest", "test:run": "vitest run", "test:coverage": "vitest run --coverage", - "mp:generate": "tsx src/lib/providers/ministry-platform/scripts/generate-types.ts", - "mp:generate:models": "tsx src/lib/providers/ministry-platform/scripts/generate-types.ts -o src/lib/providers/ministry-platform/models --zod --clean", - "mp:generate:storedprocs": "tsx src/lib/providers/ministry-platform/scripts/generate-storedprocs.ts", + "mp:generate": "tsx --conditions=react-server src/lib/providers/ministry-platform/scripts/generate-types.ts", + "mp:generate:models": "tsx --conditions=react-server src/lib/providers/ministry-platform/scripts/generate-types.ts -o src/lib/providers/ministry-platform/models --zod --clean", + "mp:generate:storedprocs": "tsx --conditions=react-server src/lib/providers/ministry-platform/scripts/generate-storedprocs.ts", "mp:build:install": "tsx src/lib/providers/ministry-platform/scripts/build-sql-install.ts", "setup": "tsx scripts/setup.ts", "setup:check": "tsx scripts/setup.ts --check", @@ -56,6 +57,7 @@ "react": "^19.3.0", "react-dom": "^19.3.0", "react-hook-form": "^7.89.0", + "server-only": "^0.0.1", "sonner": "^2.0.8", "tailwind-merge": "^3.7.0", "tsx": "^4.23.15", diff --git a/scripts/check-prerender.mjs b/scripts/check-prerender.mjs new file mode 100644 index 0000000..af90ecc --- /dev/null +++ b/scripts/check-prerender.mjs @@ -0,0 +1,55 @@ +#!/usr/bin/env node +/** + * Fails when `next build` statically prerendered a route that has no business + * being static. Run after `npm run build` (CI does). + * + * WHY: the CSP in `src/proxy.ts` is nonce-based, and Next reads the nonce off + * the incoming request at render time. A route prerendered at build time has no + * request and therefore no nonce, so under the enforced CSP its scripts are + * blocked and it never hydrates — silently, with no build error. A static page + * also skips every server-side session and role check. `src/app/signin` was + * bitten by exactly this; see the `force-dynamic` note in its `page.tsx`. + * + * ALLOWED are Next's two built-in shells, both static by design and neither + * dependent on hydration to be useful: + * /_not-found plain 404 markup; its links work without JavaScript + * /_global-error only ever rendered client-side, after a crash + * Anything else showing up here means a route lost its dynamic signal (a + * `headers()`/session read, or `export const dynamic = "force-dynamic"` in a + * SERVER module). Fix the route; do not widen this list without the same kind + * of written justification. + */ +import { readFileSync } from "node:fs"; +import path from "node:path"; + +const ALLOWED_STATIC = new Set(["/_not-found", "/_global-error"]); + +const manifestPath = path.join(process.cwd(), ".next", "prerender-manifest.json"); + +let manifest; +try { + manifest = JSON.parse(readFileSync(manifestPath, "utf8")); +} catch { + console.error(`check-prerender: cannot read ${manifestPath} — run \`npm run build\` first.`); + process.exit(1); +} + +const prerendered = [ + ...Object.keys(manifest.routes ?? {}), + ...Object.keys(manifest.dynamicRoutes ?? {}), +]; +const unexpected = prerendered.filter((route) => !ALLOWED_STATIC.has(route)); + +if (unexpected.length > 0) { + console.error("check-prerender: these routes were statically prerendered:"); + for (const route of unexpected) console.error(` ${route}`); + console.error( + "A prerendered page has no CSP nonce (it will not hydrate under the enforced policy) " + + "and skips server-side auth. Make the route dynamic; see scripts/check-prerender.mjs.", + ); + process.exit(1); +} + +console.log( + `check-prerender: OK (${prerendered.length} prerendered, all allowed: ${prerendered.join(", ") || "none"})`, +); diff --git a/src/lib/auth.ts b/src/lib/auth.ts index c43bf7d..6851af1 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -1,3 +1,4 @@ +import "server-only"; import { betterAuth, BetterAuthOptions } from "better-auth"; import { APIError, createAuthMiddleware } from "better-auth/api"; import { diff --git a/src/lib/env.ts b/src/lib/env.ts index 766adb4..958a314 100644 --- a/src/lib/env.ts +++ b/src/lib/env.ts @@ -20,6 +20,8 @@ * unit tested without re-importing anything. */ +import "server-only"; + type Env = Readonly>; /** Loopback hosts allowed over plain http (see `loopbackHttpInProduction`). */ diff --git a/src/lib/providers/ministry-platform/auth/client-credentials.ts b/src/lib/providers/ministry-platform/auth/client-credentials.ts index 5d61d50..ba9ecb2 100644 --- a/src/lib/providers/ministry-platform/auth/client-credentials.ts +++ b/src/lib/providers/ministry-platform/auth/client-credentials.ts @@ -1,3 +1,4 @@ +import "server-only"; import { getMpBaseUrl } from "@/lib/env"; import { readJsonResponse } from "../utils/http-client"; import { errorName } from "../utils/logger"; diff --git a/src/lib/providers/ministry-platform/client.ts b/src/lib/providers/ministry-platform/client.ts index 3d333d2..72a3d9c 100644 --- a/src/lib/providers/ministry-platform/client.ts +++ b/src/lib/providers/ministry-platform/client.ts @@ -1,3 +1,4 @@ +import "server-only"; import { getClientCredentialsToken, type CredentialProfile } from "./auth/client-credentials"; import { HttpClient } from "./utils/http-client"; import { errorName, logger } from "./utils/logger"; diff --git a/src/lib/providers/ministry-platform/helper.ts b/src/lib/providers/ministry-platform/helper.ts index eba9900..ca9eef7 100644 --- a/src/lib/providers/ministry-platform/helper.ts +++ b/src/lib/providers/ministry-platform/helper.ts @@ -1,3 +1,4 @@ +import "server-only"; import { MinistryPlatformProvider } from "./provider"; import { TableQueryParams, diff --git a/src/lib/tool-params.server.ts b/src/lib/tool-params.server.ts index 5d2a09a..0a9a4d2 100644 --- a/src/lib/tool-params.server.ts +++ b/src/lib/tool-params.server.ts @@ -1,3 +1,4 @@ +import "server-only"; import type { PageData, ToolParams } from "./tool-params"; /** diff --git a/src/services/addressLabelService.ts b/src/services/addressLabelService.ts index 325a7ec..b77a6dc 100644 --- a/src/services/addressLabelService.ts +++ b/src/services/addressLabelService.ts @@ -1,3 +1,4 @@ +import 'server-only'; import { MPHelper } from '@/lib/providers/ministry-platform'; import { AuthorizationService } from '@/services/authorizationService'; import { validatePositiveInt } from '@/lib/validation'; diff --git a/src/services/authorizationService.ts b/src/services/authorizationService.ts index 6d198e6..2664e03 100644 --- a/src/services/authorizationService.ts +++ b/src/services/authorizationService.ts @@ -1,3 +1,4 @@ +import "server-only"; import { cache } from "react"; import { headers } from "next/headers"; import { auth } from "@/lib/auth"; diff --git a/src/services/domainTimezoneService.ts b/src/services/domainTimezoneService.ts index 5d8a655..7cfa6ba 100644 --- a/src/services/domainTimezoneService.ts +++ b/src/services/domainTimezoneService.ts @@ -1,3 +1,4 @@ +import 'server-only'; import { MPHelper } from '@/lib/providers/ministry-platform'; /** diff --git a/src/services/familyService.ts b/src/services/familyService.ts index f8126e6..6d87663 100644 --- a/src/services/familyService.ts +++ b/src/services/familyService.ts @@ -1,3 +1,4 @@ +import "server-only"; import { MPHelper } from "@/lib/providers/ministry-platform"; import { AuthorizationService } from "@/services/authorizationService"; import { diff --git a/src/services/fieldManagementService.ts b/src/services/fieldManagementService.ts index 7631212..0d01389 100644 --- a/src/services/fieldManagementService.ts +++ b/src/services/fieldManagementService.ts @@ -1,3 +1,4 @@ +import "server-only"; import { MPHelper } from "@/lib/providers/ministry-platform"; import { AuthorizationService } from "@/services/authorizationService"; import type { TableMetadata } from "@/lib/providers/ministry-platform/types/provider.types"; diff --git a/src/services/googlePlacesService.ts b/src/services/googlePlacesService.ts index 1c3f8f1..435c762 100644 --- a/src/services/googlePlacesService.ts +++ b/src/services/googlePlacesService.ts @@ -1,3 +1,4 @@ +import "server-only"; import { MPHelper } from "@/lib/providers/ministry-platform"; import { GooglePlacesProvider } from "@/lib/providers/google-places"; import type { PlacePrediction, PlaceDetails } from "@/lib/providers/google-places"; diff --git a/src/services/groupService.ts b/src/services/groupService.ts index 8219919..553650a 100644 --- a/src/services/groupService.ts +++ b/src/services/groupService.ts @@ -1,3 +1,4 @@ +import 'server-only'; import { MPHelper } from '@/lib/providers/ministry-platform'; import { AuthorizationService } from '@/services/authorizationService'; import { escapeFilterString, validatePositiveInt } from '@/lib/validation'; diff --git a/src/services/toolService.ts b/src/services/toolService.ts index 09def91..9e1d960 100644 --- a/src/services/toolService.ts +++ b/src/services/toolService.ts @@ -1,3 +1,4 @@ +import "server-only"; import { MPHelper } from "@/lib/providers/ministry-platform"; import { PageData } from "@/lib/tool-params"; import { AuthorizationService } from "@/services/authorizationService"; diff --git a/src/services/userService.ts b/src/services/userService.ts index 9b7c468..525045e 100644 --- a/src/services/userService.ts +++ b/src/services/userService.ts @@ -1,3 +1,4 @@ +import "server-only"; import { MPUserProfile } from "@/lib/providers/ministry-platform/types"; import { MPHelper } from "@/lib/providers/ministry-platform"; import { validateGuid } from "@/lib/validation"; diff --git a/vitest.config.mts b/vitest.config.mts index 2d3b647..8a180c1 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -67,6 +67,11 @@ export default defineConfig({ // `import.meta.dirname`, not `__dirname`: this file is ESM (.mts), and // Vite's native config loader has no CommonJS globals. '@': path.resolve(import.meta.dirname, './src'), + // `server-only` throws unless resolved under the `react-server` export + // condition, which Next applies to server code and Vitest does not. Map + // it to the package's own no-op entry so modules marked server-only can + // still be unit tested. The guard itself is exercised by `next build`. + 'server-only': path.resolve(import.meta.dirname, './node_modules/server-only/empty.js'), }, }, }); From 175f3d389c47534c72f3ec908d538b7794194bac Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sat, 10 Oct 2026 14:03:39 -0400 Subject: [PATCH 3/3] docs: record step-6 hardening, CSP reporting and the 2026-10-10 walk; close TODOs - security reference: new headers, proxy path matching, CSP source validation, CSP_REPORT_URI, customer-clone notes, prerender check, and the enforced-CSP walk result. - GOTCHA-046 (server-only) and GOTCHA-047 (prerender check). - CLAUDE.md: CI now builds; server-only practice; codegen --conditions flag. - Delete the step6 and held-CSP TODOs; index at 0 open. Co-Authored-By: Claude Opus 5.5 (1M context) --- ...026-09-30-held-csp-customer-deployments.md | 32 ---------- ...26-09-30-step6-headers-csp-build-guards.md | 27 -------- .claude/TODO/INDEX.md | 27 +++----- .claude/references/GOTCHAS.md | 18 ++++++ .../references/mp-provider/type-generation.md | 2 +- .claude/references/security/README.md | 61 +++++++++++++++++-- CLAUDE.md | 18 +++--- README.md | 4 +- .../ministry-platform/docs/README.md | 2 +- src/services/domainTimezoneService.test.ts | 2 +- 10 files changed, 101 insertions(+), 92 deletions(-) delete mode 100644 .claude/TODO/2026-09-30-held-csp-customer-deployments.md delete mode 100644 .claude/TODO/2026-09-30-step6-headers-csp-build-guards.md diff --git a/.claude/TODO/2026-09-30-held-csp-customer-deployments.md b/.claude/TODO/2026-09-30-held-csp-customer-deployments.md deleted file mode 100644 index 1b17763..0000000 --- a/.claude/TODO/2026-09-30-held-csp-customer-deployments.md +++ /dev/null @@ -1,32 +0,0 @@ ---- -title: "Held — CSP enforcement notes for customer-deployed clones (report endpoint, browser walk)" -severity: medium -tags: [security, doc] -area: routing -files: [src/lib/security-headers.ts, src/proxy.ts] -discovered: 2026-09-30 -discovered_by: playbook-audit (port-downstream-hardening.md F9) -status: open ---- - -## Problem -Owner decision (2026-09-30): **skip for now; document for customer clones.** The nonce CSP (`security-headers.ts`, -`proxy.ts:33-59`) is enforced by default via `CSP_ENFORCE`, but: -- No CSP report endpoint is configured. -- The enforced-CSP browser walk (sign-in, sign-out, images, every Radix surface) hasn't been done - against a production build. - -## Notes for customer-deployed clones -- **Report endpoint:** set `report-to`/`report-uri` to a collector the customer owns (Sentry, report-uri.com, or a - small route that logs shape only). Run `CSP_ENFORCE=false` (report-only) for at least one release cycle first. -- **Customer additions:** any analytics, chat widget or embedded MP widget a customer adds needs its own origin - added — do it per deployment, never with `'unsafe-inline'` or a wildcard. -- **Walk:** after any CSP change, `npm run build && npm start`, open DevTools console, walk every tool; zero CSP - violations is the bar. - -## Impact if not fixed -Violations go unseen without a report endpoint. - -## Update (2026-10-10) -The GrapesJS font item is gone: the template editor was removed, and it was the only third-party -origin the CSP blocked. What remains is the report endpoint and the enforced-CSP browser walk. diff --git a/.claude/TODO/2026-09-30-step6-headers-csp-build-guards.md b/.claude/TODO/2026-09-30-step6-headers-csp-build-guards.md deleted file mode 100644 index 5f110e1..0000000 --- a/.claude/TODO/2026-09-30-step6-headers-csp-build-guards.md +++ /dev/null @@ -1,27 +0,0 @@ ---- -title: "Step 6 — Security headers, next.config hardening, server-only guards, prerender + build in CI" -severity: medium -tags: [security, drift] -area: routing -files: [next.config.ts, src/proxy.ts, src/lib/security-headers.ts, .github/workflows/test.yml, src/services/domainTimezoneService.ts] -discovered: 2026-09-30 -discovered_by: playbook-audit (port-downstream-hardening.md F9, port-security-review-2026-09-28.md P11/P14) -status: open ---- - -## Problem -- `next.config.ts:17-51` lacks COOP/CORP `same-origin`, `poweredByHeader:false`, `images.unoptimized`, - `logging.serverFunctions:false` — the last matters most: `next dev` prints server-action arguments (household - lists) while pointed at production MP. -- `proxy.ts:22` `startsWith('/api/auth')` has no trailing slash; matcher (`:95`) unanchored; `originOf` doesn't - validate scheme/host; CSP `base-uri 'self'` (upstream `'none'`). -- No `server-only` anywhere (e.g. `domainTimezoneService`, services); no `scripts/check-prerender.mjs`; CI never - runs `npm run build`. - -## Proposed fix -Port per the playbooks. Verify `server-only` by planting a client import that must break `next build`. -**The CSP enforcement walk is held** (the GrapesJS fonts question went away with the template editor, removed 2026-10-10) — see -`2026-09-30-held-csp-customer-deployments.md`. - -## Impact if not fixed -Server-action payloads in dev logs; weaker cross-origin isolation; server code can silently enter the client bundle. diff --git a/.claude/TODO/INDEX.md b/.claude/TODO/INDEX.md index a4e22a0..8a74d61 100644 --- a/.claude/TODO/INDEX.md +++ b/.claude/TODO/INDEX.md @@ -18,10 +18,7 @@ Severity tiers: - **medium**: doc drift, missing test, refactor with real cost - **low**: nits, minor doc fixes, stylistic improvements -Total: **2 open TODOs**. - -> **`step6`** is the remaining step from the 2026-09-30 MPNext playbook audit. -> `held-*` items wait on an owner decision. +Total: **0 open TODOs**. --- @@ -33,11 +30,8 @@ _none open_ ### High (0) _none open_ -### Medium (2) -| Area | Tags | Title | File | -|---|---|---|---| -| routing | security, drift | Step 6 — Headers, next.config, server-only, prerender + build in CI | [→](2026-09-30-step6-headers-csp-build-guards.md) | -| routing | security, doc | Held — CSP notes for customer-deployed clones | [→](2026-09-30-held-csp-customer-deployments.md) | +### Medium (0) +_none open_ ### Low (0) _none open_ @@ -46,15 +40,14 @@ _none open_ ## By tag -### security (2) -- step6-headers-csp-build-guards — medium -- held-csp-customer-deployments — medium +### security (0) +_none open_ ### bug (0) _none open_ -### drift (1) -- step6-headers-csp-build-guards — medium +### drift (0) +_none open_ ### missing-test (0) _none open_ @@ -62,8 +55,8 @@ _none open_ ### refactor (0) _none open_ -### doc (1) -- held-csp-customer-deployments — medium +### doc (0) +_none open_ ### perf (0) _none open_ @@ -75,7 +68,7 @@ _none open_ | Area | Count | |---|---| | components | 0 | -| routing | 2 | +| routing | 0 | | services | 0 | | auth | 0 | | commands | 0 | diff --git a/.claude/references/GOTCHAS.md b/.claude/references/GOTCHAS.md index b3e8b81..ea7da8f 100644 --- a/.claude/references/GOTCHAS.md +++ b/.claude/references/GOTCHAS.md @@ -48,6 +48,8 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs - GOTCHA-027: Next.js errors "sync dynamic API usage" or undefined `params` - GOTCHA-028: New public route redirects to `/signin` in a loop - GOTCHA-029: CI runs `next lint` and fails — command unknown +- GOTCHA-046: `This module cannot be imported from a Client Component module` from `server-only` +- GOTCHA-047: CI `check:prerender` fails — a route was statically prerendered ### Frontend / components - GOTCHA-030: `useUser()` throws "useUser must be used within a UserProvider" @@ -323,6 +325,22 @@ Known pitfalls for agents working on MPNext-Tools. Each entry has a symptom-firs --- +### GOTCHA-046: `This module cannot be imported from a Client Component module` from `server-only` + +**Symptom:** One of three, depending on where the import runs: `next build` fails with *"You're importing a module that depends on "server-only""* and a `[Client Component Browser]` import trace; `npm run mp:generate*` (or a hand-run `tsx` on the codegen scripts) throws from `node_modules/server-only/index.js`; or a new Vitest file throws the same error. +**Root cause:** Every service, `src/lib/auth.ts`, `src/lib/env.ts`, `src/lib/tool-params.server.ts` and the MP client/credentials modules start with `import 'server-only'`. That package throws unless resolved under the `react-server` export condition, which Next applies to server code only. +**Fix:** In the build case the guard is doing its job — the trace names the client component that reached a server module; use `import type` or call a server action instead. Codegen scripts must run as `tsx --conditions=react-server …` (the `package.json` scripts already do). Vitest aliases `server-only` to the package's `empty.js` in `vitest.config.mts`; do not remove that alias. +**Enforced where:** `next build` (CI `Build` step); `vitest.config.mts` `resolve.alias`; `package.json` `mp:generate*` scripts. +**Related:** [security/README.md](security/README.md), GOTCHA-047 + +### GOTCHA-047: CI `check:prerender` fails — a route was statically prerendered + +**Symptom:** The `Check prerendered routes` CI step lists a route; locally, the build table shows it as `○ (Static)`. +**Root cause:** The CSP nonce is read from the request at render time. A prerendered page has none, so under the enforced CSP it never hydrates — and it also skips server-side session checks. +**Fix:** Give the route a dynamic signal: a `headers()`/session read, or `export const dynamic = "force-dynamic"` in a **server** module (segment config is ignored in a `"use client"` file, the same trap as GOTCHA-032). Only Next's `/_not-found` and `/_global-error` may be static. +**Enforced where:** `scripts/check-prerender.mjs`, run by CI after `npm run build`. +**Related:** [security/README.md](security/README.md) § Nonces force dynamic rendering + ## Frontend / components ### GOTCHA-030: `useUser()` throws "useUser must be used within a UserProvider" diff --git a/.claude/references/mp-provider/type-generation.md b/.claude/references/mp-provider/type-generation.md index 74c147d..05cdccb 100644 --- a/.claude/references/mp-provider/type-generation.md +++ b/.claude/references/mp-provider/type-generation.md @@ -154,7 +154,7 @@ npx tsx src/lib/providers/ministry-platform/scripts/generate-types.ts -o ./types npx tsx src/lib/providers/ministry-platform/scripts/generate-types.ts -d --sample-size 10 # Help -npx tsx src/lib/providers/ministry-platform/scripts/generate-types.ts --help +npx tsx --conditions=react-server src/lib/providers/ministry-platform/scripts/generate-types.ts --help ``` The canonical npm script: diff --git a/.claude/references/security/README.md b/.claude/references/security/README.md index ecc1e0e..1ae25e1 100644 --- a/.claude/references/security/README.md +++ b/.claude/references/security/README.md @@ -167,8 +167,24 @@ Split across two files, and the split is not arbitrary: | Where | Headers | Why there | |---|---|---| -| `next.config.ts` on `/(.*)` | `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy`, `Permissions-Policy`, HSTS (prod only) | Request-independent, and reaches `/api` plus the static paths the proxy matcher skips | -| `src/proxy.ts` via `src/lib/security-headers.ts` | `Content-Security-Policy` | The nonce must be fresh per request; a build-time value is a constant an attacker reads off any page | +| `next.config.ts` on `/(.*)` | `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy`, `Permissions-Policy`, `Cross-Origin-Opener-Policy` / `Cross-Origin-Resource-Policy` (`same-origin`), HSTS (prod only) | Request-independent, and reaches `/api` plus the static paths the proxy matcher skips | +| `src/proxy.ts` via `src/lib/security-headers.ts` | `Content-Security-Policy`, plus `Reporting-Endpoints` when `CSP_REPORT_URI` is set | The nonce must be fresh per request; a build-time value is a constant an attacker reads off any page | + +`next.config.ts` also sets `poweredByHeader: false`, `images.unoptimized: true` +(nothing uses `next/image`, so the optimizer was only SSRF surface) and +`logging.serverFunctions: false` (otherwise `next dev` prints every server +action's arguments, i.e. household records, to the terminal). + +COOP `same-origin` is safe only because sign-in is a **full-page redirect**. A +popup-based OAuth flow would lose `window.opener` and break. + +### Proxy path matching + +The public-path check matches `/api/auth` **as a segment** (`=== '/api/auth'` or +`startsWith('/api/auth/')`), so a future `/api/authz` is not public by accident. +The matcher's exclusions are anchored (`_next/static/`, `_next/image$`, +`favicon\.ico$`, `assets/`) so a look-alike path such as `/favicon.ico-admin` +still gets the session check and the CSP. `src/proxy.test.ts` pins both. Anti-framing is expressed **twice on purpose** — `X-Frame-Options` reaches the routes the proxy skips, `frame-ancestors` covers the rest. They are not both CSP @@ -195,6 +211,13 @@ a browser-vendor list and the deploying church's call, not a repo default. `form-action` to the **whole redirect chain**, not just its first hop. 3. **`img-src` includes the MP origins**, for contact photos served from MP. +Those MP origins are accepted only over `https:` (`http:` to a loopback host in +dev). `data:`, `javascript:`, plain-http real hosts and credentialed URLs are +**dropped** from the policy rather than trusted — `new URL('data:,x').origin` +is the string `"null"`, which would otherwise land in the directive. +`base-uri` is `'none'`: nothing emits a ``, and `'self'` would still let +an injected one re-root every relative URL. + ### Nonces force dynamic rendering Next reads the nonce off the **incoming request headers** at render time, so the @@ -210,8 +233,12 @@ in `sign-in-content.tsx` and its `page.tsx` is a server component. `src/app/signin/page.test.tsx` pins **both** halves — the export, and the absence of the directive. -`/_not-found` remains static and nonce-less. Accepted: it is Next's built-in 404 -with no interactivity to lose. +`/_not-found` and `/_global-error` remain static and nonce-less. Accepted: the +404 has no interactivity to lose, and the global error shell renders client-side. + +**CI enforces this.** After `npm run build`, `npm run check:prerender` +(`scripts/check-prerender.mjs`) fails if any other route was prerendered. Fix +the route; widen its allowlist only with a written justification. ### `CSP_ENFORCE` @@ -225,6 +252,32 @@ policy can block things a clean report-only pass never flagged. Walk sign-in, sign-out, images and **every Radix surface** (dropdown, dialog, select, tooltip) against a production build before calling it done. +Last walk: **2026-10-10**, enforced policy, production build — sign-in, every +tool, Radix select/popover/command and dialog (scroll-lock `