fix(audit): non-Documenso backlog sweep — port-binding, NULLS NOT DISTINCT, custom merge tokens, company docs
Some checks failed
Build & Push Docker Images / lint (push) Successful in 1m36s
Build & Push Docker Images / build-and-push (push) Failing after 4m27s

Wave through the remaining audit-final-deferred items that aren't blocked
on the back-burnered Documenso work.

Multi-tenant isolation:
- Storage proxy ProxyTokenPayload gains optional `p` (port slug) claim;
  verifier asserts `key.startsWith(${p}/)`. Defense-in-depth against a
  buggy issuer in some future code path that mixes port scopes — every
  storage key generated by generateStorageKey() already prefixes the
  slug. document-sends opts in for 24h emailed download links; other
  callers continue working unchanged via the optional field.

DB schema reconciliation:
- Migration 0047 rebuilds system_settings unique index with NULLS NOT
  DISTINCT (Postgres 15+) so global settings (port_id IS NULL) are
  uniquely keyed by `key` alone. Surfaced + dedupe'd 65 duplicate
  (storage_backend, NULL) rows that had accumulated from race-prone
  delete-then-insert patterns in ocr-config / settings / residential-
  stages / ai-budget services. All four services converted to true
  onConflictDoUpdate upserts so the race window is closed.

API uniformity:
- Response shape standardization: 16 routes converted from
  `{ success: true }` to 204 No Content. CLAUDE.md documents the
  convention (`{ data: <T> }` for content, 204 for empty mutations,
  portal-auth retains `{ success: true }` for the frontend's auth chain).
- req.json() → parseBody() migration across 9 admin/CRM routes
  (custom-fields, expenses/export ×3, currency convert,
  search/recently-viewed, admin/duplicates, berths/pdf-{upload-url,
  versions, parse-results}). Uniform 400 error shapes for
  ZodError-flagged bodies.

Custom-fields merge tokens (shipped end-to-end):
- merge-fields.ts gains CUSTOM_MERGE_TOKEN_RE + helpers for the
  `{{custom.<fieldName>}}` shape.
- document-templates validator accepts the dynamic shape alongside
  the static catalog tokens.
- document-sends.service mergeCustomFieldValues resolver fetches
  per-port custom_field_definitions for client/interest/berth contexts
  and substitutes stored values keyed by `{{custom.fieldName}}`.
- custom-fields-manager amber banner updated to reflect that merge
  tokens now expand (search index + entity-diff remain documented
  design limitations).

/api/v1/files cross-entity filtering:
- Validator + listFiles + uploadFile accept companyId AND yachtId
  alongside clientId. file-upload-zone propagates both.
- New CompanyFilesTab component mirrors ClientFilesTab; restored as a
  visible Documents tab in company-tabs.tsx (was a hidden stub).

Inline TODOs:
- Reviewed remaining two TODOs (per-user reminder schedule, import
  worker handlers). Both are placeholders for future feature surfaces,
  not bugs — per-port digest works for every customer; nothing
  currently enqueues import jobs (verified). Annotated in BACKLOG.

BACKLOG.md updated to reflect what landed and what's still pending
(Documenso-related items still bundled with the back-burnered phases).

Tests: 1185/1185 vitest, tsc clean.
This commit is contained in:
2026-05-08 02:20:27 +02:00
parent 60365dc3de
commit 8dc16dcd2e
49 changed files with 578 additions and 254 deletions

View File

@@ -0,0 +1,34 @@
-- Reconcile the system_settings unique-index drift surfaced in the
-- final-deferred audit. The Drizzle schema declares a uniqueIndex on
-- (key, port_id), but Postgres treats NULL values as distinct by default.
-- That means two rows with `(same_key, NULL)` would BOTH be allowed —
-- a global-setting collision the index claims to prevent.
--
-- This was not just theoretical: the dev DB had 60+ duplicate
-- `(storage_backend, NULL)` rows from buggy non-upsert call sites that
-- predated the upsert hardening. Those rows accumulated invisibly because
-- the index allowed them. Step 1 dedupes (keeps the most recent row per
-- `(key, port_id)` group); step 2 rebuilds the unique index with
-- `NULLS NOT DISTINCT` (Postgres 15+) so future inserts can't recreate the
-- ambiguity.
-- Step 1: dedupe duplicate rows, keeping the row with the latest updated_at.
-- Uses a CTE + ROW_NUMBER() so the keeper is deterministic across reruns.
WITH ranked AS (
SELECT ctid,
ROW_NUMBER() OVER (
PARTITION BY "key", "port_id"
ORDER BY "updated_at" DESC, ctid DESC
) AS rn
FROM "system_settings"
)
DELETE FROM "system_settings"
USING ranked
WHERE "system_settings".ctid = ranked.ctid AND ranked.rn > 1;
-- Step 2: replace the unique index with one that treats NULLs as equal,
-- so global settings (port_id IS NULL) are unique by key alone.
DROP INDEX IF EXISTS "system_settings_key_port_idx";
CREATE UNIQUE INDEX "system_settings_key_port_idx"
ON "system_settings" ("key", "port_id")
NULLS NOT DISTINCT;

View File

@@ -135,9 +135,14 @@ export const systemSettings = pgTable(
updatedAt: timestamp('updated_at', { withTimezone: true }).notNull().defaultNow(),
},
(table) => [
// Migration 0047 rebuilds this index with `NULLS NOT DISTINCT` so a
// global setting (port_id IS NULL) is unique by key alone — the
// default `NULLS DISTINCT` semantics let duplicates accumulate.
// Drizzle's `uniqueIndex` builder doesn't surface NULLS NOT DISTINCT,
// so the migration is the source of truth for that flag and
// `db:push` against an empty DB would skip it (matches the
// documented limitation for `berths.current_pdf_version_id`).
uniqueIndex('system_settings_key_port_idx').on(table.key, table.portId),
// Note: the PRIMARY KEY is `key` alone based on schema, but unique on (key, port_id)
// We use key as primary key per SQL schema
],
);

View File

@@ -76,15 +76,26 @@ export async function setAiBudget(
if (next.softCapTokens > next.hardCapTokens) {
throw new ValidationError('softCapTokens cannot exceed hardCapTokens');
}
// True upsert (atomic on the (key, port_id) NULLS NOT DISTINCT index
// — migration 0047). Replaces a delete-then-insert pattern that had a
// race window where two concurrent updates could both DELETE and both
// INSERT, accumulating duplicates.
await db
.delete(systemSettings)
.where(and(eq(systemSettings.key, KEY), eq(systemSettings.portId, portId)));
await db.insert(systemSettings).values({
key: KEY,
portId,
value: next as unknown as Record<string, unknown>,
updatedBy: userId,
});
.insert(systemSettings)
.values({
key: KEY,
portId,
value: next as unknown as Record<string, unknown>,
updatedBy: userId,
})
.onConflictDoUpdate({
target: [systemSettings.key, systemSettings.portId],
set: {
value: next as unknown as Record<string, unknown>,
updatedBy: userId,
updatedAt: new Date(),
},
});
return next;
}

View File

@@ -38,9 +38,12 @@ import {
berthPdfVersions,
clients,
clientContacts,
customFieldDefinitions,
customFieldValues,
interests,
ports,
} from '@/lib/db/schema';
import { inArray } from 'drizzle-orm';
import type { DocumentSend } from '@/lib/db/schema';
import { ForbiddenError, NotFoundError, ValidationError } from '@/lib/errors';
import { logger } from '@/lib/logger';
@@ -162,9 +165,93 @@ export async function buildMergeValues(
}
}
// Custom-field tokens (`{{custom.<fieldName>}}`). The validator allows
// any matching shape; the resolver here looks up real values per-port,
// per-entity and substitutes them. Unknown field names stay
// unresolved — `findUnresolvedTokens` flags them at preview time so
// the rep can edit the template before sending.
await mergeCustomFieldValues(values, portId, recipient, context);
return values;
}
interface CustomMergeContext {
berthId?: string;
brochureLabel?: string;
}
/**
* Resolve `{{custom.<fieldName>}}` tokens. Reads every per-port custom
* field definition for the entity types currently in scope (client,
* interest, berth) and joins to the actual stored value for each entity
* id we have on hand. Boolean values render as 'true' / 'false', dates
* as ISO yyyy-mm-dd, numbers as plain numerics, selects/text verbatim.
*/
async function mergeCustomFieldValues(
values: Record<string, string>,
portId: string,
recipient: SendRecipientInput,
context: CustomMergeContext,
): Promise<void> {
// Build the (entityType → entityId) map for the current send context.
const entityIdsByType = new Map<string, string>();
if (recipient.clientId) entityIdsByType.set('client', recipient.clientId);
if (recipient.interestId) entityIdsByType.set('interest', recipient.interestId);
if (context.berthId) entityIdsByType.set('berth', context.berthId);
if (entityIdsByType.size === 0) return;
const definitions = await db
.select()
.from(customFieldDefinitions)
.where(
and(
eq(customFieldDefinitions.portId, portId),
inArray(customFieldDefinitions.entityType, Array.from(entityIdsByType.keys())),
),
);
if (definitions.length === 0) return;
const fieldIds = definitions.map((d) => d.id);
const entityIds = Array.from(entityIdsByType.values());
const valueRows = await db
.select()
.from(customFieldValues)
.where(
and(
inArray(customFieldValues.fieldId, fieldIds),
inArray(customFieldValues.entityId, entityIds),
),
);
const valueByFieldEntity = new Map<string, unknown>();
for (const row of valueRows) {
valueByFieldEntity.set(`${row.fieldId}|${row.entityId}`, row.value);
}
for (const def of definitions) {
const entityId = entityIdsByType.get(def.entityType);
if (!entityId) continue;
const raw = valueByFieldEntity.get(`${def.id}|${entityId}`);
if (raw === undefined || raw === null) continue;
const token = `{{custom.${def.fieldName}}}`;
values[token] = stringifyCustomValue(raw, def.fieldType);
}
}
function stringifyCustomValue(raw: unknown, fieldType: string): string {
if (raw === null || raw === undefined) return '';
switch (fieldType) {
case 'boolean':
return raw ? 'true' : 'false';
case 'date':
return typeof raw === 'string' ? raw.slice(0, 10) : String(raw);
case 'number':
return String(raw);
default:
return typeof raw === 'string' ? raw : JSON.stringify(raw);
}
}
/**
* Render a body for the dry-run UI. Returns `{ html, unresolved }`. The UI
* uses `unresolved` to populate the warning chip; the rep can't submit
@@ -295,9 +382,18 @@ async function streamAttachmentOrLink(
// to the body. Per §11.1 the size decision is made BEFORE the SMTP relay,
// so we never produce duplicate sends.
const storage = await getStorageBackend();
// Bind the proxy token to the issuing port slug. The storage key is
// already structured `${portSlug}/...` via generateStorageKey() — this
// closes the loop so a buggy future call site that hands us a key from
// a different port can't mint a valid 24h URL for it.
const portRow = await db.query.ports.findFirst({
where: eq(ports.id, portId),
columns: { slug: true },
});
const { url } = await storage.presignDownload(attachment.storageKey, {
expirySeconds: 24 * 60 * 60,
filename: attachment.fileName,
portSlug: portRow?.slug,
});
// HTML-escape the filename: brochure filenames are admin-supplied and
// could in theory carry markup (e.g. `"><script>...`). Even a benign

View File

@@ -71,6 +71,8 @@ export async function uploadFile(
.values({
portId,
clientId: data.clientId ?? null,
yachtId: data.yachtId ?? null,
companyId: data.companyId ?? null,
filename: sanitizedFilename,
originalName: sanitizedOriginal,
mimeType: file.mimeType,
@@ -219,13 +221,19 @@ export async function deleteFile(id: string, portId: string, meta: AuditMeta) {
// ─── List ─────────────────────────────────────────────────────────────────────
export async function listFiles(portId: string, query: ListFilesInput) {
const { page, limit, sort, order, search, clientId, category } = query;
const { page, limit, sort, order, search, clientId, yachtId, companyId, category } = query;
const filters = [];
if (clientId) {
filters.push(eq(files.clientId, clientId));
}
if (yachtId) {
filters.push(eq(files.yachtId, yachtId));
}
if (companyId) {
filters.push(eq(files.companyId, companyId));
}
if (category) {
filters.push(eq(files.category, category));
}

View File

@@ -66,20 +66,27 @@ async function readRow(portId: string | null): Promise<StoredOcrConfig | null> {
}
async function writeRow(portId: string | null, value: StoredOcrConfig, userId: string) {
// upsert: delete + insert keeps logic simple given the (key, port_id) unique index.
// True upsert. The previous delete-then-insert pattern had a race
// window where two concurrent writes could both DELETE and both INSERT,
// accumulating duplicate rows (caught and dedupe'd by migration 0047).
// The (key, port_id) NULLS NOT DISTINCT unique index makes this
// upsert atomic.
await db
.delete(systemSettings)
.where(
portId === null
? and(eq(systemSettings.key, KEY), isNull(systemSettings.portId))
: and(eq(systemSettings.key, KEY), eq(systemSettings.portId, portId)),
);
await db.insert(systemSettings).values({
key: KEY,
portId,
value: value as unknown as Record<string, unknown>,
updatedBy: userId,
});
.insert(systemSettings)
.values({
key: KEY,
portId,
value: value as unknown as Record<string, unknown>,
updatedBy: userId,
})
.onConflictDoUpdate({
target: [systemSettings.key, systemSettings.portId],
set: {
value: value as unknown as Record<string, unknown>,
updatedBy: userId,
updatedAt: new Date(),
},
});
}
/**

View File

@@ -130,23 +130,28 @@ export async function saveStages(args: SaveStagesArgs, meta: AuditMeta): Promise
}
}
// Upsert the stage list.
// Upsert the stage list. Read first for the audit-log diff; the actual
// write goes through onConflictDoUpdate so concurrent admin saves can't
// race-insert duplicates (migration 0047 made the index NULLS NOT DISTINCT).
const existing = await db.query.systemSettings.findFirst({
where: and(eq(systemSettings.key, SETTING_KEY), eq(systemSettings.portId, args.portId)),
});
if (existing) {
await db
.update(systemSettings)
.set({ value: args.stages, updatedBy: meta.userId, updatedAt: new Date() })
.where(and(eq(systemSettings.key, SETTING_KEY), eq(systemSettings.portId, args.portId)));
} else {
await db.insert(systemSettings).values({
await db
.insert(systemSettings)
.values({
key: SETTING_KEY,
value: args.stages,
portId: args.portId,
updatedBy: meta.userId,
})
.onConflictDoUpdate({
target: [systemSettings.key, systemSettings.portId],
set: {
value: args.stages,
updatedBy: meta.userId,
updatedAt: new Date(),
},
});
}
void createAuditLog({
userId: meta.userId,

View File

@@ -38,23 +38,30 @@ export async function getSetting(key: string, portId: string) {
}
export async function upsertSetting(key: string, value: unknown, portId: string, meta: AuditMeta) {
// Read existing first for the audit-log diff (before/after). The actual
// write goes through onConflictDoUpdate so two concurrent calls can't
// both observe `existing=null` and both INSERT — the (key, port_id)
// unique index now treats NULLs as equal (migration 0047).
const existing = await db.query.systemSettings.findFirst({
where: and(eq(systemSettings.key, key), eq(systemSettings.portId, portId)),
});
if (existing) {
await db
.update(systemSettings)
.set({ value, updatedBy: meta.userId, updatedAt: new Date() })
.where(and(eq(systemSettings.key, key), eq(systemSettings.portId, portId)));
} else {
await db.insert(systemSettings).values({
await db
.insert(systemSettings)
.values({
key,
value,
value: value as Record<string, unknown>,
portId,
updatedBy: meta.userId,
})
.onConflictDoUpdate({
target: [systemSettings.key, systemSettings.portId],
set: {
value: value as Record<string, unknown>,
updatedBy: meta.userId,
updatedAt: new Date(),
},
});
}
void createAuditLog({
userId: meta.userId,

View File

@@ -97,6 +97,17 @@ interface ProxyTokenPayload {
f?: string;
/** Optional content-type override. */
c?: string;
/**
* Port-binding: the port slug the issuer was scoped to when minting
* the token. The verifier asserts the storage key starts with
* `${p}/`. Defense-in-depth against a buggy issuer in some future
* code path that mixes up port scopes — every storage key generated
* by `generateStorageKey()` already prefixes the port slug, so this
* check costs nothing and catches any drift. Optional for backwards
* compatibility with tokens minted before this field shipped; new
* tokens always include it.
*/
p?: string;
}
function b64urlEncode(buf: Buffer): string {
@@ -165,6 +176,16 @@ export function verifyProxyToken(
if (payload.op !== expectedOp) {
return { ok: false, reason: 'op-mismatch' };
}
// Port-binding: when the issuer attached `p`, assert the key starts
// with `${p}/`. This is the actual enforcement — `validateStorageKey`
// already prevents path traversal but doesn't constrain which port's
// namespace the key belongs to. Tokens without `p` skip this check
// (legacy / non-port-scoped issuers continue to work).
if (payload.p !== undefined) {
if (!payload.k.startsWith(`${payload.p}/`)) {
return { ok: false, reason: 'port-mismatch' };
}
}
return { ok: true, payload };
}
@@ -301,7 +322,14 @@ export class FilesystemBackend implements StorageBackend {
validateStorageKey(key);
const expiresAt = Math.floor(Date.now() / 1000) + (opts.expirySeconds ?? 900);
const token = signProxyToken(
{ k: key, e: expiresAt, n: randomUUID(), op: 'put', c: opts.contentType },
{
k: key,
e: expiresAt,
n: randomUUID(),
op: 'put',
c: opts.contentType,
...(opts.portSlug ? { p: opts.portSlug } : {}),
},
this.hmacSecret,
);
return { url: `/api/storage/${token}`, method: 'PUT' };
@@ -319,6 +347,7 @@ export class FilesystemBackend implements StorageBackend {
op: 'get',
f: opts.filename,
c: opts.contentType,
...(opts.portSlug ? { p: opts.portSlug } : {}),
},
this.hmacSecret,
);

View File

@@ -38,6 +38,15 @@ export interface PresignOpts {
contentType?: string;
/** Filename used in Content-Disposition for downloads. */
filename?: string;
/**
* Optional port slug to bind the token to. The filesystem proxy
* verifier asserts the storage key starts with `${portSlug}/` when
* present. S3 backend ignores this field (presigned S3 URLs carry
* their own signature scope). Pass it whenever the issuer is in a
* port-scoped request — `generateStorageKey()` already prefixes the
* slug, so this is the matching enforcement.
*/
portSlug?: string;
}
export interface StorageBackend {
@@ -216,9 +225,10 @@ export async function presignDownloadUrl(
key: string,
expirySeconds = 900,
filename?: string,
portSlug?: string,
): Promise<string> {
const backend = await getStorageBackend();
const { url } = await backend.presignDownload(key, { expirySeconds, filename });
const { url } = await backend.presignDownload(key, { expirySeconds, filename, portSlug });
return url;
}

View File

@@ -91,3 +91,25 @@ export const MERGE_FIELDS: MergeFieldCatalog = {
export const VALID_MERGE_TOKENS: ReadonlySet<string> = new Set(
Object.values(MERGE_FIELDS).flatMap((scope) => scope.map((field) => field.token)),
);
/**
* Custom-field merge tokens follow the pattern `{{custom.<fieldName>}}`
* where fieldName matches the per-port custom field's `fieldName` (the
* machine identifier, not the display label). Field names are validated
* at definition time as `[a-z][a-z0-9_]*` so this regex is the matching
* recogniser. Tokens that match this shape bypass the static
* `VALID_MERGE_TOKENS` check — the resolver fetches the actual
* definitions per port at expand time.
*/
export const CUSTOM_MERGE_TOKEN_RE = /^\{\{custom\.[a-z][a-z0-9_]*\}\}$/;
/** True when the token is a `{{custom.<fieldName>}}` shape. */
export function isCustomMergeToken(token: string): boolean {
return CUSTOM_MERGE_TOKEN_RE.test(token);
}
/** Extract the fieldName from a `{{custom.<fieldName>}}` token. */
export function extractCustomFieldName(token: string): string | null {
const match = token.match(/^\{\{custom\.([a-z][a-z0-9_]*)\}\}$/);
return match?.[1] ?? null;
}

View File

@@ -1,16 +1,24 @@
import { z } from 'zod';
import { baseListQuerySchema } from '@/lib/api/list-query';
import { VALID_MERGE_TOKENS } from '@/lib/templates/merge-fields';
import { VALID_MERGE_TOKENS, isCustomMergeToken } from '@/lib/templates/merge-fields';
// A token is acceptable if it's in the static catalog OR matches the
// dynamic `{{custom.<fieldName>}}` shape. The resolver checks the actual
// per-port custom-field definition at expand time and substitutes the
// stored value (or leaves the token unresolved if no definition matches).
function isAcceptableMergeToken(token: string): boolean {
return VALID_MERGE_TOKENS.has(token) || isCustomMergeToken(token);
}
const mergeFieldsSchema = z
.array(z.string())
.optional()
.default([])
.refine(
(tokens) => tokens.every((t) => VALID_MERGE_TOKENS.has(t)),
(tokens) => tokens.every(isAcceptableMergeToken),
(tokens) => {
const unknown = tokens?.filter((t) => !VALID_MERGE_TOKENS.has(t)) ?? [];
const unknown = tokens?.filter((t) => !isAcceptableMergeToken(t)) ?? [];
return { message: `Unknown merge tokens: ${unknown.join(', ')}` };
},
);

View File

@@ -5,6 +5,8 @@ import { baseListQuerySchema } from '@/lib/api/list-query';
export const uploadFileSchema = z.object({
filename: z.string().min(1).max(255),
clientId: z.string().optional(),
yachtId: z.string().optional(),
companyId: z.string().optional(),
category: z.string().optional(),
entityType: z.string().optional(),
entityId: z.string().optional(),
@@ -17,6 +19,8 @@ export const updateFileSchema = z.object({
export const listFilesSchema = baseListQuerySchema.extend({
clientId: z.string().optional(),
yachtId: z.string().optional(),
companyId: z.string().optional(),
category: z.string().optional(),
entityType: z.string().optional(),
entityId: z.string().optional(),