fix(audit): H11 — gate cross-port coverBrandPortId in report runs
Layer 1: createReportRun rejects a user-triggered run whose coverBrandPortId is a port the triggering user can't access (userCanAccessPort: super-admin or userPortRoles membership). Layer 2: renderReportRun only honors the override when it equals run.portId or the run's user is a member, else falls back to the source port's branding — so a forged/scheduled config can't leak another tenant's logo/name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -30,7 +30,7 @@ import { getStorageBackend } from '@/lib/storage';
|
|||||||
import { resolvePortLogo } from '@/lib/pdf/brand-kit/logo';
|
import { resolvePortLogo } from '@/lib/pdf/brand-kit/logo';
|
||||||
import { renderPdf } from '@/lib/pdf/render';
|
import { renderPdf } from '@/lib/pdf/render';
|
||||||
import { sendEmail } from '@/lib/email';
|
import { sendEmail } from '@/lib/email';
|
||||||
import { updateReportRunStatus } from '@/lib/services/report-runs.service';
|
import { updateReportRunStatus, userCanAccessPort } from '@/lib/services/report-runs.service';
|
||||||
import { CodedError, NotFoundError } from '@/lib/errors';
|
import { CodedError, NotFoundError } from '@/lib/errors';
|
||||||
import {
|
import {
|
||||||
fetchActivityData,
|
fetchActivityData,
|
||||||
@@ -221,15 +221,39 @@ export async function renderReportRun(reportRunId: string): Promise<ReportRun> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// P7: optional cover-brand swap. When config.coverBrandPortId points
|
// P7: optional cover-brand swap. When config.coverBrandPortId points
|
||||||
// at a port the rep has access to, the cover-page logo + port name
|
// at a port the triggering user has access to, the cover-page logo +
|
||||||
// come from THAT port's brand kit instead of the report's source
|
// port name come from THAT port's brand kit instead of the report's
|
||||||
// port. Useful for cross-port leadership decks; falls back to the
|
// source port. Useful for cross-port leadership decks; falls back to
|
||||||
// source port when the override port is missing / inaccessible.
|
// the source port when the override port is missing / inaccessible.
|
||||||
|
//
|
||||||
|
// H11 defense-in-depth: enqueue-time validation (createReportRun)
|
||||||
|
// already rejects forbidden overrides, but a stale/forged config row
|
||||||
|
// could still reach the worker. Only honour the override when it equals
|
||||||
|
// the run's own port OR the triggering user is a member of that port
|
||||||
|
// (super-admins included via userCanAccessPort). Otherwise silently
|
||||||
|
// fall back to the source port's branding — a forged config can never
|
||||||
|
// leak a foreign tenant's logo/name onto the rendered PDF.
|
||||||
const params = (run.config as Record<string, unknown>) ?? {};
|
const params = (run.config as Record<string, unknown>) ?? {};
|
||||||
const overrideBrandPortId =
|
const requestedBrandPortId =
|
||||||
typeof params.coverBrandPortId === 'string' && params.coverBrandPortId.length > 0
|
typeof params.coverBrandPortId === 'string' && params.coverBrandPortId.length > 0
|
||||||
? params.coverBrandPortId
|
? params.coverBrandPortId
|
||||||
: null;
|
: null;
|
||||||
|
const overrideAllowed =
|
||||||
|
requestedBrandPortId !== null &&
|
||||||
|
(requestedBrandPortId === run.portId ||
|
||||||
|
(await userCanAccessPort(run.triggeredByUserId, requestedBrandPortId)));
|
||||||
|
if (requestedBrandPortId !== null && !overrideAllowed) {
|
||||||
|
logger.warn(
|
||||||
|
{
|
||||||
|
reportRunId: run.id,
|
||||||
|
runPortId: run.portId,
|
||||||
|
requestedBrandPortId,
|
||||||
|
triggeredByUserId: run.triggeredByUserId,
|
||||||
|
},
|
||||||
|
'Ignoring cover-brand override: triggering user is not a member of the requested port (H11)',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
const overrideBrandPortId = overrideAllowed ? requestedBrandPortId : null;
|
||||||
const brandPortId = overrideBrandPortId ?? run.portId;
|
const brandPortId = overrideBrandPortId ?? run.portId;
|
||||||
const brandPort =
|
const brandPort =
|
||||||
overrideBrandPortId === null
|
overrideBrandPortId === null
|
||||||
|
|||||||
@@ -22,8 +22,9 @@ import {
|
|||||||
type ReportRun,
|
type ReportRun,
|
||||||
type NewReportRun,
|
type NewReportRun,
|
||||||
} from '@/lib/db/schema/reports';
|
} from '@/lib/db/schema/reports';
|
||||||
|
import { userPortRoles, userProfiles } from '@/lib/db/schema/users';
|
||||||
import { createAuditLog, type AuditMeta } from '@/lib/audit';
|
import { createAuditLog, type AuditMeta } from '@/lib/audit';
|
||||||
import { NotFoundError, ValidationError } from '@/lib/errors';
|
import { ForbiddenError, NotFoundError, ValidationError } from '@/lib/errors';
|
||||||
import type {
|
import type {
|
||||||
CreateReportRunInput,
|
CreateReportRunInput,
|
||||||
ListReportRunsInput,
|
ListReportRunsInput,
|
||||||
@@ -36,6 +37,31 @@ export interface ListReportRunsResult {
|
|||||||
hasMore: boolean;
|
hasMore: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* True when the user may act on behalf of `portId`: either a platform
|
||||||
|
* super-admin, or the holder of a `userPortRoles` membership row for that
|
||||||
|
* port. Mirrors the Socket.IO `userCanAccessPort` guard — the canonical
|
||||||
|
* "is this user a member of this tenant" check. Used to gate the
|
||||||
|
* cover-brand override so a rep can't stamp a foreign port's logo +
|
||||||
|
* name onto a report (audit finding H11). Empty/falsy userId → false.
|
||||||
|
*/
|
||||||
|
export async function userCanAccessPort(
|
||||||
|
userId: string | null | undefined,
|
||||||
|
portId: string,
|
||||||
|
): Promise<boolean> {
|
||||||
|
if (!userId) return false;
|
||||||
|
const profile = await db.query.userProfiles.findFirst({
|
||||||
|
where: eq(userProfiles.userId, userId),
|
||||||
|
columns: { isSuperAdmin: true },
|
||||||
|
});
|
||||||
|
if (profile?.isSuperAdmin) return true;
|
||||||
|
const role = await db.query.userPortRoles.findFirst({
|
||||||
|
where: and(eq(userPortRoles.userId, userId), eq(userPortRoles.portId, portId)),
|
||||||
|
columns: { id: true },
|
||||||
|
});
|
||||||
|
return Boolean(role);
|
||||||
|
}
|
||||||
|
|
||||||
export async function listReportRuns(
|
export async function listReportRuns(
|
||||||
portId: string,
|
portId: string,
|
||||||
query: ListReportRunsInput,
|
query: ListReportRunsInput,
|
||||||
@@ -98,6 +124,31 @@ export async function createReportRun(
|
|||||||
throw new ValidationError(`config.kind must equal "${input.kind}"`);
|
throw new ValidationError(`config.kind must equal "${input.kind}"`);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// H11: the cover-brand override (`config.coverBrandPortId`) swaps the
|
||||||
|
// cover-page logo + port name to another port's brand kit. All report
|
||||||
|
// DATA still comes from `options.portId`, so the override's only effect
|
||||||
|
// is rendering a foreign port's branding — a cross-tenant leak vector.
|
||||||
|
//
|
||||||
|
// For USER-triggered runs there is an accountable user, so gate the
|
||||||
|
// override to ports that user belongs to (super-admins exempt) and reject
|
||||||
|
// before the row is even queued. For SCHEDULE-triggered runs there is no
|
||||||
|
// human actor to attribute access to, so we don't reject here (that would
|
||||||
|
// silently kill a whole scheduled report); the renderer's defense-in-depth
|
||||||
|
// check strips an unauthorized override and falls back to the source
|
||||||
|
// port's branding instead.
|
||||||
|
const coverBrandPortId = (input.config as { coverBrandPortId?: unknown }).coverBrandPortId;
|
||||||
|
if (
|
||||||
|
options.triggeredBy === 'user' &&
|
||||||
|
typeof coverBrandPortId === 'string' &&
|
||||||
|
coverBrandPortId.length > 0 &&
|
||||||
|
coverBrandPortId !== options.portId
|
||||||
|
) {
|
||||||
|
const allowed = await userCanAccessPort(options.triggeredByUserId, coverBrandPortId);
|
||||||
|
if (!allowed) {
|
||||||
|
throw new ForbiddenError('You do not have access to the port selected for cover branding');
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Verify template ownership when provided. Belt-and-braces: the route
|
// Verify template ownership when provided. Belt-and-braces: the route
|
||||||
// already gates by port via withAuth, but a stale template id from a
|
// already gates by port via withAuth, but a stale template id from a
|
||||||
// cached UI would otherwise produce an opaque FK constraint error.
|
// cached UI would otherwise produce an opaque FK constraint error.
|
||||||
|
|||||||
@@ -72,7 +72,12 @@ export const createReportRunSchema = z.object({
|
|||||||
kind: z.enum(REPORT_KINDS),
|
kind: z.enum(REPORT_KINDS),
|
||||||
templateId: z.string().optional(),
|
templateId: z.string().optional(),
|
||||||
// Same opaque shape report_templates accepts — the render queue
|
// Same opaque shape report_templates accepts — the render queue
|
||||||
// re-validates per-kind at use time.
|
// re-validates per-kind at use time. NOTE: the optional
|
||||||
|
// `config.coverBrandPortId` (cover-page brand swap) is intentionally NOT
|
||||||
|
// shape-validated here — it requires a runtime per-user membership check
|
||||||
|
// that Zod can't express. `createReportRun` rejects an override the
|
||||||
|
// triggering user can't access (H11), and the renderer strips a
|
||||||
|
// stale/forged one as defense-in-depth.
|
||||||
config: z.record(z.string(), z.unknown()),
|
config: z.record(z.string(), z.unknown()),
|
||||||
outputFormat: z.enum(REPORT_OUTPUT_FORMATS).default('pdf'),
|
outputFormat: z.enum(REPORT_OUTPUT_FORMATS).default('pdf'),
|
||||||
emailTo: z.array(recipientSchema).max(50).optional(),
|
emailTo: z.array(recipientSchema).max(50).optional(),
|
||||||
|
|||||||
Reference in New Issue
Block a user