From 818c979479da4959cb0550b2a62f3cd9c4ac73af Mon Sep 17 00:00:00 2001 From: Joel Brock Date: Fri, 30 Oct 2026 11:17:28 -0700 Subject: [PATCH 1/6] Replace raw staff key in URLs with signed, org-bound tokens The Engagement Report tab wrote STAFF_REPORT_KEY into the iframe src for every user with "access CiviCRM", without checking contact permissions. Anyone could copy it from the DOM and read any org's report and files, bypassing Civi ACLs; the key never expired and landed in access logs. - Extension (0.4.0): Tab.php checks Contact_Permission::allow(VIEW) and mints sig = base64url(HMAC-SHA256(key, "webform-mw.staff-report.v1: :")) with an 8h exp. The secret no longer leaves the server. - App: lib/staff-token.mjs verifies the token (constant-time, org must match, rejects expired or >12h tokens). /staff/report, /api/staff/report and /api/staff/file share staffAuthQuery(); client links carry exp/sig. - Legacy ?key= is still accepted (constant-time compare, logs a warning) so the app can deploy before the extension. Remove it once every Civi install runs extension 0.4.0. - PHP/Node signature compatibility is pinned by a PHP-generated vector. --- app/api/staff/file/route.ts | 17 ++-- app/api/staff/report/route.ts | 17 ++-- app/staff/report/page.tsx | 15 ++-- .../webform-mw/CRM/WebformMw/Page/Tab.php | 35 ++++++++- civi-extension/webform-mw/README.md | 18 +++-- civi-extension/webform-mw/info.xml | 4 +- components/StaffReportView.tsx | 31 ++++---- components/report/FileLink.tsx | 9 ++- lib/staff-auth.ts | 54 ++++++++++--- lib/staff-token.mjs | 78 +++++++++++++++++++ lib/staff-token.test.mjs | 64 +++++++++++++++ package.json | 3 +- 12 files changed, 282 insertions(+), 63 deletions(-) create mode 100644 lib/staff-token.mjs create mode 100644 lib/staff-token.test.mjs diff --git a/app/api/staff/file/route.ts b/app/api/staff/file/route.ts index a90e435..9249699 100644 --- a/app/api/staff/file/route.ts +++ b/app/api/staff/file/route.ts @@ -1,5 +1,5 @@ /** - * GET /api/staff/file?id=&org=&key= + * GET /api/staff/file?id=&org=&exp=&sig= * * Server-side proxy that fetches a CiviCRM attachment and re-streams it * with `Content-Disposition: inline`, so the staff-report lightbox can @@ -8,7 +8,8 @@ * its UI but wrong for an embedded preview. * * Authorization layers: - * 1. STAFF_REPORT_KEY query param (same gate as /api/staff/report). + * 1. Signed org-bound token (same gate as /api/staff/report; see + * lib/staff-auth.ts). The token's org must match `org`. * 2. Per-file ownership probe: the requested fileId must appear as a * value in one of the org's file-typed custom-field columns, OR in * one of the org's activities' file-typed columns. This is the same @@ -25,7 +26,7 @@ */ import { NextRequest, NextResponse } from "next/server"; -import { isStaffKeyValid } from "@/lib/staff-auth"; +import { staffAuthQuery } from "@/lib/staff-auth"; import { civi, civi3 } from "@/lib/civicrm"; import { resolveMime } from "@/lib/mime.mjs"; @@ -204,12 +205,18 @@ async function fileBelongsToOrg(fileId: number, orgId: number): Promise export async function GET(req: NextRequest) { const url = new URL(req.url); - const key = url.searchParams.get("key"); const idStr = url.searchParams.get("id"); const orgStr = url.searchParams.get("org"); const wantsDownload = url.searchParams.get("dl") === "1"; - if (!isStaffKeyValid(key)) { + if ( + !staffAuthQuery({ + org: orgStr, + exp: url.searchParams.get("exp"), + sig: url.searchParams.get("sig"), + key: url.searchParams.get("key"), + }) + ) { return new NextResponse("Not found", { status: 404 }); } diff --git a/app/api/staff/report/route.ts b/app/api/staff/report/route.ts index a7d3733..3e15dd4 100644 --- a/app/api/staff/report/route.ts +++ b/app/api/staff/report/route.ts @@ -1,18 +1,19 @@ /** - * GET /api/staff/report?org=&key= + * GET /api/staff/report?org=&exp=&sig= * * Returns a comprehensive read-only StaffReportPayload for the given * organization. Field list is built at request time from * CustomField.get (no static config). * - * Auth: STAFF_REPORT_KEY must match the `key` query param. + * Auth: a signed org-bound token (or the transitional legacy `key`); see + * lib/staff-auth.ts. * * STUB MODE: if CiviCRM env vars are unset, returns a fabricated payload * exercising every render kind so the staff page is usable in dev. */ import { NextRequest, NextResponse } from "next/server"; -import { isStaffKeyValid } from "@/lib/staff-auth"; +import { staffAuthQuery } from "@/lib/staff-auth"; import { civi, civi3 } from "@/lib/civicrm"; import { mapCustomFieldRow } from "@/lib/staff-field-mapping.mjs"; import type { @@ -226,10 +227,16 @@ function buildStubPayload(orgId: number): StaffReportPayload { export async function GET(req: NextRequest) { const url = new URL(req.url); - const key = url.searchParams.get("key"); const orgStr = url.searchParams.get("org"); - if (!isStaffKeyValid(key)) { + if ( + !staffAuthQuery({ + org: orgStr, + exp: url.searchParams.get("exp"), + sig: url.searchParams.get("sig"), + key: url.searchParams.get("key"), + }) + ) { // Don't leak whether the route exists. return new NextResponse("Not found", { status: 404 }); } diff --git a/app/staff/report/page.tsx b/app/staff/report/page.tsx index f0eef31..a7b17e1 100644 --- a/app/staff/report/page.tsx +++ b/app/staff/report/page.tsx @@ -1,10 +1,10 @@ import { Suspense } from "react"; import { StaffReportView } from "@/components/StaffReportView"; import { SiteHeader, SiteFooter } from "@/components/SiteChrome"; -import { isStaffKeyValid } from "@/lib/staff-auth"; +import { staffAuthQuery } from "@/lib/staff-auth"; interface PageProps { - searchParams: Promise<{ org?: string; key?: string; frame?: string }>; + searchParams: Promise<{ org?: string; key?: string; exp?: string; sig?: string; frame?: string }>; } export const metadata = { @@ -13,12 +13,13 @@ export const metadata = { }; export default async function StaffReportPage({ searchParams }: PageProps) { - const { org, key, frame } = await searchParams; + const { org, key, exp, sig, frame } = await searchParams; const isFramed = frame === "1"; - // Generic "not found" if the key is missing or wrong — don't confirm - // route existence. - if (!isStaffKeyValid(key)) { + // Generic "not found" if the token/key is missing or wrong — don't + // confirm route existence. + const authQuery = staffAuthQuery({ org, exp, sig, key }); + if (!authQuery) { return ; } @@ -43,7 +44,7 @@ export default async function StaffReportPage({ searchParams }: PageProps) { diff --git a/civi-extension/webform-mw/CRM/WebformMw/Page/Tab.php b/civi-extension/webform-mw/CRM/WebformMw/Page/Tab.php index 4a07a9f..48c88ae 100644 --- a/civi-extension/webform-mw/CRM/WebformMw/Page/Tab.php +++ b/civi-extension/webform-mw/CRM/WebformMw/Page/Tab.php @@ -7,24 +7,41 @@ * template emits a single iframe pointing at the WebForm-mw staff report * plus a small postMessage listener that auto-sizes the iframe to the * report's content height. + * + * The iframe URL carries a signed, org-bound, expiring token rather than + * the shared secret itself — see lib/staff-token.mjs in the app. We only + * mint it after confirming the current user can view this contact, so + * Civi's contact ACLs carry through to the embedded report. */ class CRM_WebformMw_Page_Tab extends CRM_Core_Page { + /** + * Token lifetime. The app rejects anything valid for more than 12 hours. + */ + const TOKEN_TTL_SECONDS = 8 * 60 * 60; + public function run() { - $cid = CRM_Utils_Request::retrieve('cid', 'Positive', $this, TRUE); + $cid = (int) CRM_Utils_Request::retrieve('cid', 'Positive', $this, TRUE); + + if (!CRM_Contact_BAO_Contact_Permission::allow($cid, CRM_Core_Permission::VIEW)) { + CRM_Utils_System::permissionDenied(); + return; + } $appUrl = _webform_mw_app_url(); $key = _webform_mw_staff_key(); $configured = ($appUrl !== '' && $key !== ''); if ($configured) { + $exp = time() + self::TOKEN_TTL_SECONDS; $src = $appUrl . '/staff/report' - . '?org=' . urlencode((string) $cid) - . '&key=' . urlencode($key) + . '?org=' . $cid + . '&exp=' . $exp + . '&sig=' . urlencode(self::sign($key, $cid, $exp)) . '&frame=1'; $this->assign('iframeSrc', $src); // Expose just the app origin so the parent-side postMessage - // listener can validate event.origin without leaking the secret. + // listener can validate event.origin. $this->assign('appOrigin', parse_url($appUrl, PHP_URL_SCHEME) . '://' . parse_url($appUrl, PHP_URL_HOST)); } @@ -32,4 +49,14 @@ class CRM_WebformMw_Page_Tab extends CRM_Core_Page { parent::run(); } + + /** + * base64url(HMAC-SHA256(key, "webform-mw.staff-report.v1::")). + * Must stay byte-for-byte compatible with signStaffToken() in the app. + */ + private static function sign(string $key, int $org, int $exp): string { + $mac = hash_hmac('sha256', "webform-mw.staff-report.v1:{$org}:{$exp}", $key, TRUE); + return rtrim(strtr(base64_encode($mac), '+/', '-_'), '='); + } + } diff --git a/civi-extension/webform-mw/README.md b/civi-extension/webform-mw/README.md index c9dfce3..dc1aac1 100644 --- a/civi-extension/webform-mw/README.md +++ b/civi-extension/webform-mw/README.md @@ -53,7 +53,7 @@ without prior context can self-serve. - Visible only on **Organization** contacts (Individuals and Households see no tab). The check happens server-side in the tabset hook. - Tab body is an iframe pointing at - `${WEBFORM_MW_APP_URL}/staff/report?org=&key=&frame=1`. + `${WEBFORM_MW_APP_URL}/staff/report?org=&exp=&sig=&frame=1`. - `frame=1` tells the embedded app to suppress its site header/footer and emit a `postMessage({type:"webform-mw-height", height})` payload on render and on resize. The tab's small inline script listens for @@ -63,13 +63,15 @@ without prior context can self-serve. ## Security notes -- The staff secret travels with each tab render inside the iframe `src`. - Anyone permitted to view the Engagement Report tab (i.e., anyone with - CiviCRM access) can read the URL in their browser's DevTools and reuse - the secret to view any org's report. Acceptable model if "CiviCRM - access" and "should view any staff report" overlap; otherwise consider - upgrading the embedded app to per-user signed tokens and minting them - in the page controller. +- The shared secret never leaves the server. Each tab render mints a + signed token, `HMAC-SHA256(secret, "webform-mw.staff-report.v1::")`, + valid for one org for 8 hours. The app rejects tokens for other orgs, + expired tokens, and anything valid for longer than 12 hours. +- Tokens are only minted after `CRM_Contact_BAO_Contact_Permission::allow()` + confirms the current user can view the org, so Civi contact ACLs carry + through to the embedded report. +- Rotating the secret: change `WEBFORM_MW_STAFF_KEY` and the app's + `STAFF_REPORT_KEY` together; outstanding tokens stop working at once. - The extension uses `access CiviCRM` as its access argument — anyone with that permission sees the tab on Organization contacts. Tighten by changing the `` value in `xml/Menu/webform_mw.xml` diff --git a/civi-extension/webform-mw/info.xml b/civi-extension/webform-mw/info.xml index f4b6e8e..7a5704c 100644 --- a/civi-extension/webform-mw/info.xml +++ b/civi-extension/webform-mw/info.xml @@ -16,8 +16,8 @@ https://github.com/joelbrock/WebForm-mw - 2026-06-05 - 0.3.1 + 2026-10-30 + 0.4.0 beta 5.50 diff --git a/components/StaffReportView.tsx b/components/StaffReportView.tsx index b1eb6b3..52d0630 100644 --- a/components/StaffReportView.tsx +++ b/components/StaffReportView.tsx @@ -23,7 +23,8 @@ import { FileLink } from "./report/FileLink"; interface StaffReportViewProps { org: number; - authKey: string; + /** Validated staff auth params (`exp=&sig=` or legacy `key=`), sans org. */ + authQuery: string; /** CIVI_BASE_URL, used to build outbound file links. */ civiBaseUrl: string; /** True when the page is being embedded in a CiviCRM tab via iframe. */ @@ -39,7 +40,7 @@ const STAGE_OPTION_GROUP_ID = 75; export function StaffReportView({ org, - authKey, + authQuery, civiBaseUrl, framed = false, }: StaffReportViewProps) { @@ -50,7 +51,7 @@ export function StaffReportView({ (async () => { try { const res = await fetch( - `/api/staff/report?org=${encodeURIComponent(String(org))}&key=${encodeURIComponent(authKey)}`, + `/api/staff/report?org=${encodeURIComponent(String(org))}&${authQuery}`, { cache: "no-store" }, ); if (!res.ok) { @@ -72,7 +73,7 @@ export function StaffReportView({ return () => { alive = false; }; - }, [org, authKey]); + }, [org, authQuery]); // When embedded, post our content height to the parent so the Civi tab's // iframe can resize to fit (no nested scrollbars). The receiving script @@ -209,7 +210,7 @@ export function StaffReportView({ options={data.options} civiBaseUrl={civiBaseUrl} org={org} - authKey={authKey} + authQuery={authQuery} /> ))} @@ -275,13 +276,13 @@ function StaffSection({ options, civiBaseUrl, org, - authKey, + authQuery, }: { section: StaffReportSection; options: Record; civiBaseUrl: string; org: number; - authKey: string; + authQuery: string; }) { const filled = section.fields.filter((f) => f.history.length > 0); const empty = section.fields.filter((f) => f.history.length === 0); @@ -336,7 +337,7 @@ function StaffSection({ options={options} civiBaseUrl={civiBaseUrl} org={org} - authKey={authKey} + authQuery={authQuery} /> ))} @@ -384,13 +385,13 @@ function CompactFieldRow({ options, civiBaseUrl, org, - authKey, + authQuery, }: { field: StaffReportField; options: Record; civiBaseUrl: string; org: number; - authKey: string; + authQuery: string; }) { const [open, setOpen] = useState(false); const latest = field.history[0]; @@ -408,7 +409,7 @@ function CompactFieldRow({ options={options} civiBaseUrl={civiBaseUrl} org={org} - authKey={authKey} + authQuery={authQuery} /> {latest.date ? ( @@ -447,7 +448,7 @@ function CompactFieldRow({ options={options} civiBaseUrl={civiBaseUrl} org={org} - authKey={authKey} + authQuery={authQuery} /> @@ -464,14 +465,14 @@ function FieldValue({ options, civiBaseUrl, org, - authKey, + authQuery, }: { field: StaffReportField; entry: FieldHistoryEntry; options: Record; civiBaseUrl: string; org: number; - authKey: string; + authQuery: string; }) { if (field.descriptor.render === "file") { const v = entry.value as @@ -487,7 +488,7 @@ function FieldValue({ civiSignedUrl={v.url} mime={v.mime} org={org} - authKey={authKey} + authQuery={authQuery} civiBaseUrl={civiBaseUrl} /> ); diff --git a/components/report/FileLink.tsx b/components/report/FileLink.tsx index 6ab5c43..de221cb 100644 --- a/components/report/FileLink.tsx +++ b/components/report/FileLink.tsx @@ -12,7 +12,7 @@ import { categoryFromMime, resolveMime } from "@/lib/mime.mjs"; * - office -> download link + "View in Google Docs" secondary link * - other -> plain download link * - * The proxy URL is `/api/staff/file?id=&org=&key=` (re-streams with + * The proxy URL is `/api/staff/file?id=&org=&` (re-streams with * Content-Disposition: inline for previews, or `&dl=1` for downloads). The * Civi-signed URL (carrying its short-lived fcs JWT) is passed straight to * Google's Docs Viewer for office files; we deliberately don't proxy that @@ -24,7 +24,7 @@ export function FileLink({ civiSignedUrl, mime: explicitMime, org, - authKey, + authQuery, civiBaseUrl, }: { fileId: number | string; @@ -33,7 +33,8 @@ export function FileLink({ civiSignedUrl?: string; mime?: string; org: number; - authKey: string; + /** Validated staff auth params (`exp=&sig=` or legacy `key=`), sans org. */ + authQuery: string; civiBaseUrl: string; }) { const [open, setOpen] = useState(false); @@ -44,7 +45,7 @@ export function FileLink({ const proxyBase = `/api/staff/file?id=${encodeURIComponent(String(fileId))}` + `&org=${encodeURIComponent(String(org))}` + - `&key=${encodeURIComponent(authKey)}`; + `&${authQuery}`; const previewSrc = proxyBase; // Prefer Civi's signed URL for downloads when present (one fewer hop // through our Lambda); the proxy is the fallback. diff --git a/lib/staff-auth.ts b/lib/staff-auth.ts index 0fa79ae..f9acba6 100644 --- a/lib/staff-auth.ts +++ b/lib/staff-auth.ts @@ -1,18 +1,48 @@ /** - * Shared-secret auth for the internal staff routes. + * Auth for the internal staff routes (/staff/report, /api/staff/report, + * /api/staff/file). * - * Staff hit URLs of the form /staff/report?org=&key=. The - * secret is read from the STAFF_REPORT_KEY env var. If unset, the routes - * refuse every request (closed by default). + * Preferred: signed, org-bound, expiring tokens minted by the CiviCRM + * extension — /staff/report?org=&exp=&sig=. See + * lib/staff-token.mjs. The extension only mints a token after checking the + * Civi user can view that org, so Civi ACLs carry through. * - * Stub mode (CIVI_* unset) does NOT bypass this check — we want to test - * the auth surface in dev too. For local dev, set STAFF_REPORT_KEY=dev in - * .env.local. + * Legacy (transitional): ?key=. Still accepted so the app + * can deploy before the updated extension reaches the Civi server. Remove + * the legacy branch once every Civi install emits signed tokens. + * + * If STAFF_REPORT_KEY is unset, every request is refused (closed by + * default). Stub mode (CIVI_* unset) does NOT bypass this check — for local + * dev, set STAFF_REPORT_KEY=dev in .env.local. */ -export function isStaffKeyValid(key: string | null | undefined): boolean { - const expected = process.env.STAFF_REPORT_KEY; - if (!expected) return false; - if (!key) return false; - return key === expected; +import { safeEqual, verifyStaffToken } from "./staff-token.mjs"; + +interface StaffAuthParams { + org: string | null | undefined; + exp?: string | null; + sig?: string | null; + key?: string | null; +} + +/** + * Validate staff credentials for `org`. Returns the query-string fragment + * (without `org`) that downstream fetches and file links must carry, or + * null when unauthorized. + */ +export function staffAuthQuery({ org, exp, sig, key }: StaffAuthParams): string | null { + const secret = process.env.STAFF_REPORT_KEY; + if (!secret) return null; + + if (exp || sig) { + return verifyStaffToken(secret, org, exp, sig) + ? `exp=${encodeURIComponent(exp!)}&sig=${encodeURIComponent(sig!)}` + : null; + } + + if (key && safeEqual(key, secret)) { + console.warn("[staff-auth] legacy ?key= access used; update the CiviCRM extension to signed tokens"); + return `key=${encodeURIComponent(key)}`; + } + return null; } diff --git a/lib/staff-token.mjs b/lib/staff-token.mjs new file mode 100644 index 0000000..9916324 --- /dev/null +++ b/lib/staff-token.mjs @@ -0,0 +1,78 @@ +// @ts-check +/** + * Signed, org-bound, expiring tokens for the internal staff report. + * + * The CiviCRM extension's Engagement Report tab mints a token per render: + * + * sig = base64url( HMAC-SHA256( STAFF_REPORT_KEY, + * "webform-mw.staff-report.v1::" ) ) + * + * and links to /staff/report?org=&exp=&sig=. The raw shared + * secret never leaves the two servers, and a captured URL only unlocks one + * org until it expires — unlike the legacy `?key=` form, which + * unlocked every org forever. + * + * The PHP side (civi-extension/.../Page/Tab.php) must build the message + * string byte-for-byte the same way. The fixed test vector in + * staff-token.test.mjs was produced by the PHP implementation. + * + * Written as JS+JSDoc rather than TS so Node's built-in --test runner can + * import this file directly — matches lib/mime.mjs. + */ + +import { createHmac, timingSafeEqual } from "node:crypto"; + +/** + * Longest validity window we accept, measured from now. The extension + * issues 8-hour tokens; the extra headroom absorbs clock skew between the + * Civi server and Amplify without letting anyone mint long-lived links. + */ +export const STAFF_TOKEN_MAX_TTL_SECONDS = 12 * 60 * 60; + +/** + * @param {string} org + * @param {string} exp + */ +function message(org, exp) { + return `webform-mw.staff-report.v1:${org}:${exp}`; +} + +/** + * @param {string} secret + * @param {string} org + * @param {string} exp + * @returns {string} + */ +export function signStaffToken(secret, org, exp) { + return createHmac("sha256", secret).update(message(org, exp)).digest("base64url"); +} + +/** + * Constant-time string comparison. Lengths may differ; that case returns + * false without calling timingSafeEqual (which would throw). + * + * @param {string} a + * @param {string} b + */ +export function safeEqual(a, b) { + const ab = Buffer.from(a); + const bb = Buffer.from(b); + return ab.length === bb.length && timingSafeEqual(ab, bb); +} + +/** + * @param {string | null | undefined} secret + * @param {string | null | undefined} org + * @param {string | null | undefined} exp + * @param {string | null | undefined} sig + * @param {number} [nowSeconds] + * @returns {boolean} + */ +export function verifyStaffToken(secret, org, exp, sig, nowSeconds = Math.floor(Date.now() / 1000)) { + if (!secret || !org || !exp || !sig) return false; + if (!/^[1-9]\d*$/.test(org) || !/^[1-9]\d*$/.test(exp)) return false; + const expN = Number(exp); + if (expN <= nowSeconds) return false; + if (expN - nowSeconds > STAFF_TOKEN_MAX_TTL_SECONDS) return false; + return safeEqual(signStaffToken(secret, org, exp), sig); +} diff --git a/lib/staff-token.test.mjs b/lib/staff-token.test.mjs new file mode 100644 index 0000000..2c46ded --- /dev/null +++ b/lib/staff-token.test.mjs @@ -0,0 +1,64 @@ +// Run with: npm run test:token +import test from "node:test"; +import assert from "node:assert/strict"; +import { + signStaffToken, + verifyStaffToken, + safeEqual, + STAFF_TOKEN_MAX_TTL_SECONDS, +} from "./staff-token.mjs"; + +const SECRET = "test-secret"; +const NOW = 1_800_000_000 - 3600; // one hour before the vector's exp + +// Produced by the PHP implementation in civi-extension Tab.php: +// rtrim(strtr(base64_encode(hash_hmac('sha256', +// 'webform-mw.staff-report.v1:1234:1800000000', 'test-secret', TRUE)), +// '+/', '-_'), '=') +const PHP_VECTOR = "HlcxJCgUHfa6fhuvldUxZmCk_y62TcuQ4dH2JjFSqCo"; + +test("signStaffToken matches the PHP implementation", () => { + assert.equal(signStaffToken(SECRET, "1234", "1800000000"), PHP_VECTOR); +}); + +test("verifyStaffToken accepts a valid, unexpired token", () => { + assert.equal(verifyStaffToken(SECRET, "1234", "1800000000", PHP_VECTOR, NOW), true); +}); + +test("verifyStaffToken rejects a token for a different org", () => { + assert.equal(verifyStaffToken(SECRET, "1235", "1800000000", PHP_VECTOR, NOW), false); +}); + +test("verifyStaffToken rejects a tampered exp", () => { + assert.equal(verifyStaffToken(SECRET, "1234", "1800000001", PHP_VECTOR, NOW), false); +}); + +test("verifyStaffToken rejects a wrong secret", () => { + assert.equal(verifyStaffToken("other-secret", "1234", "1800000000", PHP_VECTOR, NOW), false); +}); + +test("verifyStaffToken rejects an expired token", () => { + assert.equal(verifyStaffToken(SECRET, "1234", "1800000000", PHP_VECTOR, 1_800_000_000), false); +}); + +test("verifyStaffToken rejects exp too far in the future", () => { + const now = 1_800_000_000 - STAFF_TOKEN_MAX_TTL_SECONDS - 1; + assert.equal(verifyStaffToken(SECRET, "1234", "1800000000", PHP_VECTOR, now), false); +}); + +test("verifyStaffToken rejects missing or malformed inputs", () => { + assert.equal(verifyStaffToken("", "1234", "1800000000", PHP_VECTOR, NOW), false); + assert.equal(verifyStaffToken(SECRET, null, "1800000000", PHP_VECTOR, NOW), false); + assert.equal(verifyStaffToken(SECRET, "1234", undefined, PHP_VECTOR, NOW), false); + assert.equal(verifyStaffToken(SECRET, "1234", "1800000000", "", NOW), false); + assert.equal(verifyStaffToken(SECRET, "01234", "1800000000", PHP_VECTOR, NOW), false); + assert.equal(verifyStaffToken(SECRET, "1234", "18e8", PHP_VECTOR, NOW), false); + assert.equal(verifyStaffToken(SECRET, "-1", "1800000000", PHP_VECTOR, NOW), false); +}); + +test("safeEqual compares by value and tolerates length mismatch", () => { + assert.equal(safeEqual("abc", "abc"), true); + assert.equal(safeEqual("abc", "abd"), false); + assert.equal(safeEqual("abc", "abcd"), false); + assert.equal(safeEqual("", ""), true); +}); diff --git a/package.json b/package.json index 15a8eb5..d5fe79f 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,8 @@ "sync-help": "node --env-file=.env.local scripts/sync-help-from-civi.mjs", "test:mapping": "node --test lib/staff-field-mapping.test.mjs", "test:mime": "node --test lib/mime.test.mjs", - "test": "node --test lib/staff-field-mapping.test.mjs lib/mime.test.mjs" + "test:token": "node --test lib/staff-token.test.mjs", + "test": "node --test lib/staff-field-mapping.test.mjs lib/mime.test.mjs lib/staff-token.test.mjs" }, "dependencies": { "next": "16.4.3", From c9b610590eac10f14395b4516803e3d95a35165c Mon Sep 17 00:00:00 2001 From: Joel Brock Date: Fri, 30 Oct 2026 11:17:52 -0700 Subject: [PATCH 2/6] Fail closed in verifyChecksum; drop the Contact.get fallback If Contact.validateChecksum threw for any reason, verifyChecksum retried with Contact.get plus an unknown `checksum` param. Our API key is privileged, so if Civi ever ignored that param the probe would resolve the contact and accept any checksum. Treat errors as invalid instead, and reject non-positive/non-integer contact ids up front. --- app/api/health/route.ts | 7 +++---- lib/civicrm.ts | 25 +++++++++++-------------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/app/api/health/route.ts b/app/api/health/route.ts index ae9b67a..810ad40 100644 --- a/app/api/health/route.ts +++ b/app/api/health/route.ts @@ -245,9 +245,8 @@ async function probeStageOptionGroup(): Promise { async function probeChecksumApi(): Promise { // Tries to call validateChecksum with a deliberately invalid checksum. // If the API exists, we get back {valid: false} or similar — that's - // success for THIS check (the API responded). If the API doesn't exist - // we fall back to a Contact.get probe in verifyChecksum, which will - // also fail gracefully. + // success for THIS check (the API responded). If the API doesn't exist, + // verifyChecksum fails closed and every form link will be rejected. try { await civi("Contact", "validateChecksum", { contactId: 1, @@ -266,7 +265,7 @@ async function probeChecksumApi(): Promise { return { check: "contact_validate_checksum_api", ok: false, - detail: `validateChecksum action not present in this Civi version. The verifyChecksum() helper falls back to a Contact.get probe with cs param. Detail: ${msg}`, + detail: `validateChecksum action not present in this Civi version. verifyChecksum() fails closed, so every form link will be rejected until Civi is upgraded. Detail: ${msg}`, }; } return { diff --git a/lib/civicrm.ts b/lib/civicrm.ts index 02b7d4c..28e7439 100644 --- a/lib/civicrm.ts +++ b/lib/civicrm.ts @@ -297,32 +297,29 @@ export async function civiMultipart( } /** - * Validate a contact checksum (cid + cs) against CiviCRM. + * Validate a contact checksum (cid + cs) against CiviCRM via APIv4 + * Contact.validateChecksum. * - * APIv4 exposes Contact.validateChecksum in newer Civi versions. For older - * versions you may need to call Contact.get with the cs param and verify - * the contact resolves. We use validateChecksum here and fall back to a - * Contact.get probe if it returns a "missing API" error. + * Fails closed: any API error is treated as an invalid checksum. There is + * deliberately no fallback — our API key is privileged, so any probe that + * merely checks "does the contact resolve" would accept every checksum. + * /api/health confirms validateChecksum is available on the install. */ export async function verifyChecksum(cid: string, cs: string): Promise { if (isStubMode()) { // STUB: any non-empty cs is "valid" so the UI can be exercised locally. return Boolean(cid && cs); } + const contactId = Number(cid); + if (!Number.isSafeInteger(contactId) || contactId <= 0) return false; try { const res = await civi<{ valid: boolean }>("Contact", "validateChecksum", { - contactId: Number(cid), + contactId, checksum: cs, }); return Boolean(res.values?.[0]?.valid); } catch (e) { - // Fallback: try Contact.get with the checksum as `cs` URL param. If the - // contact resolves, the checksum is valid. - const res = await civi<{ id: number }>("Contact", "get", { - where: [["id", "=", Number(cid)]], - select: ["id"], - checksum: cs, - }); - return Array.isArray(res.values) && res.values.length === 1; + console.error("[civi] validateChecksum failed; treating as invalid:", e instanceof Error ? e.message : String(e)); + return false; } } From 0a19b57a8676d7ae5db5e9ac6743bede2678d7f1 Mon Sep 17 00:00:00 2001 From: Joel Brock Date: Fri, 30 Oct 2026 11:18:10 -0700 Subject: [PATCH 3/6] Harden the extension upload endpoint (CSRF, record ACL, mime allowlist) civicrm/webform-mw/upload accepted any session-authenticated POST, called civicrm_api3('Attachment','create') (which skips permission checks from PHP) against any contact/activity id, and stored any client mime type. - Require X-Requested-With: XMLHttpRequest (already sent by the app), so cross-site form posts can't ride a logged-in staff session. - Require EDIT permission on the target contact/activity. - Enforce the same mime allowlist as the app's /api/upload route. --- .../webform-mw/CRM/WebformMw/Page/Upload.php | 44 ++++++++++++++++++- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/civi-extension/webform-mw/CRM/WebformMw/Page/Upload.php b/civi-extension/webform-mw/CRM/WebformMw/Page/Upload.php index 14b0e30..673be86 100644 --- a/civi-extension/webform-mw/CRM/WebformMw/Page/Upload.php +++ b/civi-extension/webform-mw/CRM/WebformMw/Page/Upload.php @@ -28,17 +28,43 @@ * * Returns JSON: { id, name } on success, { error } with 4xx/5xx otherwise. * - * Authorization: `access CiviCRM`. AuthX is expected to authenticate the - * Bearer + Site-Key headers WebForm-mw sends. + * Authorization: `access CiviCRM`, plus edit permission on the target + * contact/activity. AuthX is expected to authenticate the Bearer + + * Site-Key headers WebForm-mw sends. + * + * CSRF: browsers can't attach a custom header to a cross-site form post + * without a CORS preflight, which Civi doesn't grant. Requiring + * `X-Requested-With: XMLHttpRequest` (which WebForm-mw already sends) + * stops a malicious page from riding a logged-in staff session. */ class CRM_WebformMw_Page_Upload extends CRM_Core_Page { + /** + * Mirrors ALLOWED_MIME in the app's /api/upload route. + */ + const ALLOWED_MIME = [ + 'application/pdf', + 'application/msword', + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + 'application/vnd.ms-excel', + 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + 'image/jpeg', + 'image/png', + 'image/gif', + 'image/webp', + ]; + public function run() { if (($_SERVER['REQUEST_METHOD'] ?? 'GET') !== 'POST') { $this->jsonError('POST required', 405); return; } + if (($_SERVER['HTTP_X_REQUESTED_WITH'] ?? '') !== 'XMLHttpRequest') { + $this->jsonError('Missing X-Requested-With header', 403); + return; + } + if (!CRM_Core_Permission::check('access CiviCRM')) { $this->jsonError('Permission denied', 403); return; @@ -69,9 +95,23 @@ class CRM_WebformMw_Page_Upload extends CRM_Core_Page { $this->jsonError('Invalid entity_id', 400); return; } + // civicrm_api3() from PHP skips permission checks, so check the target + // record here — otherwise any Civi user could attach files to any + // contact or activity. + $canEdit = $entityTable === 'civicrm_contact' + ? CRM_Contact_BAO_Contact_Permission::allow($entityId, CRM_Core_Permission::EDIT) + : CRM_Activity_BAO_Activity::checkPermission($entityId, CRM_Core_Permission::EDIT); + if (!$canEdit) { + $this->jsonError('Permission denied', 403); + return; + } $name = (string) ($_POST['name'] ?? $upload['name'] ?? 'upload'); $mime = (string) ($_POST['mime_type'] ?? $upload['type'] ?? 'application/octet-stream'); + if (!in_array($mime, self::ALLOWED_MIME, TRUE)) { + $this->jsonError('File type not allowed', 415); + return; + } try { $result = civicrm_api3('Attachment', 'create', [ From df471e0e88268c13c4b043c2ff2d506e8f72b080 Mon Sep 17 00:00:00 2001 From: Joel Brock Date: Fri, 30 Oct 2026 11:19:07 -0700 Subject: [PATCH 4/6] Verify client-supplied file ids on submit /api/submit wrote whatever {id} the client sent into file custom fields. A link holder could point their org at any civicrm_file id, learn its filename via /api/report, and make the staff file proxy treat it as theirs. Each file id must now either be anchored to the submitting contact (how /api/upload stores uploads) or equal the org's current value for that field; otherwise the submit is rejected with a 400. --- app/api/submit/route.ts | 76 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/app/api/submit/route.ts b/app/api/submit/route.ts index b995f0f..ea274be 100644 --- a/app/api/submit/route.ts +++ b/app/api/submit/route.ts @@ -14,7 +14,8 @@ */ import { NextResponse } from "next/server"; -import { civi, verifyChecksum } from "@/lib/civicrm"; +import { civi, civi3, verifyChecksum } from "@/lib/civicrm"; +import { loadPrefill } from "@/lib/prefill"; import { allFields, ACTIVITY_TYPE_NAME, FORM_CONTACT_RELATIONSHIP } from "@/config/form"; import type { SubmitPayload } from "@/types/form"; import { appEnv, redact } from "@/lib/env"; @@ -29,6 +30,65 @@ function isStubMode(): boolean { } const FIELD_BY_NAME = new Map(allFields.map((f) => [f.name, f])); +const FILE_FIELDS = allFields.filter((f) => f.type === "file"); + +/** + * File fields arrive as {id, file_name} (or a bare id). Normalize to a + * positive integer civicrm_file id, or null when cleared/malformed. + */ +function fileIdOf(value: unknown): number | null { + const raw = + value && typeof value === "object" && !Array.isArray(value) + ? (value as { id?: unknown }).id + : value; + if (typeof raw !== "number" && typeof raw !== "string") return null; + const n = Number(raw); + return Number.isSafeInteger(n) && n > 0 ? n : null; +} + +/** The org's current file-field values, keyed by form field name. */ +async function currentFileValues(orgId: number): Promise> { + const { values } = await loadPrefill(orgId, FILE_FIELDS, ACTIVITY_TYPE_NAME); + const contactRefs = FILE_FIELDS.map((f) => f.civiContactField).filter((s): s is string => Boolean(s)); + if (contactRefs.length > 0) { + const res = await civi>("Contact", "get", { + select: ["id", ...contactRefs], + where: [["id", "=", orgId]], + }); + const org = res.values?.[0] ?? {}; + for (const f of FILE_FIELDS) { + if (f.civiContactField && org[f.civiContactField] != null) values[f.name] = org[f.civiContactField]; + } + } + return values; +} + +/** + * File ids come from the client, so prove each one belongs to this + * submitter: either uploaded by this contact via /api/upload (which + * anchors the file to civicrm_contact/) or unchanged from the org's + * current value. Otherwise a link holder could attach — and then read the + * filename of — any file in Civi, and the staff file proxy would treat it + * as belonging to their org. + */ +async function fileRefsAllowed(cid: number, orgId: number, values: Record): Promise { + let current: Record | undefined; + for (const field of FILE_FIELDS) { + if (!(field.name in values)) continue; + const id = fileIdOf(values[field.name]); + if (id === null) continue; // cleared + const att = await civi3<{ entity_table?: string; entity_id?: string | number }>("Attachment", "get", { + id, + sequential: 1, + }).catch(() => ({ values: [] })); + const row = att.values?.[0]; + if (row?.entity_table === "civicrm_contact" && Number(row.entity_id) === cid) continue; + current ??= await currentFileValues(orgId); + if (fileIdOf(current[field.name]) === id) continue; + return false; + } + return true; +} export async function POST(req: Request) { // Rate limit per client IP. Generous default — 10 submissions per minute. @@ -103,6 +163,13 @@ async function runSubmit(cid: string, cs: string, values: Record Date: Fri, 30 Oct 2026 11:20:30 -0700 Subject: [PATCH 5/6] Give /api/preview-link its own Bearer secret; constant-time token checks preview-link mints working form links for any contact, but was gated by HEALTH_TOKEN (the diagnostics secret) passed in the query string, where it lands in access logs. README already documented PREVIEW_ADMIN_TOKEN as a Bearer header; make the code match. - PREVIEW_ADMIN_TOKEN, when set, is required as Authorization: Bearer and is never accepted from the URL. - Until it is set, HEALTH_TOKEN still works (Bearer or legacy ?token=) so the support runbook in EMAIL_DELIVERY.md keeps working. - preview-link and /api/health compare tokens in constant time. - redact() also scrubs PREVIEW_ADMIN_TOKEN and STAFF_REPORT_KEY. --- EMAIL_DELIVERY.md | 12 ++++++++---- README.md | 2 +- app/api/health/route.ts | 3 ++- app/api/preview-link/route.ts | 30 ++++++++++++++++++------------ lib/env.ts | 15 ++++++++++++++- 5 files changed, 43 insertions(+), 19 deletions(-) diff --git a/EMAIL_DELIVERY.md b/EMAIL_DELIVERY.md index 432cd34..8a8b032 100644 --- a/EMAIL_DELIVERY.md +++ b/EMAIL_DELIVERY.md @@ -147,13 +147,17 @@ You can use any combination. Common pattern: Option C for stage transitions Generates a working URL for any contact, returning the URL and the contact/org context for verification. -**Auth:** requires the `HEALTH_TOKEN` env var (same secret as -`/api/health`). In development without `HEALTH_TOKEN` set, no auth required. +**Auth:** send `PREVIEW_ADMIN_TOKEN` as a Bearer header. This endpoint +mints working links for any contact, so it has its own secret, kept out of +URLs. Until `PREVIEW_ADMIN_TOKEN` is set in Amplify, `HEALTH_TOKEN` is +accepted instead (Bearer header or legacy `?token=`). In development with +neither set, no auth is required. **Request:** ```bash -curl "https://survey.fci.coop/api/preview-link?cid=513&token=$HEALTH_TOKEN" +curl -H "Authorization: Bearer $PREVIEW_ADMIN_TOKEN" \ + "https://survey.fci.coop/api/preview-link?cid=513" ``` **Response:** @@ -196,7 +200,7 @@ curl "https://survey.fci.coop/api/preview-link?cid=513&token=$HEALTH_TOKEN" ### "Contact reports the link doesn't work" -1. Hit `/api/preview-link?cid=&token=`. +1. Hit `/api/preview-link?cid=` with `Authorization: Bearer `. 2. Verify the response shows the right org name + stage. 3. Send the URL from the response directly to the contact (or have them try it). 4. If `/api/preview-link` 404s, the contact lacks a Primary Contact relationship — fix in CiviCRM first. diff --git a/README.md b/README.md index b88a957..a0d3d6d 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ no live CRM calls. Production startup will refuse to boot in stub mode. | `CIVI_HTTP_AUTH_USER` | optional | Username, if CiviCRM sits behind HTTP Basic Auth at the webserver layer | | `CIVI_HTTP_AUTH_PASS` | optional | Password for the above | | `HEALTH_TOKEN` | optional | If set, `/api/health` requires `?token=…` in production | -| `PREVIEW_ADMIN_TOKEN` | optional | If set, `/api/preview-link` requires a matching `Authorization: Bearer …` header (admin convenience endpoint for minting form links) | +| `PREVIEW_ADMIN_TOKEN` | recommended | `/api/preview-link` requires a matching `Authorization: Bearer …` header (admin endpoint for minting form links). Until set, `HEALTH_TOKEN` is accepted instead | | `NODE_ENV` | — | Set to `production` for the production build | Copy `.env.example` to `.env.local` for local development. diff --git a/app/api/health/route.ts b/app/api/health/route.ts index 810ad40..28cf629 100644 --- a/app/api/health/route.ts +++ b/app/api/health/route.ts @@ -22,6 +22,7 @@ import { NextResponse } from "next/server"; import { civi, verifyChecksum } from "@/lib/civicrm"; import { ACTIVITY_TYPE_NAME, FORM_CONTACT_RELATIONSHIP } from "@/config/form"; import { appEnv } from "@/lib/env"; +import { safeEqual } from "@/lib/staff-token.mjs"; interface CheckResult { check: string; @@ -362,7 +363,7 @@ export async function GET(req: Request) { if (!e.healthToken) { return new NextResponse("Not Found", { status: 404 }); } - if (url.searchParams.get("token") !== e.healthToken) { + if (!safeEqual(url.searchParams.get("token") ?? "", e.healthToken)) { return new NextResponse("Not Found", { status: 404 }); } } diff --git a/app/api/preview-link/route.ts b/app/api/preview-link/route.ts index 94aa23a..7cc809b 100644 --- a/app/api/preview-link/route.ts +++ b/app/api/preview-link/route.ts @@ -1,5 +1,6 @@ /** - * GET /api/preview-link?cid=&token= + * GET /api/preview-link?cid= + * Authorization: Bearer * * Returns a ready-to-send survey URL for a given contact, plus enough * context for staff to verify it's the right org. Used for: @@ -7,10 +8,12 @@ * - Testing a contact's setup before sending the real CiviCRM email * - QA before rollout * - * AUTH: Always requires the HEALTH_TOKEN (same secret as /api/health). In - * development, if HEALTH_TOKEN is unset, anyone can hit the endpoint — - * matches /api/health behavior so dev workflow stays frictionless. In - * production, the endpoint returns 404 if HEALTH_TOKEN is unset. + * AUTH: this mints working form links for ANY contact, so it gets its own + * secret, PREVIEW_ADMIN_TOKEN, sent as a Bearer header to keep it out of + * URLs and access logs. Until PREVIEW_ADMIN_TOKEN is set, HEALTH_TOKEN is + * accepted instead (as a Bearer header or the legacy `?token=`) so the + * existing support workflow keeps working. With neither set, production + * returns 404 and development allows passthrough. * * Returns: * { @@ -26,6 +29,7 @@ import { NextResponse } from "next/server"; import { civi } from "@/lib/civicrm"; import { FORM_CONTACT_RELATIONSHIP } from "@/config/form"; import { appEnv } from "@/lib/env"; +import { safeEqual } from "@/lib/staff-token.mjs"; interface PreviewLinkResponse { contactId: number; @@ -42,13 +46,15 @@ export async function GET(req: Request) { const env = appEnv(); const url = new URL(req.url); const cid = url.searchParams.get("cid"); - const token = url.searchParams.get("token"); - - // Auth gate: production requires HEALTH_TOKEN. Dev allows passthrough. - if (env.isProduction) { - if (!env.healthToken) return new NextResponse("Not Found", { status: 404 }); - if (token !== env.healthToken) return new NextResponse("Not Found", { status: 404 }); - } else if (env.healthToken && token !== env.healthToken) { + // Auth gate — see header comment. + const secret = env.previewAdminToken || env.healthToken; + if (secret) { + const bearer = req.headers.get("authorization")?.match(/^Bearer\s+(.+)$/i)?.[1]; + const presented = bearer ?? (env.previewAdminToken ? null : url.searchParams.get("token")); + if (!presented || !safeEqual(presented, secret)) { + return new NextResponse("Not Found", { status: 404 }); + } + } else if (env.isProduction) { return new NextResponse("Not Found", { status: 404 }); } diff --git a/lib/env.ts b/lib/env.ts index 0e9e27d..e6d6731 100644 --- a/lib/env.ts +++ b/lib/env.ts @@ -22,6 +22,11 @@ export interface AppEnv { * string and pass via `?token=<>` to the health endpoint to read diagnostics. */ healthToken?: string; + /** + * Dedicated secret for /api/preview-link, sent as `Authorization: Bearer`. + * When unset, that endpoint falls back to HEALTH_TOKEN. + */ + previewAdminToken?: string; /** * Same-origin host this app runs at. Used to build `frame-ancestors 'self'` * and any absolute self-links. Defaults to inferring from the request. @@ -46,6 +51,7 @@ export function appEnv(): AppEnv { basicAuthUser: process.env.CIVI_HTTP_AUTH_USER, basicAuthPass: process.env.CIVI_HTTP_AUTH_PASS, healthToken: process.env.HEALTH_TOKEN, + previewAdminToken: process.env.PREVIEW_ADMIN_TOKEN, publicOrigin: process.env.PUBLIC_ORIGIN, }; // In production, refuse to start in stub mode — that's almost certainly a @@ -64,7 +70,14 @@ export function appEnv(): AppEnv { export function redact(s: string): string { let out = s; const e = appEnv(); - for (const v of [e.civiApiKey, e.civiSiteKey, e.basicAuthPass, e.healthToken]) { + for (const v of [ + e.civiApiKey, + e.civiSiteKey, + e.basicAuthPass, + e.healthToken, + e.previewAdminToken, + process.env.STAFF_REPORT_KEY, + ]) { if (v && v.length >= 6) out = out.split(v).join("«redacted»"); } return out; From 335cdd0c233b79a13fae1f44256200b098f8ccb4 Mon Sep 17 00:00:00 2001 From: Joel Brock Date: Fri, 30 Oct 2026 11:20:30 -0700 Subject: [PATCH 6/6] Refresh Next.js-generated agent rules block in AGENTS.md --- AGENTS.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8bd0e39..af3b7c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,5 +1,9 @@ -# This is NOT the Next.js you know -This version has breaking changes — APIs, conventions, and file structure may all differ from your training data. Read the relevant guide in `node_modules/next/dist/docs/` before writing any code. Heed deprecation notices. +## This is NOT the Next.js you know + +This version has breaking changes — APIs, conventions, and file structure may all differ from your training data. Read the relevant guide in `node_modules/next/dist/docs/` (resolved from this file's directory; in monorepos the `next` package may not be visible from the repo root) before writing any code. Heed deprecation notices. + +This block is written and re-added by `next dev` — verify at `node_modules/next/dist/server/lib/generate-agent-files.js`. Removing it from a diff only re-creates the uncommitted change; committing it with your work keeps the tree clean. +