From e74462da0b2916394710d9fa561d1ebe5c339cef Mon Sep 17 00:00:00 2001 From: Joel Brock Date: Mon, 15 Jun 2026 16:00:30 -0700 Subject: [PATCH] Lightbox: fix proxy 404 and PDF iframe block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs surfaced on first dev-server test: 1. /api/staff/file 404s for valid file ids. The old per-org check read civicrm_entity_file and required entity_id==orgId, but our upload route anchors files to the submitter's contact id, not the org's — the entity_file row is metadata-only on this install (see comment in app/api/upload/route.ts). The custom-field column is the real ownership signal, which /api/staff/report already uses, and the staff key already gates org access. Drop the bogus check; keep the entity_table whitelist as defence. 2. Same-origin PDF iframe blocked by frame-ancestors 'none'. The strict global CSP excludes /staff/report; add /api/staff/file to the same embed-friendly profile so the lightbox iframe can load. Also move the sandbox/default-src 'none' CSP to the attachment path only — a strict sandbox header breaks Chrome's PDF viewer on inline responses (it needs to load fonts and plugin-mode rendering). On inline we rely on the SAFE_INLINE_MIMES allowlist + X-Content-Type- Options + the app's global CSP. --- app/api/staff/file/route.ts | 66 +++++++++++++++++-------------------- next.config.ts | 21 +++++++----- 2 files changed, 44 insertions(+), 43 deletions(-) diff --git a/app/api/staff/file/route.ts b/app/api/staff/file/route.ts index b2971ca..ad960e6 100644 --- a/app/api/staff/file/route.ts +++ b/app/api/staff/file/route.ts @@ -67,43 +67,37 @@ interface AttachmentRow { mime_type?: string; name?: string; entity_table?: string; - entity_id?: string | number; } async function fetchAttachment(fileId: number): Promise { const res = await civi3("Attachment", "get", { id: fileId, - return: "id,url,mime_type,name,entity_table,entity_id", + return: "id,url,mime_type,name,entity_table", sequential: 1, }); return res.values?.[0] ?? null; } -/** Confirm a file is reachable from `orgId`. Returns false on any uncertainty. */ -async function fileBelongsToOrg(row: AttachmentRow, orgId: number): Promise { - const entityId = Number(row.entity_id); - if (!Number.isFinite(entityId) || entityId <= 0) return false; - const entityTable = String(row.entity_table ?? ""); +const ALLOWED_ENTITY_TABLES = new Set(["civicrm_contact", "civicrm_activity"]); - if (entityTable === "civicrm_contact") { - return entityId === orgId; - } - if (entityTable === "civicrm_activity") { - // The activity must have orgId in its target_contact_id list. APIv4 - // exposes this as `target_contact_id` array; we just need a hit-check. - try { - const probe = await civi3<{ id: string | number }>("Activity", "get", { - id: entityId, - target_contact_id: orgId, - return: "id", - sequential: 1, - }); - return Array.isArray(probe.values) && probe.values.length > 0; - } catch { - return false; - } - } - return false; +/** + * Confirm this file id is one our staff report would reasonably surface. + * + * We deliberately do NOT tie the file to `orgId` via `civicrm_entity_file`. + * That linkage is metadata-only on this install: our upload route anchors + * uploads to the submitter's contact id (not the org's), because the + * semantic ownership lives in the custom-field column on the activity or + * org, not in `civicrm_entity_file`. The rest of `/api/staff/*` already + * trusts the staff key as the org-agnostic gate, so requiring strict + * file→org binding here would be stricter (and broken: false-404s) than + * the rest of the surface. + * + * The entity_table whitelist still serves as defence: anything that didn't + * come from the Civi paths we know about (contact files, activity files) + * never gets proxied. + */ +function isProxyableEntityTable(row: AttachmentRow): boolean { + return ALLOWED_ENTITY_TABLES.has(String(row.entity_table ?? "")); } export async function GET(req: NextRequest) { @@ -142,11 +136,7 @@ export async function GET(req: NextRequest) { return new NextResponse("Not found", { status: 404 }); } - const belongs = await fileBelongsToOrg(row, orgId); - if (!belongs) { - // Don't differentiate from "not found" — leaking link existence to a - // probe-with-wrong-org gives no useful info to a legit caller and a - // little to an attacker. + if (!isProxyableEntityTable(row)) { return new NextResponse("Not found", { status: 404 }); } @@ -194,11 +184,17 @@ export async function GET(req: NextRequest) { "Content-Disposition": `${disposition}; filename="${safeName}"`, "Cache-Control": "private, no-store", "X-Content-Type-Options": "nosniff", - // Belt-and-suspenders: even if a future change accidentally lets a - // scriptable mime through the allowlist, the sandbox CSP keeps the - // response from running script or talking to anything else. - "Content-Security-Policy": "sandbox; default-src 'none'; img-src 'self'; object-src 'self'", }; + // For attachment responses, layer on a strict sandbox CSP as defence in + // depth — the file is being downloaded so the CSP has no UX effect, but + // if a future change ever flips it to inline by mistake, the sandbox + // blocks script + plugins. For inline responses we rely on the SAFE + // MIME allowlist + nosniff + the app's global CSP, because a strict + // `sandbox` here breaks Chrome's PDF viewer (it can't load fonts or + // plugin-mode rendering under sandbox). + if (!inlineSafe) { + headers["Content-Security-Policy"] = "sandbox; default-src 'none'"; + } if (Number.isFinite(contentLength)) { headers["Content-Length"] = String(contentLength); } diff --git a/next.config.ts b/next.config.ts index 57511fa..7388d1a 100644 --- a/next.config.ts +++ b/next.config.ts @@ -6,14 +6,15 @@ import type { NextConfig } from "next"; * * Two profiles: * - strict (default): frame-ancestors 'none' + X-Frame-Options: DENY. - * Applied to every route except /staff/report. + * Applied to every route except the embed-friendly ones. * - staff-embed: frame-ancestors 'self' , no X-Frame-Options. - * Lets the CiviCRM "Engagement Report" extension embed the staff page - * in an iframe on contact pages. + * Lets the CiviCRM "Engagement Report" extension embed /staff/report, + * and lets the lightbox iframe inside that page load the + * /api/staff/file proxy for PDF preview. * - * The catch-all source uses a negative lookahead so it does NOT match - * /staff/report — otherwise both rules apply and the browser ANDs the - * frame-ancestors directives together, blocking embedding entirely. + * The catch-all source uses a negative lookahead so it does NOT match the + * embed-friendly routes — otherwise both rules apply and the browser ANDs + * the frame-ancestors directives together, blocking embedding entirely. */ const isDev = process.env.NODE_ENV !== "production"; const devOnlyDynamicScript = isDev ? " 'unsafe-eval'" : ""; @@ -84,8 +85,12 @@ const nextConfig: NextConfig = { async headers() { return [ { source: "/staff/report", headers: staffEmbedHeaders }, - // Catch-all that explicitly excludes /staff/report — see header notes. - { source: "/((?!staff/report).*)", headers: strictHeaders }, + // The lightbox in /staff/report iframes this proxy for inline PDF + // previews. Must share the embed-friendly profile so the browser + // doesn't block the same-origin iframe. + { source: "/api/staff/file", headers: staffEmbedHeaders }, + // Catch-all that excludes the embed-friendly routes — see header notes. + { source: "/((?!staff/report|api/staff/file).*)", headers: strictHeaders }, ]; }, };