From 0ee8e08e20ef8a279bfed171b4423865a20cb0eb Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 19 Aug 2026 14:54:03 -0700 Subject: [PATCH 1/3] fix(provenance): stop size limits silently dropping secret provenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bundle-selection cap counted cells rather than rows, so a 25-column table insert lost secret provenance for every row past 400 — the whole batch was stamped unknown with nothing logged. The same number lived in the sender, the runtime type guard, and the route contract. Consolidate every provenance limit into one definition: an 8MB serialized envelope and 10,000 distinct secrets. The pair had been copied into seven modules under fourteen names, and several copies had drifted into bounding inputs — rows, cells, files, chunks — rather than the envelope. Remove every limit that could refuse a legal payload, add write-side cause logging and a workspace-visible audit entry when a read proceeds on unrecorded provenance, and repair the existing unknown rows. --- .../app/api/memory/secret-provenance.test.ts | 21 ++- apps/sim/app/api/memory/secret-provenance.ts | 115 +++++++----- .../content/blog/secret-provenance/index.mdx | 4 +- .../resolved-secret-trace-registry.test.ts | 68 ++++++- .../utils/resolved-secret-trace-registry.ts | 154 ++++++++++++++-- apps/sim/lib/api/contracts/primitives.ts | 8 +- ...able-secret-provenance-enforcement.test.ts | 47 ++++- .../durable-secret-provenance-enforcement.ts | 34 ++++ .../execution/durable-secret-provenance.ts | 12 +- .../execution/model-input-provenance.test.ts | 139 ++++++++++++++- .../lib/execution/model-input-provenance.ts | 76 ++++++-- apps/sim/lib/execution/provenance-limits.ts | 35 ++++ apps/sim/lib/knowledge/secret-provenance.ts | 10 -- .../lib/table/rows/secret-provenance.test.ts | 49 +++++ apps/sim/lib/table/rows/secret-provenance.ts | 167 ++++++++++++------ .../workspace-file-secret-provenance.ts | 75 ++++---- packages/audit/src/types.ts | 13 ++ ...rations-paused-billing-attribution.test.ts | 1 + ...005_repair_unknown_table_row_provenance.ts | 72 ++++++++ packages/db/script-migrations/index.ts | 2 + packages/testing/src/mocks/audit.mock.ts | 2 + 21 files changed, 909 insertions(+), 195 deletions(-) create mode 100644 apps/sim/lib/execution/provenance-limits.ts create mode 100644 packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts diff --git a/apps/sim/app/api/memory/secret-provenance.test.ts b/apps/sim/app/api/memory/secret-provenance.test.ts index 476c513bc36..6c204427d06 100644 --- a/apps/sim/app/api/memory/secret-provenance.test.ts +++ b/apps/sim/app/api/memory/secret-provenance.test.ts @@ -200,7 +200,12 @@ describe('memory write secret provenance', () => { if (!result.success) expect(result.response.status).toBe(400) }) - it('bounds only requested private response provenance without querying sidecars', async () => { + /** + * A read of this width used to be refused outright on the record count alone. How many memories + * crossed said nothing about whether their provenance could be established, so the read now + * vouches for them and the page size is what keeps the statement count bounded. + */ + it('vouches for a very wide crossing instead of refusing on the record count', async () => { const request = new NextRequest('http://localhost/api/memory', { headers: { [PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1, @@ -220,12 +225,18 @@ describe('memory write secret provenance', () => { }) await expect(response.json()).resolves.toMatchObject({ - [RESOLVED_SECRET_PROVENANCE_FIELD]: { version: 1, complete: false, entries: [] }, + [RESOLVED_SECRET_PROVENANCE_FIELD]: { version: 1, complete: true, entries: [] }, }) - expect(dbChainMockFns.select).not.toHaveBeenCalled() + /** Eleven pages of a thousand, not one statement per handful of memories. */ + expect(dbChainMockFns.select.mock.calls.length).toBeLessThanOrEqual(11) }) - it('marks oversized aggregate sidecar provenance incomplete before importing it', async () => { + /** + * A sidecar too large to carry reads as unrecorded, not as a reason to fail the read. The run + * keeps its other provenance and proceeds without this memory's — best effort, with the risk + * recorded — rather than refusing every projection for the rest of the run. + */ + it('treats a sidecar too large to carry as unrecorded rather than failing the read', async () => { queueTableRows(memorySecretProvenance, [ { memoryId: 'memory-1', @@ -253,7 +264,7 @@ describe('memory write secret provenance', () => { }) await expect(response.json()).resolves.toMatchObject({ - [RESOLVED_SECRET_PROVENANCE_FIELD]: { version: 1, complete: false, entries: [] }, + [RESOLVED_SECRET_PROVENANCE_FIELD]: { version: 1, complete: true, entries: [] }, }) }) }) diff --git a/apps/sim/app/api/memory/secret-provenance.ts b/apps/sim/app/api/memory/secret-provenance.ts index 589ede10188..fa97a3dce20 100644 --- a/apps/sim/app/api/memory/secret-provenance.ts +++ b/apps/sim/app/api/memory/secret-provenance.ts @@ -1,4 +1,3 @@ -import { Buffer } from 'buffer' import { db } from '@sim/db' import { memorySecretProvenance } from '@sim/db/schema' import { inArray } from 'drizzle-orm' @@ -10,6 +9,7 @@ import { EXACT_EMPTY_DURABLE_SECRET_PROVENANCE, importDurableSecretProvenance, } from '@/lib/execution/durable-secret-provenance' +import { reportUnrecordedDurableProvenance } from '@/lib/execution/durable-secret-provenance-enforcement' import { inspectPrivateSecretProvenanceRequest, isPrivateSecretProvenanceBundleV1, @@ -22,10 +22,15 @@ import { import { readBoundMemorySecretProvenance } from '@/lib/memory/secret-provenance' import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' -const MAX_PRIVATE_MEMORY_CROSSINGS = 10_000 -const PRIVATE_MEMORY_QUERY_CHUNK_SIZE = 8 -const MAX_PRIVATE_MEMORY_PROVENANCE_ENTRIES = 10_000 -const MAX_PRIVATE_MEMORY_PROVENANCE_BYTES = 8 * 1024 * 1024 +/** + * Ids per sidecar lookup, matching how the rest of the codebase chunks an `inArray`. + * + * It was 8, which only worked because a cap refused any read over ten thousand memories — at that + * width the page loop would otherwise have issued more than a thousand sequential statements. The + * cap is gone because refusing on a record count told us nothing about the data, so the page size + * now has to be the thing that keeps the read bounded. + */ +const PRIVATE_MEMORY_QUERY_CHUNK_SIZE = 1_000 interface MemoryCrossing { id: string @@ -97,52 +102,68 @@ export async function createMemoryResponse(options: { userId: options.userId, workspaceId: options.workspaceId, }) - if (options.memories.length > MAX_PRIVATE_MEMORY_CROSSINGS) { - registry.markIncomplete('memory-crossing-capacity-exceeded') - } else { - const ids = [...new Set(options.memories.map((record) => record.id))] - const memoriesById = new Map() - for (const memory of options.memories) { - const matching = memoriesById.get(memory.id) ?? [] - matching.push(memory) - memoriesById.set(memory.id, matching) - } - let provenanceEntryCount = 0 - let provenanceBytes = 0 - for (let index = 0; index < ids.length; index += PRIVATE_MEMORY_QUERY_CHUNK_SIZE) { - const pageIds = ids.slice(index, index + PRIVATE_MEMORY_QUERY_CHUNK_SIZE) - const sidecars = await db - .select() - .from(memorySecretProvenance) - .where(inArray(memorySecretProvenance.memoryId, pageIds)) - for (const sidecar of sidecars) { - provenanceEntryCount += Array.isArray(sidecar.entries) ? sidecar.entries.length : 0 - provenanceBytes += Buffer.byteLength(JSON.stringify(sidecar.entries ?? []), 'utf8') - } - if ( - provenanceEntryCount > MAX_PRIVATE_MEMORY_PROVENANCE_ENTRIES || - provenanceBytes > MAX_PRIVATE_MEMORY_PROVENANCE_BYTES - ) { - registry.markIncomplete('memory-crossing-capacity-exceeded') - break - } - const sidecarById = new Map(sidecars.map((sidecar) => [sidecar.memoryId, sidecar])) - for (const memoryId of pageIds) { - for (const record of memoriesById.get(memoryId) ?? []) { - const sidecar = sidecarById.get(record.id) - const provenance = readBoundMemorySecretProvenance({ - secretProvenanceVersion: record.secretProvenanceVersion, - data: record.data, - provenanceContentHash: sidecar?.contentHash ?? null, - status: sidecar?.status ?? null, - entries: sidecar?.entries, - }) - await importDurableSecretProvenance(registry, provenance, record.data, 'memory') - } + /** + * No cap on how many memories may cross, and no separate accounting of what they carry. + * + * There was both: a refusal past ten thousand records, and a running total of every sidecar's + * entries. The first said nothing about the data. The second summed entries *before* they were + * folded, so a page of memories sharing a handful of secrets counted once per mention and could + * refuse a read whose envelope would have held a dozen entries. + * + * Neither is needed, because the registry already bounds the only thing that has a real limit — + * the serialized envelope — as each entry is added, at the granularity it actually dedupes on. + * A second estimate in front of it could only ever be wrong in one of two directions. + */ + const ids = [...new Set(options.memories.map((record) => record.id))] + const memoriesById = new Map() + for (const memory of options.memories) { + const matching = memoriesById.get(memory.id) ?? [] + matching.push(memory) + memoriesById.set(memory.id, matching) + } + let unrecordedMemoryCount = 0 + for (let index = 0; index < ids.length; index += PRIVATE_MEMORY_QUERY_CHUNK_SIZE) { + const pageIds = ids.slice(index, index + PRIVATE_MEMORY_QUERY_CHUNK_SIZE) + const sidecars = await db + .select() + .from(memorySecretProvenance) + .where(inArray(memorySecretProvenance.memoryId, pageIds)) + const sidecarById = new Map(sidecars.map((sidecar) => [sidecar.memoryId, sidecar])) + for (const memoryId of pageIds) { + for (const record of memoriesById.get(memoryId) ?? []) { + const sidecar = sidecarById.get(record.id) + const provenance = readBoundMemorySecretProvenance({ + secretProvenanceVersion: record.secretProvenanceVersion, + data: record.data, + provenanceContentHash: sidecar?.contentHash ?? null, + status: sidecar?.status ?? null, + entries: sidecar?.entries, + }) + if (provenance.status === 'unknown') unrecordedMemoryCount += 1 + await importDurableSecretProvenance(registry, provenance, record.data, 'memory') } } } + /** + * Counted here and reported once, rather than left to the per-record import. + * + * That import reports without a workspace, so the workspace-visible half of the trail never + * reached the people it concerns — the audit entry is skipped when it cannot name one. Passing + * the workspace down instead would have written one row per record, which on a wide read is + * thousands of fire-and-forget inserts for a single event. One read is one thing that happened, + * so it is one entry carrying how many records it covered — the shape the table surface uses. + */ + if (unrecordedMemoryCount > 0) { + reportUnrecordedDurableProvenance({ + surface: 'memory', + cause: 'durable-provenance-unknown', + affectedCount: unrecordedMemoryCount, + workspaceId: options.workspaceId, + actorUserId: options.userId, + }) + } + const envelope = serializePrivateToolMetadataResponseEnvelope( options.body, RESOLVED_SECRET_PROVENANCE_METADATA_V1, diff --git a/apps/sim/content/blog/secret-provenance/index.mdx b/apps/sim/content/blog/secret-provenance/index.mdx index 754d20cac88..20a843d2055 100644 --- a/apps/sim/content/blog/secret-provenance/index.mdx +++ b/apps/sim/content/blog/secret-provenance/index.mdx @@ -64,7 +64,9 @@ Results returning from tools and Function blocks are checked before they become Storage is the boundary that is easiest to miss. Workspace files, table cells, knowledge documents, and agent memory can all survive the execution that created them. Without durable provenance, a secret written during one run would look like ordinary data when another run read it back. Sim stores an encrypted provenance record and version marker alongside that content, allowing a later read to distinguish data known to be clean from data whose history is unknown. -When that history cannot be established, we do not treat `unknown` as `clean`. The payload is withheld, while safe parts of the record remain available: block identity, timing, status, and error type. A tool result can be kept out of model context without hiding the fact that the tool ran. The refusal also records where it happened and what made the registry incomplete, so failing closed does not mean debugging blind. +When that history cannot be established, we do not treat `unknown` as `clean`. At a run's own boundaries the payload is withheld, while safe parts of the record remain available: block identity, timing, status, and error type. A tool result can be kept out of model context without hiding the fact that the tool ran. The refusal also records where it happened and what made the registry incomplete, so failing closed does not mean debugging blind. + +Stored data is different, because a record written months ago outlives the run that could explain it. Refusing to read it would strand a team on its own tables and memory to protect against a risk we cannot confirm is there. So for that content the read proceeds and the workspace gets an audit entry naming the surface and what could not be vouched for. The judgment is deliberate: withholding is right when a run is asking to send something out, and visibility is right when the alternative is a workspace that can no longer read what it stored. ## Exact Matching, With One Limit diff --git a/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts b/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts index ed8e06242a7..f79ad9c7290 100644 --- a/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts +++ b/apps/sim/executor/utils/resolved-secret-trace-registry.test.ts @@ -416,6 +416,72 @@ describe('ResolvedSecretTraceRegistry', () => { }) }) + it('narrows each grouped export to its own root', () => { + const scope = { userId: 'user-1', workspaceId: 'workspace-1' } + const registry = new ResolvedSecretTraceRegistry( + [ + { name: 'FIRST', plaintext: 'alpha', encryptedValue: 'encrypted-first' }, + { name: 'SECOND', plaintext: 'beta', encryptedValue: 'encrypted-second' }, + ], + scope + ) + registry.recordResolvedAtInputPath('FIRST', 'alpha', ['rows', '0', 'a']) + registry.recordResolvedAtInputPath('SECOND', 'beta', ['rows', '1', 'b']) + registry.recordResolvedAtInputPath('FIRST', 'alpha', ['rows', '2', 'c', 'nested']) + + const exported = registry.exportCommittedProvenanceForInputPathGroups([ + [['rows', '0', 'a']], + [['rows', '1', 'b']], + [['rows', '2', 'c']], + [['rows']], + [['rows', '3', 'untouched']], + [], + [ + ['rows', '0', 'a'], + ['rows', '1', 'b'], + ], + ]) + + expect(exported.map((provenance) => provenance.entries)).toEqual([ + [{ name: 'FIRST', encryptedValue: 'encrypted-first' }], + [{ name: 'SECOND', encryptedValue: 'encrypted-second' }], + [{ name: 'FIRST', encryptedValue: 'encrypted-first' }], + [ + { name: 'FIRST', encryptedValue: 'encrypted-first' }, + { name: 'SECOND', encryptedValue: 'encrypted-second' }, + ], + [], + [], + [ + { name: 'FIRST', encryptedValue: 'encrypted-first' }, + { name: 'SECOND', encryptedValue: 'encrypted-second' }, + ], + ]) + expect(exported.every((provenance) => provenance.complete)).toBe(true) + }) + + it('fails only the grouped exports an incomplete input path overlaps', async () => { + const scope = { userId: 'user-1', workspaceId: 'workspace-1' } + const registry = new ResolvedSecretTraceRegistry( + [{ name: 'FIRST', plaintext: 'alpha', encryptedValue: 'encrypted-first' }], + scope + ) + registry.recordResolvedAtInputPath('FIRST', 'alpha', ['rows', '0', 'a']) + await registry.importProvenanceForValueAtInputPath(null, 'alpha', ['rows', '1', 'b'], { + trusted: false, + }) + + const exported = registry.exportCommittedProvenanceForInputPathGroups([ + [['rows', '1', 'b']], + [['rows', '1']], + [['rows', '1', 'b', 'deeper']], + [['rows', '0', 'a']], + ]) + + expect(exported.map((provenance) => provenance.complete)).toEqual([false, false, false, true]) + expect(exported[3].entries).toEqual([{ name: 'FIRST', encryptedValue: 'encrypted-first' }]) + }) + it('fails closed when independent secret paths collapse into one transformed string', () => { const registry = new ResolvedSecretTraceRegistry([ { name: 'FIRST', plaintext: 'first', encryptedValue: 'encrypted-first' }, @@ -1598,8 +1664,6 @@ describe('incompleteness diagnostics', () => { 'client-tool-execution-untrusted', 'client-tool-content-unavailable', 'knowledge-result-provenance-unavailable', - 'knowledge-response-capacity-exceeded', - 'memory-crossing-capacity-exceeded', 'table-result-provenance-unavailable', 'mounted-file-provenance-unavailable', 'workspace-file-provenance-unknown', diff --git a/apps/sim/executor/utils/resolved-secret-trace-registry.ts b/apps/sim/executor/utils/resolved-secret-trace-registry.ts index 7281c75b832..30abd837e86 100644 --- a/apps/sim/executor/utils/resolved-secret-trace-registry.ts +++ b/apps/sim/executor/utils/resolved-secret-trace-registry.ts @@ -4,6 +4,10 @@ import { decryptSecret } from '@/lib/core/security/encryption' import { isLargeArrayManifest } from '@/lib/execution/payloads/large-array-manifest-metadata' import { isLargeValueRef } from '@/lib/execution/payloads/large-value-ref' import { MAX_INLINE_MATERIALIZATION_BYTES } from '@/lib/execution/payloads/limits' +import { + PROVENANCE_MAX_ENTRIES, + PROVENANCE_MAX_SERIALIZED_BYTES, +} from '@/lib/execution/provenance-limits' import { isNonIdentifyingSecretLiteral } from '@/executor/utils/resolved-secret-match-policy' import { createResolvedSecretMatcher, @@ -57,10 +61,8 @@ export type ResolvedSecretIncompletenessReason = | 'client-tool-execution-untrusted' | 'client-tool-content-unavailable' | 'knowledge-result-provenance-unavailable' - | 'knowledge-response-capacity-exceeded' | 'knowledge-row-missing' | 'knowledge-row-content-mismatch' - | 'memory-crossing-capacity-exceeded' | 'table-result-provenance-unavailable' | 'mounted-file-provenance-unavailable' | 'workspace-file-provenance-unknown' @@ -172,10 +174,10 @@ export interface ResolvedSecretIncompletenessDiagnostics { export const ANONYMOUS_SECRET_TRACE_REPLACEMENT = OPAQUE_RESOLVED_SECRET_REPLACEMENT export const RESOLVED_SECRET_TRACE_CHECKPOINT_VERSION = 1 -const MAX_PROVENANCE_ENTRIES = 10_000 -const MAX_SERIALIZED_PROVENANCE_BYTES = 8 * 1024 * 1024 -const MAX_TRACE_CATALOG_ENTRIES = MAX_PROVENANCE_ENTRIES -const MAX_TRACE_CATALOG_BYTES = 8 * 1024 * 1024 +const MAX_PROVENANCE_ENTRIES = PROVENANCE_MAX_ENTRIES +const MAX_SERIALIZED_PROVENANCE_BYTES = PROVENANCE_MAX_SERIALIZED_BYTES +const MAX_TRACE_CATALOG_ENTRIES = PROVENANCE_MAX_ENTRIES +const MAX_TRACE_CATALOG_BYTES = PROVENANCE_MAX_SERIALIZED_BYTES const MAX_PROVENANCE_FILTER_NODES = 50_000 const MAX_PROVENANCE_FILTER_CHARACTERS = MAX_INLINE_MATERIALIZATION_BYTES const MAX_PROVENANCE_FILTER_MATCH_EVENTS = 1_000_000 @@ -372,6 +374,32 @@ function inputPathsOverlap(left: readonly string[], right: readonly string[]): b return isInputPathWithin(left, right) || isInputPathWithin(right, left) } +const EMPTY_GROUP_MATCH: readonly number[] = [] + +/** + * Indices of every group whose root sits at or above `path`. + * + * The prefix form of {@link isInputPathWithin}, read from an index of the roots rather than by + * testing each one. Scanning the roots per path is what forced a cap on how many a caller could + * vouch for at once; walking `path`'s own prefixes is bounded by its depth instead. + * + * Copies on the first hit rather than aliasing, because the caller owns the index and a returned + * alias would let an append mutate it. + */ +function groupsAlongInputPath( + groupsByRoot: ReadonlyMap, + path: ResolvedSecretInputPath +): readonly number[] { + let matched: number[] | undefined + for (let length = 0; length <= path.length; length += 1) { + const indices = groupsByRoot.get(inputPathKey(path.slice(0, length))) + if (!indices) continue + if (!matched) matched = [...indices] + else matched.push(...indices) + } + return matched ?? EMPTY_GROUP_MATCH +} + function readInputPath(root: unknown, path: readonly string[]): unknown { let current = root for (const segment of path) { @@ -1334,19 +1362,111 @@ export class ResolvedSecretTraceRegistry { paths: readonly ResolvedSecretInputPath[], options: ExportResolvedSecretTraceProvenanceForValueOptions = {} ): ResolvedSecretTraceProvenanceV1 { - if (!this.complete || this.hasIncompleteInputPathOverlapping(paths)) { - return this.incompleteProvenance() + return this.exportCommittedProvenanceForInputPathGroups([paths], options)[0] + } + + /** + * Exports resolver-recorded provenance for many input-path groups in a single pass. + * + * One group per cell a write vouches for. Called per group, each export rescans every resolved + * input path and every active entry, so vouching for N cells cost O(N x paths) — and a wide + * table write is exactly that shape. That cost is what a selection cap was really bounding, and + * the cap failed the whole bundle rather than the work, so every row of an oversized write + * landed `unknown` in its durable sidecar with nothing recorded about why. + * + * Indexing the group roots once makes the batch linear in the resolved paths and the active + * entries, so there is no size at which a caller has to stop vouching. Groups are answered + * independently and in order: an incomplete input path fails only the groups it overlaps, which + * is the same per-group judgement the single-path form has always made. + */ + exportCommittedProvenanceForInputPathGroups( + groups: ReadonlyArray, + options: ExportResolvedSecretTraceProvenanceForValueOptions = {} + ): ResolvedSecretTraceProvenanceV1[] { + if (!this.complete) return groups.map(() => this.incompleteProvenance()) + + const groupsByRoot = new Map() + groups.forEach((paths, index) => { + for (const path of paths) { + const key = inputPathKey(path) + const existing = groupsByRoot.get(key) + if (existing) existing.push(index) + else groupsByRoot.set(key, [index]) + } + }) + + /** + * Overlap is symmetric, so a group fails on an incomplete path at or below its root — matched + * by walking that path — or at or above it, matched by walking the root's own prefixes. + */ + const incompleteGroups = new Set() + for (const incompletePath of this.incompleteInputPaths.values()) { + for (const index of groupsAlongInputPath(groupsByRoot, incompletePath)) { + incompleteGroups.add(index) + } } - const selectedKeys = this.collectInputPathEntryKeys(paths) - const entries = [...this.activeEntries] - .filter(([key]) => selectedKeys.has(key)) - .map(([, entry]) => entry) - return { - version: 1, - complete: true, - entries: this.buildProvenanceEntries(entries, options.anonymous), - ...(this.scope ? { scope: cloneProvenanceScope(this.scope) } : {}), + if (incompleteGroups.size < groups.length) { + const incompleteRoots = new Set(this.incompleteInputPaths.keys()) + groups.forEach((paths, index) => { + if (incompleteGroups.has(index)) return + for (const path of paths) { + for (let length = 0; length <= path.length; length += 1) { + if (!incompleteRoots.has(inputPathKey(path.slice(0, length)))) continue + incompleteGroups.add(index) + return + } + } + }) } + + /** + * Allocated per group only once that group actually selects something. A write whose cells + * carry no secrets is the common case and the widest one, and it is the shape that used to + * exceed the cap — it should not pay a collection per cell to say so. + */ + const entryKeysByGroup: Array | undefined> = new Array(groups.length) + for (const state of this.resolvedInputPaths.values()) { + if (state.entryKeys.size === 0) continue + for (const index of groupsAlongInputPath(groupsByRoot, state.path)) { + if (incompleteGroups.has(index)) continue + const selected = (entryKeysByGroup[index] ??= new Set()) + for (const entryKey of state.entryKeys) selected.add(entryKey) + } + } + + /** + * Inverted before the single walk of `activeEntries` so each group's entries keep that map's + * insertion order, which is the order the per-group export produced and the order + * {@link buildProvenanceEntries} breaks its ties on. + */ + const groupsByEntryKey = new Map() + entryKeysByGroup.forEach((entryKeys, index) => { + if (!entryKeys) return + for (const entryKey of entryKeys) { + const existing = groupsByEntryKey.get(entryKey) + if (existing) existing.push(index) + else groupsByEntryKey.set(entryKey, [index]) + } + }) + const entriesByGroup: Array = new Array(groups.length) + if (groupsByEntryKey.size > 0) { + for (const [entryKey, entry] of this.activeEntries) { + const indices = groupsByEntryKey.get(entryKey) + if (!indices) continue + for (const index of indices) (entriesByGroup[index] ??= []).push(entry) + } + } + + return groups.map((_, index) => + incompleteGroups.has(index) + ? this.incompleteProvenance() + : { + version: 1, + complete: true, + entries: this.buildProvenanceEntries(entriesByGroup[index] ?? [], options.anonymous), + ...(this.scope ? { scope: cloneProvenanceScope(this.scope) } : {}), + } + ) } /** Imports encrypted provenance only from a boundary that has already established trust. */ diff --git a/apps/sim/lib/api/contracts/primitives.ts b/apps/sim/lib/api/contracts/primitives.ts index 90306e53060..c2eeec7228f 100644 --- a/apps/sim/lib/api/contracts/primitives.ts +++ b/apps/sim/lib/api/contracts/primitives.ts @@ -82,7 +82,13 @@ export const privateSecretProvenanceBundleSchema = z }) .strict() ) - .max(10_000) + /** + * Deliberately uncounted. One selection per cell a write vouches for, so a count cap here + * is a cap on how wide a write may be — a 25-column table crossed 10,000 at 401 rows. The + * sender that used to enforce the same number silently gave up and marked every row of the + * write `unknown`; rejecting the request instead would turn that into a failed write. The + * aggregate byte bound below and the route's body limit are the real bounds. + */ .describe('Selections and their encrypted provenance.'), }) .strict() diff --git a/apps/sim/lib/execution/durable-secret-provenance-enforcement.test.ts b/apps/sim/lib/execution/durable-secret-provenance-enforcement.test.ts index 46310bb9b29..1d6552d5180 100644 --- a/apps/sim/lib/execution/durable-secret-provenance-enforcement.test.ts +++ b/apps/sim/lib/execution/durable-secret-provenance-enforcement.test.ts @@ -3,13 +3,20 @@ */ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockEnv, mockLogger } = vi.hoisted(() => ({ +const { mockEnv, mockLogger, mockRecordAudit } = vi.hoisted(() => ({ mockEnv: {} as Record, mockLogger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + mockRecordAudit: vi.fn(), })) vi.mock('@/lib/core/config/env', () => ({ env: mockEnv })) vi.mock('@sim/logger', () => ({ createLogger: () => mockLogger })) +/** Literal values rather than the real constants: these reach the database and the trail. */ +vi.mock('@sim/audit', () => ({ + recordAudit: mockRecordAudit, + AuditAction: { SECRET_PROVENANCE_UNRECORDED: 'secret_provenance.unrecorded' }, + AuditResourceType: { SECRET_PROVENANCE: 'secret_provenance' }, +})) import { DURABLE_SECRET_PROVENANCE_SURFACES, @@ -89,4 +96,42 @@ describe('durable secret provenance enforcement', () => { } ) }) + it('records a workspace-visible audit entry so a fail-open read is not only in our logs', () => { + reportUnrecordedDurableProvenance({ + surface: 'table-row', + cause: 'row-sidecar-not-exact', + affectedCount: 8, + workspaceId: 'workspace-1', + actorUserId: 'user-1', + }) + + expect(mockRecordAudit).toHaveBeenCalledWith( + expect.objectContaining({ + workspaceId: 'workspace-1', + actorId: 'user-1', + action: 'secret_provenance.unrecorded', + resourceType: 'secret_provenance', + resourceId: 'table-row', + metadata: { surface: 'table-row', cause: 'row-sidecar-not-exact', affectedCount: 8 }, + }) + ) + }) + + it('still records the entry when the surface cannot name an actor', () => { + reportUnrecordedDurableProvenance({ + surface: 'memory', + cause: 'durable-provenance-unknown', + workspaceId: 'workspace-1', + }) + + expect(mockRecordAudit).toHaveBeenCalledWith(expect.objectContaining({ actorId: null })) + }) + + /** An entry with no workspace names nobody it concerns; the log line still carries it. */ + it('skips the audit entry when there is no workspace to show it to', () => { + reportUnrecordedDurableProvenance({ surface: 'knowledge', cause: 'durable-provenance-unknown' }) + + expect(mockRecordAudit).not.toHaveBeenCalled() + expect(mockLogger.error).toHaveBeenCalled() + }) }) diff --git a/apps/sim/lib/execution/durable-secret-provenance-enforcement.ts b/apps/sim/lib/execution/durable-secret-provenance-enforcement.ts index 5058cfa3b2a..8c9356549dd 100644 --- a/apps/sim/lib/execution/durable-secret-provenance-enforcement.ts +++ b/apps/sim/lib/execution/durable-secret-provenance-enforcement.ts @@ -1,3 +1,4 @@ +import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' import { env } from '@/lib/core/config/env' @@ -90,6 +91,11 @@ export interface UnrecordedDurableProvenanceReport { /** How many records in this one read were unrecorded, when the caller reads a page at a time. */ affectedCount?: number workspaceId?: string + /** + * Whose access authorized the read. Null where the surface cannot name one — an audit row with + * no actor still carries the workspace, surface, and cause, which is what the trail is for. + */ + actorUserId?: string | null } /** @@ -109,6 +115,34 @@ export function reportUnrecordedDurableProvenance(report: UnrecordedDurableProve ...(report.affectedCount !== undefined ? { affectedCount: report.affectedCount } : {}), ...(report.workspaceId ? { workspaceId: report.workspaceId } : {}), }) + + /** + * The log line is for us; this is for the people who own the secrets. + * + * Recorded here rather than where provenance was lost, because losing it costs nothing on its + * own — a write nobody could vouch for is just data at rest. The exposure is this moment: a + * value crossing into a run that will project it to a model with no way to recognise a secret + * inside it and redact it. That is what a reader needs told, and it is why the entry names the + * surface and the count rather than a secret, which is precisely what was not recorded. + * + * Fire-and-forget by construction — `recordAudit` never throws — so the trail can never be the + * reason a run fails. Skipped without a workspace: the entry would name no one it concerns. + */ + if (!report.workspaceId) return + recordAudit({ + workspaceId: report.workspaceId, + actorId: report.actorUserId ?? null, + action: AuditAction.SECRET_PROVENANCE_UNRECORDED, + resourceType: AuditResourceType.SECRET_PROVENANCE, + resourceId: report.surface, + description: + 'A run read data whose secret provenance was never recorded, so any secret it carries could not be redacted before reaching a model.', + metadata: { + surface: report.surface, + cause: report.cause, + ...(report.affectedCount !== undefined ? { affectedCount: report.affectedCount } : {}), + }, + }) } /** Test seam: forces the next read to re-resolve the env-configured surfaces. */ diff --git a/apps/sim/lib/execution/durable-secret-provenance.ts b/apps/sim/lib/execution/durable-secret-provenance.ts index 724f0ac340d..228a6d7ebf1 100644 --- a/apps/sim/lib/execution/durable-secret-provenance.ts +++ b/apps/sim/lib/execution/durable-secret-provenance.ts @@ -9,14 +9,16 @@ import { isPrivateSecretProvenanceBundleV1, type PrivateSecretProvenanceBundleV1, } from '@/lib/execution/model-input-provenance' +import { + PROVENANCE_MAX_ENTRIES, + PROVENANCE_MAX_SERIALIZED_BYTES, +} from '@/lib/execution/provenance-limits' import { type ResolvedSecretTraceProvenanceV1, ResolvedSecretTraceRegistry, type ResolvedSecretTraceScopeV1, } from '@/executor/utils/resolved-secret-trace-registry' -const MAX_DURABLE_SECRET_PROVENANCE_ENTRIES = 10_000 -const MAX_DURABLE_SECRET_PROVENANCE_BYTES = 8 * 1024 * 1024 const MAX_DURABLE_HASH_NODES = 50_000 const MAX_DURABLE_HASH_DEPTH = 100 const MAX_DURABLE_HASH_BYTES = 16 * 1024 * 1024 @@ -38,7 +40,7 @@ function compareStrings(left: string, right: string): number { export function normalizeDurableSecretProvenanceEntries( value: unknown ): DurableSecretProvenanceEntry[] | undefined { - if (!Array.isArray(value) || value.length > MAX_DURABLE_SECRET_PROVENANCE_ENTRIES) { + if (!Array.isArray(value) || value.length > PROVENANCE_MAX_ENTRIES) { return undefined } @@ -74,7 +76,7 @@ export function normalizeDurableSecretProvenanceEntries( const key = `${entry.sourceUserId ?? ''}\u0000${entry.sourceWorkspaceId ?? ''}\u0000${entry.sourceValueHash ?? ''}\u0000${entry.name ?? ''}\u0000${entry.encryptedValue}` if (entries.has(key)) continue bytes += Buffer.byteLength(key, 'utf8') - if (bytes > MAX_DURABLE_SECRET_PROVENANCE_BYTES) return undefined + if (bytes > PROVENANCE_MAX_SERIALIZED_BYTES) return undefined entries.set(key, entry) } @@ -86,7 +88,7 @@ export function normalizeDurableSecretProvenanceEntries( compareStrings(left.name ?? '', right.name ?? '') || compareStrings(left.encryptedValue, right.encryptedValue) ) - if (Buffer.byteLength(JSON.stringify(normalized), 'utf8') > MAX_DURABLE_SECRET_PROVENANCE_BYTES) { + if (Buffer.byteLength(JSON.stringify(normalized), 'utf8') > PROVENANCE_MAX_SERIALIZED_BYTES) { return undefined } return normalized diff --git a/apps/sim/lib/execution/model-input-provenance.test.ts b/apps/sim/lib/execution/model-input-provenance.test.ts index 80a8b7b9ad5..5d302cda13b 100644 --- a/apps/sim/lib/execution/model-input-provenance.test.ts +++ b/apps/sim/lib/execution/model-input-provenance.test.ts @@ -1,14 +1,27 @@ /** * @vitest-environment node */ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' + +const { mockLogger } = vi.hoisted(() => ({ + mockLogger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})) + +vi.mock('@sim/logger', () => ({ + createLogger: () => mockLogger, +})) + +import { privateSecretProvenanceBundleSchema } from '@/lib/api/contracts/primitives' import { createModelInputProvenanceRequestMetadata, + createPrivateSecretProvenanceRequestMetadata, inspectModelInputProjectionState, inspectModelInputProvenanceRequest, + isPrivateSecretProvenanceBundleV1, PRIVATE_MODEL_INPUT_PROVENANCE_HEADER, PRIVATE_MODEL_INPUT_STATE_HEADER, PROJECTED_MODEL_INPUT_PATHS_V1, + type PrivateSecretProvenanceSelection, projectModelSchemaAnnotations, projectResolvedModelInput, selectModelSchemaInputPaths, @@ -302,3 +315,127 @@ describe('model input provenance transport', () => { ).toEqual({ success: false, error: 'Invalid model input provenance', status: 400 }) }) }) + +/** One selection per populated cell, the shape `selectTableRowSecretProvenance` produces. */ +function cellSelections(rows: number, columns: number): PrivateSecretProvenanceSelection[] { + const selections: PrivateSecretProvenanceSelection[] = [] + for (let row = 0; row < rows; row += 1) { + for (let column = 0; column < columns; column += 1) { + selections.push({ + key: JSON.stringify([row, `column${column}`]), + inputPaths: [['rows', String(row), `column${column}`]], + }) + } + } + return selections +} + +describe('private secret provenance bundle', () => { + const scope = { userId: 'user-1', workspaceId: 'workspace-1' } + + /** + * A 25-column table insert crossed the removed selection cap at 401 rows, and crossing it made + * the whole bundle incomplete — so every row of the write landed `unknown` in its durable + * sidecar. The count here is the production write that surfaced it. + * + * Every cell resolves a secret, which is also what guards the cost: answering each selection by + * rescanning the resolved paths is quadratic at this width and cannot finish inside the default + * timeout, so a reintroduced per-group scan fails here rather than in production. + */ + it('vouches for a write far wider than the removed selection cap', () => { + const registry = new ResolvedSecretTraceRegistry([ENTRY], scope) + const selections = cellSelections(500, 25) + expect(selections).toHaveLength(12_500) + for (const selection of selections) { + registry.recordResolvedAtInputPath(ENTRY.name, ENTRY.plaintext, selection.inputPaths[0]) + } + + const metadata = createPrivateSecretProvenanceRequestMetadata(registry, selections) + + expect(metadata?.provenance.complete).toBe(true) + expect(metadata?.provenance.selections).toHaveLength(12_500) + expect(isPrivateSecretProvenanceBundleV1(metadata?.provenance)).toBe(true) + expect( + metadata?.provenance.selections.every( + (selection) => selection.provenance.entries.length === 1 + ) + ).toBe(true) + }) + + it('still narrows a wide write to the one cell that carried a secret', () => { + const registry = new ResolvedSecretTraceRegistry([ENTRY], scope) + registry.recordResolvedAtInputPath(ENTRY.name, ENTRY.plaintext, ['rows', '7', 'column3']) + + const metadata = createPrivateSecretProvenanceRequestMetadata(registry, cellSelections(500, 25)) + + expect( + metadata?.provenance.selections.filter((selection) => selection.provenance.entries.length > 0) + ).toEqual([ + { + key: JSON.stringify([7, 'column3']), + provenance: expect.objectContaining({ + complete: true, + entries: [{ name: ENTRY.name, encryptedValue: ENTRY.encryptedValue }], + }), + }, + ]) + }) + + it('fails the bundle and names the cause when an input path cannot be vouched for', async () => { + const registry = new ResolvedSecretTraceRegistry([ENTRY], scope) + registry.recordResolvedAtInputPath(ENTRY.name, ENTRY.plaintext, ['rows', '0', 'column0']) + await registry.importProvenanceForValueAtInputPath( + null, + ENTRY.plaintext, + ['rows', '1', 'column1'], + { trusted: false } + ) + mockLogger.error.mockClear() + + const metadata = createPrivateSecretProvenanceRequestMetadata(registry, cellSelections(3, 3)) + + expect(metadata?.provenance.complete).toBe(false) + expect(metadata?.provenance.selections).toEqual([]) + expect(mockLogger.error).toHaveBeenCalledWith( + 'Private secret provenance bundle is incomplete', + expect.objectContaining({ failure: 'registry-incomplete', selectionCount: 9 }) + ) + }) + + it('rejects a duplicate selection key without consulting the registry', () => { + const registry = new ResolvedSecretTraceRegistry([], scope) + const exportGroups = vi.spyOn(registry, 'exportCommittedProvenanceForInputPathGroups') + + const metadata = createPrivateSecretProvenanceRequestMetadata(registry, [ + { key: 'same', inputPaths: [['rows', '0', 'a']] }, + { key: 'same', inputPaths: [['rows', '1', 'b']] }, + ]) + + expect(metadata?.provenance.complete).toBe(false) + expect(exportGroups).not.toHaveBeenCalled() + }) +}) + +/** + * The sender, the runtime type guard, and the route contract each used to enforce their own + * selection count. Removing it from two of the three would have converted a silently + * under-recorded write into a rejected one — worse than the bug being fixed — so the agreement + * is pinned end to end at the width that surfaced it rather than layer by layer. + */ +describe('private secret provenance bundle crosses its own contract', () => { + it('accepts a bundle at the width that used to trip every layer', () => { + const registry = new ResolvedSecretTraceRegistry([ENTRY], { + userId: 'user-1', + workspaceId: 'workspace-1', + }) + const selections = cellSelections(500, 25) + for (const selection of selections) { + registry.recordResolvedAtInputPath(ENTRY.name, ENTRY.plaintext, selection.inputPaths[0]) + } + + const metadata = createPrivateSecretProvenanceRequestMetadata(registry, selections) + + expect(metadata?.provenance.selections).toHaveLength(12_500) + expect(privateSecretProvenanceBundleSchema.safeParse(metadata?.provenance).success).toBe(true) + }) +}) diff --git a/apps/sim/lib/execution/model-input-provenance.ts b/apps/sim/lib/execution/model-input-provenance.ts index 6bc03c62bce..6b9feb6eee9 100644 --- a/apps/sim/lib/execution/model-input-provenance.ts +++ b/apps/sim/lib/execution/model-input-provenance.ts @@ -1,3 +1,4 @@ +import { createLogger } from '@sim/logger' import { isPlainRecord, isRecordLike } from '@sim/utils/object' import { PRIVATE_SECRET_PROVENANCE_BUNDLE_V1, @@ -6,6 +7,7 @@ import { RESOLVED_SECRET_PROVENANCE_FIELD, RESOLVED_SECRET_PROVENANCE_METADATA_V1, } from '@/lib/execution/private-tool-metadata' +import { PROVENANCE_MAX_SERIALIZED_BYTES } from '@/lib/execution/provenance-limits' import { isResolvedSecretTraceProvenanceV1, type ResolvedSecretInputPath, @@ -21,8 +23,29 @@ export const OPAQUE_MODEL_INPUT_PROVENANCE_UNAVAILABLE_ERROR = export const OPAQUE_MODEL_INPUT_RESOLVED_SECRET_ERROR = 'Model input contains a resolved secret that cannot be safely projected' -const MAX_PRIVATE_SECRET_PROVENANCE_SELECTIONS = 10_000 -const MAX_PRIVATE_SECRET_PROVENANCE_BYTES = 8 * 1024 * 1024 +const logger = createLogger('ModelInputProvenance') + +/** + * Ceiling on the serialized envelope, not on how much a caller may vouch for. + * + * A selection count cap used to sit beside this one. It was reachable by an ordinary write — a + * 25-column table insert crossed it at 401 rows — and crossing it failed the entire bundle, so + * every row of the write landed `unknown` in its durable sidecar. It existed to bound a per-group + * rescan in the registry that is now a single indexed pass, so there is nothing left for a count + * to protect. This bound stays because a request body is a real transport limit. + */ + +/** + * Why a bundle could not vouch for the cells one write touched. + * + * A closed union rather than a free-form string, for the reason the resolved-secret registry's + * reason set is one: an incomplete bundle stamps every row of the write `unknown` in its durable + * sidecar, and a cause a call site can spell freely cannot be aggregated or alerted on. + */ +export type PrivateSecretProvenanceBundleFailure = + | 'selection-key-invalid' + | 'registry-incomplete' + | 'bundle-oversized' interface HeaderReader { get(name: string): string | null @@ -372,33 +395,51 @@ export function createPrivateSecretProvenanceRequestMetadata( if (!registry) return undefined const keys = new Set() - let complete = selections.length <= MAX_PRIVATE_SECRET_PROVENANCE_SELECTIONS + let failure: PrivateSecretProvenanceBundleFailure | undefined const provenanceSelections: PrivateSecretProvenanceBundleV1['selections'] = [] - if (complete) { - for (const selection of selections) { - if (!selection.key || keys.has(selection.key)) { - complete = false - break - } - keys.add(selection.key) - const provenance = registry.exportCommittedProvenanceForInputPaths(selection.inputPaths) + for (const selection of selections) { + if (!selection.key || keys.has(selection.key)) { + failure = 'selection-key-invalid' + break + } + keys.add(selection.key) + } + if (!failure) { + const exported = registry.exportCommittedProvenanceForInputPathGroups( + selections.map((selection) => selection.inputPaths) + ) + for (const [index, provenance] of exported.entries()) { if (!provenance.complete) { - complete = false + failure = 'registry-incomplete' break } - provenanceSelections.push({ key: selection.key, provenance }) + provenanceSelections.push({ key: selections[index].key, provenance }) } } const bundle: PrivateSecretProvenanceBundleV1 = { version: 1, - complete, - selections: complete ? provenanceSelections : [], + complete: !failure, + selections: failure ? [] : provenanceSelections, } - if (Buffer.byteLength(JSON.stringify(bundle), 'utf8') > MAX_PRIVATE_SECRET_PROVENANCE_BYTES) { + if (Buffer.byteLength(JSON.stringify(bundle), 'utf8') > PROVENANCE_MAX_SERIALIZED_BYTES) { + failure ??= 'bundle-oversized' bundle.complete = false bundle.selections = [] } + if (failure) { + /** + * Error, not warn, for the reason the originating-fault reasons use it: every row of this + * write lands `unknown` in a durable sidecar, a durable surface stays open on the strength of + * this line trending to zero, and error is the only level surviving every default the logger + * falls back to. + */ + logger.error('Private secret provenance bundle is incomplete', { + failure, + selectionCount: selections.length, + ...(registry.getIncompletenessDiagnostics() ?? {}), + }) + } return { provenance: bundle, headerName: PRIVATE_SECRET_PROVENANCE_HEADER, @@ -443,7 +484,6 @@ export function isPrivateSecretProvenanceBundleV1( bundle.version !== 1 || typeof bundle.complete !== 'boolean' || !Array.isArray(bundle.selections) || - bundle.selections.length > MAX_PRIVATE_SECRET_PROVENANCE_SELECTIONS || (!bundle.complete && bundle.selections.length > 0) ) { return false @@ -462,7 +502,7 @@ export function isPrivateSecretProvenanceBundleV1( } keys.add(record.key) } - return Buffer.byteLength(JSON.stringify(value), 'utf8') <= MAX_PRIVATE_SECRET_PROVENANCE_BYTES + return Buffer.byteLength(JSON.stringify(value), 'utf8') <= PROVENANCE_MAX_SERIALIZED_BYTES } /** diff --git a/apps/sim/lib/execution/provenance-limits.ts b/apps/sim/lib/execution/provenance-limits.ts new file mode 100644 index 00000000000..278be992afa --- /dev/null +++ b/apps/sim/lib/execution/provenance-limits.ts @@ -0,0 +1,35 @@ +/** + * The only bounds secret provenance is allowed to have. + * + * Provenance describes which configured secrets a value carries. Its information content is + * therefore the size of the workspace's secret catalog — not the size of the thing described. A + * bound on the envelope is legitimate; a bound on rows, cells, files, chunks, or attachments is + * not, because exceeding it says nothing about the data and everything about our encoding. + * + * That distinction was not academic. The same two numbers were copied into seven modules under + * fourteen names, and several of the copies had drifted into counting inputs rather than output: a + * 25-column table insert lost provenance for every row past 400, a knowledge response gave up past + * 100 rows, and a Function block that exported 21 files threw. Each looked local and defensible. + * + * So: bound the serialized envelope, and bound the distinct secrets it can name. Nothing else. A + * new limit on how much work a provenance path will do is the signal that the path needs to fold + * incrementally or page, not that it needs a number. + */ + +/** + * Ceiling on one serialized provenance envelope. + * + * Matches the platform's other single-payload ceilings (`LARGE_VALUE_THRESHOLD_BYTES`, the Redis + * single-write bound) so a provenance envelope is never the first thing to fail on a payload the + * rest of the system would carry. + */ +export const PROVENANCE_MAX_SERIALIZED_BYTES = 8 * 1024 * 1024 + +/** + * Ceiling on the distinct secrets one envelope can name. + * + * Counts entries after they are folded by encrypted value, so it scales with a workspace's secret + * catalog rather than with how many records mention those secrets. A page of a thousand rows + * sharing eleven secrets carries eleven entries, not eleven thousand. + */ +export const PROVENANCE_MAX_ENTRIES = 10_000 diff --git a/apps/sim/lib/knowledge/secret-provenance.ts b/apps/sim/lib/knowledge/secret-provenance.ts index 8f9da22d64e..7ef6a8522b0 100644 --- a/apps/sim/lib/knowledge/secret-provenance.ts +++ b/apps/sim/lib/knowledge/secret-provenance.ts @@ -431,8 +431,6 @@ export async function loadKnowledgeDocumentDurableSecretProvenance(documentId: s } } -const MAX_KNOWLEDGE_RESPONSE_PROVENANCE_ROWS = 100 - /** * Imports provenance for one bounded, exact persisted response snapshot. The supplied values are * compared with a fresh joined row before import, so a concurrent write cannot pair stale response @@ -454,14 +452,6 @@ export async function importKnowledgePersistedResponseSecretProvenance(options: }): Promise { const documents = options.documents ?? [] const chunks = options.chunks ?? [] - if ( - documents.length > MAX_KNOWLEDGE_RESPONSE_PROVENANCE_ROWS || - chunks.length > MAX_KNOWLEDGE_RESPONSE_PROVENANCE_ROWS - ) { - options.registry.markIncomplete('knowledge-response-capacity-exceeded') - return false - } - const documentIds = [...new Set(documents.map((item) => item.id))] const chunkIds = [...new Set(chunks.map((item) => item.id))] const [documentRows, chunkRows] = await Promise.all([ diff --git a/apps/sim/lib/table/rows/secret-provenance.test.ts b/apps/sim/lib/table/rows/secret-provenance.test.ts index 7e460e4f12c..acb20fc811c 100644 --- a/apps/sim/lib/table/rows/secret-provenance.test.ts +++ b/apps/sim/lib/table/rows/secret-provenance.test.ts @@ -217,6 +217,54 @@ describe('table row secret provenance', () => { }) }) + /** + * The response carries one entry per distinct secret, so a page of many rows sharing a few + * secrets is small. Counting the collected per-cell entries instead refused this page at 11,000 + * on its way to reporting 11 — the same rows-times-columns bound a write-side selection cap + * used to impose. + */ + it('vouches for a page whose collected entries far exceed the secrets it reports', async () => { + const secretCount = 11 + const rowCount = 1_000 + const entries = Array.from({ length: secretCount }, (_, index) => ({ + columnId: `column-${String(index).padStart(2, '0')}`, + encryptedValue: `encrypted-${String(index).padStart(2, '0')}`, + name: `SECRET_${String(index).padStart(2, '0')}`, + sourceUserId: 'user-1', + sourceWorkspaceId: 'workspace-1', + })) + queueTableRows( + userTableRows, + Array.from({ length: rowCount }, (_, index) => ({ + id: `row-${index}`, + updatedAt: ROW_UPDATED_AT, + secretProvenanceVersion: 1, + sidecarRowId: `row-${index}`, + sidecarStatus: 'exact', + sidecarEntries: entries, + sidecarIsCurrent: true, + })) + ) + + await expect( + loadTableRowSecretProvenance( + Array.from({ length: rowCount }, (_, index) => ({ + id: `row-${index}`, + updatedAt: ROW_UPDATED_AT, + })), + { userId: 'user-1', workspaceId: 'workspace-1' } + ) + ).resolves.toEqual({ + version: 1, + complete: true, + entries: entries.map((entry) => ({ + encryptedValue: entry.encryptedValue, + name: entry.name, + })), + scope: { userId: 'user-1', workspaceId: 'workspace-1' }, + }) + }) + it('fails closed for stale tracked rows once the table-row surface is enforced', async () => { mockIsEnforced.mockReturnValue(true) queueTableRows(userTableRows, [ @@ -297,6 +345,7 @@ describe('table row secret provenance', () => { cause: 'row-sidecar-not-exact', affectedCount: 1, workspaceId: 'workspace-1', + actorUserId: 'user-1', }) }) diff --git a/apps/sim/lib/table/rows/secret-provenance.ts b/apps/sim/lib/table/rows/secret-provenance.ts index 35f27878297..48e9ac0e73d 100644 --- a/apps/sim/lib/table/rows/secret-provenance.ts +++ b/apps/sim/lib/table/rows/secret-provenance.ts @@ -5,11 +5,16 @@ import { userTableRowSecretProvenance, userTableRows, } from '@sim/db/schema' +import { createLogger } from '@sim/logger' import { and, asc, eq, gt, inArray, type SQL, sql } from 'drizzle-orm' import { isDurableSecretProvenanceEnforced, reportUnrecordedDurableProvenance, } from '@/lib/execution/durable-secret-provenance-enforcement' +import { + PROVENANCE_MAX_ENTRIES, + PROVENANCE_MAX_SERIALIZED_BYTES, +} from '@/lib/execution/provenance-limits' import type { DbExecutor, DbTransaction } from '@/lib/table/planner' import type { RowData, TableRowSecretProvenanceWrite } from '@/lib/table/types' import { @@ -20,13 +25,10 @@ import { type ResolvedSecretTraceScopeV1, } from '@/executor/utils/resolved-secret-trace-registry' +const logger = createLogger('TableRowSecretProvenance') + export const TABLE_ROW_SECRET_PROVENANCE_VERSION = 1 -const MAX_PROVENANCE_ROWS = 10_000 -const MAX_PROVENANCE_COLUMNS_PER_ROW = 10_000 -const MAX_PROVENANCE_ENTRIES_PER_ROW = 10_000 -const MAX_PROVENANCE_ENTRIES_PER_RESPONSE = 10_000 -const MAX_PROVENANCE_BYTES = 8 * 1024 * 1024 const QUERY_CHUNK_SIZE = 1_000 type StoredTableRowSecretProvenanceEntry = TableRowSecretProvenanceEntry @@ -81,6 +83,41 @@ const STORED_ENTRY_KEYS = new Set([ 'sourceWorkspaceId', ]) +/** + * Why a durable table row write could not vouch for the cells it persisted. + * + * Every path that stamps a row `unknown` through this module funnels into one mutation, so this is + * the whole cause set for the durable write — a closed union for the reason the read side's is one, + * and because these are the lines a surface would be closed on the strength of reaching zero. + */ +type UnvouchedTableRowWriteCause = + | 'incoming-provenance-incomplete' + | 'merge-base-unvouchable' + | 'merge-base-unnormalizable' + | 'merge-result-unnormalizable' + +/** + * Records that a write persisted rows nobody could vouch for. + * + * Summarised per mutation rather than per row: one incomplete envelope marks every row of a batch, + * and a batch runs to a thousand rows. Error for the same reason the read side uses it — an + * unrecorded row is durable, and error is the only level surviving every default the logger falls + * back to. + */ +function reportUnvouchedTableRowWrite( + countsByCause: ReadonlyMap, + mode: 'replace' | 'merge' +): void { + for (const [cause, rowCount] of countsByCause) { + logger.error('Table row write persisted unrecorded secret provenance', { + surface: 'table-row', + cause, + mode, + rowCount, + }) + } +} + function compareStrings(left: string, right: string): number { if (left < right) return -1 if (left > right) return 1 @@ -125,7 +162,7 @@ function storedEntryKey(entry: StoredTableRowSecretProvenanceEntry): string { function normalizeStoredEntries(value: unknown): StoredTableRowSecretProvenanceEntry[] | undefined { if ( !Array.isArray(value) || - value.length > MAX_PROVENANCE_ENTRIES_PER_ROW || + value.length > PROVENANCE_MAX_ENTRIES || !value.every(isStoredEntry) ) { return undefined @@ -136,8 +173,8 @@ function normalizeStoredEntries(value: unknown): StoredTableRowSecretProvenanceE compareStrings(storedEntryKey(left), storedEntryKey(right)) ) if ( - entries.length > MAX_PROVENANCE_ENTRIES_PER_ROW || - serializedBytes(entries) > MAX_PROVENANCE_BYTES + entries.length > PROVENANCE_MAX_ENTRIES || + serializedBytes(entries) > PROVENANCE_MAX_SERIALIZED_BYTES ) { return undefined } @@ -153,7 +190,6 @@ function toStoredEntries(provenance: TableRowSecretProvenanceWrite): { const touchedColumns = new Set(columnEntries.map(([columnId]) => columnId)) if ( !provenance.complete || - columnEntries.length > MAX_PROVENANCE_COLUMNS_PER_ROW || columnEntries.some( ([columnId, columnProvenance]) => !columnId || @@ -343,12 +379,16 @@ export async function mutateTableRowsWithSecretProvenance( } >() + const unvouchedRowCounts = new Map() for (const mutation of mutations) { if (mutation.provenance === undefined) continue const row = rowsById.get(mutation.rowId) const incoming = toStoredEntries(mutation.provenance) let status: 'exact' | 'unknown' = incoming.complete ? 'exact' : 'unknown' let entries = incoming.entries + let cause: UnvouchedTableRowWriteCause | undefined = incoming.complete + ? undefined + : 'incoming-provenance-incomplete' if (options.mode === 'merge' && status === 'exact') { if (row?.secretProvenanceVersion === null) { @@ -365,20 +405,27 @@ export async function mutateTableRowsWithSecretProvenance( ...incoming.entries, ]) if (merged) entries = merged - else status = 'unknown' + else { + status = 'unknown' + cause = 'merge-result-unnormalizable' + } } else { status = 'unknown' + cause = 'merge-base-unnormalizable' } } else { status = 'unknown' + cause = 'merge-base-unvouchable' } } + if (cause) unvouchedRowCounts.set(cause, (unvouchedRowCounts.get(cause) ?? 0) + 1) pendingByRowId.set(mutation.rowId, { row_id: mutation.rowId, status, entries: status === 'exact' ? entries : [], }) } + reportUnvouchedTableRowWrite(unvouchedRowCounts, options.mode) const outcome = await options.mutate() const affectedRowIds = new Set() @@ -437,6 +484,14 @@ export async function mutateTableRowsWithSecretProvenance( * Applies a trusted data transformation in bounded, keyset-paginated sets. Each page locks and * classifies the old payload, writes the matching sidecar, then restores the tracked marker only * after that sidecar exists in the same transaction. + * + * A cleared `secret_provenance_version` classifies as untracked whether or not a sidecar row + * survives beside it. The demote trigger clears the marker and advances `updated_at`, so any + * sidecar left behind is stale by construction and says nothing about the current payload — + * exactly the row {@link loadTableRowSecretProvenance} and + * {@link classifyTableRowSecretProvenanceForCopy} already read as legacy. Requiring the sidecar to + * be absent made this the one classifier that called such a row unknown, which turned an ordinary + * column operation into a bulk producer of durable unknowns. */ export async function updateTableRowsWithDerivedSecretProvenance( trx: DbTransaction, @@ -451,10 +506,7 @@ export async function updateTableRowsWithDerivedSecretProvenance( : [] if (options.transformation.mode === 'remove-columns') { if (removedColumnIds.length === 0) return 0 - if ( - removedColumnIds.length > MAX_PROVENANCE_COLUMNS_PER_ROW || - removedColumnIds.some((columnId) => columnId.length === 0) - ) { + if (removedColumnIds.some((columnId) => columnId.length === 0)) { throw new Error('Derived table row transformation contains invalid columns') } } @@ -534,7 +586,6 @@ export async function updateTableRowsWithDerivedSecretProvenance( updated.content_updated_at, CASE WHEN source.old_provenance_version IS NULL - AND source.provenance_row_id IS NULL THEN '[]'::jsonb WHEN source.old_provenance_version = ${TABLE_ROW_SECRET_PROVENANCE_VERSION} AND source.provenance_status = 'exact' @@ -546,8 +597,8 @@ export async function updateTableRowsWithDerivedSecretProvenance( THEN source.provenance_entries ELSE '[]'::jsonb END - ) <= ${MAX_PROVENANCE_ENTRIES_PER_ROW} - AND octet_length(source.provenance_entries::text) <= ${MAX_PROVENANCE_BYTES} + ) <= ${PROVENANCE_MAX_ENTRIES} + AND octet_length(source.provenance_entries::text) <= ${PROVENANCE_MAX_SERIALIZED_BYTES} AND NOT EXISTS ( SELECT 1 FROM jsonb_array_elements( @@ -702,36 +753,47 @@ export async function isTableSnapshotSafeForModelMount(options: { return (await readTableRowsVersion(options.tableId, options.workspaceId)) === options.rowsVersion } -function aggregateStoredEntries( - entries: StoredTableRowSecretProvenanceEntry[], - scope: ResolvedSecretTraceScopeV1 -): ResolvedSecretTraceProvenanceEntryV1[] | undefined { +/** + * Folds crossing entries into the distinct secrets the response actually carries. + * + * A response holds one entry per distinct `encryptedValue`, so its size is the workspace's secret + * catalog, not the page's cells. Collecting every cell's entry first and capping that count made a + * page refuse to vouch for a response it could have built — a page of 1,000 rows carrying 11 + * distinct secrets each is 11,000 collected entries but only 11 reported ones. Folding as rows + * arrive means only the reported set is ever held, and only it is bounded. + */ +function createStoredEntryAggregator(scope: ResolvedSecretTraceScopeV1) { const byEncryptedValue = new Map< string, { names: Set; hasForeignOrAnonymousSource: boolean } >() - for (const entry of entries) { - const aggregate = byEncryptedValue.get(entry.encryptedValue) ?? { - names: new Set(), - hasForeignOrAnonymousSource: false, - } - const sameScope = - entry.sourceUserId === scope.userId && entry.sourceWorkspaceId === scope.workspaceId - if (sameScope && entry.name) aggregate.names.add(entry.name) - else aggregate.hasForeignOrAnonymousSource = true - byEncryptedValue.set(entry.encryptedValue, aggregate) + return { + /** False once the distinct secrets outgrow what one response may carry. */ + add(entry: StoredTableRowSecretProvenanceEntry): boolean { + let aggregate = byEncryptedValue.get(entry.encryptedValue) + if (!aggregate) { + if (byEncryptedValue.size >= PROVENANCE_MAX_ENTRIES) return false + aggregate = { names: new Set(), hasForeignOrAnonymousSource: false } + byEncryptedValue.set(entry.encryptedValue, aggregate) + } + const sameScope = + entry.sourceUserId === scope.userId && entry.sourceWorkspaceId === scope.workspaceId + if (sameScope && entry.name) aggregate.names.add(entry.name) + else aggregate.hasForeignOrAnonymousSource = true + return true + }, + build(): ResolvedSecretTraceProvenanceEntryV1[] | undefined { + const result = [...byEncryptedValue.entries()] + .sort(([left], [right]) => compareStrings(left, right)) + .map(([encryptedValue, aggregate]) => ({ + encryptedValue, + ...(!aggregate.hasForeignOrAnonymousSource && aggregate.names.size === 1 + ? { name: [...aggregate.names][0] } + : {}), + })) + return serializedBytes(result) <= PROVENANCE_MAX_SERIALIZED_BYTES ? result : undefined + }, } - if (byEncryptedValue.size > MAX_PROVENANCE_ENTRIES_PER_RESPONSE) return undefined - - const result = [...byEncryptedValue.entries()] - .sort(([left], [right]) => compareStrings(left, right)) - .map(([encryptedValue, aggregate]) => ({ - encryptedValue, - ...(!aggregate.hasForeignOrAnonymousSource && aggregate.names.size === 1 - ? { name: [...aggregate.names][0] } - : {}), - })) - return serializedBytes(result) <= MAX_PROVENANCE_BYTES ? result : undefined } /** @@ -744,9 +806,6 @@ export async function loadTableRowSecretProvenance( rows: TableRowCrossing[], scope: ResolvedSecretTraceScopeV1 ): Promise { - if (rows.length > MAX_PROVENANCE_ROWS) { - return { version: 1, complete: false, entries: [], scope } - } if (rows.length === 0) { return { version: 1, complete: true, entries: [], scope } } @@ -760,9 +819,6 @@ export async function loadTableRowSecretProvenance( const selectedColumnIds = row.selectedValues ? new Set(Object.keys(row.selectedValues)) : undefined - if (selectedColumnIds && selectedColumnIds.size > MAX_PROVENANCE_COLUMNS_PER_ROW) { - return { version: 1, complete: false, entries: [], scope } - } if (!existing) { crossingById.set(row.id, { ...row, selectedColumnIds }) continue @@ -776,7 +832,7 @@ export async function loadTableRowSecretProvenance( const rowIds = [...crossingById.keys()] const currentRows = await selectRowsWithSidecars(db, rowIds) const currentById = new Map(currentRows.map((row) => [row.id, row])) - const storedEntries: StoredTableRowSecretProvenanceEntry[] = [] + const aggregator = createStoredEntryAggregator(scope) let unrecordedRowCount = 0 for (const rowId of rowIds) { @@ -804,13 +860,9 @@ export async function loadTableRowSecretProvenance( } const parsed = normalizeStoredEntries(current.sidecarEntries) if (!parsed) return { version: 1, complete: false, entries: [], scope } - storedEntries.push( - ...parsed.filter( - (entry) => !crossing.selectedColumnIds || crossing.selectedColumnIds.has(entry.columnId) - ) - ) - if (storedEntries.length > MAX_PROVENANCE_ENTRIES_PER_RESPONSE) { - return { version: 1, complete: false, entries: [], scope } + for (const entry of parsed) { + if (crossing.selectedColumnIds && !crossing.selectedColumnIds.has(entry.columnId)) continue + if (!aggregator.add(entry)) return { version: 1, complete: false, entries: [], scope } } } @@ -820,10 +872,11 @@ export async function loadTableRowSecretProvenance( cause: 'row-sidecar-not-exact', affectedCount: unrecordedRowCount, ...(scope.workspaceId ? { workspaceId: scope.workspaceId } : {}), + actorUserId: scope.userId, }) } - const entries = aggregateStoredEntries(storedEntries, scope) + const entries = aggregator.build() if (!entries) return { version: 1, complete: false, entries: [], scope } const provenance: ResolvedSecretTraceProvenanceV1 = { version: 1, diff --git a/apps/sim/lib/uploads/contexts/workspace/workspace-file-secret-provenance.ts b/apps/sim/lib/uploads/contexts/workspace/workspace-file-secret-provenance.ts index dd13b94524e..f1034b82614 100644 --- a/apps/sim/lib/uploads/contexts/workspace/workspace-file-secret-provenance.ts +++ b/apps/sim/lib/uploads/contexts/workspace/workspace-file-secret-provenance.ts @@ -12,20 +12,22 @@ import { importDurableSecretProvenance, isPrivateSecretProvenanceScopeCompatible, } from '@/lib/execution/durable-secret-provenance' +import { + PROVENANCE_MAX_ENTRIES, + PROVENANCE_MAX_SERIALIZED_BYTES, +} from '@/lib/execution/provenance-limits' import type { ResolvedSecretTraceProvenanceV1, ResolvedSecretTraceRegistry, } from '@/executor/utils/resolved-secret-trace-registry' -const MAX_WORKSPACE_FILE_SECRET_PROVENANCE_ENTRIES = 10_000 -const MAX_WORKSPACE_FILE_SECRET_PROVENANCE_BYTES = 8 * 1024 * 1024 +/** Ids per statement. Bounds the query, never how many files a caller may classify. */ +const FILE_PROVENANCE_QUERY_CHUNK_SIZE = 1_000 const ANONYMOUS_WORKSPACE_FILE_SECRET_STORAGE_NAME = 'MOUNTED_FILE_SECRET' const LEGACY_ANONYMOUS_WORKSPACE_FILE_SECRET_STORAGE_NAME = ':SIM_INTERNAL_ANONYMOUS_SECRET_PROVENANCE_V1:' export const MODEL_UNSAFE_WORKSPACE_FILE_ERROR_MESSAGE = 'File cannot be sent to a model because its secret provenance is unavailable' -const MAX_MODEL_ATTACHMENT_PROVENANCE_LOOKUPS = 1_000 -const MAX_FUNCTION_EXPORT_PROVENANCE_FILES = 20 export type WorkspaceFileSecretProvenance = | { status: 'exact'; entries: readonly WorkspaceFileSecretProvenanceEntry[] } @@ -140,7 +142,7 @@ function storedEntryLogicalByteSize(entry: StoredWorkspaceFileSecretProvenanceEn function normalizeExactEntries( entries: readonly WorkspaceFileSecretProvenanceEntry[] ): WorkspaceFileSecretProvenanceEntry[] { - if (entries.length > MAX_WORKSPACE_FILE_SECRET_PROVENANCE_ENTRIES) { + if (entries.length > PROVENANCE_MAX_ENTRIES) { throw new Error('Workspace file secret provenance exceeds its entry limit') } @@ -157,7 +159,7 @@ function normalizeExactEntries( const key = `${entry.sourceUserId}\u0000${entry.sourceWorkspaceId ?? ''}\u0000${entry.name ?? ''}\u0000${entry.encryptedValue}` if (normalized.has(key)) continue bytes += exactEntryByteSize(entry) - if (bytes > MAX_WORKSPACE_FILE_SECRET_PROVENANCE_BYTES) { + if (bytes > PROVENANCE_MAX_SERIALIZED_BYTES) { throw new Error('Workspace file secret provenance exceeds its size limit') } normalized.set(key, { @@ -283,7 +285,7 @@ export async function createWorkspaceFileSecretProvenanceFromRegistry( ...(sourceScope.workspaceId ? { sourceWorkspaceId: sourceScope.workspaceId } : {}), }) } - if (entries.length + derivedRepresentations.size > MAX_WORKSPACE_FILE_SECRET_PROVENANCE_ENTRIES) { + if (entries.length + derivedRepresentations.size > PROVENANCE_MAX_ENTRIES) { return { safe: false } } try { @@ -313,7 +315,7 @@ export async function createWorkspaceFileSecretProvenanceFromRegistry( } function isValidStoredEntries(value: unknown): value is StoredWorkspaceFileSecretProvenanceEntry[] { - if (!Array.isArray(value) || value.length > MAX_WORKSPACE_FILE_SECRET_PROVENANCE_ENTRIES) { + if (!Array.isArray(value) || value.length > PROVENANCE_MAX_ENTRIES) { return false } let bytes = 0 @@ -340,7 +342,7 @@ function isValidStoredEntries(value: unknown): value is StoredWorkspaceFileSecre return false } bytes += storedEntryLogicalByteSize(entry as StoredWorkspaceFileSecretProvenanceEntry) - if (bytes > MAX_WORKSPACE_FILE_SECRET_PROVENANCE_BYTES) return false + if (bytes > PROVENANCE_MAX_SERIALIZED_BYTES) return false } return true } @@ -560,29 +562,42 @@ export async function markWorkspaceFileSecretProvenanceUnknown( ): Promise { const uniqueIds = [...new Set(fileIds.filter((fileId) => fileId.length > 0))] if (uniqueIds.length === 0) return - if (uniqueIds.length > MAX_FUNCTION_EXPORT_PROVENANCE_FILES) { - throw new Error('Too many Function export files to classify') - } await db.transaction(async (tx) => { - const rows = await tx - .select({ id: workspaceFiles.id, contentUpdatedAt: workspaceFiles.contentUpdatedAt }) - .from(workspaceFiles) - .where( - and( - inArray(workspaceFiles.id, uniqueIds), - eq(workspaceFiles.workspaceId, workspaceId), - eq(workspaceFiles.context, 'workspace'), - isNull(workspaceFiles.deletedAt) + /** + * Paged rather than capped. This marks files unknown, so refusing to run because there were + * too many would leave every one of them carrying whatever provenance it had before — the + * failure this function exists to prevent, reached by declining to prevent it. The former + * twenty-file limit threw, which made an ordinary Function block that wrote twenty-one files + * fail outright. The page size bounds the statement, never the caller. + */ + let bound = 0 + for (let index = 0; index < uniqueIds.length; index += FILE_PROVENANCE_QUERY_CHUNK_SIZE) { + const chunk = uniqueIds.slice(index, index + FILE_PROVENANCE_QUERY_CHUNK_SIZE) + const rows = await tx + .select({ id: workspaceFiles.id, contentUpdatedAt: workspaceFiles.contentUpdatedAt }) + .from(workspaceFiles) + .where( + and( + inArray(workspaceFiles.id, chunk), + eq(workspaceFiles.workspaceId, workspaceId), + eq(workspaceFiles.context, 'workspace'), + isNull(workspaceFiles.deletedAt) + ) ) - ) - if (rows.length !== uniqueIds.length) { - throw new Error('Function export file provenance could not be bound to canonical records') + bound += rows.length + for (const row of rows) { + await replaceWorkspaceFileSecretProvenanceInTx(tx, row.id, row.contentUpdatedAt, { + status: 'unknown', + }) + } } - for (const row of rows) { - await replaceWorkspaceFileSecretProvenanceInTx(tx, row.id, row.contentUpdatedAt, { - status: 'unknown', - }) + /** + * Still fatal, and deliberately so: an id that matched no canonical row means the caller named + * a file this workspace does not own, which is not a capacity problem. + */ + if (bound !== uniqueIds.length) { + throw new Error('Function export file provenance could not be bound to canonical records') } }) } @@ -768,7 +783,7 @@ export async function filterModelSafeWorkspaceFileAttachments< options: { workspaceId?: string } = {} ): Promise { if (attachments.length === 0) return [] - if (attachments.length > MAX_MODEL_ATTACHMENT_PROVENANCE_LOOKUPS) { + if (attachments.length > PROVENANCE_MAX_ENTRIES) { throw new Error('Too many file attachments to verify secret provenance') } @@ -856,7 +871,7 @@ export async function areModelSafeWorkspaceFileKeys( ): Promise { const uniqueKeys = [...new Set(keys.filter((key) => key.length > 0))] if (uniqueKeys.length === 0) return true - if (uniqueKeys.length > MAX_MODEL_ATTACHMENT_PROVENANCE_LOOKUPS) { + if (uniqueKeys.length > PROVENANCE_MAX_ENTRIES) { throw new Error('Too many file keys to verify secret provenance') } diff --git a/packages/audit/src/types.ts b/packages/audit/src/types.ts index 91ed561d91b..cf14865d1a5 100644 --- a/packages/audit/src/types.ts +++ b/packages/audit/src/types.ts @@ -68,6 +68,17 @@ export const AuditAction = { ENVIRONMENT_UPDATED: 'environment.updated', ENVIRONMENT_DELETED: 'environment.deleted', + /** + * Secret provenance + * + * Recorded when a run proceeded on data whose secret provenance nobody wrote down. The value + * crossing into a model could not be checked against the workspace's secrets, so a secret it + * carries would not have been redacted. Deliberately an audit entry rather than a refusal: + * blocking the run would strand the workspace on data it can no longer read, so the risk is + * surfaced to the people who own the secrets instead. + */ + SECRET_PROVENANCE_UNRECORDED: 'secret_provenance.unrecorded', + // Files FILE_UPLOADED: 'file.uploaded', FILE_UPDATED: 'file.updated', @@ -245,6 +256,8 @@ export const AuditResourceType = { PASSWORD: 'password', PERMISSION_GROUP: 'permission_group', SCHEDULE: 'schedule', + /** Not a stored resource: the workspace's secrets, as the thing put at risk. */ + SECRET_PROVENANCE: 'secret_provenance', SKILL: 'skill', SUBSCRIPTION: 'subscription', TABLE: 'table', diff --git a/packages/db/script-migrations-paused-billing-attribution.test.ts b/packages/db/script-migrations-paused-billing-attribution.test.ts index bcf4f844b71..edc4ec2a0a9 100644 --- a/packages/db/script-migrations-paused-billing-attribution.test.ts +++ b/packages/db/script-migrations-paused-billing-attribution.test.ts @@ -441,6 +441,7 @@ describe('script migration registry', () => { '0002_backfill_paused_billing_attribution', '0003_backfill_workspace_storage_usage', '0004_backfill_fork_kb_file_ownership', + '0005_repair_unknown_table_row_provenance', ]) }) }) diff --git a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts new file mode 100644 index 00000000000..9d22ddfa87f --- /dev/null +++ b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts @@ -0,0 +1,72 @@ +import type { Sql } from 'postgres' +import type { ScriptMigration } from './types' + +export const UNKNOWN_PROVENANCE_REPAIR_BATCH_SIZE = 1000 + +/** + * Returns one page of `unknown` rows to the untracked state, and reports how many it cleared. + * + * Both halves are required and belong in one statement. Clearing the marker while a sidecar row + * survives beside it is the state a derived table transformation reads as unknown, so a split + * repair would be undone by the next column operation. + * + * `secret_provenance_version` is not a column the demote trigger watches, so this leaves + * `updated_at` alone and cannot disturb a concurrent write's sidecar binding. + */ +async function repairUnknownProvenancePage(sql: Sql, batchSize: number): Promise { + const repaired = await sql<{ id: string }[]>` + WITH page AS ( + SELECT row_id + FROM user_table_row_secret_provenance + WHERE status = 'unknown' + LIMIT ${batchSize} + ), cleared AS ( + DELETE FROM user_table_row_secret_provenance + WHERE row_id IN (SELECT row_id FROM page) + RETURNING row_id + ) + UPDATE user_table_rows + SET secret_provenance_version = NULL + WHERE id IN (SELECT row_id FROM cleared) + RETURNING id + ` + return repaired.length +} + +/** + * Clears the backlog of table rows whose secret provenance nobody recorded. + * + * A sidecar reading `unknown` asserts that nobody recorded which secrets the row's cells carry. An + * untracked row asserts exactly the same thing, and the read path already lets it through: + * `loadTableRowSecretProvenance` skips a row whose `secret_provenance_version` is NULL *before* it + * reaches the enforcement branch, so an untracked row stays readable even once the table-row + * surface is closed. The two states differ only in that one is durable. + * + * That difference is what makes the surface un-closable. Nothing heals an `unknown` row in place — + * a partial cell update keeps it unknown and only a full replace carrying complete provenance + * clears it — so every such row would fail every run that later read it, forever. This restores + * them to the state the system already tolerates, so the surface can eventually be closed against + * newly written provenance rather than against a backlog. + * + * Deliberately a relabel rather than a reconstruction. Rescanning each cell against the + * workspace's current secret catalog would recover real provenance where secrets have not rotated, + * but it is a much larger job that reports its own false negatives. The relabel claims strictly + * less than the rows did: "unrecorded", which is true of every one of them. + * + * Idempotent and resumable: a repaired row no longer has a sidecar, so it leaves the candidate set + * and a re-run after a crash resumes on what remains. Rows that become unknown after this runs are + * simply left for the writers now instrumented to report them. + */ +export const repairUnknownTableRowProvenance: ScriptMigration = { + name: '0005_repair_unknown_table_row_provenance', + async up(sql: Sql): Promise { + let repaired = 0 + for (;;) { + const page = await repairUnknownProvenancePage(sql, UNKNOWN_PROVENANCE_REPAIR_BATCH_SIZE) + if (page === 0) break + repaired += page + console.log(` repaired ${repaired} unknown table row(s)`) + } + console.log(`Unknown table row provenance repair complete: ${repaired} row(s).`) + }, +} diff --git a/packages/db/script-migrations/index.ts b/packages/db/script-migrations/index.ts index f480b5652c3..8f022c456fe 100644 --- a/packages/db/script-migrations/index.ts +++ b/packages/db/script-migrations/index.ts @@ -3,6 +3,7 @@ import { backfillTableOrderKeys } from './0001_backfill_table_order_keys' import { backfillPausedBillingAttribution } from './0002_backfill_paused_billing_attribution' import { backfillWorkspaceStorageUsage } from './0003_backfill_workspace_storage_usage' import { backfillForkKnowledgeBaseFileOwnership } from './0004_backfill_fork_kb_file_ownership' +import { repairUnknownTableRowProvenance } from './0005_repair_unknown_table_row_provenance' import type { ScriptMigration } from './types' export type { ScriptMigration } from './types' @@ -17,6 +18,7 @@ export const scriptMigrations: readonly ScriptMigration[] = [ backfillPausedBillingAttribution, backfillWorkspaceStorageUsage, backfillForkKnowledgeBaseFileOwnership, + repairUnknownTableRowProvenance, ] /** diff --git a/packages/testing/src/mocks/audit.mock.ts b/packages/testing/src/mocks/audit.mock.ts index fa33f8f708e..f93ff2d45aa 100644 --- a/packages/testing/src/mocks/audit.mock.ts +++ b/packages/testing/src/mocks/audit.mock.ts @@ -75,6 +75,7 @@ export const auditMock = { DOCUMENT_DELETED: 'document.deleted', ENVIRONMENT_UPDATED: 'environment.updated', ENVIRONMENT_DELETED: 'environment.deleted', + SECRET_PROVENANCE_UNRECORDED: 'secret_provenance.unrecorded', FILE_UPLOADED: 'file.uploaded', FILE_UPDATED: 'file.updated', FILE_DELETED: 'file.deleted', @@ -215,6 +216,7 @@ export const auditMock = { PASSWORD: 'password', PERMISSION_GROUP: 'permission_group', SCHEDULE: 'schedule', + SECRET_PROVENANCE: 'secret_provenance', SKILL: 'skill', SUBSCRIPTION: 'subscription', TABLE: 'table', From 06f5e763a0366c58ef8e7179527d9e309ae6b115 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 19 Aug 2026 15:28:41 -0700 Subject: [PATCH 2/3] fix(provenance): close a repair race and stop memory double-reporting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repair matched sidecars by the id its page captured, so a provenance-aware write committing between the snapshot and the delete had its fresh exact sidecar removed and its marker cleared behind it — a secret-bearing row left reading as legacy. The delete now re-checks status, which under READ COMMITTED re-evaluates against the writer's committed row so it no longer matches. Walk the candidate set by keyset over row_id. A page whose rows were all repaired concurrently clears nothing, and terminating on "cleared nothing" ended the walk with the rest of the backlog untouched. Memory reported unrecorded provenance twice, and counted records even when the surface was enforced — auditing a fail-open read that had actually failed closed. --- .../app/api/memory/secret-provenance.test.ts | 75 ++++++++++++++++- apps/sim/app/api/memory/secret-provenance.ts | 17 +++- .../execution/durable-secret-provenance.ts | 15 +++- ...epair_unknown_table_row_provenance.test.ts | 83 +++++++++++++++++++ ...005_repair_unknown_table_row_provenance.ts | 76 +++++++++++++---- 5 files changed, 244 insertions(+), 22 deletions(-) create mode 100644 packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts diff --git a/apps/sim/app/api/memory/secret-provenance.test.ts b/apps/sim/app/api/memory/secret-provenance.test.ts index 6c204427d06..866c9474af6 100644 --- a/apps/sim/app/api/memory/secret-provenance.test.ts +++ b/apps/sim/app/api/memory/secret-provenance.test.ts @@ -5,7 +5,19 @@ import { memorySecretProvenance } from '@sim/db/schema' import { dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing' import { NextRequest } from 'next/server' -import { beforeEach, describe, expect, it } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockIsEnforced, mockReport } = vi.hoisted(() => ({ + mockIsEnforced: vi.fn(() => false), + mockReport: vi.fn(), +})) + +vi.mock('@/lib/execution/durable-secret-provenance-enforcement', () => ({ + DURABLE_SECRET_PROVENANCE_SURFACES: ['memory', 'table-row', 'knowledge'], + isDurableSecretProvenanceEnforced: mockIsEnforced, + reportUnrecordedDurableProvenance: mockReport, +})) + import { AuthType } from '@/lib/auth/hybrid' import { PRIVATE_SECRET_PROVENANCE_BUNDLE_V1, @@ -47,6 +59,8 @@ function privateMemoryWrite( describe('memory write secret provenance', () => { beforeEach(() => { resetDbChainMock() + mockReport.mockClear() + mockIsEnforced.mockReturnValue(false) }) it('classifies a headerless external write as exact-empty', () => { const request = new NextRequest('http://localhost/api/memory', { method: 'POST' }) @@ -267,4 +281,63 @@ describe('memory write secret provenance', () => { [RESOLVED_SECRET_PROVENANCE_FIELD]: { version: 1, complete: true, entries: [] }, }) }) + /** + * One entry for the read, not one per record: the per-record import knows no workspace, so its + * report can only ever be a log line, and passing the workspace down instead would write + * thousands of audit rows for a single event. + */ + it('reports one aggregated entry for a read that proceeded unvouched', async () => { + const request = new NextRequest('http://localhost/api/memory', { + headers: { + [PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1, + }, + }) + + await createMemoryResponse({ + request, + authType: AuthType.INTERNAL_JWT, + userId: 'user-1', + workspaceId: 'workspace-1', + body: { success: true }, + memories: [ + { id: 'memory-1', data: 'value', secretProvenanceVersion: 1 }, + { id: 'memory-2', data: 'value', secretProvenanceVersion: 1 }, + ], + }) + + expect(mockReport).toHaveBeenCalledTimes(1) + expect(mockReport).toHaveBeenCalledWith( + expect.objectContaining({ + surface: 'memory', + cause: 'durable-provenance-unknown', + affectedCount: 2, + workspaceId: 'workspace-1', + }) + ) + }) + + /** + * Under enforcement the import fails the registry closed rather than proceeding, so there is no + * fail-open read to record. Counting those records anyway would audit something that never + * happened, in the one trail whose whole purpose is to say a read went ahead unvouched. + */ + it('records nothing when the surface is enforced and the read fails closed', async () => { + mockIsEnforced.mockReturnValue(true) + const request = new NextRequest('http://localhost/api/memory', { + headers: { + [PRIVATE_TOOL_METADATA_REQUEST_HEADER]: RESOLVED_SECRET_PROVENANCE_METADATA_V1, + }, + }) + + await createMemoryResponse({ + request, + authType: AuthType.INTERNAL_JWT, + userId: 'user-1', + workspaceId: 'workspace-1', + body: { success: true }, + memories: [{ id: 'memory-1', data: 'value', secretProvenanceVersion: 1 }], + }) + + expect(mockReport).not.toHaveBeenCalled() + }) }) diff --git a/apps/sim/app/api/memory/secret-provenance.ts b/apps/sim/app/api/memory/secret-provenance.ts index fa97a3dce20..b2439c7be95 100644 --- a/apps/sim/app/api/memory/secret-provenance.ts +++ b/apps/sim/app/api/memory/secret-provenance.ts @@ -9,7 +9,10 @@ import { EXACT_EMPTY_DURABLE_SECRET_PROVENANCE, importDurableSecretProvenance, } from '@/lib/execution/durable-secret-provenance' -import { reportUnrecordedDurableProvenance } from '@/lib/execution/durable-secret-provenance-enforcement' +import { + isDurableSecretProvenanceEnforced, + reportUnrecordedDurableProvenance, +} from '@/lib/execution/durable-secret-provenance-enforcement' import { inspectPrivateSecretProvenanceRequest, isPrivateSecretProvenanceBundleV1, @@ -121,6 +124,12 @@ export async function createMemoryResponse(options: { matching.push(memory) memoriesById.set(memory.id, matching) } + /** + * Counted only while the surface is open. Under enforcement the import fails the registry closed + * instead of proceeding, so counting those records would audit a fail-open read that never + * happened — and this entry exists precisely to say a read went ahead unvouched. + */ + const memoryEnforced = isDurableSecretProvenanceEnforced('memory') let unrecordedMemoryCount = 0 for (let index = 0; index < ids.length; index += PRIVATE_MEMORY_QUERY_CHUNK_SIZE) { const pageIds = ids.slice(index, index + PRIVATE_MEMORY_QUERY_CHUNK_SIZE) @@ -139,8 +148,10 @@ export async function createMemoryResponse(options: { status: sidecar?.status ?? null, entries: sidecar?.entries, }) - if (provenance.status === 'unknown') unrecordedMemoryCount += 1 - await importDurableSecretProvenance(registry, provenance, record.data, 'memory') + if (provenance.status === 'unknown' && !memoryEnforced) unrecordedMemoryCount += 1 + await importDurableSecretProvenance(registry, provenance, record.data, 'memory', { + reportUnrecorded: false, + }) } } } diff --git a/apps/sim/lib/execution/durable-secret-provenance.ts b/apps/sim/lib/execution/durable-secret-provenance.ts index 228a6d7ebf1..d6918c764b9 100644 --- a/apps/sim/lib/execution/durable-secret-provenance.ts +++ b/apps/sim/lib/execution/durable-secret-provenance.ts @@ -209,11 +209,22 @@ export async function importDurableSecretProvenance( registry: ResolvedSecretTraceRegistry, provenance: DurableSecretProvenance, value?: unknown, - surface?: DurableSecretProvenanceSurface + surface?: DurableSecretProvenanceSurface, + /** + * Set by a caller that reports the whole read itself. + * + * This function sees one record and knows no workspace, so its report can only ever be a log + * line, one per record. A caller reading a page can say the same thing once, with the workspace + * and the count — which is the entry that reaches the people who own the secrets. Both reporting + * would double-count the same event at two different granularities. + */ + options: { reportUnrecorded?: boolean } = {} ): Promise { if (provenance.status === 'unknown') { if (surface && !isDurableSecretProvenanceEnforced(surface)) { - reportUnrecordedDurableProvenance({ surface, cause: 'durable-provenance-unknown' }) + if (options.reportUnrecorded !== false) { + reportUnrecordedDurableProvenance({ surface, cause: 'durable-provenance-unknown' }) + } return true } registry.markIncomplete('durable-provenance-unknown') diff --git a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts new file mode 100644 index 00000000000..c2e14374655 --- /dev/null +++ b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts @@ -0,0 +1,83 @@ +/** + * @vitest-environment node + */ +import type { Sql } from 'postgres' +import { describe, expect, it, vi } from 'vitest' +import { repairUnknownTableRowProvenance } from './0005_repair_unknown_table_row_provenance' + +interface PageResult { + candidates: number + repaired: number + lastRowId: string | null +} + +function normalizeSql(value: string): string { + return value.replace(/\s+/g, ' ').trim() +} + +/** Replays a scripted sequence of pages and records the `afterRowId` each pass asked for. */ +function createSqlHarness(pages: PageResult[]): { + sql: Sql + cursors: unknown[] + statements: string[] +} { + const cursors: unknown[] = [] + const statements: string[] = [] + let call = 0 + const query = vi.fn((strings: TemplateStringsArray, ...values: unknown[]) => { + statements.push(normalizeSql(strings.join('?'))) + /** `afterRowId` is interpolated before the page size, so it is the first bound value. */ + cursors.push(values[0]) + const page = pages[call] ?? { candidates: 0, repaired: 0, lastRowId: null } + call += 1 + return Promise.resolve([page]) + }) + return { sql: query as unknown as Sql, cursors, statements } +} + +describe('0005 repair unknown table row provenance', () => { + /** + * A provenance-aware write commits its exact sidecar between this statement's snapshot and its + * delete. Matching on the captured id alone would drop that fresh sidecar and clear the marker + * behind it, leaving a secret-bearing row reading as legacy — provenance destroyed by the repair + * meant to make provenance safe. The re-check is what makes the writer's row stop matching. + */ + it('only deletes sidecars still reading unknown', async () => { + const { sql, statements } = createSqlHarness([ + { candidates: 1, repaired: 1, lastRowId: 'row-1' }, + { candidates: 0, repaired: 0, lastRowId: null }, + ]) + + await repairUnknownTableRowProvenance.up(sql) + + expect(statements[0]).toContain('DELETE FROM user_table_row_secret_provenance') + expect(statements[0]).toContain("AND status = 'unknown'") + }) + + /** + * A page whose rows were all repaired by a concurrent writer clears nothing. Stopping there would + * have ended the walk and left the rest of the backlog untouched. + */ + it('keeps walking past a page a concurrent writer already repaired', async () => { + const { sql, cursors } = createSqlHarness([ + { candidates: 2, repaired: 0, lastRowId: 'row-2' }, + { candidates: 1, repaired: 1, lastRowId: 'row-9' }, + { candidates: 0, repaired: 0, lastRowId: null }, + ]) + + await repairUnknownTableRowProvenance.up(sql) + + expect(cursors).toEqual(['', 'row-2', 'row-9']) + }) + + it('stops on the first page with no candidates left', async () => { + const { sql, cursors } = createSqlHarness([ + { candidates: 1, repaired: 1, lastRowId: 'row-1' }, + { candidates: 0, repaired: 0, lastRowId: null }, + ]) + + await repairUnknownTableRowProvenance.up(sql) + + expect(cursors).toEqual(['', 'row-1']) + }) +}) diff --git a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts index 9d22ddfa87f..27d922e5c04 100644 --- a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts +++ b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts @@ -3,34 +3,62 @@ import type { ScriptMigration } from './types' export const UNKNOWN_PROVENANCE_REPAIR_BATCH_SIZE = 1000 +interface RepairPage { + /** Rows still reading `unknown` when the page was selected; zero means the walk is done. */ + candidates: number + /** Rows actually returned to untracked. Lower than `candidates` when a writer got there first. */ + repaired: number + /** Highest `row_id` in the page, so the next pass resumes past it. */ + lastRowId: string | null +} + /** - * Returns one page of `unknown` rows to the untracked state, and reports how many it cleared. + * Returns one page of `unknown` rows to the untracked state. + * + * Both halves belong in one statement. Clearing the marker while a sidecar row survives beside it + * is the state a derived table transformation reads as unknown, so a split repair would be undone + * by the next column operation. * - * Both halves are required and belong in one statement. Clearing the marker while a sidecar row - * survives beside it is the state a derived table transformation reads as unknown, so a split - * repair would be undone by the next column operation. + * The delete re-checks `status` rather than trusting the id the page captured. A provenance-aware + * write commits its exact sidecar and its version marker together, and can land between this + * statement's snapshot and its delete; matching on `row_id` alone would drop that fresh exact + * sidecar and clear the marker behind it, leaving a genuinely secret-bearing row reading as + * legacy — provenance destroyed by the repair meant to make provenance safe. Under READ COMMITTED + * the delete re-evaluates its condition against the updated row, so the writer's row no longer + * matches and is left alone; whichever of the two commits second sees the other's result. * * `secret_provenance_version` is not a column the demote trigger watches, so this leaves * `updated_at` alone and cannot disturb a concurrent write's sidecar binding. */ -async function repairUnknownProvenancePage(sql: Sql, batchSize: number): Promise { - const repaired = await sql<{ id: string }[]>` +async function repairUnknownProvenancePage( + sql: Sql, + batchSize: number, + afterRowId: string +): Promise { + const [page] = await sql<[RepairPage]>` WITH page AS ( SELECT row_id FROM user_table_row_secret_provenance - WHERE status = 'unknown' + WHERE status = 'unknown' AND row_id > ${afterRowId} + ORDER BY row_id LIMIT ${batchSize} ), cleared AS ( DELETE FROM user_table_row_secret_provenance WHERE row_id IN (SELECT row_id FROM page) + AND status = 'unknown' RETURNING row_id + ), marked AS ( + UPDATE user_table_rows + SET secret_provenance_version = NULL + WHERE id IN (SELECT row_id FROM cleared) + RETURNING id ) - UPDATE user_table_rows - SET secret_provenance_version = NULL - WHERE id IN (SELECT row_id FROM cleared) - RETURNING id + SELECT + (SELECT count(*) FROM page)::int AS "candidates", + (SELECT count(*) FROM marked)::int AS "repaired", + (SELECT max(row_id) FROM page) AS "lastRowId" ` - return repaired.length + return page } /** @@ -56,17 +84,33 @@ async function repairUnknownProvenancePage(sql: Sql, batchSize: number): Promise * Idempotent and resumable: a repaired row no longer has a sidecar, so it leaves the candidate set * and a re-run after a crash resumes on what remains. Rows that become unknown after this runs are * simply left for the writers now instrumented to report them. + * + * Walked by keyset over `row_id` rather than by re-selecting the head of the candidate set. A page + * whose rows were all repaired by a concurrent writer clears nothing, and terminating on "cleared + * nothing" would have ended the walk there and left the rest of the backlog untouched. Advancing + * past the page instead makes each pass finite and the whole walk terminate on the only condition + * that means finished: a page with no candidates left in it. */ export const repairUnknownTableRowProvenance: ScriptMigration = { name: '0005_repair_unknown_table_row_provenance', async up(sql: Sql): Promise { let repaired = 0 + let skipped = 0 + let afterRowId = '' for (;;) { - const page = await repairUnknownProvenancePage(sql, UNKNOWN_PROVENANCE_REPAIR_BATCH_SIZE) - if (page === 0) break - repaired += page + const page = await repairUnknownProvenancePage( + sql, + UNKNOWN_PROVENANCE_REPAIR_BATCH_SIZE, + afterRowId + ) + if (page.candidates === 0 || page.lastRowId === null) break + repaired += page.repaired + skipped += page.candidates - page.repaired + afterRowId = page.lastRowId console.log(` repaired ${repaired} unknown table row(s)`) } - console.log(`Unknown table row provenance repair complete: ${repaired} row(s).`) + console.log( + `Unknown table row provenance repair complete: ${repaired} row(s) repaired, ${skipped} left to a concurrent writer.` + ) }, } From a8e6233c3908d49d0e9097a9e1ec243f5d0bc8d0 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 19 Aug 2026 15:36:59 -0700 Subject: [PATCH 3/3] fix(provenance): take the repair's locks in the writer's order The repair deleted the sidecar and only then updated its parent row, while mutateTableRowsWithSecretProvenance locks user_table_rows up front and upserts the sidecar inside the same transaction. Opposite orders, so an overlapping write deadlocked and Postgres resolved it by aborting either the deployment or somebody's table write. Lock the parent first, in id order, matching lockTableRows. Holding that lock is also what makes the status re-check decisive rather than racy: the writer commits its sidecar and its marker under the same lock, so once it is held the write is either wholly done or has not begun. --- ...epair_unknown_table_row_provenance.test.ts | 102 +++++++++++------- ...005_repair_unknown_table_row_provenance.ts | 74 ++++++++----- 2 files changed, 108 insertions(+), 68 deletions(-) diff --git a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts index c2e14374655..0972f61a5e0 100644 --- a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts +++ b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.test.ts @@ -5,53 +5,84 @@ import type { Sql } from 'postgres' import { describe, expect, it, vi } from 'vitest' import { repairUnknownTableRowProvenance } from './0005_repair_unknown_table_row_provenance' -interface PageResult { - candidates: number - repaired: number - lastRowId: string | null -} - function normalizeSql(value: string): string { return value.replace(/\s+/g, ' ').trim() } -/** Replays a scripted sequence of pages and records the `afterRowId` each pass asked for. */ -function createSqlHarness(pages: PageResult[]): { +/** + * Replays a scripted sequence of candidate pages and records every statement in the order it was + * issued, so a test can assert on lock ordering rather than only on the final counts. + */ +function createSqlHarness(pages: string[][]): { sql: Sql - cursors: unknown[] statements: string[] + cursors: unknown[] } { - const cursors: unknown[] = [] const statements: string[] = [] - let call = 0 - const query = vi.fn((strings: TemplateStringsArray, ...values: unknown[]) => { - statements.push(normalizeSql(strings.join('?'))) - /** `afterRowId` is interpolated before the page size, so it is the first bound value. */ - cursors.push(values[0]) - const page = pages[call] ?? { candidates: 0, repaired: 0, lastRowId: null } - call += 1 - return Promise.resolve([page]) - }) - return { sql: query as unknown as Sql, cursors, statements } + const cursors: unknown[] = [] + let page = 0 + + const run = (strings: TemplateStringsArray, ...values: unknown[]) => { + const text = normalizeSql(strings.join('?')) + statements.push(text) + + if (text.startsWith('SELECT row_id AS "rowId"')) { + cursors.push(values[0]) + const rows = (pages[page] ?? []).map((rowId) => ({ rowId })) + page += 1 + return Promise.resolve(rows) + } + if (text.startsWith('DELETE FROM user_table_row_secret_provenance')) { + const ids = (values[0] as string[]) ?? [] + return Promise.resolve(ids.map((rowId) => ({ rowId }))) + } + if (text.startsWith('UPDATE user_table_rows')) { + const ids = (values[0] as string[]) ?? [] + return Promise.resolve(ids.map((id) => ({ id }))) + } + return Promise.resolve([]) + } + + const sql = run as unknown as Sql + sql.begin = vi.fn(async (callback) => (callback as (tx: Sql) => unknown)(sql)) as Sql['begin'] + return { sql, statements, cursors } } describe('0005 repair unknown table row provenance', () => { /** - * A provenance-aware write commits its exact sidecar between this statement's snapshot and its - * delete. Matching on the captured id alone would drop that fresh sidecar and clear the marker - * behind it, leaving a secret-bearing row reading as legacy — provenance destroyed by the repair - * meant to make provenance safe. The re-check is what makes the writer's row stop matching. + * `mutateTableRowsWithSecretProvenance` locks `user_table_rows` up front and upserts the sidecar + * inside the same transaction. Touching the sidecar first is the opposite order, and an + * overlapping write would deadlock — Postgres resolving it by aborting either the deployment or + * somebody's table write. + */ + it('locks the parent row before touching the sidecar, in the order writers take them', async () => { + const { sql, statements } = createSqlHarness([['row-1', 'row-2'], []]) + + await repairUnknownTableRowProvenance.up(sql) + + const lockIndex = statements.findIndex((s) => s.includes('FOR UPDATE')) + const deleteIndex = statements.findIndex((s) => + s.startsWith('DELETE FROM user_table_row_secret_provenance') + ) + expect(lockIndex).toBeGreaterThanOrEqual(0) + expect(deleteIndex).toBeGreaterThan(lockIndex) + expect(statements[lockIndex]).toContain('ORDER BY id') + }) + + /** + * A provenance-aware write commits its exact sidecar and its marker together. Matching on the + * captured id alone would drop that fresh sidecar and clear the marker behind it, leaving a + * secret-bearing row reading as legacy. */ it('only deletes sidecars still reading unknown', async () => { - const { sql, statements } = createSqlHarness([ - { candidates: 1, repaired: 1, lastRowId: 'row-1' }, - { candidates: 0, repaired: 0, lastRowId: null }, - ]) + const { sql, statements } = createSqlHarness([['row-1'], []]) await repairUnknownTableRowProvenance.up(sql) - expect(statements[0]).toContain('DELETE FROM user_table_row_secret_provenance') - expect(statements[0]).toContain("AND status = 'unknown'") + const deleteStatement = statements.find((s) => + s.startsWith('DELETE FROM user_table_row_secret_provenance') + ) + expect(deleteStatement).toContain("AND status = 'unknown'") }) /** @@ -59,11 +90,7 @@ describe('0005 repair unknown table row provenance', () => { * have ended the walk and left the rest of the backlog untouched. */ it('keeps walking past a page a concurrent writer already repaired', async () => { - const { sql, cursors } = createSqlHarness([ - { candidates: 2, repaired: 0, lastRowId: 'row-2' }, - { candidates: 1, repaired: 1, lastRowId: 'row-9' }, - { candidates: 0, repaired: 0, lastRowId: null }, - ]) + const { sql, cursors } = createSqlHarness([['row-1', 'row-2'], ['row-9'], []]) await repairUnknownTableRowProvenance.up(sql) @@ -71,10 +98,7 @@ describe('0005 repair unknown table row provenance', () => { }) it('stops on the first page with no candidates left', async () => { - const { sql, cursors } = createSqlHarness([ - { candidates: 1, repaired: 1, lastRowId: 'row-1' }, - { candidates: 0, repaired: 0, lastRowId: null }, - ]) + const { sql, cursors } = createSqlHarness([['row-1'], []]) await repairUnknownTableRowProvenance.up(sql) diff --git a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts index 27d922e5c04..b04cd4d7053 100644 --- a/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts +++ b/packages/db/script-migrations/0005_repair_unknown_table_row_provenance.ts @@ -15,17 +15,19 @@ interface RepairPage { /** * Returns one page of `unknown` rows to the untracked state. * - * Both halves belong in one statement. Clearing the marker while a sidecar row survives beside it - * is the state a derived table transformation reads as unknown, so a split repair would be undone - * by the next column operation. + * Takes the parent row lock before touching the sidecar, in `id` order, because that is the order + * `mutateTableRowsWithSecretProvenance` takes them: it locks `user_table_rows` up front, then + * upserts the sidecar inside the same transaction. Deleting the sidecar first and only then + * updating the parent is the opposite order, so an overlapping write would deadlock — and Postgres + * would resolve it by aborting either the deployment or somebody's table write. Sharing the + * writer's order means the two serialize instead. * - * The delete re-checks `status` rather than trusting the id the page captured. A provenance-aware - * write commits its exact sidecar and its version marker together, and can land between this - * statement's snapshot and its delete; matching on `row_id` alone would drop that fresh exact - * sidecar and clear the marker behind it, leaving a genuinely secret-bearing row reading as - * legacy — provenance destroyed by the repair meant to make provenance safe. Under READ COMMITTED - * the delete re-evaluates its condition against the updated row, so the writer's row no longer - * matches and is left alone; whichever of the two commits second sees the other's result. + * Holding the parent lock is also what makes the status re-check below decisive rather than + * racy: a provenance-aware write commits its exact sidecar and its version marker together under + * that same lock, so once it is held the write is either wholly done or has not begun. Matching on + * the id alone would drop a freshly exact sidecar and clear the marker behind it, leaving a + * genuinely secret-bearing row reading as legacy — provenance destroyed by the repair meant to make + * provenance safe. * * `secret_provenance_version` is not a column the demote trigger watches, so this leaves * `updated_at` alone and cannot disturb a concurrent write's sidecar binding. @@ -35,30 +37,44 @@ async function repairUnknownProvenancePage( batchSize: number, afterRowId: string ): Promise { - const [page] = await sql<[RepairPage]>` - WITH page AS ( - SELECT row_id - FROM user_table_row_secret_provenance - WHERE status = 'unknown' AND row_id > ${afterRowId} - ORDER BY row_id - LIMIT ${batchSize} - ), cleared AS ( + const candidates = await sql<{ rowId: string }[]>` + SELECT row_id AS "rowId" + FROM user_table_row_secret_provenance + WHERE status = 'unknown' AND row_id > ${afterRowId} + ORDER BY row_id + LIMIT ${batchSize} + ` + if (candidates.length === 0) return { candidates: 0, repaired: 0, lastRowId: null } + const rowIds = candidates.map((candidate) => candidate.rowId) + + const repaired = await sql.begin(async (tx) => { + await tx` + SELECT id FROM user_table_rows + WHERE id = ANY(${rowIds}::text[]) + ORDER BY id + FOR UPDATE + ` + const cleared = await tx<{ rowId: string }[]>` DELETE FROM user_table_row_secret_provenance - WHERE row_id IN (SELECT row_id FROM page) + WHERE row_id = ANY(${rowIds}::text[]) AND status = 'unknown' - RETURNING row_id - ), marked AS ( + RETURNING row_id AS "rowId" + ` + if (cleared.length === 0) return 0 + const marked = await tx<{ id: string }[]>` UPDATE user_table_rows SET secret_provenance_version = NULL - WHERE id IN (SELECT row_id FROM cleared) + WHERE id = ANY(${cleared.map((row) => row.rowId)}::text[]) RETURNING id - ) - SELECT - (SELECT count(*) FROM page)::int AS "candidates", - (SELECT count(*) FROM marked)::int AS "repaired", - (SELECT max(row_id) FROM page) AS "lastRowId" - ` - return page + ` + return marked.length + }) + + return { + candidates: rowIds.length, + repaired: repaired as number, + lastRowId: rowIds[rowIds.length - 1], + } } /**