diff --git a/.changeset/webhook-headers-secret-shape-gate.md b/.changeset/webhook-headers-secret-shape-gate.md new file mode 100644 index 0000000000..e9fe381115 --- /dev/null +++ b/.changeset/webhook-headers-secret-shape-gate.md @@ -0,0 +1,75 @@ +--- +"@objectstack/plugin-webhooks": patch +--- + +fix(webhooks): refuse a malformed `sys_webhook.headers_secret` at the write door instead of at the next delivery (#8566) + + + +`sys_webhook.headers_secret` is a `Field.secret()` whose plaintext is **not** an +opaque blob: it is a serialized header map with a required shape — a flat JSON +object of string values — and `parseStoredHeaders` is its only reader. Nothing +validated that shape on the way in. The ordinary data API accepted any string, +encrypted it like any other secret, minted a real `sys_secret` row, and left the +column holding a perfectly valid `secret:` ref that read back as the mask with +`active: true`. + +Measured on a real engine through `engine.update()` — the ordinary data API, no +privileged access — every one of these was **accepted** and is a value the +plugin can never use: `{}`, `[]`, `{"X-Count": 5}`, a nested object, and +`{X-Team: crm}` (a typo). The field is directly admin-authorable and its own +description instructs the author to type a JSON object into it, which makes a +typo the *expected* failure rather than an exotic one. + +**This is not an exposure fix and must not be read as one.** #8558/#8565 already +closed the consumer half: a webhook whose stored header map does not come back +as a flat string map parks the subscription and reports at `error`, rather than +delivering header-less with a valid signature. Nothing leaks, and nothing is +silently lost today. What this changes is **when the author finds out** — at the +write door where they typed it, instead of at the next matching record change, +an unbounded time later and in a different surface. + +**What is refused:** a `headers_secret` plaintext that does not parse back as a +flat JSON object of string values with at least one entry, with a located +ADR-0112 `VALIDATION_ERROR` / 400 naming `sys_webhook.headers_secret`, quoting +the shape the field's own description asks for, and diagnosing the specific +spelling (invalid JSON / an array / an empty object / which key's value is not a +string). ⛔ The message never echoes the rejected value — this column carries +credentials, and quoting the input would print an `Authorization: Bearer …` into +logs and error bodies, re-opening in the diagnostic exactly the exposure #7986 +moved this field onto the encrypted channel to close. It names header *keys* and +value *types* only. + +**What stays accepted, byte for byte:** every valid flat string map (as JSON +text, or as an authored object the engine serializes into the same form); `null` +to clear; an omitted key to leave the stored value unchanged; and an **echoed +read-mask**, so the ordinary Setup-form round-trip (GET a row, edit an unrelated +field, PATCH it back) is untouched. `""` is deliberately passed through to +#8559's `EmptyCredentialWriteError` rather than re-refused here — one door, one +owner, one message. + +**Where it runs, and why that is the whole mechanism:** a `beforeInsert` / +`beforeUpdate` hook on `sys_webhook`, bound by `WebhookOutboxPlugin` before its +first seeded write. It has to run *before* the engine's `encryptSecretFields` — +one step later the plaintext is gone and the column holds an opaque ref, so a +validator behind it would have nothing left to validate. The suite measures that +ordering rather than asserting it: every refusal pins that **no `sys_secret` +cipher row was minted**, which is only true if the gate ran first. + +A hook rather than checks on the plugin's own write paths +(`bootstrapDeclaredWebhooks` / `headersPatch` / the migration sweep), because a +direct `PATCH /api/v1/data/sys_webhook` goes through none of them and that is +the measured trigger. Those paths inherit the validation through the hook and +deliberately carry no second check. + +A general `secret`-channel plaintext validator — letting any `secret`-typed +field declare its own plaintext shape — is the principled generalization and is +recorded as the **promotion path**, not built here: it becomes the shape the +moment a second shaped-plaintext `secret` field exists (maintainer ruling +2026-08-13; one consumer does not justify a general capability). diff --git a/packages/plugins/plugin-webhooks/src/index.ts b/packages/plugins/plugin-webhooks/src/index.ts index b244f0e4cc..9f89b7beb2 100644 --- a/packages/plugins/plugin-webhooks/src/index.ts +++ b/packages/plugins/plugin-webhooks/src/index.ts @@ -40,6 +40,21 @@ export { WEBHOOK_SECRET_FIELD } from './webhook-secret.js'; * moved onto the same encrypted channel by the same boot sweep. */ export { WEBHOOK_HEADERS_FIELD } from './webhook-headers.js'; + +/** + * [#8566] The write door for that map's plaintext shape. Exported so a host + * that boots the pieces itself (rather than mounting {@link WebhookOutboxPlugin}) + * still gets the refusal, and so a consumer can branch on the ADR-0112 pair + * rather than on message text. + */ +export { + bindWebhookHeadersShapeGate, + unbindWebhookHeadersShapeGate, + assertWritableWebhookHeaders, + WebhookHeadersShapeError, + WEBHOOK_HEADERS_SHAPE_REFUSAL_CODE, + WEBHOOK_HEADERS_SHAPE_REFUSAL_STATUS, +} from './webhook-headers-gate.js'; export { migrateLegacyWebhookSecrets, type MigrateWebhookSecretsResult, diff --git a/packages/plugins/plugin-webhooks/src/webhook-headers-gate.test.ts b/packages/plugins/plugin-webhooks/src/webhook-headers-gate.test.ts new file mode 100644 index 0000000000..7b4b850dea --- /dev/null +++ b/packages/plugins/plugin-webhooks/src/webhook-headers-gate.test.ts @@ -0,0 +1,636 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8566 — `sys_webhook.headers_secret` must refuse a plaintext that is not a + * flat JSON object of string values, AT THE WRITE DOOR. + * + * These run against a REAL {@link ObjectQL} engine with the REAL + * {@link SysWebhook} schema and the REAL encrypted-field write path, for the + * same reason `webhook-secret-at-rest.test.ts` does: the entire claim is about + * where the gate sits relative to the engine's own `encryptSecretFields`, and + * an engine fake would answer that question by construction instead of + * measuring it. Only the DRIVER is a double (equality-only WHERE, in-memory + * maps) — an `IDataDriver` (`update(object, id, data)`, primary key SECOND), + * not an `IDataEngine`, so the engine's real dispatch contract still runs above + * it. + * + * ## What the suite has to prove, and why each half is here + * A refusal-only suite is satisfiable by refusing EVERYTHING, which would be a + * far worse bug than the one being fixed — a webhook that can no longer be + * given headers at all. So the accept side is pinned as hard as the refuse + * side: a valid flat string map still writes, still encrypts, still reads back + * as the mask, and still resolves to the authored map end to end. + * + * The ordering claim gets its own measurement rather than a comment. If the + * gate ran AFTER `encryptSecretFields`, a refused write would already have + * minted a `sys_secret` ciphertext row for the value it then rejected — an + * orphan cipher row per rejected keystroke, and proof the plaintext was gone + * before anyone looked at it. So every refusal asserts the cipher store is + * untouched, which is a fact about ORDER that no amount of message-matching + * could establish. + */ + +import { describe, expect, it } from 'vitest'; +import { ObjectQL, SECRET_MASK, SECRET_REF_PREFIX } from '@objectstack/objectql'; +import type { + ICryptoProvider, + CryptoHandle, + CryptoContext, +} from '@objectstack/spec/contracts'; +import { SysWebhook } from './sys-webhook.object.js'; +import { WEBHOOK_HEADERS_FIELD, resolveWebhookHeaders } from './webhook-headers.js'; +import { bootstrapDeclaredWebhooks } from './bootstrap-declared-webhooks.js'; +import { + WebhookHeadersShapeError, + WEBHOOK_HEADERS_SHAPE_REFUSAL_CODE, + WEBHOOK_HEADERS_SHAPE_REFUSAL_STATUS, + assertWritableWebhookHeaders, + bindWebhookHeadersShapeGate, + unbindWebhookHeadersShapeGate, +} from './webhook-headers-gate.js'; + +// Not `as const`: the engine's ExecutionContext declares these as mutable +// `string[]`, and a readonly tuple is not assignable to one. +const SYSTEM_CTX = { isSystem: true, positions: [] as string[], permissions: [] as string[] }; + +/** A header map that is valid by every definition on this seam. */ +const GOOD_HEADERS = { 'X-Team': 'crm', Authorization: 'Bearer real_token_value' }; + +// --------------------------------------------------------------------------- +// Doubles — driver (in-memory) + reversible crypto, as the sibling suite uses +// --------------------------------------------------------------------------- + +function makeStubDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + // Rows leave the driver as COPIES, exactly as a real driver's do — the read + // path mutates what it is handed (it stamps the mask), and a shared + // reference would rewrite the "at rest" bytes this suite scans. + const copy = (r: T): T => (r == null ? r : ({ ...r } as T)); + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)).map(copy); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return copy(r); + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return copy(row); + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) throw new Error(`not found: ${object}/${id}`); + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return copy(updated); + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + +/** Reversible test crypto — base64 is not encryption, only a real TRANSFORM. */ +function makeFakeCrypto(): ICryptoProvider { + let n = 0; + return { + async encrypt(plain: string, _ctx: CryptoContext): Promise { + n += 1; + return { + id: `sec_${n}`, kmsKeyId: 'local', alg: 'test-b64', version: 1, + ciphertext: Buffer.from(plain, 'utf8').toString('base64'), + }; + }, + async decrypt(handle: CryptoHandle, _ctx: CryptoContext): Promise { + return Buffer.from(handle.ciphertext, 'base64').toString('utf8'); + }, + async rotateKey(handle: CryptoHandle): Promise { + return { ...handle, version: handle.version + 1 }; + }, + digest(plain: string): string { return `d:${plain.length}`; }, + }; +} + +const sysSecretObject = { + name: 'sys_secret', label: 'Secret', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + namespace: { name: 'namespace', label: 'Namespace', type: 'text' as const }, + key: { name: 'key', label: 'Key', type: 'text' as const }, + kms_key_id: { name: 'kms_key_id', label: 'KMS', type: 'text' as const }, + alg: { name: 'alg', label: 'Alg', type: 'text' as const }, + version: { name: 'version', label: 'Version', type: 'number' as const }, + ciphertext: { name: 'ciphertext', label: 'Ciphertext', type: 'text' as const }, + created_at: { name: 'created_at', label: 'Created', type: 'datetime' as const }, + }, +}; + +/** + * A booted engine WITH the gate bound — i.e. the production wiring, where + * `WebhookOutboxPlugin.bootDeclaredWebhooks` binds it before the first write. + * `bindGate: false` reproduces a host that never mounted the plugin, which is + * how the counterfactual below shows the gate is what refuses. + */ +async function buildEngine(opts: { bindGate?: boolean } = {}) { + const engine = new ObjectQL(); + const { driver, stores } = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(sysSecretObject as any, 'test'); + engine.registry.registerObject(SysWebhook as any, 'test'); + engine.setCryptoProvider(makeFakeCrypto()); + if (opts.bindGate !== false) bindWebhookHeadersShapeGate(engine as any); + return { engine, stores, driver }; +} + +/** + * The minimum a `sys_webhook` row needs to exist — every `required: true` + * column without a default, so the engine's own record validation passes and + * what these tests measure is the gate rather than a malformed fixture. + */ +function webhookRow(overrides: Record = {}) { + return { + name: 'crm_hook', + object_name: 'contact', + url: 'https://receiver.example/hook', + method: 'post', + active: true, + definition_json: JSON.stringify({ + name: 'crm_hook', + object: 'contact', + triggers: ['create'], + url: 'https://receiver.example/hook', + method: 'POST', + }), + ...overrides, + }; +} + +/** Seed one row with no stored headers, and hand back its id. */ +async function seedRow(engine: any, overrides: Record = {}): Promise { + const created = await engine.insert('sys_webhook', webhookRow(overrides), { context: SYSTEM_CTX }); + return String(created.id); +} + +const cipherRows = (stores: Map>>) => + Array.from(stores.get('sys_secret')?.values() ?? []); + +const rowAtRest = (stores: Map>>) => + Array.from(stores.get('sys_webhook')!.values())[0] as Record; + +/** + * The five spellings measured on a real engine in the issue body — every one of + * them accepted, encrypted, and left behind a valid `secret:` ref that reads + * back as the mask with `active: true`, while being a value the plugin can + * never use. This list is the card's table, verbatim. + */ +const unusableSpellings: Array<[label: string, written: string]> = [ + ['an empty JSON object — "no headers" spelled as a value rather than as null', '{}'], + ['a JSON array instead of an object', '[]'], + ['a header whose value is a number, not a string', '{"X-Count":5}'], + ['a nested object where a flat string map is required', '{"X-Team":{"name":"crm"}}'], + ['not JSON at all — a typo in the authoring box', '{X-Team: crm}'], +]; + +// --------------------------------------------------------------------------- + +describe('sys_webhook.headers_secret shape gate — the write door (#8566)', () => { + describe('the measured table: every accepted-but-unusable shape is now refused', () => { + it.each(unusableSpellings)( + 'update through the ordinary data API refuses %s', + async (_label, written) => { + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + + // The MEASURED TRIGGER, in its engine form: a direct + // `PATCH /api/v1/data/sys_webhook`. No privileged access, no + // plugin write path — this is the road option 1 would have left + // wide open, which is why the ruling rejected option 1. + const write = engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: written }, + { where: { id }, context: SYSTEM_CTX }, + ); + + await expect(write).rejects.toBeInstanceOf(WebhookHeadersShapeError); + + // ADR-0112: a consumer branches on the PAIR, not on message + // text. Asserting only `toThrow()` would stay green against a + // driver that threw a bare Error for an unrelated reason. + await expect(write).rejects.toMatchObject({ + code: 'VALIDATION_ERROR', + status: 400, + object: 'sys_webhook', + field: WEBHOOK_HEADERS_FIELD, + }); + + // ⭐ The ORDER proof. `encryptSecretFields` mints a `sys_secret` + // row as its first side effect, so an empty cipher store is a + // measurement that the gate ran BEFORE it — the one thing this + // whole card turns on. + expect(cipherRows(stores)).toHaveLength(0); + + // …and nothing landed on the row either: no ref, no mask, no + // half-written state for the next reader to puzzle over. + expect(rowAtRest(stores)[WEBHOOK_HEADERS_FIELD] ?? null).toBeNull(); + }, + ); + + it.each(unusableSpellings)('insert refuses %s at the same door', async (_label, written) => { + const { engine, stores } = await buildEngine(); + + const write = engine.insert( + 'sys_webhook', + webhookRow({ [WEBHOOK_HEADERS_FIELD]: written }), + { context: SYSTEM_CTX }, + ); + + await expect(write).rejects.toMatchObject({ + code: WEBHOOK_HEADERS_SHAPE_REFUSAL_CODE, + status: WEBHOOK_HEADERS_SHAPE_REFUSAL_STATUS, + }); + expect(cipherRows(stores)).toHaveLength(0); + // The whole row is refused, not written-then-blanked. + expect(stores.get('sys_webhook')?.size ?? 0).toBe(0); + }); + + it('the refusal is the GATE\'s, not something the engine already did', async () => { + // The counterfactual, on the same input: with the gate unbound the + // write still sails through exactly as the card measured, ref and + // all. Without this, a refusal coming from somewhere else entirely + // would read as this gate working. + const { engine, stores } = await buildEngine({ bindGate: false }); + const id = await seedRow(engine); + + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: '{"X-Count":5}' }, + { where: { id }, context: SYSTEM_CTX }, + ); + + expect(String(rowAtRest(stores)[WEBHOOK_HEADERS_FIELD])).toMatch(/^secret:/); + expect(cipherRows(stores)).toHaveLength(1); + }); + }); + + describe('a valid flat string map still writes — the gate is not a blanket', () => { + it('writes, encrypts, masks on read, and resolves back to the authored map', async () => { + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: JSON.stringify(GOOD_HEADERS) }, + { where: { id }, context: SYSTEM_CTX }, + ); + + // At rest: an opaque ref plus exactly one cipher row. + expect(String(rowAtRest(stores)[WEBHOOK_HEADERS_FIELD])).toMatch( + new RegExp(`^${SECRET_REF_PREFIX}`), + ); + expect(cipherRows(stores)).toHaveLength(1); + + // On the generic read path: the mask, never the headers. + const [read] = await engine.find('sys_webhook', { where: { id }, context: SYSTEM_CTX }); + expect(read[WEBHOOK_HEADERS_FIELD]).toBe(SECRET_MASK); + + // …and end to end, the consumer gets back exactly what was authored. + await expect(resolveWebhookHeaders(engine, read as any, 'sys_webhook')).resolves.toEqual( + GOOD_HEADERS, + ); + }); + + it('accepts a single-entry map, and accepts it on insert too', async () => { + const { engine, stores } = await buildEngine(); + await engine.insert( + 'sys_webhook', + webhookRow({ [WEBHOOK_HEADERS_FIELD]: '{"X-Team":"crm"}' }), + { context: SYSTEM_CTX }, + ); + expect(cipherRows(stores)).toHaveLength(1); + expect(String(rowAtRest(stores)[WEBHOOK_HEADERS_FIELD])).toMatch(/^secret:/); + }); + + it('accepts an authored OBJECT, which the engine serializes into the same usable form', async () => { + // Deliberate, and worth pinning: `encryptSecretFields` JSON-stringifies + // a non-string secret value, so a caller that PATCHes a real JSON + // object lands a perfectly usable serialized map today. The gate + // normalizes the same way rather than refusing it — refusing a value + // the consumer can use would be a regression wearing a fix's clothes. + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: GOOD_HEADERS }, + { where: { id }, context: SYSTEM_CTX }, + ); + + // Encrypted for real — the object took the same road the string did. + expect(cipherRows(stores)).toHaveLength(1); + const [read] = await engine.find('sys_webhook', { where: { id }, context: SYSTEM_CTX }); + await expect(resolveWebhookHeaders(engine, read as any, 'sys_webhook')).resolves.toEqual( + GOOD_HEADERS, + ); + }); + + it('refuses an authored OBJECT whose values are not strings', async () => { + // The other side of the same normalization: an object is judged by + // what it serializes to, so `{"X-Count": 5}` is refused whether it + // arrives as JSON text or as a live object. + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + + await expect( + engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: { 'X-Count': 5 } }, + { where: { id }, context: SYSTEM_CTX }, + ), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR', status: 400 }); + expect(cipherRows(stores)).toHaveLength(0); + }); + + it('the declared-webhook seeder still materializes headers through the bound gate', async () => { + // Ruling item 2: the plugin's own write paths inherit this validation + // rather than carrying a second check. That is only safe if they pass + // — `bootstrapDeclaredWebhooks` writes `serializeHeaders()` of an + // already-filtered map, so it does, and this is the pin that keeps it + // true if either side moves. + const { engine, stores } = await buildEngine(); + await bootstrapDeclaredWebhooks(engine, { + list: (type: string) => (type === 'webhook' + ? [{ + name: 'crm_hook', object: 'contact', triggers: ['create'], + url: 'https://receiver.example/hook', method: 'POST', + headers: { 'X-Team': 'crm' }, + }] + : []), + } as any); + + const [row] = await engine.find('sys_webhook', { + where: { name: 'crm_hook' }, context: SYSTEM_CTX, + }); + expect(row).toBeDefined(); + // The seeder's write really went through the encrypted channel — it + // was not skipped, and the gate did not stand in its way. + expect(cipherRows(stores)).toHaveLength(1); + await expect(resolveWebhookHeaders(engine, row as any, 'sys_webhook')).resolves.toEqual({ + 'X-Team': 'crm', + }); + }); + }); + + describe('the four values the gate deliberately lets through', () => { + it('an echoed read-mask leaves the stored map untouched (the Setup form round-trip)', async () => { + // Ruling item 3, and the most ordinary write this object receives: a + // caller GETs the row — headers come back as the mask — edits an + // unrelated field and PATCHes the whole thing back. Refusing the mask + // would break every such round-trip, which is a far bigger outage + // than the bug being fixed. + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: JSON.stringify(GOOD_HEADERS) }, + { where: { id }, context: SYSTEM_CTX }, + ); + const refBefore = rowAtRest(stores)[WEBHOOK_HEADERS_FIELD]; + + const [read] = await engine.find('sys_webhook', { where: { id }, context: SYSTEM_CTX }); + expect(read[WEBHOOK_HEADERS_FIELD]).toBe(SECRET_MASK); + + await engine.update( + 'sys_webhook', + { ...read, active: false }, + { where: { id }, context: SYSTEM_CTX }, + ); + + // Same ref, no new cipher row: the engine dropped the echoed mask as + // "unchanged" and the gate never stood in its way. + expect(rowAtRest(stores)[WEBHOOK_HEADERS_FIELD]).toBe(refBefore); + expect(cipherRows(stores)).toHaveLength(1); + const [after] = await engine.find('sys_webhook', { where: { id }, context: SYSTEM_CTX }); + await expect(resolveWebhookHeaders(engine, after as any, 'sys_webhook')).resolves.toEqual( + GOOD_HEADERS, + ); + }); + + it('null still CLEARS the stored map', async () => { + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: JSON.stringify(GOOD_HEADERS) }, + { where: { id }, context: SYSTEM_CTX }, + ); + + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: null }, + { where: { id }, context: SYSTEM_CTX }, + ); + + expect(rowAtRest(stores)[WEBHOOK_HEADERS_FIELD] ?? null).toBeNull(); + const [read] = await engine.find('sys_webhook', { where: { id }, context: SYSTEM_CTX }); + await expect( + resolveWebhookHeaders(engine, read as any, 'sys_webhook'), + ).resolves.toBeUndefined(); + }); + + it('omitting the field leaves the stored map alone', async () => { + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: JSON.stringify(GOOD_HEADERS) }, + { where: { id }, context: SYSTEM_CTX }, + ); + const refBefore = rowAtRest(stores)[WEBHOOK_HEADERS_FIELD]; + + await engine.update('sys_webhook', { active: false }, { where: { id }, context: SYSTEM_CTX }); + + expect(rowAtRest(stores)[WEBHOOK_HEADERS_FIELD]).toBe(refBefore); + expect(cipherRows(stores)).toHaveLength(1); + }); + + it('"" is left to #8559\'s seam — one door, one owner, one message', async () => { + // ⚠️ The dispatch note said this gate "can assume it never sees \"\"". + // Measured, that is inverted: `before*` hooks run FIRST, so the gate + // does see it — and passes it through, which is what lets the + // engine's own EmptyCredentialWriteError answer with the message + // #8559 ruled on (it names `null` as the way to clear). Re-refusing + // it here would have put two different messages on one door. + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + + const write = engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: '' }, + { where: { id }, context: SYSTEM_CTX }, + ); + + // Still refused — the door is closed either way… + await expect(write).rejects.toMatchObject({ code: 'VALIDATION_ERROR', status: 400 }); + // …but by #8559's seam, not by this one. + await expect(write).rejects.not.toBeInstanceOf(WebhookHeadersShapeError); + await expect(write).rejects.toMatchObject({ name: 'EmptyCredentialWriteError' }); + expect(cipherRows(stores)).toHaveLength(0); + }); + + it('an existing secret: ref re-saved verbatim is not refused', async () => { + // The engine leaves an already-encrypted ref alone; the gate has to + // agree, or an internal caller copying a row forward would be refused + // for holding a value the engine itself considers settled. + const { engine } = await buildEngine(); + const id = await seedRow(engine); + expect(() => + assertWritableWebhookHeaders({ + [WEBHOOK_HEADERS_FIELD]: `${SECRET_REF_PREFIX}sec_1`, + }), + ).not.toThrow(); + expect(id).toBeTruthy(); + }); + }); + + describe('the refusal message', () => { + it('⛔ never echoes the rejected value, and names the offending header instead', async () => { + // This column carries credentials — an `Authorization: Bearer …` is + // the field description's own example. A message that quoted the + // input would print that token into logs and HTTP error bodies, i.e. + // re-open in the diagnostic exactly the exposure #7986 moved this + // field onto the encrypted channel to close. + const { engine } = await buildEngine(); + const id = await seedRow(engine); + const token = 'Bearer super_secret_token_do_not_log'; + + const err = await engine + .update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: JSON.stringify({ Authorization: [token] }) }, + { where: { id }, context: SYSTEM_CTX }, + ) + .catch((e: Error) => e); + + expect(err).toBeInstanceOf(WebhookHeadersShapeError); + expect((err as Error).message).not.toContain(token); + expect((err as Error).message).not.toContain('super_secret_token'); + // The header NAME is safe to say and is the useful half. + expect((err as Error).message).toContain('"Authorization"'); + }); + + it('is LOCATED and quotes the shape the field itself asks for', async () => { + const { engine } = await buildEngine(); + const id = await seedRow(engine); + + const err = await engine + .update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: '[]' }, + { where: { id }, context: SYSTEM_CTX }, + ) + .catch((e: Error) => e); + + const msg = (err as Error).message; + // Located: the object and the field, both. + expect(msg).toContain('sys_webhook.headers_secret'); + // The required shape, in the words the field's own description uses. + expect(msg).toContain('FLAT JSON object of string values'); + expect(msg).toContain('as a JSON object'); + // And the remedy, shared verbatim with the delivery-time refusal. + expect(msg).toMatch(/CLEAR the field to null/); + }); + + it('tells an author which spelling went wrong, per shape', async () => { + const cases: Array<[input: unknown, expected: RegExp]> = [ + ['{X-Team: crm}', /not valid JSON at all/], + ['[]', /JSON array/], + ['{}', /EMPTY JSON object/], + ['{"X-Count":5}', /"X-Count" \(number\)/], + ['{"X-Team":{"name":"crm"}}', /"X-Team" \(object\)/], + ['"just a quoted string"', /JSON string/], + [42, /JSON number/], + ]; + for (const [input, expected] of cases) { + expect(() => + assertWritableWebhookHeaders({ [WEBHOOK_HEADERS_FIELD]: input }), + ).toThrow(expected); + } + }); + }); + + describe('binding', () => { + it('unbinding removes the gate — the write goes through again', async () => { + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + + await expect( + engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: '[]' }, + { where: { id }, context: SYSTEM_CTX }, + ), + ).rejects.toBeInstanceOf(WebhookHeadersShapeError); + + unbindWebhookHeadersShapeGate(engine as any); + + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: '[]' }, + { where: { id }, context: SYSTEM_CTX }, + ); + expect(cipherRows(stores)).toHaveLength(1); + }); + + it('only guards sys_webhook — a neighbouring object with a secret field is untouched', async () => { + // The hook is registered with `object: 'sys_webhook'`, so it must not + // fire for anything else. `sys_secret` is written by the engine's own + // encryption path on every accepted write, which is the sharpest + // available proof that the gate is not global: were it, the valid + // write above could not have completed. + const { engine, stores } = await buildEngine(); + const id = await seedRow(engine); + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: JSON.stringify(GOOD_HEADERS) }, + { where: { id }, context: SYSTEM_CTX }, + ); + expect(cipherRows(stores)).toHaveLength(1); + }); + }); +}); diff --git a/packages/plugins/plugin-webhooks/src/webhook-headers-gate.ts b/packages/plugins/plugin-webhooks/src/webhook-headers-gate.ts new file mode 100644 index 0000000000..ead2dfc6f6 --- /dev/null +++ b/packages/plugins/plugin-webhooks/src/webhook-headers-gate.ts @@ -0,0 +1,328 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8566] The WRITE DOOR for `sys_webhook.headers_secret`'s plaintext shape. + * + * ## The defect this closes + * `headers_secret` is a `Field.secret()` whose plaintext is not an opaque blob: + * it is a serialized header map with a required shape — a flat JSON object of + * string values — and {@link parseStoredHeaders} is its only reader. Nothing + * validated that shape on the way in. The ordinary data API accepted any + * string, the engine encrypted it like any other secret, minted a real + * `sys_secret` row, and left the column holding a perfectly valid `secret:` ref + * that reads back as the mask with `active: true`. Measured on a real engine + * through `engine.update()` — no privileged access — every one of these was + * accepted and is a value the plugin can never use: `{}`, `[]`, + * `{"X-Count": 5}`, a nested object, and `{X-Team: crm}` (a typo). + * + * ## What this is NOT + * ⛔ Not an exposure fix, and it must not be graded as one. #8558/#8565 already + * closed the consumer half: a webhook whose stored header map does not come + * back as a flat string map PARKS the subscription and reports at `error` + * rather than delivering header-less with a valid signature. Nothing leaks and + * nothing is silently lost today. + * + * What remains — and all this file changes — is **when the author finds out**. + * Today: at the next matching record change, an unbounded time after the + * mistake and in a completely different surface from the one where it was made. + * With this gate: at the write door, where the author is still standing. The + * field is directly admin-authorable and its own description instructs the + * author to type a JSON object into it, which makes a typo the EXPECTED failure + * rather than an exotic one. + * + * ## Why a hook, and why THIS hook + * Maintainer ruling 2026-08-13 (option 2). Validating at the plugin's own write + * paths (`bootstrapDeclaredWebhooks` / `headersPatch` / the migration sweep) + * was rejected as insufficient: a direct `PATCH /api/v1/data/sys_webhook` never + * goes through any of them, and that is the measured trigger. An engine hook + * covers every door at once — the generic data API, the Setup UI, scripts, the + * console — and the plugin's own write paths inherit it automatically, so there + * is deliberately NO second check on them. + * + * Same rationale, and the same shape, as {@link bindWebhookProvenanceStamp} + * next door: one engine hook rather than N door-side checks. + * + * ## ⭐ Order is the whole mechanism: this MUST run before `encryptSecretFields` + * The engine encrypts a `secret` field on the way to the driver; one step later + * the plaintext is gone and the column holds an opaque ref. A validator that + * ran after it would have nothing left to validate. `beforeInsert` / + * `beforeUpdate` hooks are dispatched BEFORE that encryption on every write + * path (measured in `packages/objectql/src/engine.ts`: insert triggers its + * hooks and then encrypts; both the by-id and the multi update arms do the + * same), which is what makes this seam the right one and not merely a + * convenient one. `webhook-headers-gate.test.ts` pins the ordering against the + * real engine rather than trusting this paragraph. + * + * ## The four values this gate deliberately lets through + * Each is someone else's verdict, and duplicating any of them here would create + * a second owner for a rule that already has one: + * + * 1. **the key is absent** — "leave the stored value unchanged"; + * 2. **`null` / `undefined`** — the CLEAR spelling, which the engine honours + * and which the refusal message below points authors at; + * 3. **`""`** — governed by #8559's ruling and refused by the engine's own + * `encryptSecretFields` a few lines later, with a message that already + * names `null` as the way to clear. ⚠️ The dispatch note said this gate + * "can assume it never sees `\"\"`"; measured, that is inverted — this hook + * runs FIRST, so it does see it and must pass it through untouched for + * #8559's seam to answer. Refusing it here would duplicate that ruling and + * put two different messages on one door; + * 4. **the engine's opaque wire forms** ({@link isOpaqueSecretForm}) — the + * read mask and a `secret:` ref. The mask is the echoed-read-mask case the + * ruling calls out by name: a caller that GETs a row and PATCHes it back + * unchanged sends the mask, and the engine drops that key as "unchanged". + * Refusing it would break every round-trip through the Setup form, which is + * the single most ordinary write this object receives. A ref is the same + * story one layer down (the engine leaves an already-encrypted ref alone). + * + * ## Why the verdict is `parseStoredHeaders`, not a second shape rule + * The door refuses EXACTLY what the consumer cannot use, because it asks the + * consumer's own question: the value is normalized the way the engine will + * normalize it, then handed to {@link parseStoredHeaders} — the same function + * the enqueuer reads stored headers with. A hand-written second predicate here + * could drift from that one, and a door that refuses a value the consumer would + * have accepted (or accepts one it cannot use) is worse than no door. One rule, + * one definition. + * + * ## ⛔ The refusal never echoes the value + * This column carries credentials — an `Authorization: Bearer …` is the header + * the field's own description uses as its example. A validation message that + * quoted the rejected input would print that token into logs and HTTP error + * bodies, i.e. re-open in the diagnostic exactly the exposure #7986 moved this + * field onto the encrypted channel to close. So the diagnostic names TYPES and + * KEYS only — header names are not credentials, their values are — and never a + * value. + * + * ## Promotion path (⛔ not built here) + * A general capability on the `secret` channel — letting any `secret`-typed + * field declare a plaintext validator — is the principled generalization and is + * recorded as the shape this becomes the moment a SECOND shaped-plaintext + * `secret` field exists. It is deliberately not built for one consumer + * (maintainer ruling 2026-08-13, item 3; startup scope). Whoever hits that + * second field files against this precedent. + */ + +import { + HEADERS_REMEDY, + WEBHOOK_HEADERS_FIELD, + parseStoredHeaders, +} from './webhook-headers.js'; +import { WEBHOOK_OBJECT, isOpaqueSecretForm } from './webhook-secret.js'; + +/** + * ADR-0112 envelope for this refusal. `VALIDATION_ERROR`/400 is the standard + * catalog member for "the payload is not acceptable" — the SAME pair #8559's + * `EmptyCredentialWriteError` carries at the same door for the same class of + * verdict, so a client branching on `code`/`status` handles both malformed + * credential writes identically. A standard-catalog code needs no ledger entry. + */ +export const WEBHOOK_HEADERS_SHAPE_REFUSAL_CODE = 'VALIDATION_ERROR'; +export const WEBHOOK_HEADERS_SHAPE_REFUSAL_STATUS = 400; + +/** + * The shape the field's own description asks for, quoted in the refusal so the + * error and the authoring surface cannot drift into two different specs. + * Kept verbatim from `sys-webhook.object.ts`'s `headers_secret` description. + */ +const DECLARED_SHAPE = + 'Custom HTTP headers sent with each delivery, as a JSON object ' + + '({"Authorization": "Bearer ..."})'; + +/** + * [#8566] Refusal to persist a `headers_secret` plaintext that is not a flat + * JSON object of string values. + * + * Carries the ADR-0112 pair plus the LOCATION (`object`/`field`) as fields, so + * a consumer branches on `code`/`status` rather than on message text — the same + * discipline {@link WebhookHeadersUnresolvableError} follows on the read side + * of this seam, and `EmptyCredentialWriteError` follows on the write side. + */ +export class WebhookHeadersShapeError extends Error { + readonly code = WEBHOOK_HEADERS_SHAPE_REFUSAL_CODE; + readonly status = WEBHOOK_HEADERS_SHAPE_REFUSAL_STATUS; + readonly object: string; + readonly field: string; + + constructor(object: string, field: string, diagnosis: string) { + super( + `Custom headers refused for "${object}.${field}": ${diagnosis}. The required shape is a FLAT ` + + 'JSON object of string values, which is what the field itself asks for — its description ' + + `reads: "${DECLARED_SHAPE}". This is checked at the write door because one step later ` + + 'there is nothing left to check: the engine encrypts this value into sys_secret and every ' + + 'read path returns only the mask, so a stored value that can never be used is ' + + 'indistinguishable from one that works until the next delivery tries to send it — at ' + + 'which point the subscription parks and the report arrives an unbounded time later, in a ' + + `different surface from the one it was typed into (#7986, #8558, #8566). ${HEADERS_REMEDY}`, + ); + this.name = 'WebhookHeadersShapeError'; + this.object = object; + this.field = field; + } +} + +/** + * Describe a parsed value's SHAPE for the diagnostic — types and keys only, + * never values (see the file header's note on why this message must not echo + * the input). Header names are safe to name and are the single most useful + * thing a typo-hunting author can be told. + */ +function describeParsed(parsed: unknown): string { + if (parsed === null) return 'null'; + if (Array.isArray(parsed)) return 'a JSON array'; + if (typeof parsed !== 'object') return `a JSON ${typeof parsed}`; + + const entries = Object.entries(parsed as Record); + if (entries.length === 0) { + return 'an EMPTY JSON object, which is not the same thing as "send no custom headers"'; + } + const bad = entries.filter(([, v]) => typeof v !== 'string'); + if (bad.length > 0) { + const named = bad + .map(([k, v]) => `${JSON.stringify(k)} (${Array.isArray(v) ? 'array' : v === null ? 'null' : typeof v})`) + .join(', '); + return ( + `a JSON object, but the wire carries only strings and ${bad.length === 1 ? 'this value is' : 'these values are'} ` + + `not a string: ${named}` + ); + } + // Unreachable while `parseStoredHeaders` accepts exactly non-empty flat + // string maps; kept truthful rather than asserting a shape we did not check. + return 'a JSON object the header seam does not accept'; +} + +/** Describe the raw payload value, resolving the string/JSON layer first. */ +function describeRejected(value: unknown): string { + if (typeof value === 'string') { + let parsed: unknown; + try { + parsed = JSON.parse(value); + } catch { + return ( + 'the value is a string that is not valid JSON at all — check for unquoted keys or values ' + + '({X-Team: crm}), single quotes instead of double, or a trailing comma' + ); + } + return `the value parses as JSON but is ${describeParsed(parsed)}`; + } + return `the value is ${describeParsed(value)}`; +} + +/** + * The verdict, as a pure function of the write payload — exported so the gate + * can be reasoned about and tested without booting an engine, and so any future + * caller uses the same one rule rather than restating it. + * + * Mutates nothing and returns nothing: it either passes or throws + * {@link WebhookHeadersShapeError}. + */ +export function assertWritableWebhookHeaders( + data: Record | null | undefined, + object: string = WEBHOOK_OBJECT, + field: string = WEBHOOK_HEADERS_FIELD, +): void { + if (!data || typeof data !== 'object') return; + if (!Object.prototype.hasOwnProperty.call(data, field)) return; // omitted ⇒ unchanged + + const value = data[field]; + if (value === null || typeof value === 'undefined') return; // the CLEAR spelling + if (value === '') return; // #8559's seam owns this — see the file header + if (isOpaqueSecretForm(value)) return; // echoed read-mask, or an existing ref + + // Normalize EXACTLY as the engine is about to: a string is taken as the + // serialized map it claims to be, and anything else is JSON.stringify'd — + // which is what `encryptSecretFields` does with a non-string secret value, so + // an authored object that really is a flat string map keeps working (it + // serializes to precisely the form the consumer reads back). + let serialized: string; + if (typeof value === 'string') { + serialized = value; + } else { + try { + serialized = JSON.stringify(value) as string; + } catch { + // Circular / unserializable: the engine would store "[object Object]"- + // class garbage or throw deeper in. Refuse it here, where the message can + // say something useful. + throw new WebhookHeadersShapeError( + object, + field, + 'the value cannot be serialized to JSON at all (it contains a circular reference)', + ); + } + // `JSON.stringify` answers `undefined` for a function or a symbol. + if (typeof serialized !== 'string') { + throw new WebhookHeadersShapeError(object, field, `the value is a ${typeof value}`); + } + } + + // The consumer's own question, asked at the door (see the file header). + if (parseStoredHeaders(serialized)) return; + + throw new WebhookHeadersShapeError(object, field, describeRejected(value)); +} + +/** Minimal engine surface this binding needs — mirrors `webhook-provenance.ts`. */ +interface MinimalEngine { + registerHook(event: string, handler: (ctx: any) => any, options?: Record): void; + unregisterHooksByPackage(packageId: string): number; +} + +interface MinimalLogger { + info?: (msg: string, meta?: Record) => void; +} + +export const WEBHOOK_HEADERS_GATE_PACKAGE = 'plugin-webhooks:headers-shape-gate'; + +/** + * Priority 50 — ahead of the provenance stamp's 150 (lower runs first), so a + * refused write is refused before anything else spends work on it. The stamp + * issues a `find` against `sys_webhook` on every non-system update; there is no + * reason to pay for it on a payload that is about to be rejected. Nothing about + * correctness depends on the two hooks' relative order — only on both running + * before `encryptSecretFields`, which every `before*` hook does. + */ +const GATE_PRIORITY = 50; + +/** + * Bind the shape gate to both write events on `sys_webhook`. + * + * ## Deliberately NOT exempt for `isSystem` + * The provenance stamp next door skips system writes because it is detecting an + * ADMIN edit; this is a validity verdict on a payload, and a malformed header + * map is exactly as unusable when a seeder writes it. Ruling item 2 says the + * plugin's own write paths inherit this validation through the hook, which is + * only true if system writes are covered. They pass by construction — + * `bootstrapDeclaredWebhooks` and the migration sweep both write + * `serializeHeaders(...)` of an already `isHeaderMap`-filtered map — so + * covering them costs nothing and closes the door for a future write path that + * is less careful. + * + * Registered in CODE rather than from metadata, which also means + * `session.skipAutomations` (an import run with automations unchecked) cannot + * suppress it: the engine only skips metadata-bound entries. A validation door + * that an import could switch off would not be a door. + */ +export function bindWebhookHeadersShapeGate(engine: MinimalEngine, logger?: MinimalLogger): void { + if (typeof engine?.registerHook !== 'function') return; + + const handler = (ctx: any) => { + assertWritableWebhookHeaders(ctx?.input?.data as Record | undefined); + }; + + for (const event of ['beforeInsert', 'beforeUpdate'] as const) { + engine.registerHook(event, handler, { + object: WEBHOOK_OBJECT, + packageId: WEBHOOK_HEADERS_GATE_PACKAGE, + priority: GATE_PRIORITY, + }); + } + + logger?.info?.('[webhook] headers_secret shape gate bound (refuses non-flat-string-map plaintext)'); +} + +/** Remove the gate — mirrors `unbindWebhookProvenanceStamp`, for `dispose()`. */ +export function unbindWebhookHeadersShapeGate(engine: MinimalEngine): void { + if (typeof engine?.unregisterHooksByPackage === 'function') { + engine.unregisterHooksByPackage(WEBHOOK_HEADERS_GATE_PACKAGE); + } +} diff --git a/packages/plugins/plugin-webhooks/src/webhook-headers.ts b/packages/plugins/plugin-webhooks/src/webhook-headers.ts index f0904c3483..43849050d3 100644 --- a/packages/plugins/plugin-webhooks/src/webhook-headers.ts +++ b/packages/plugins/plugin-webhooks/src/webhook-headers.ts @@ -213,8 +213,15 @@ export class WebhookHeadersUnresolvableError extends Error { } } -/** The remedy clause both refusals end with — one wording, stated once. */ -const HEADERS_REMEDY = +/** + * The remedy clause both refusals end with — one wording, stated once. + * + * [#8566] Exported because the WRITE door quotes it too: the shape gate refuses + * the same malformed map at authoring time that this file refuses at delivery + * time, and an author who meets both should be told to do the same thing both + * times. Two hand-kept copies of one remedy is how they drift. + */ +export const HEADERS_REMEDY = 'Fix: re-save the webhook headers as a flat JSON object of string values so the column holds a ' + 'fresh ref, or CLEAR the field to null if this webhook is meant to send no custom headers — an ' + 'empty or unparseable header map is not the same thing as no header map, and only the second ' diff --git a/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts b/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts index 50a3af2e6f..045683afcb 100644 --- a/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts +++ b/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts @@ -14,6 +14,10 @@ import { bootstrapDeclaredWebhooks } from './bootstrap-declared-webhooks.js'; import { migrateLegacyWebhookSecrets } from './migrate-webhook-secrets.js'; import { createWebhookRedeliverGuard } from './redeliver-guard.js'; import { bindWebhookProvenanceStamp, unbindWebhookProvenanceStamp } from './webhook-provenance.js'; +import { + bindWebhookHeadersShapeGate, + unbindWebhookHeadersShapeGate, +} from './webhook-headers-gate.js'; /** * Structural view of `@objectstack/service-messaging`'s HTTP-outbox surface @@ -169,6 +173,7 @@ export class WebhookOutboxPlugin implements Plugin { await this.autoEnqueuer?.stop(); if (this.boundEngine) { try { unbindWebhookProvenanceStamp(this.boundEngine); } catch { /* best effort */ } + try { unbindWebhookHeadersShapeGate(this.boundEngine); } catch { /* best effort */ } this.boundEngine = undefined; } } @@ -199,6 +204,13 @@ export class WebhookOutboxPlugin implements Plugin { // Bind the provenance stamp so an admin edit freezes a seeded row. this.boundEngine = engine; bindWebhookProvenanceStamp(engine as any, ctx.logger as any); + // [#8566] And the headers_secret shape gate, BEFORE the seeder below + // runs its first write — a validation door that arms after the first + // write it is meant to judge is not a door. It covers every write path + // at once (the generic data API included, which is the measured + // trigger), so the plugin's own writers deliberately carry no second + // check of their own. + bindWebhookHeadersShapeGate(engine as any, ctx.logger as any); let metadataService: IMetadataService | undefined; try { metadataService = ctx.getService('metadata'); } catch { /* optional */ } try {