Lightbox: fix proxy 404 and PDF iframe block
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.
This commit is contained in:
Joel Brock
+31
-35
@@ -67,43 +67,37 @@ interface AttachmentRow {
|
||||
mime_type?: string;
|
||||
name?: string;
|
||||
entity_table?: string;
|
||||
entity_id?: string | number;
|
||||
}
|
||||
|
||||
async function fetchAttachment(fileId: number): Promise<AttachmentRow | null> {
|
||||
const res = await civi3<AttachmentRow>("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<boolean> {
|
||||
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);
|
||||
}
|
||||
|
||||
+13
-8
@@ -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' <civi-origin>, 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 },
|
||||
];
|
||||
},
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user