diff --git a/.changeset/write-response-internal-fields-property-guard.md b/.changeset/write-response-internal-fields-property-guard.md new file mode 100644 index 0000000000..8145c32581 --- /dev/null +++ b/.changeset/write-response-internal-fields-property-guard.md @@ -0,0 +1,74 @@ +--- +"@objectstack/core": patch +"@objectstack/metadata-protocol": patch +"@objectstack/mcp": patch +--- + +fix(security): the MCP stdio bridge stops echoing `internal: true` columns from a write, and the write-response guarantee is guarded as a PROPERTY rather than per-class (#8497) + +**A live leak, found by widening a guard.** #7823 relocated the `internal: true` +write-response strip to the generic-data-path ingress and gated the relocation on +a tripwire that enumerates every `*Data` face on the protocol class. The card that +produced this change observed that the guard's coverage — *"every `*Data` face on +one class"* — is narrower than the property that needs holding — *"no response body +an external caller receives from a write carries an `internal: true` value"* — and +that `@objectstack/rest`'s cross-object batch (a direct `ql.update`) was the +standing proof the two are not the same set. + +Widening the guard to the property immediately found a second direct mouth that +was **not** covered, and it was leaking. `@objectstack/mcp`'s stdio bridge +(`stdio-data-bridge.ts`) is engine-only by construction — the long-lived stdio +host cannot reuse the runtime's request-shaped `callData` builder — and its +`create` arm handed `engine.insert`'s result straight back to the MCP caller. +Since #7823 the engine deliberately keeps its write results whole, so the flagged +column rode the tool response verbatim. Measured before the fix: + +``` +{"object":"vault","id":"r1","record":{"name":"row","id":"r1","vault_secret":""}} +``` + +The file's own header had listed its protocol-layer divergences as *"deliberate, +filed, not security"*. One limb of that list **was** security, and the header now +says so. + +**What changed** + +- `@objectstack/mcp` — the stdio bridge's `create` runs its response record + through the shared strip. `update` does too: that arm discards the engine's + write result and echoes the read-path row plus the caller's own patch, so no + *stored* value could reach it, but a caller who puts an `internal: true` key in + `data` would otherwise get it echoed back — their own bytes used as an oracle + for a column the flag says is never returned. Read verbs are untouched (the + engine's read-path strip is unchanged). +- `@objectstack/core` — the strip helper + (`omitInternalFieldsFromWriteResponse` / `collectInternalWriteResponseFields`) + moved here from `@objectstack/metadata-protocol`. It shipped beside the protocol + class when that class was its only caller, but the generic write mouths are not + all on it: `rest` and `mcp` both reach the engine directly and **neither depends + on `@objectstack/metadata-protocol`**, so the old home forced each new mouth to + choose between a duck-typed reach through a protocol instance and a private + restatement of a security-relevant rule. `core` is the floor all three already + depend on, and already hosts this class of shared write-path helper + (`bulk-write.ts`). No behaviour change and no API change: + `@objectstack/metadata-protocol` re-exports both names unchanged. + +**What guards it now.** Two new tripwires join the shipped one — which is **not** +replaced: its runtime prototype walk and its `leakyData` negative control are +untouched. Each is a runtime enumeration no author can dodge by adding code +without touching it, and each fails on a surface it has no disposition for: + +- `metadata-protocol` — walks the protocol class for `*Data` faces (unchanged); +- `rest` — walks `RestServer.getRoutes()` for HTTP write routes, drives the ten + data-plane ones (including `POST /batch`, the direct-`ql.update` mouth) against + a fixture whose stored rows carry a flagged sentinel, and deep-scans each + response body; +- `mcp` — walks the `McpDataBridge` faces the factory actually returns. + +Every driven case also asserts a control value is present, so a refusal or an +empty body cannot satisfy "no sentinel" by returning nothing. + +Reverse-verified in both directions, the discipline #7823's own fix used: deleting +the strip from the REST batch arm turned the REST tripwire red on exactly that +route; adding a *second* unstripped direct engine mouth turned it red again; +removing the new MCP strip turned the MCP tripwire red; every restore was proven +byte-identical with `git hash-object`. diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9b7b1f37ff..9773b97da2 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -36,6 +36,14 @@ export * from './utils/datetime.js'; // Export the shared batched-write helper (framework#2678) export * from './utils/bulk-write.js'; +// Export the shared write-response `internal: true` strip (#7823, #8497) — the +// ONE helper every write mouth that answers an external caller runs its +// records through. It lives here, on the floor all three transports share, +// because the mouths are not all on the protocol class: `rest` and `mcp` both +// reach the engine directly and neither depends on `@objectstack/metadata- +// protocol` (which re-exports these two names unchanged). +export * from './utils/internal-write-response.js'; + // Export the migration-journal runner (ADR-0119 D2, #4617) — chunk-atomic // migrations with durable recovery, plus the shared `engineCanRollBack` gate // that `@objectstack/metadata-protocol`'s atomic `batchData` also uses. diff --git a/packages/core/src/utils/internal-write-response.ts b/packages/core/src/utils/internal-write-response.ts new file mode 100644 index 0000000000..70d534319f --- /dev/null +++ b/packages/core/src/utils/internal-write-response.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7823 / #8497] The write-response half of the `internal: true` guarantee — + * THE single helper every write mouth that returns a body to an external + * caller passes its record(s) through. + * + * ## The contract + * + * A field declared `internal: true` is *never returned on the generic data + * path* (#7728). The READ half lives in the engine (`omitInternalFields` runs + * on every find/findOne result). The WRITE-RESPONSE half lives HERE. + * + * ## Why the ingress and not the engine (the measured history, #7823) + * + * The first shape stripped `internal` fields inside the engine's insert and + * by-id-update paths. That conflated two different guarantees: + * + * - "never returned on the generic data path" — the flag's sentence, about + * what an EXTERNAL caller receives; and + * - "never returned to the engine-level caller that performed the write" — + * which no ruling ever asked for, and which is FALSE for credential mint: + * better-auth's `createWithHooks` reads the minted `sys_session` row back + * off the insert result, so the engine-side strip broke `signIn`/`signUp` + * outright (measured: `verify signIn: no token in response`). + * + * Plain removal of the engine limbs was ALSO measured wrong: the by-id-update + * strip was the sole closure of #7728's fourth surface — with it neutralised, + * `PATCH /data/sys_api_key/{id}` answered 200 with the stored 64-hex `key` + * hash in the body. Both measurements are satisfiable at exactly one boundary: + * the mouth that builds the external 201/200 body. Engine write results keep + * the stored row whole (mint works); every external write response is stripped + * through this helper (the hash never leaves); the read path is untouched. + * + * ## Why this module sits in `@objectstack/core` (#8497) + * + * It shipped inside `@objectstack/metadata-protocol`, next to the protocol + * class that was then its only caller. That placement encoded an assumption + * the surface does not honour: **the generic write mouths are not all on the + * protocol class.** Two transports reach the engine directly — + * + * - `@objectstack/rest` (`rest-server.ts`, the cross-object `POST /batch` + * update arm's direct `ql.update`), and + * - `@objectstack/mcp` (`stdio-data-bridge.ts`, whose `create` handed the + * engine's insert result straight back to the MCP client — a MEASURED leak + * of the flagged column, found by widening this guard's scope in #8497), + * + * — and neither package depends on `@objectstack/metadata-protocol`. The old + * home therefore forced each new mouth to choose between a duck-typed reach + * through a protocol instance (what `rest` does) and a private restatement of + * the rule (a third copy of a security-relevant predicate). `@objectstack/core` + * is the floor all three already depend on, and it already hosts exactly this + * class of shared write-path helper (`bulk-write.ts`, used by both + * `metadata-protocol` and `rest` so neither reimplements batching). One helper, + * reachable from every mouth, is the whole point of the flag being structural. + * + * `@objectstack/metadata-protocol` re-exports both functions unchanged, so its + * public API is byte-identical across the move. + * + * ## The residual risk, and what gates it + * + * Response-body policy at the mouth means a FUTURE write mouth that forgets + * this helper leaks silently. Three tripwires hold the property, each an + * enumeration no author can dodge by adding code without touching it: + * + * - `protocol.write-response-internal-fields.tripwire.test.ts` + * (`metadata-protocol`) walks the protocol class's prototype for `*Data` + * faces; + * - `rest-write-response-internal-fields.tripwire.test.ts` (`rest`) walks + * `RestServer.getRoutes()` for HTTP write routes; + * - `mcp-write-response-internal-fields.tripwire.test.ts` (`mcp`) walks the + * `McpDataBridge` write faces. + * + * Together they assert the PROPERTY — "no response body an external caller + * receives from a write carries an `internal: true` value" — rather than the + * shape of any one class. Adding a write mouth? Route its response records + * through this helper and register it with the tripwire that enumerates its + * surface. + * + * ## Semantics + * + * Mirrors the engine's `collectInternalReadFields` rule exactly — a field + * participates iff its declaration carries `internal === true` (strict + * boolean; truthy strings and numbers do not count, same as the engine). + * `@objectstack/core` cannot import that collector (`@objectstack/objectql` + * sits above this package), so the rule is restated here in full; + * `internal-fields.test.ts` in objectql and the tripwires above pin the same + * spelling from both sides. OMIT, not mask, for the #7728 reasons: the flag's + * columns are `required`, so a mask carries zero bits while still shipping a + * value under a field whose description promises none. + * + * Deletion is IN PLACE and idempotent: records that already lack the field + * (a re-stripped read result, a fake engine that never returned it) pass + * through unchanged, and non-record values (`null`, an affected-row count, a + * driver's boolean delete verdict) are skipped rather than judged. + */ + +/** Minimal view of an object schema this module reads — the field map only. */ +interface SchemaWithFields { + fields?: Record | undefined; +} + +/** + * Collect the names of fields declared `internal: true` on `schema`. + * + * Same verdicts as objectql's `collectInternalReadFields` (see the module + * header for why it is restated rather than imported): strict `=== true`, + * empty result for a missing/field-less schema. + */ +export function collectInternalWriteResponseFields(schema: unknown): string[] { + const fields = (schema as SchemaWithFields | null | undefined)?.fields; + if (!fields || typeof fields !== 'object') return []; + const out: string[] = []; + for (const [name, def] of Object.entries(fields)) { + if (def && def.internal === true) out.push(name); + } + return out; +} + +/** + * Drop every `internal: true` field from a write response's record(s), in + * place. THE single helper every external write mouth goes through — see the + * module header; the three tripwires enforce the "every". + * + * @param schema The registered object schema (`engine.registry.getObject(...)` + * / the protocol's own registry view / `metadataService + * .getObject(...)`). An unknown object (no schema) strips + * nothing — the write itself would have been refused upstream + * by the object-existence gate. + * @param records A single record, an array of records, or anything a write + * mouth hands back where a record could sit (`null`, a count, a + * boolean): non-objects are skipped, arrays are walked. + */ +export function omitInternalFieldsFromWriteResponse(schema: unknown, records: unknown): void { + if (!records) return; + const internalFields = collectInternalWriteResponseFields(schema); + if (internalFields.length === 0) return; + const list = Array.isArray(records) ? records : [records]; + for (const row of list) { + if (!row || typeof row !== 'object') continue; + for (const field of internalFields) delete (row as Record)[field]; + } +} diff --git a/packages/mcp/src/mcp-write-response-internal-fields.tripwire.test.ts b/packages/mcp/src/mcp-write-response-internal-fields.tripwire.test.ts new file mode 100644 index 0000000000..3c576f4f02 --- /dev/null +++ b/packages/mcp/src/mcp-write-response-internal-fields.tripwire.test.ts @@ -0,0 +1,219 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #8497 — THE TRIPWIRE, MCP HALF: no response body an MCP caller receives from +// a write carries an `internal: true` value. +// +// ## Why a second tripwire rather than a wider first one +// +// #7823 shipped `protocol.write-response-internal-fields.tripwire.test.ts`, +// which walks the protocol class's prototype for `*Data` faces. That guard is +// strong and is NOT replaced here — but its coverage is "every `*Data` face on +// one class", while the property that needs holding is "every response body an +// external caller receives from a write". Those were the same set on the day it +// was written and are not the same set by construction: this transport reaches +// `IDataEngine` directly (see `stdio-data-bridge.ts`'s header — the stdio host +// cannot reuse the runtime's request-shaped `callData` builder), so no walk of +// the protocol class can see it. +// +// ⚠️ That gap was not hypothetical when this file was written. The `create` +// arm handed `engine.insert`'s result — whole since #7823 relocated the strip +// off the engine — straight back to the caller, and a flagged column rode the +// tool response verbatim. Measured, then fixed, then pinned here. +// +// ## How the enumeration catches a NEW write face +// +// The face list is NOT hand-written. It is read off the bridge object that +// `createStdioDataBridge` actually returns, at runtime. Every face must have a +// RECIPE below; a face with no recipe FAILS the suite with instructions, so a +// future verb cannot ship unexamined. Read faces are enumerated too — with +// `writesRecords: false` — so the map stays total and a rename is noticed +// rather than silently dropping a face out of coverage. +// +// ## What each recipe proves +// +// The fixture engine mirrors the post-#7823 engine contract: WRITE results +// carry the flagged column holding SENTINEL, READ results do not. Each recipe +// drives its face and the suite deep-scans the full response JSON: +// +// - SENTINEL anywhere in the response → the mouth skipped the helper → RED +// - CONTROL missing where a record was promised → the probe went blind → RED +// +// A negative control at the bottom proves the machinery can go red: a bridge +// whose create arm skips the strip is shown to leak and to be caught. + +import { describe, it, expect, vi } from 'vitest'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts'; +import { omitInternalFieldsFromWriteResponse } from '@objectstack/core'; +import { createStdioDataBridge } from './stdio-data-bridge.js'; + +/** The value that must NEVER appear in any MCP response. */ +const SENTINEL = 'INTERNAL-SENTINEL-8497-NEVER-SERIALIZED'; +/** The value that MUST appear wherever a record was promised (falsifiability). */ +const CONTROL = 'CONTROL-VALUE-8497-RECORD-FLOWED'; + +const VAULT = { + name: 'vault', + label: 'Vault', + fields: { + id: { name: 'id', type: 'text' }, + name: { name: 'name', type: 'text' }, + vault_secret: { name: 'vault_secret', type: 'text', internal: true }, + }, + // No `apiEnabled`/`apiMethods` narrowing: the ADR-0049 exposure gate must let + // every verb through, or a recipe would be measuring a 404 instead of a body. + enable: {}, +}; + +/** + * Engine double mirroring the post-#7823 contract: + * - WRITE results (insert / update) carry the flagged column holding SENTINEL; + * - READ results (find / findOne) do NOT — the engine's read-path strip is + * unchanged by #7823 and this transport depends on it. + */ +function makeSentinelEngine(): IDataEngine { + const storedRow = (id = 'row-1') => ({ id, name: CONTROL }); + const writtenRow = (id: string, data?: Record) => ({ + id, + name: (data?.name as string) ?? CONTROL, + vault_secret: SENTINEL, + }); + return { + find: vi.fn(async () => [storedRow()]), + findOne: vi.fn(async () => storedRow()), + insert: vi.fn(async (_o: string, data: Record) => + writtenRow((data?.id as string) ?? 'new-1', data)), + update: vi.fn(async (_o: string, data: Record) => + writtenRow('row-1', data)), + delete: vi.fn(async () => ({ deleted: 1 })), + count: vi.fn(async () => 1), + aggregate: vi.fn(async () => [{ n: 1 }]), + } as unknown as IDataEngine; +} + +function makeMetadata(): IMetadataService { + return { + listObjects: vi.fn(async () => [VAULT]), + getObject: vi.fn(async () => VAULT), + } as unknown as IMetadataService; +} + +function makeBridge(engine: IDataEngine = makeSentinelEngine()) { + return createStdioDataBridge({ + engine, + metadataService: makeMetadata(), + resolvePrincipal: async () => ({ userId: 'u1' }) as unknown as ExecutionContext, + }); +} + +/** + * Every callable face on the bridge the factory actually returns — the runtime + * enumeration a future author cannot dodge by adding a verb without touching + * this file. `aggregate` is attached conditionally (graceful degradation), so + * the walk reads the built object rather than any declared type. + */ +function enumerateBridgeFaces(bridge: object): string[] { + return Object.entries(bridge) + .filter(([, v]) => typeof v === 'function') + .map(([k]) => k) + .sort(); +} + +/** + * One entry per enumerated face. `invoke` drives it; `writesRecords` demands + * CONTROL in the response (faces that promise a record back from a write). + */ +type Recipe = { + invoke: (b: any) => Promise; + /** True for faces whose response echoes a record produced by a WRITE. */ + writesRecords: boolean; +}; + +const RECIPES: Record = { + // ── read / summary faces: no engine write result to strip. Enumerated so the + // map stays total and a rename is noticed. ───────────────────────────── + listObjects: { invoke: (b) => b.listObjects(), writesRecords: false }, + describeObject: { invoke: (b) => b.describeObject('vault'), writesRecords: false }, + query: { invoke: (b) => b.query('vault', {}), writesRecords: false }, + get: { invoke: (b) => b.get('vault', 'row-1'), writesRecords: false }, + aggregate: { invoke: (b) => b.aggregate('vault', {}), writesRecords: false }, + + // ── write faces: an engine write result rides (or could ride) the response ── + create: { invoke: (b) => b.create('vault', { name: CONTROL }), writesRecords: true }, + update: { invoke: (b) => b.update('vault', 'row-1', { name: CONTROL }), writesRecords: true }, + // `remove` answers `{ object, id, success }` — no record echo by contract, + // driven anyway so a future receipt that starts carrying the row is caught. + remove: { invoke: (b) => b.remove('vault', 'row-1'), writesRecords: false }, +}; + +describe('#8497 tripwire: no MCP write response carries an `internal: true` value', () => { + it('the enumeration is real: it sees the bridge write verbs', () => { + const faces = enumerateBridgeFaces(makeBridge()); + expect(faces).toEqual(expect.arrayContaining(['create', 'update', 'remove'])); + }); + + it('every bridge face has a recipe — a NEW verb must register here', () => { + const faces = enumerateBridgeFaces(makeBridge()); + const missing = faces.filter((name) => !(name in RECIPES)); + expect( + missing, + `New McpDataBridge face(s) with no tripwire recipe: ${missing.join(', ')}. ` + + 'Every face this transport serves is a generic data mouth answering an ' + + 'external caller (#7728/#7823): if it returns an engine WRITE result, ' + + 'route the record(s) through `omitInternalFieldsFromWriteResponse` ' + + '(@objectstack/core) first, then add a recipe here so the strip is held ' + + 'by measurement rather than by review.', + ).toEqual([]); + // …and the map carries no dead entries for faces that no longer exist. + const stale = Object.keys(RECIPES).filter((name) => !faces.includes(name)); + expect(stale, `Tripwire recipes for faces that no longer exist: ${stale.join(', ')}`).toEqual([]); + }); + + for (const [name, recipe] of Object.entries(RECIPES)) { + it(`${name}: response never carries the internal sentinel${recipe.writesRecords ? ', and really returned a record' : ''}`, async () => { + const bridge = makeBridge(); + const wire = JSON.stringify((await recipe.invoke(bridge)) ?? null); + expect(wire.includes(SENTINEL), `${name} leaked an internal field: ${wire}`).toBe(false); + if (recipe.writesRecords) { + expect( + wire.includes(CONTROL), + `${name} returned no record at all — the probe is blind: ${wire}`, + ).toBe(true); + } + }); + } + + it('the caller cannot use their own patch as an oracle on an internal column', async () => { + // The one remaining way an `internal: true` KEY can reach the update echo: + // the caller put it in `data`. Answering with it back would confirm-or-deny + // a guess about a column the flag says is never returned. + const bridge = makeBridge(); + const wire = JSON.stringify(await bridge.update('vault', 'row-1', { + name: CONTROL, + vault_secret: 'caller-guess', + })); + expect(wire.includes('caller-guess')).toBe(false); + expect(wire.includes(CONTROL)).toBe(true); // still a real record echo + }); + + it('NEGATIVE CONTROL: the machinery goes red on a write mouth that skips the helper', async () => { + // Exactly the defect this file was written after: an engine-only mouth that + // echoes `engine.insert`'s (whole) result. Reintroduce it locally and prove + // BOTH halves — the sentinel scan catches it, and the shared helper closes + // it — without touching the shipped bridge. + const engine = makeSentinelEngine(); + const leaky = { + async create(object: string, data: Record) { + const written = await (engine as any).insert(object, data, {}); + return { object, id: written.id, record: { ...data, ...written } }; + }, + }; + + const leaked = await leaky.create('vault', { name: CONTROL }); + expect(JSON.stringify(leaked).includes(SENTINEL)).toBe(true); // the scan bites + + omitInternalFieldsFromWriteResponse(VAULT, (leaked as any).record); + expect(JSON.stringify(leaked).includes(SENTINEL)).toBe(false); // the helper closes it + expect(JSON.stringify(leaked).includes(CONTROL)).toBe(true); // …without eating the record + }); +}); diff --git a/packages/mcp/src/stdio-data-bridge.exposure.test.ts b/packages/mcp/src/stdio-data-bridge.exposure.test.ts index ba596d72f1..8651196d95 100644 --- a/packages/mcp/src/stdio-data-bridge.exposure.test.ts +++ b/packages/mcp/src/stdio-data-bridge.exposure.test.ts @@ -329,7 +329,19 @@ describe('#8083 the fail-open and bypass behaviours match the HTTP path', () => await expect(bridge.create('task', { title: 'x' })).resolves.toMatchObject({ object: 'task' }); expect(engine.insert).toHaveBeenCalledTimes(1); // Bypassed outright — not "read the metadata then allow". - expect(metadataService.getObject).not.toHaveBeenCalled(); + // + // [#8497] This used to assert `getObject` was never called AT ALL, which + // was a proxy for the property, not the property: since #8497 the create + // arm reads the object definition a SECOND time, after the write, to strip + // `internal: true` columns out of the response body. That read cannot + // inform a gate that already ran. So the assertion now says what it always + // meant — nothing reads metadata BEFORE the dispatch decision — and stays + // falsifiable: a gate that read the definition and then allowed would land + // its read ahead of `engine.insert` and fail this. + const gateRead = (metadataService.getObject.mock.invocationCallOrder ?? []).filter( + (order) => order < engine.insert.mock.invocationCallOrder[0], + ); + expect(gateRead, 'the exposure gate read metadata before dispatching a system-context write').toEqual([]); }); }); diff --git a/packages/mcp/src/stdio-data-bridge.ts b/packages/mcp/src/stdio-data-bridge.ts index 07fe7e5ab9..53e08e6aa6 100644 --- a/packages/mcp/src/stdio-data-bridge.ts +++ b/packages/mcp/src/stdio-data-bridge.ts @@ -54,8 +54,21 @@ * transport-neutral data seam is filed as follow-up work rather than forked * here (route-ownership rule 1: a mirrored copy of `callData` would be a second * implementation that drifts). + * + * ⚠️ [#8497] ONE limb of that divergence WAS security, and the sentence above + * used to deny it. #7823 relocated the `internal: true` WRITE-RESPONSE strip + * from the engine to the protocol ingress — deliberately, and for measured + * reasons (credential mint reads its own insert result back). The engine + * therefore returns write results whole, and an engine-only bridge that echoes + * one hands the caller a field the flag promises is never returned on the + * generic data path (#7728). Measured on the `create` arm: the flagged column + * rode the tool response verbatim. The strip is applied below, through the same + * single helper every other write mouth uses; the read verbs are unaffected + * (the engine's read path still strips, unchanged). What remains of the + * divergence above is genuinely not security. */ +import { omitInternalFieldsFromWriteResponse } from '@objectstack/core'; import { resolveEffectiveApiMethods, isApiOperationAllowed, @@ -337,6 +350,13 @@ export function createStdioDataBridge(deps: StdioDataBridgeDeps): McpDataBridge | Record | undefined; const record = { ...data, ...(written ?? {}) }; + // [#8497] `written` is the engine's WRITE result, which since #7823 keeps + // the stored row whole — flagged columns included. This is a generic data + // mouth answering an external caller, so it owns the response-body half + // of the `internal: true` guarantee (#7728) exactly as the protocol + // ingress and the REST batch arm do. Measured before the fix: a create + // returned `vault_secret` verbatim in `record`. + omitInternalFieldsFromWriteResponse(await metadataService.getObject(object), record); return { object, id: record.id, record }; }, @@ -349,7 +369,18 @@ export function createStdioDataBridge(deps: StdioDataBridgeDeps): McpDataBridge const existing = await findById(engine, object, id, context); if (!existing) throw recordNotFound(object, id); await engine.update(object, data, { where: { id }, context }); - return { object, id, record: { ...existing, ...data } }; + const record = { ...(existing as Record), ...data }; + // [#8497] The engine's update RESULT is deliberately discarded here (this + // arm echoes the read-path row plus the caller's own patch), so no STORED + // flagged value can reach this line. The strip still runs, for the one + // remaining way an `internal: true` key can appear in the body: the + // caller put it in `data`. Echoing it back would answer a read of a field + // the flag says is never returned — using the caller's own bytes as the + // oracle for whether their guess matched storage. Cheap, and it makes the + // property literally true on every verb of this bridge rather than true- + // by-argument on one. + omitInternalFieldsFromWriteResponse(await metadataService.getObject(object), record); + return { object, id, record }; }, async remove(object, id) { diff --git a/packages/metadata-protocol/src/write-response-internal-fields.ts b/packages/metadata-protocol/src/write-response-internal-fields.ts index 8e88526e45..9a92d1317d 100644 --- a/packages/metadata-protocol/src/write-response-internal-fields.ts +++ b/packages/metadata-protocol/src/write-response-internal-fields.ts @@ -1,112 +1,29 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * [#7823] The write-response half of the `internal: true` guarantee — applied - * at the generic-data-path INGRESS, by maintainer ruling (2026-08-13, A-prime). - * - * ## The contract - * - * A field declared `internal: true` is *never returned on the generic data - * path* (#7728). The READ half lives in the engine (`omitInternalFields` runs - * on every find/findOne result). The WRITE-RESPONSE half lives HERE: every - * protocol `*Data` face that hands an engine write result back to its caller - * passes the record(s) through {@link omitInternalFieldsFromWriteResponse} - * before building its response. - * - * ## Why the ingress and not the engine (the measured history) - * - * The first shape stripped `internal` fields inside the engine's insert and - * by-id-update paths. That conflated two different guarantees: - * - * - "never returned on the generic data path" — the flag's sentence, about - * what an EXTERNAL caller receives; and - * - "never returned to the engine-level caller that performed the write" — - * which no ruling ever asked for, and which is FALSE for credential mint: - * better-auth's `createWithHooks` reads the minted `sys_session` row back - * off the insert result, so the engine-side strip broke `signIn`/`signUp` - * outright (measured: `verify signIn: no token in response`). - * - * Plain removal of the engine limbs was ALSO measured wrong: the by-id-update - * strip was the sole closure of #7728's fourth surface — with it neutralised, - * `PATCH /data/sys_api_key/{id}` answered 200 with the stored 64-hex `key` - * hash in the body. Both measurements are satisfiable at exactly one boundary: - * the ingress that builds the external 201/200 bodies. Engine write results - * keep the stored row whole (mint works); every external write response is - * stripped here (the hash never leaves); the read path is untouched. - * - * ## The residual risk, and what gates it - * - * Response-body policy at the ingress means a FUTURE generic write face that - * forgets this helper leaks silently. The ruling does not accept that as a - * future problem: `protocol.write-response-internal-fields.tripwire.test.ts` - * enumerates every `*Data` method on the protocol class (by name convention, - * walking the prototype), drives each against a fixture engine whose write - * results carry a flagged sentinel, and fails on any response the sentinel - * reaches — AND fails when a `*Data` method exists that the tripwire has no - * recipe for, so a new ingress cannot ship unexamined. Adding a `*Data` face? - * Route its response records through this helper and give the tripwire a - * recipe. - * - * ## Semantics - * - * Mirrors the engine's `collectInternalReadFields` rule exactly — a field - * participates iff its declaration carries `internal === true` (strict - * boolean; truthy strings and numbers do not count, same as the engine). - * `@objectstack/metadata-protocol` cannot import that collector - * (`@objectstack/objectql` depends on this package), so the rule is restated - * here in full; `internal-fields.test.ts` in objectql and the tripwire here - * pin the same spelling from both sides. OMIT, not mask, for the #7728 - * reasons: the flag's columns are `required`, so a mask carries zero bits - * while still shipping a value under a field whose description promises none. - * - * Deletion is IN PLACE and idempotent: records that already lack the field - * (a re-stripped read result, a fake engine that never returned it) pass - * through unchanged, and non-record values (`null`, an affected-row count, a - * driver's boolean delete verdict) are skipped rather than judged. + * [#7823 / #8497] The write-response `internal: true` strip — RE-EXPORT. + * + * The implementation moved to `@objectstack/core` + * (`utils/internal-write-response.ts`) in #8497, and that module's header + * carries the full reasoning: the A-prime placement measurement, why the + * engine keeps its write results whole, and why the helper had to sit on the + * floor every transport shares rather than beside the protocol class. + * + * The short version: this package was the helper's home when the protocol + * class was its only caller, but the generic write mouths are not all on that + * class. `@objectstack/rest`'s cross-object batch and `@objectstack/mcp`'s + * stdio bridge both write through the engine directly, and neither depends on + * this package — so the old home forced every new mouth to choose between a + * duck-typed reach through a protocol instance and a private restatement of a + * security-relevant rule. + * + * This file stays so the names remain importable from + * `@objectstack/metadata-protocol` exactly as before (`index.ts` re-exports + * them, and `protocol.ts` imports the strip from here): the move is invisible + * to every existing consumer. */ -/** Minimal view of an object schema this module reads — the field map only. */ -interface SchemaWithFields { - fields?: Record | undefined; -} - -/** - * Collect the names of fields declared `internal: true` on `schema`. - * - * Same verdicts as objectql's `collectInternalReadFields` (see the module - * header for why it is restated rather than imported): strict `=== true`, - * empty result for a missing/field-less schema. - */ -export function collectInternalWriteResponseFields(schema: unknown): string[] { - const fields = (schema as SchemaWithFields | null | undefined)?.fields; - if (!fields || typeof fields !== 'object') return []; - const out: string[] = []; - for (const [name, def] of Object.entries(fields)) { - if (def && def.internal === true) out.push(name); - } - return out; -} - -/** - * Drop every `internal: true` field from a write response's record(s), in - * place. THE single helper every generic write ingress goes through — see the - * module header; the tripwire test enforces the "every". - * - * @param schema The registered object schema (`engine.registry.getObject(...)` - * / the protocol's own registry view). An unknown object (no - * schema) strips nothing — the write itself would have been - * refused upstream by the object-existence gate. - * @param records A single record, an array of records, or anything a write - * face hands back where a record could sit (`null`, a count, a - * boolean): non-objects are skipped, arrays are walked. - */ -export function omitInternalFieldsFromWriteResponse(schema: unknown, records: unknown): void { - if (!records) return; - const internalFields = collectInternalWriteResponseFields(schema); - if (internalFields.length === 0) return; - const list = Array.isArray(records) ? records : [records]; - for (const row of list) { - if (!row || typeof row !== 'object') continue; - for (const field of internalFields) delete (row as Record)[field]; - } -} +export { + collectInternalWriteResponseFields, + omitInternalFieldsFromWriteResponse, +} from '@objectstack/core'; diff --git a/packages/rest/src/rest-write-response-internal-fields.tripwire.test.ts b/packages/rest/src/rest-write-response-internal-fields.tripwire.test.ts new file mode 100644 index 0000000000..aae29ec7b0 --- /dev/null +++ b/packages/rest/src/rest-write-response-internal-fields.tripwire.test.ts @@ -0,0 +1,475 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #8497 — THE TRIPWIRE, REST HALF: no response body an HTTP caller receives +// from a write carries an `internal: true` value. +// +// ## The property, and why it needed a second guard +// +// #7823 shipped `protocol.write-response-internal-fields.tripwire.test.ts`, +// which walks the protocol class's prototype for `*Data` faces. That guard is +// strong and is NOT replaced here — its runtime enumeration and its `leakyData` +// negative control stay exactly as they are. What it cannot do is see a write +// mouth that is not ON that class, and this server owns one: the cross-object +// transactional batch (`POST /batch`) reaches `ql.update` DIRECTLY, by a +// deliberate #3835-era choice, and pushes the returned row into `results`. +// The shared strip is applied there today — nothing leaks — but a prototype +// walk over another package's class is structurally incapable of proving it, +// and equally incapable of noticing the NEXT direct mouth. +// +// So the guarded property is stated at the boundary where it is actually true: +// +// no response body an external caller receives from a write carries an +// `internal: true` value +// +// …rather than "every `*Data` face on one class". Those two were the same set +// on the day the first tripwire was written; `POST /batch` is the standing +// proof they are not the same set by construction. +// +// ## How the enumeration catches a NEW route +// +// The route list is NOT hand-written. It is read off `RestServer.getRoutes()` +// at runtime — the same enumeration `rest-route-ledger.conformance.test.ts` +// audits this package's surface with. Every write-method route must carry a +// DISPOSITION below; a write route with no disposition FAILS the suite with +// instructions, so a new write surface cannot ship unexamined. Three +// dispositions, and the distinction between them is the whole point: +// +// - `driven` — a recipe below drives the route against a fixture +// whose stored rows carry a flagged sentinel, and the +// suite deep-scans the response body. MEASURED. +// - `protocol-ingress` — the route's write goes through a protocol `*Data` +// face, so the strip is held by the #7823 tripwire. +// A reviewed claim about the code, recorded so it is +// visible and can be re-checked. +// - `no-record-echo` — the body carries no record of a user data object +// (metadata items, receipts, verdicts, per-row import +// results, sharing grants). Reviewed claim. +// +// ⚠️ Only `driven` is a measurement. The other two are declarations, and they +// are here so that adding a write route forces an explicit decision rather +// than silently widening the surface — which is exactly the failure this file +// exists to prevent. When in doubt, DRIVE it. +// +// ## What each recipe proves +// +// The driver stores a flagged column on every row holding SENTINEL — the real +// shape of `sys_api_key.key` / `sys_session.token`, a stored secret the caller +// never sent. The engine's READ path strips it (unchanged by #7823); the +// engine's WRITE results carry it whole (that is #7823's whole point), so every +// write body below is one un-run strip away from leaking. Each driven route is +// scanned: +// +// - SENTINEL anywhere in the response → a mouth skipped the helper → RED +// - CONTROL missing where a record was promised → the probe went blind → RED +// (falsifiability: an exposure refusal or a 501 would otherwise satisfy +// "no sentinel" with an empty body) +// +// Reverse-verified at authoring time, both halves, against the real code: +// deleting the strip from the `POST /batch` update arm turned this suite RED on +// exactly that route, and adding a SECOND unstripped direct `ql.insert` mouth +// to the batch create arm turned it RED again — then `git hash-object` proved +// the restored files byte-identical to their committed state. + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { omitInternalFieldsFromWriteResponse } from '@objectstack/core'; +import { RestServer } from './rest-server.js'; + +/** The value that must NEVER appear in any write response. */ +const SENTINEL = 'INTERNAL-SENTINEL-8497-NEVER-SERIALIZED'; +/** The value that MUST appear wherever a record was promised (falsifiability). */ +const CONTROL = 'CONTROL-VALUE-8497-RECORD-FLOWED'; + +/** The flagged column every stored row carries — the caller never sends it. */ +const SECRET_FIELD = 'vault_secret'; + +const VAULT = { + name: 'vault', + label: 'Vault', + fields: { + id: { name: 'id', label: 'ID', type: 'text', primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' }, + [SECRET_FIELD]: { name: SECRET_FIELD, label: 'Secret', type: 'text', internal: true }, + }, + // `clone` so the clone route is reachable; no `api`/`bulk` narrowing so the + // ADR-0049 exposure gate resolves unrestricted and every bulk route is + // reachable. A refusal here would show up as a BLIND probe (no CONTROL), not + // as a false pass. + enable: { clone: true, bulk: true }, +}; + +/** + * In-memory driver that stamps the flagged column on every stored row, the way + * a hook or the platform mints `sys_api_key.key`. Copy-on-read, `RETURNING *` + * write semantics — the same shape `rest-write-response-formula.test.ts` uses. + */ +function memoryDriver() { + const rows = new Map>>(); + const table = (o: string) => { + let t = rows.get(o); + if (!t) { t = new Map(); rows.set(o, t); } + return t; + }; + let seq = 0; + const matches = (row: Record, where: unknown) => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where as Record)) { + if (k.startsWith('$')) continue; + const want = (v && typeof v === 'object' && '$eq' in (v as Record)) + ? (v as Record).$eq + : v; + if ((row[k] ?? null) !== (want ?? null)) return false; + } + return true; + }; + const driver = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: { where?: unknown }) { + return Array.from(table(object).values()).filter((r) => matches(r, ast?.where)).map((r) => ({ ...r })); + }, + async findOne(object: string, ast: { where?: unknown }) { + for (const r of table(object).values()) if (matches(r, ast?.where)) return { ...r }; + return null; + }, + async create(object: string, data: Record) { + seq += 1; + const id = (data.id as string) ?? `r_${seq}`; + // The stored secret the caller never sent — present on every row. + const row = { ...data, id, [SECRET_FIELD]: SENTINEL }; + table(object).set(id, row); + return { ...row }; + }, + async update(object: string, id: string, data: Record) { + const t = table(object); + const cur = t.get(id); + if (!cur) return null; + const next = { ...cur, ...data, id, [SECRET_FIELD]: SENTINEL }; + t.set(id, next); + return { ...next }; + }, + async delete(object: string, id: string) { return table(object).delete(id); }, + async upsert(object: string, data: Record) { return this.create(object, data); }, + async count(object: string, ast: { where?: unknown }) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, list: Record[]) { + const out = []; + for (const r of list) out.push(await this.create(object, r)); + return out; + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return driver; +} + +function createMockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeRes() { + const res: Record = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: unknown) => { res.body = b; return res; }); + res.header = vi.fn(() => res); + res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn(); res.send = vi.fn(); + return res; +} + +/** + * A REAL engine, a REAL protocol, and the REAL registered routes. Nothing about + * the write path is hand-built here: the point is to measure what an HTTP + * caller receives, so every layer between the driver and `res.json` must be the + * shipped one. `objectQLProvider` (positional arg #8) is what makes + * `POST /batch` — the direct-`ql.update` mouth this file exists for — reachable + * rather than answering 501. + */ +async function bootRest() { + const engine = new ObjectQL(); + engine.registerDriver(memoryDriver() as never, true); + await engine.init(); + engine.registry.registerObject(VAULT as never, 'tripwire-8497'); + const protocol = new ObjectStackProtocolImplementation(engine as never); + const rest = new RestServer( + createMockServer() as never, + protocol as never, + { api: { requireAuth: false } } as never, + undefined, undefined, undefined, undefined, + (async () => engine) as never, + ); + (rest as unknown as { resolveExecCtx: () => Promise }).resolveExecCtx = + async () => ({ userId: 'u1' }); + rest.registerRoutes(); + return { rest, engine }; +} + +type Booted = Awaited>; + +async function call(booted: Booted, method: string, path: string, req: Record) { + const route = (booted.rest.getRoutes() as Array<{ + method: string; path: string; handler: (rq: unknown, rs: unknown) => Promise; + }>).find((r) => r.method === method && r.path === path); + if (!route) throw new Error(`${method} ${path} route not registered`); + const res = makeRes(); + await route.handler({ method, params: {}, query: {}, body: {}, headers: {}, ...req }, res); + return res as unknown as { statusCode: number; body: unknown }; +} + +/** Seed one stored row (secret included, by the driver) and return its id. */ +async function seed(booted: Booted, id = 'row-1') { + await (booted.engine as unknown as { + insert: (o: string, d: unknown) => Promise; + }).insert('vault', { id, name: CONTROL }); + return id; +} + +const WRITE_METHODS = ['POST', 'PUT', 'PATCH', 'DELETE']; + +type Disposition = + /** Driven below and scanned. `expectRecord`: the body promises a record. */ + | { kind: 'driven'; expectRecord: boolean; invoke: (b: Booted) => Promise<{ body: unknown }> } + /** Writes through a protocol `*Data` face — held by the #7823 tripwire. */ + | { kind: 'protocol-ingress'; why: string } + /** Body carries no record of a user data object. */ + | { kind: 'no-record-echo'; why: string }; + +const D = '/api/v1/data'; + +const DISPOSITIONS: Record = { + // ── DRIVEN: the data plane, where a user-object record rides the body ───── + [`POST ${D}/:object`]: { + kind: 'driven', expectRecord: true, + invoke: (b) => call(b, 'POST', `${D}/:object`, { + params: { object: 'vault' }, body: { name: CONTROL }, + }), + }, + [`PATCH ${D}/:object/:id`]: { + kind: 'driven', expectRecord: true, + invoke: async (b) => call(b, 'PATCH', `${D}/:object/:id`, { + params: { object: 'vault', id: await seed(b) }, body: { name: CONTROL }, + }), + }, + [`POST ${D}/:object/:id/clone`]: { + kind: 'driven', expectRecord: true, + invoke: async (b) => call(b, 'POST', `${D}/:object/:id/clone`, { + params: { object: 'vault', id: await seed(b) }, body: {}, + }), + }, + [`POST ${D}/:object/createMany`]: { + kind: 'driven', expectRecord: true, + // Body IS the records array on this route. + invoke: (b) => call(b, 'POST', `${D}/:object/createMany`, { + params: { object: 'vault' }, body: [{ name: CONTROL }, { name: 'second' }], + }), + }, + [`POST ${D}/:object/updateMany`]: { + kind: 'driven', expectRecord: true, + invoke: async (b) => call(b, 'POST', `${D}/:object/updateMany`, { + params: { object: 'vault' }, + body: { records: [{ id: await seed(b), data: { name: CONTROL } }] }, + }), + }, + [`POST ${D}/:object/batch`]: { + kind: 'driven', expectRecord: true, + invoke: (b) => call(b, 'POST', `${D}/:object/batch`, { + params: { object: 'vault' }, + body: { operation: 'create', records: [{ data: { name: CONTROL } }], options: {} }, + }), + }, + // ⭐ THE MOUTH THIS FILE EXISTS FOR: cross-object batch, whose update arm + // calls `ql.update` DIRECTLY (not through a protocol `*Data` face) and + // pushes the returned row into `results`. + 'POST /api/v1/batch': { + kind: 'driven', expectRecord: true, + invoke: async (b) => { + const id = await seed(b); + return call(b, 'POST', '/api/v1/batch', { + body: { + operations: [ + { object: 'vault', action: 'create', data: { name: CONTROL } }, + { object: 'vault', action: 'update', id, data: { name: CONTROL } }, + ], + }, + }); + }, + }, + // Receipts, driven anyway: a future receipt that starts echoing the row is + // caught the day it does. + [`DELETE ${D}/:object/:id`]: { + kind: 'driven', expectRecord: false, + invoke: async (b) => call(b, 'DELETE', `${D}/:object/:id`, { + params: { object: 'vault', id: await seed(b) }, + }), + }, + [`POST ${D}/:object/deleteMany`]: { + kind: 'driven', expectRecord: false, + invoke: async (b) => call(b, 'POST', `${D}/:object/deleteMany`, { + params: { object: 'vault' }, body: { ids: [await seed(b)] }, + }), + }, + // A READ over POST. Driven because it is the one place the engine's read-path + // strip is asserted from this boundary: if that strip ever regressed, the + // write-side result would be indistinguishable from a read-side one. + [`POST ${D}/:object/query`]: { + kind: 'driven', expectRecord: true, + invoke: async (b) => { + await seed(b); + return call(b, 'POST', `${D}/:object/query`, { params: { object: 'vault' }, body: {} }); + }, + }, + + // ── PROTOCOL INGRESS: the write runs through a `*Data` face ─────────────── + 'POST /api/v1/forms/:slug/submit': { + kind: 'protocol-ingress', + why: 'Public form submit calls `p.createData(...)` and 201s its result — the ' + + 'same ingress `POST /data/:object` uses, stripped there.', + }, + + // ── NO RECORD ECHO: no user-object record in the body ───────────────────── + // Metadata plane: bodies carry metadata ITEMS. `internal: true` is a field + // flag on a data object's field, and a metadata item is not a data record. + 'POST /api/v1/meta/_migrate-stored': { kind: 'no-record-echo', why: 'Metadata plane: migration receipt.' }, + 'PUT /api/v1/meta/:type/:name': { kind: 'no-record-echo', why: 'Metadata plane: metadata item, not a data record.' }, + 'DELETE /api/v1/meta/:type/:name': { kind: 'no-record-echo', why: 'Metadata plane: delete receipt.' }, + 'POST /api/v1/meta/:type/:name/publish': { kind: 'no-record-echo', why: 'Metadata plane: publish receipt.' }, + 'POST /api/v1/meta/:type/:name/rollback': { kind: 'no-record-echo', why: 'Metadata plane: rollback receipt.' }, + 'PUT /api/v1/meta/:type/:section/:name': { kind: 'no-record-echo', why: 'Metadata plane: compound metadata section.' }, + + 'POST /api/v1/email/send': { kind: 'no-record-echo', why: 'Send receipt (message id / status), no object row.' }, + + // Sharing: the bodies are SHARE GRANTS on the sharing surface, not rows of + // the `:object` in the path — `svc.grant(...)`'s return, 201'd verbatim. + [`POST ${D}/:object/:id/shares`]: { kind: 'no-record-echo', why: 'Sharing service grant row, not a row of `:object`.' }, + [`DELETE ${D}/:object/:id/shares/:shareId`]: { kind: 'no-record-echo', why: 'Revoke receipt.' }, + 'POST /api/v1/sharing/rules': { kind: 'no-record-echo', why: 'Sharing RULE (metadata), not a data record.' }, + 'DELETE /api/v1/sharing/rules/:idOrName': { kind: 'no-record-echo', why: 'Rule delete receipt.' }, + 'POST /api/v1/sharing/rules/:idOrName/evaluate': { kind: 'no-record-echo', why: 'Evaluation verdict (ids/counts).' }, + + 'POST /api/v1/reports': { kind: 'no-record-echo', why: 'Report definition (metadata).' }, + 'DELETE /api/v1/reports/:id': { kind: 'no-record-echo', why: 'Delete receipt.' }, + 'POST /api/v1/reports/:id/run': { kind: 'no-record-echo', why: 'Aggregated report result set, not a write response.' }, + 'POST /api/v1/reports/:id/schedule': { kind: 'no-record-echo', why: 'Schedule definition (metadata).' }, + 'DELETE /api/v1/reports/schedules/:scheduleId': { kind: 'no-record-echo', why: 'Delete receipt.' }, + + // Approvals: every arm answers the approval REQUEST/step state machine. + 'POST /api/v1/approvals/requests/:id/approve': { kind: 'no-record-echo', why: 'Approval request state, not the target row.' }, + 'POST /api/v1/approvals/requests/:id/reject': { kind: 'no-record-echo', why: 'Approval request state.' }, + 'POST /api/v1/approvals/requests/:id/recall': { kind: 'no-record-echo', why: 'Approval request state.' }, + 'POST /api/v1/approvals/requests/:id/revise': { kind: 'no-record-echo', why: 'Approval request state.' }, + 'POST /api/v1/approvals/requests/:id/resubmit': { kind: 'no-record-echo', why: 'Approval request state.' }, + 'POST /api/v1/approvals/requests/:id/reassign': { kind: 'no-record-echo', why: 'Approval request state.' }, + 'POST /api/v1/approvals/requests/:id/remind': { kind: 'no-record-echo', why: 'Reminder receipt.' }, + 'POST /api/v1/approvals/requests/:id/request-info': { kind: 'no-record-echo', why: 'Approval request state.' }, + 'POST /api/v1/approvals/requests/:id/comment': { kind: 'no-record-echo', why: 'Approval comment row.' }, + + 'POST /api/v1/analytics/dataset/query': { kind: 'no-record-echo', why: 'A READ (aggregated dataset) over POST.' }, + + 'POST /api/v1/security/suggested-bindings/:id/confirm': { kind: 'no-record-echo', why: 'Binding receipt.' }, + 'POST /api/v1/security/suggested-bindings/:id/dismiss': { kind: 'no-record-echo', why: 'Dismissal receipt.' }, + 'POST /api/v1/security/explain': { kind: 'no-record-echo', why: 'Permission explanation verdict.' }, + + // Import: per-row RECEIPTS (row number, action, id, warnings/errors) plus + // counts — measured, the written row is never spread into a result. + [`POST ${D}/:object/import`]: { kind: 'no-record-echo', why: 'Import summary: counts + per-row receipts, never the written row.' }, + [`POST ${D}/:object/import/jobs`]: { kind: 'no-record-echo', why: 'Job handle (id/status).' }, + [`POST ${D}/import/jobs/:jobId/cancel`]: { kind: 'no-record-echo', why: 'Job state receipt.' }, + [`POST ${D}/import/jobs/:jobId/undo`]: { kind: 'no-record-echo', why: 'Undo summary (counts).' }, +}; + +const keyOf = (r: { method: string; path: string }) => `${r.method} ${r.path}`; + +describe('#8497 tripwire: no REST write response carries an `internal: true` value', () => { + it('the enumeration is real: it sees the direct-engine mouth this guard exists for', async () => { + const { rest } = await bootRest(); + const writes = (rest.getRoutes() as Array<{ method: string; path: string }>) + .filter((r) => WRITE_METHODS.includes(r.method)) + .map(keyOf); + expect(writes).toEqual(expect.arrayContaining([ + 'POST /api/v1/batch', // the cross-object direct `ql.update` mouth + `POST ${D}/:object`, + `PATCH ${D}/:object/:id`, + ])); + // A surface this small would mean the boot registered almost nothing and + // every "no sentinel" below would be vacuous. + expect(writes.length).toBeGreaterThan(20); + }, 60_000); + + it('every write route has a disposition — a NEW write surface must register here', async () => { + const { rest } = await bootRest(); + const writes = (rest.getRoutes() as Array<{ method: string; path: string }>) + .filter((r) => WRITE_METHODS.includes(r.method)) + .map(keyOf); + const missing = writes.filter((k) => !(k in DISPOSITIONS)); + expect( + missing, + `New REST write route(s) with no #8497 disposition: ${missing.join(', ')}. ` + + 'Every route that answers an external caller with a write response owes ' + + 'the `internal: true` guarantee (#7728/#7823). If the body can carry a ' + + 'record of a user data object, route it through ' + + '`omitInternalFieldsFromWriteResponse` (@objectstack/core) and add a ' + + '`driven` recipe here. If it cannot, say so with a `no-record-echo` / ' + + '`protocol-ingress` disposition and a reason — an explicit decision, ' + + 'never silence.', + ).toEqual([]); + // …and no dead entries for routes that no longer exist. + const stale = Object.keys(DISPOSITIONS).filter((k) => !writes.includes(k)); + expect(stale, `Dispositions for write routes that no longer exist: ${stale.join(', ')}`).toEqual([]); + }, 60_000); + + for (const [key, disposition] of Object.entries(DISPOSITIONS)) { + if (disposition.kind !== 'driven') continue; + it(`${key}: response never carries the internal sentinel${disposition.expectRecord ? ', and really returned a record' : ''}`, async () => { + const booted = await bootRest(); + const res = await disposition.invoke(booted); + const wire = JSON.stringify(res.body ?? null); + expect(wire.includes(SENTINEL), `${key} leaked an internal field: ${wire}`).toBe(false); + if (disposition.expectRecord) { + expect( + wire.includes(CONTROL), + `${key} returned no record at all — the probe is blind (refusal? 501?): ${wire}`, + ).toBe(true); + } + }, 60_000); + } + + it('the fixture is honest: the stored row really does carry the flagged value', async () => { + // Without this, every "no sentinel" above could be true because the secret + // was never stored — the blindness that makes a guard worthless. + const booted = await bootRest(); + const id = await seed(booted); + const raw = await (booted.engine as unknown as { + update: (o: string, d: unknown, opt: unknown) => Promise>; + }).update('vault', { name: CONTROL }, { where: { id } }); + // The engine's WRITE result is whole — that is exactly what #7823 chose, + // and exactly why every mouth above must strip. + expect(raw[SECRET_FIELD]).toBe(SENTINEL); + }, 60_000); + + it('NEGATIVE CONTROL: the machinery goes red on a direct engine mouth that skips the helper', async () => { + // A future author adds a second direct `ql.*` write mouth beside the batch + // arm and forgets the strip. Reproduce that mouth here — same engine, same + // object, same response shape — and prove both halves without touching the + // shipped server. + const booted = await bootRest(); + const id = await seed(booted); + const leakyMouth = async () => { + const updated = await (booted.engine as unknown as { + update: (o: string, d: unknown, opt: unknown) => Promise>; + }).update('vault', { name: CONTROL }, { where: { id } }); + return { results: [updated] }; // straight into the response body + }; + + const leaked = await leakyMouth(); + expect(JSON.stringify(leaked).includes(SENTINEL)).toBe(true); // half 1: the scan bites + + // half 2: the shared helper is exactly what closes it. + omitInternalFieldsFromWriteResponse(VAULT, leaked.results); + expect(JSON.stringify(leaked).includes(SENTINEL)).toBe(false); + expect(JSON.stringify(leaked).includes(CONTROL)).toBe(true); // …and the record survives + }, 60_000); +});