From 56d8b5991616b6f9eec856b515f6df4896651559 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 21:48:55 +0000 Subject: [PATCH] fix(plugin-webhooks): park a subscription whose stored header map resolves to nothing (#8558) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveWebhookHeaders` answered `undefined` for two different facts — "the author configured no custom headers" and "a map IS stored and did not come back as one" — and `AutoEnqueuer.attachHeaders` acts on the first reading. So the second became the first: the subscription armed and every delivery went out missing its entire authored header map, the ordinary place an `Authorization` goes, while the row kept reading `active: true` with `headers_secret` masked. Measured end to end: the delivery SUCCEEDED carrying a byte-correct `X-Objectstack-Signature`, which is the worst available combination — the signature tells the receiver the request is genuinely ours while it no longer matches the configuration its operator wrote. Sibling of #8542 on the same seam's other credential, but WIDER rather than symmetric: a signing secret is an opaque scalar so only `''` collapsed, while a header map's content decides and every string that is not a flat JSON object of string values collapses — through the ordinary data API, on a field whose own description tells the admin to type JSON into it. Fixed at the seam, so no caller re-derives the rule: stored headers that do not come back as a map now raise `WebhookHeadersUnresolvableError`, reaching `attachHeaders`' existing `catch` exactly the way a throwing resolver already did. The park (#8069), the durable `sys_http_delivery` record and the say-once ADR-0112 `error` all apply unchanged; `attachHeaders` needed no new branch. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk --- .../webhook-stored-headers-unresolvable.md | 78 +++++ .../plugin-webhooks/src/auto-enqueuer.ts | 12 + .../plugin-webhooks/src/webhook-headers.ts | 173 ++++++++++- .../src/webhook-secret-at-rest.test.ts | 276 +++++++++++++++++- 4 files changed, 525 insertions(+), 14 deletions(-) create mode 100644 .changeset/webhook-stored-headers-unresolvable.md diff --git a/.changeset/webhook-stored-headers-unresolvable.md b/.changeset/webhook-stored-headers-unresolvable.md new file mode 100644 index 0000000000..ff9a5a2824 --- /dev/null +++ b/.changeset/webhook-stored-headers-unresolvable.md @@ -0,0 +1,78 @@ +--- +"@objectstack/plugin-webhooks": patch +--- + +fix(plugin-webhooks): a stored header map that cannot be recovered parks the subscription instead of arming it and delivering the headers MISSING (#8558) + +A webhook whose `sys_webhook.headers_secret` held a value that did not come back +as a header map was treated as **authored without custom headers**. The +subscription armed, every matching record change was delivered, and the entire +authored map — the ordinary place an `Authorization: Bearer …` goes — was +silently absent. Nothing logged, nothing dropped, and +`GET /api/v1/data/sys_webhook` kept reporting `active: true` with the header +column masked, so both the operator and the Setup UI still read "custom headers +are configured". + +Measured end to end against a real engine, what reached the receiver was worse +than "a delivery with something missing": the request SUCCEEDED +(`sys_http_delivery.status = 'success'`) carrying a byte-correct +`X-Objectstack-Signature`. The signature is the receiver's proof the request is +genuinely ours, so a receiver that authenticates by signature had every reason +to accept a request that no longer matched the configuration its operator wrote. +Against an endpoint that requires the credential the result is a 401 nobody +attributes correctly; against one that does not — a routing `X-Tenant-Id`, an +`X-Environment: staging` — the delivery is simply wrong and nobody finds out. + +The cause was one return value carrying two facts. `resolveWebhookHeaders` +answered `undefined` both for *"the author configured no custom headers"* — +legitimate, `headers` is optional on the envelope — and for *"a map is stored +and did not come back as one"*. Its caller acts on the first reading, so the +second became the first. This is the sibling of the signing-secret collapse +(#8542) on the same seam's other credential, and the file's own header comment +already promised the opposite: *"It does not deliver partially. A row whose +stored headers cannot be resolved DROPS the subscription."* + +**This path is wider than the signing-secret one, not symmetric to it.** A +signing secret is an opaque scalar, so any non-empty answer is a usable key and +only the empty string collapsed. A header map's CONTENT decides, and +`parseStoredHeaders` answers `undefined` — correctly, for its own job — for every +string that is not a flat JSON object of string values. Four states reach the +seam, all confirmed against a real engine: + +- the `sys_webhook` row is deleted between the enqueuer's cache read and the + per-row dereference; +- the column holds something that is not a `secret:` ref — reachable only + through a write that bypasses the engine (a column edited in SQL, a dump + restored without its `sys_secret` rows, a seed script writing at driver level); +- the stored value decrypts to an **empty string**; +- the stored value decrypts to a perfectly readable string that is **not a flat + string map** — `{}`, `[]`, `{"X-Count": 5}`, a nested object, or any typo. + This is the widest road rather than an exotic one: `headers_secret` is an + admin-authorable field whose own description instructs the author to type a + JSON object into it, and every one of these spellings is accepted by the + ordinary data API, encrypted like any other value, and left behind a + perfectly valid ref that reads back as the mask. + +The fix is at the seam, so no consumer has to re-derive the rule: presence is +already decidable there (`headers_secret` is a map only in the plaintext — at +the storage layer it is an ordinary scalar `secret` column, so a set map comes +back from the generic read path as the engine's mask and an unset one as `null`), +and stored headers that do not come back as a map now raise rather than +answering `undefined`. They therefore reach `AutoEnqueuer.attachHeaders` exactly +the way a throwing resolver already did — the subscription is parked, the +discarded event lands in `sys_http_delivery` with a cause (#8069), and the +operator gets the existing remedy-bearing say-once `error` carrying +`INTERNAL_ERROR` / `500` (ADR-0112) and naming `headers_secret`, so it cannot be +confused with the signing secret's identical-looking drop. + +**Unchanged:** a webhook authored with no custom headers at all still arms and +delivers — that is a legitimate authored configuration, and it is pinned as the +control for this change, as is a webhook whose stored map resolves normally and +still delivers every header including the credential entry. + +**What an operator sees after upgrading.** A webhook that was quietly delivering +without its headers stops delivering and starts reporting. Re-save the headers +as a flat JSON object of string values so the column holds a fresh ref, or +**clear** the field to `null` if the 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 one means "send nothing extra". diff --git a/packages/plugins/plugin-webhooks/src/auto-enqueuer.ts b/packages/plugins/plugin-webhooks/src/auto-enqueuer.ts index 24b34b2a3f..a072941287 100644 --- a/packages/plugins/plugin-webhooks/src/auto-enqueuer.ts +++ b/packages/plugins/plugin-webhooks/src/auto-enqueuer.ts @@ -550,6 +550,18 @@ export class AutoEnqueuer { * from the configuration the author wrote, and nothing anywhere records * that it went out incomplete. A subscription that stops is visible; a * delivery that arrives subtly wrong is not. + * + * [#8558] And that is what this method used to do, for the same reason its + * signing sibling did (#8542): `resolveWebhookHeaders` answered `undefined` + * for BOTH "no headers are stored" and "a map is stored and did not come + * back as one", so this method read the second as the first and armed the + * subscription — the paragraph above failing OPEN. Measured, the delivery + * then went out SUCCESSFULLY and correctly SIGNED with the whole authored + * map missing, which is the worst available combination: the signature + * tells the receiver the request is genuinely ours. Nothing here changed: + * the seam now raises, so it lands in the `catch` below exactly the way a + * throwing resolver already did, and the drop, the say-once `error` and the + * #8069 park all apply to it unchanged. */ private async attachHeaders(sub: CachedSubscription, row: any): Promise { try { diff --git a/packages/plugins/plugin-webhooks/src/webhook-headers.ts b/packages/plugins/plugin-webhooks/src/webhook-headers.ts index fcc94f3ed1..f0904c3483 100644 --- a/packages/plugins/plugin-webhooks/src/webhook-headers.ts +++ b/packages/plugins/plugin-webhooks/src/webhook-headers.ts @@ -66,10 +66,21 @@ * - It does not deliver partially. A row whose stored headers cannot be * resolved DROPS the subscription rather than delivering it with the headers * missing — see {@link resolveWebhookHeaders}. + * + * [#8558] That last line was a statement of intent this file did not keep. Only + * a THROWING resolver reached the caller's `catch`; a resolver that answered + * `null` — or handed back a value that was not a flat string map — folded onto + * the `undefined` this seam uses for "no headers stored", and the subscription + * armed and delivered without them. {@link WebhookHeadersUnresolvableError} is + * what makes the sentence true. */ import type { IDataEngine } from '@objectstack/spec/contracts'; -import { isOpaqueSecretForm } from './webhook-secret.js'; +import { + WEBHOOK_SECRET_REFUSAL_CODE, + WEBHOOK_SECRET_REFUSAL_STATUS, + isOpaqueSecretForm, +} from './webhook-secret.js'; /** Column on `sys_webhook` holding the encrypted custom-header map. */ export const WEBHOOK_HEADERS_FIELD = 'headers_secret'; @@ -151,13 +162,131 @@ export function readLegacyHeaders(definitionJson: unknown): WebhookHeaders | und } /** - * Recover a row's custom headers. Returns `undefined` when the row stores none - * — which is not an error: `headers` is optional on the authoring envelope. + * [#8558] A header map IS stored on the row and did not come back as one. + * + * ## Why this is an error and not an `undefined` + * `resolveWebhookHeaders` used to return `undefined` for two different facts — + * *"the author configured this webhook with no custom headers"* and *"a map is + * stored and did not come back"* — and its caller acts on the first reading, + * which is the legitimate one. So the second silently became the first: the + * subscription ARMED and every delivery went out missing the entire authored + * header map, while `sys_webhook` kept reading `active: true` with + * `headers_secret` masked, i.e. still reporting "custom headers are + * configured". Measured end to end, what reached the receiver was a delivery + * that SUCCEEDED, carrying a byte-correct `X-Objectstack-Signature`, with the + * `Authorization` the author declared simply absent — and nothing logged. + * + * That the signature is VALID is what makes the direction so bad. It tells the + * receiver the request is genuinely ours, so a receiver that authenticates by + * signature has every reason to accept a request that no longer matches the + * configuration its operator wrote. Against an endpoint that does not require + * the header at all — a routing `X-Tenant-Id`, an `X-Environment: staging` — + * the delivery is simply wrong and nobody finds out. + * + * Presence is decidable even when the value is not, and this is the one place + * worth stating plainly because the field LOOKS like it should behave + * differently: `headers_secret` is a map only in the plaintext. At the storage + * layer it is an ordinary scalar `secret` column holding the serialized map, so + * the generic read path returns the engine's mask for a set map and `null` for + * an unset one — the same decidable signal `signing_secret` gives, for the same + * reason. The "it is a map, not a scalar" worry does not survive measurement. + * + * Carries the ADR-0112 pair as fields so a consumer branches on `code`/`status` + * rather than on message text — the same pair `attachHeaders`' drop report and + * the signing seam's refusal already carry for the same class of cause. + * + * ## Why ONE error class for two conditions + * A stored map reaches this seam and fails in two distinguishable ways: it + * could not be RECOVERED (nothing came back), or it was recovered fine and is + * not a usable header map. They deserve different remedies and get different + * messages. They do not deserve different types: every consumer of this seam + * branches on the ADR-0112 pair and the disposition, both identical — park the + * subscription, record the discarded event, say it once. A second class with no + * consumer would be a distinction the tree cannot act on. + */ +export class WebhookHeadersUnresolvableError extends Error { + readonly code = WEBHOOK_SECRET_REFUSAL_CODE; + readonly status = WEBHOOK_SECRET_REFUSAL_STATUS; + constructor(message: string) { + super(message); + this.name = 'WebhookHeadersUnresolvableError'; + } +} + +/** The remedy clause both refusals end with — one wording, stated once. */ +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 ' + + 'one means "send nothing extra".'; + +/** + * Parse a recovered value into the map, or refuse. + * + * {@link parseStoredHeaders} answers `undefined` for every string that is not a + * flat `Record` of strings, which is right for its own job and wrong as an + * answer to *"what are this webhook's headers?"* once a value is known to be + * stored. This is the narrow wrapper that turns the second reading into a + * refusal, so the rule lives at the seam and no caller re-derives it. + */ +function requireHeaderMap( + recovered: unknown, + row: { id: string; [k: string]: unknown }, + where: string, +): WebhookHeaders { + const parsed = parseStoredHeaders(recovered); + if (parsed) return parsed; + + throw new WebhookHeadersUnresolvableError( + `Webhook "${String(row.name ?? row.id)}" stores custom headers in ${where} that came back but are ` + + 'not a flat JSON object of string values, so there is no header map to send. A value IS stored ' + + '— the read path returns the engine mask for it — so this is NOT a webhook authored without ' + + 'headers, and delivering it without them would silently drop whatever the author put in that ' + + 'map, including an Authorization credential, on a delivery that is otherwise correctly signed ' + + 'and therefore looks genuine to the receiver (#7986, #8558). Causes, in the order worth ' + + 'checking: the value was typed into the Custom Headers field and is not valid JSON; it parses ' + + 'but is an array, an empty object, or has a non-string value ({"X-Count": 5}); or it is a ' + + `nested object where the wire format allows only strings. ${HEADERS_REMEDY}`, + ); +} + +/** + * Recover a row's custom headers. Returns `undefined` for EXACTLY one fact — + * the row stores no headers — which is not an error: `headers` is optional on + * the authoring envelope, and a webhook with no custom headers is a legitimate + * authored configuration. + * + * Throws {@link WebhookHeadersUnresolvableError} when a map IS stored and does + * not come back as one. Callers must treat that as "drop this subscription", + * never as "deliver without them" — see `AutoEnqueuer.attachCredentials` for + * why partial delivery is the invisible failure and a stopped subscription is + * the visible one. + * + * ## [#8558] Why "did not come back" is not spelled `undefined` + * This is the sibling of #8542 on `webhook-secret.ts`, and the measurement that + * produced it found the header path is WIDER than the signing path rather than + * symmetric to it. A signing secret is an opaque scalar: any non-empty answer + * is a usable key, so only the empty string collapses. A header map's CONTENT + * decides, so every one of these reaches this function as a stored-but-unusable + * value, all confirmed against a real engine: * - * Throws when headers ARE stored but cannot be dereferenced. Callers must treat - * that as "drop this subscription", never as "deliver without them" — see - * `AutoEnqueuer.attachCredentials` for why partial delivery is the invisible - * failure and a stopped subscription is the visible one. + * 1. the `sys_webhook` row is deleted between the enqueuer's cache read and + * this dereference (`resolveSecretField` opens `if (!row) return null`); + * 2. the column holds something that is not a `secret:` ref — reachable only + * through a write that BYPASSES the engine (a hand-edited column, a dump + * restored without its `sys_secret` rows, a seed script writing at driver + * level). The engine's own write path defends both obvious routes: an + * echoed mask is dropped and cleartext is re-encrypted; + * 3. the ciphertext decrypts to the empty string; + * 4. ⭐ the ciphertext decrypts to a perfectly readable string that is not a + * flat string map — `{}`, `[]`, `{"X-Count": 5}`, a nested object, or any + * typo. Reachable through the ORDINARY data API with no privileged access, + * and it is the WIDEST road here rather than an exotic one: + * `sys_webhook.headers_secret` is an admin-authorable field whose own + * description instructs the author to type a JSON object into it. + * + * In all four the row still advertises stored headers on every read path, so + * returning `undefined` told the caller the opposite of what the row says. */ export async function resolveWebhookHeaders( engine: IDataEngine, @@ -167,24 +296,42 @@ export async function resolveWebhookHeaders( const stored = row[WEBHOOK_HEADERS_FIELD]; // Unset / cleared. On the generic read path a set secret comes back as the // engine's mask (a non-empty string) and an unset one as `null`, so presence - // is decidable here WITHOUT the value ever being readable. + // is decidable here WITHOUT the value ever being readable. Everything below + // this line therefore runs with "headers ARE stored" already established — + // which is the knowledge the old `undefined` return threw away. if (stored == null || stored === '') return undefined; const resolver = engine as SecretResolvingEngine; if (typeof resolver.resolveSecretField !== 'function') { // An engine with no encrypted-field channel stored verbatim what the seeder // handed it, so the column IS the serialized map — reading it is correct, - // not a fallback. The refusal below is for the narrow case where the value - // is one of objectql's opaque forms and there is no way to invert it. - if (!isOpaqueSecretForm(stored)) return parseStoredHeaders(stored); - throw new Error( + // not a fallback. It can still fail to parse, and that arm used to answer + // `undefined` too; it is refused here for the same reason as everything + // else on this seam. + if (!isOpaqueSecretForm(stored)) { + return requireHeaderMap(stored, row, `${object}.${WEBHOOK_HEADERS_FIELD}`); + } + throw new WebhookHeadersUnresolvableError( `Webhook "${String(row.name ?? row.id)}" stores encrypted custom headers, but this data engine ` + 'does not implement resolveSecretField() — they cannot be recovered, so the subscription is ' + 'dropped rather than delivered without the headers it was authored with (#7986).', ); } const plain = await resolver.resolveSecretField(object, String(row.id), WEBHOOK_HEADERS_FIELD); - return parseStoredHeaders(plain); + if (plain == null || plain === '') { + throw new WebhookHeadersUnresolvableError( + `Webhook "${String(row.name ?? row.id)}" stores custom headers in ` + + `${object}.${WEBHOOK_HEADERS_FIELD} that resolved to nothing. A value IS stored — the read ` + + 'path returns the engine mask for it — so this is NOT a webhook authored without headers, ' + + 'and delivering it without them would silently drop whatever the author put in that map, ' + + 'including an Authorization credential, on a delivery that is otherwise correctly signed and ' + + 'therefore looks genuine to the receiver (#7986, #8558). Causes, in the order worth checking: ' + + 'the row was deleted while this refresh was reading it; the column holds something that is ' + + 'not a secret: ref (a hand-edited column, or a dump restored without its sys_secret rows); ' + + `or the stored value decrypts to an empty string. ${HEADERS_REMEDY}`, + ); + } + return requireHeaderMap(plain, row, `${object}.${WEBHOOK_HEADERS_FIELD}`); } /** diff --git a/packages/plugins/plugin-webhooks/src/webhook-secret-at-rest.test.ts b/packages/plugins/plugin-webhooks/src/webhook-secret-at-rest.test.ts index 441ca3cd12..6a060430f4 100644 --- a/packages/plugins/plugin-webhooks/src/webhook-secret-at-rest.test.ts +++ b/packages/plugins/plugin-webhooks/src/webhook-secret-at-rest.test.ts @@ -46,7 +46,7 @@ import { __objectqlSecretWireForms, resolveWebhookSecret, } from './webhook-secret.js'; -import { WEBHOOK_HEADERS_FIELD } from './webhook-headers.js'; +import { WEBHOOK_HEADERS_FIELD, resolveWebhookHeaders } from './webhook-headers.js'; /** * [#8069] The PRODUCTION enqueue wiring, as one helper. @@ -1127,3 +1127,277 @@ describe('a stored signing secret that resolves to nothing (#8542)', () => { expect(errors).toHaveLength(0); }); }); + +// --------------------------------------------------------------------------- +// #8558 — the same collapse on the SIBLING credential, measured rather than +// assumed symmetric. +// +// The card that filed this traced the call path and said so honestly: the three +// `null`-producing states were measured for `signing_secret` on #8542 and +// carried over here "by construction, since `resolveSecretField` is +// field-agnostic". They do all reach this path — but the measurement that +// produced these pins found the header path is **WIDER**, not equal, and the +// difference is the reason this block is not a copy of the one above. +// +// - `signing_secret` holds an opaque scalar: ANY non-empty string that comes +// back is a usable key, so only the empty string collapses. +// - `headers_secret` holds a SERIALIZED MAP, and `parseStoredHeaders` — quite +// correctly, for its own job — answers `undefined` for every string that is +// not a flat `Record`. `''`, `'{}'`, `'[]'`, `'null'`, +// `'{"X-Count":5}'`, `'{"a":{"b":"c"}}'` and any typo all land there. +// +// And that field is DIRECTLY AUTHORABLE: `sys_webhook.headers_secret` is a +// `Field.secret` whose own description instructs the admin to type "a JSON +// object ({"Authorization": "Bearer …"})" into it. So the widest road here is +// not a hand-edited column or a restored dump — it is an admin mistyping JSON +// into the field the product told them to type JSON into, through the ordinary +// data API, on a row that then reads back masked and `active: true`. +// +// What reached the wire before this fix, measured end to end: a delivery that +// SUCCEEDED (`sys_http_delivery.status = 'success'`), carrying a byte-correct +// `X-Objectstack-Signature`, with the entire authored header map — the +// `Authorization` included — silently absent, and no `error` logged anywhere. +// The valid signature is what makes this worse than it looks: it tells the +// receiver the delivery is genuinely ours, so a receiver that authenticates by +// signature has every reason to accept a request that no longer matches the +// configuration its operator wrote. +// +// ⚠️ Every pin below first asserts that a header map is genuinely STORED — the +// row reads back as the mask, the column is a ref at rest, `active: true`. +// Without that precondition a fixture that had quietly lost its headers would +// exercise only the legitimate no-headers arm and pass against a completely +// unfixed tree. Both controls at the end are that arm, deliberately separate. +// --------------------------------------------------------------------------- + +describe('a stored header map that resolves to nothing (#8558)', () => { + /** Drive one create event through the PRODUCTION wiring; report all three surfaces. */ + async function driveOnce(engine: any) { + const realtime = new FakeRealtime(); + const outbox = new MemoryHttpOutbox(); + const errors: Array<{ msg: string; meta: any }> = []; + const enqueuer = new AutoEnqueuer(engine, realtime, enqueueVia(outbox), { + // The 60s escape hatch held shut, as in the #8022 pins. + refreshIntervalMs: 0, + logger: { + error: (msg: string, _e?: unknown, meta?: unknown) => { errors.push({ msg, meta: meta as any }); }, + debug: () => {}, warn: () => {}, + }, + }); + await enqueuer.start(); + await realtime.publish(recordEvent('contact', { id: 'c1', name: 'Ada' })); + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); + const { impl, calls } = makeFetch(); + await new HttpDispatcher({ nodeId: 'n1', outbox, fetchImpl: impl, partitionCount: 1 }).tick(); + await enqueuer.stop(); + return { calls, rows: await outbox.list(), errors }; + } + + /** + * The anti-vacuity precondition: a header map IS stored, everything an + * operator can read says so, and the webhook is armed. A test that reached + * the assertions below WITHOUT this state would be exercising the + * legitimate no-headers arm and would pass on an unfixed tree. + */ + async function expectHeadersGenuinelyStored(engine: any, stores: any) { + const [viaApi] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } }); + expect(viaApi[WEBHOOK_HEADERS_FIELD]).toBe(SECRET_MASK); + expect(viaApi.active).toBe(true); + const atRest = Array.from(stores.get('sys_webhook')!.values())[0] as any; + expect(atRest[WEBHOOK_HEADERS_FIELD]).toBeTruthy(); + return viaApi; + } + + /** What a fail-closed outcome has to look like on all three surfaces. */ + function expectParkedNotDelivered(result: Awaited>) { + // ── The invariant, on the wire. Asserting only that `Authorization` is + // absent would PASS on the broken tree — the broken tree's whole + // signature is a delivery that arrives looking normal — so the pin is + // that nothing arrives at all. + expect(result.calls).toHaveLength(0); + // ── #8069: the discarded event still leaves a durable trace, and the + // parked row carries neither the credential-bearing map nor a signature. + expect(result.rows).toHaveLength(1); + expect(result.rows[0]).toMatchObject({ status: 'dead', attempts: 0 }); + expect(result.rows[0].headers).toBeUndefined(); + expect(result.rows[0].signature).toBeUndefined(); + // ── #8022/#8043: one say-once `error` carrying the ADR-0112 pair, and + // naming `headers_secret` — the signing secret resolves perfectly well + // in every pin here, so a report that named the wrong credential would + // send an operator to rotate a key that was never the problem. + expect(result.errors).toHaveLength(1); + expect(result.errors[0].meta).toMatchObject({ + code: 'INTERNAL_ERROR', + status: 500, + field: WEBHOOK_HEADERS_FIELD, + }); + } + + it('refuses to arm when the stored map was emptied through the ordinary data API', async () => { + const { engine, stores } = await buildEngine(); + await bootstrapDeclaredWebhooks(engine, metadataWith([headerBearingWebhook()])); + const [row] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } }); + + // Needs no privileged access at all: the engine accepts an empty string + // for a `secret` field, encrypts it like any other value, mints a real + // `sys_secret` row and leaves the column holding a VALID ref. Every read + // path then reports headers are configured, and the dereference answers ''. + await engine.update('sys_webhook', { [WEBHOOK_HEADERS_FIELD]: '' }, { where: { id: row.id } }); + + await expectHeadersGenuinelyStored(engine, stores); + const result = await driveOnce(engine); + expectParkedNotDelivered(result); + expect(result.rows[0].error).toMatch(/could not be decrypted/); + expect(result.errors[0].msg).toMatch(/NO delivery/); + }); + + // The road that has no counterpart on the signing-secret side. For a key, + // any non-empty answer is usable; for a map, the CONTENT decides — and this + // is the field an admin is told to type JSON into. Each spelling below is a + // realistic thing to find in that box, and every one of them delivered + // header-less before this fix. + const unusableStoredValues: Array<[label: string, stored: 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}'], + ]; + + it.each(unusableStoredValues)( + 'refuses to arm when the stored value decrypts fine but is unusable: %s', + async (_label, badValue) => { + const { engine, stores } = await buildEngine(); + await bootstrapDeclaredWebhooks(engine, metadataWith([headerBearingWebhook()])); + const [row] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } }); + + // Written through the ORDINARY data API — no driver access, no SQL. + await engine.update( + 'sys_webhook', + { [WEBHOOK_HEADERS_FIELD]: badValue }, + { where: { id: row.id } }, + ); + + // …and the row is indistinguishable from the healthy one it was a + // moment ago: masked on read, a real ref at rest, still `active`. + await expectHeadersGenuinelyStored(engine, stores); + const atRest = Array.from(stores.get('sys_webhook')!.values())[0] as any; + expect(String(atRest[WEBHOOK_HEADERS_FIELD])).toMatch(/^secret:/); + + expectParkedNotDelivered(await driveOnce(engine)); + }, + ); + + it('refuses to arm when the column no longer holds a resolvable ref', async () => { + const { engine, stores, driver } = await buildEngine(); + await bootstrapDeclaredWebhooks(engine, metadataWith([headerBearingWebhook()])); + const [row] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } }); + + // Written BELOW the engine deliberately: that is the only route measured + // to reach this state, since the engine's own write path drops an echoed + // mask and re-encrypts cleartext. A dump restored without its sys_secret + // rows, a column edited in SQL, a seed script writing at driver level. + // + // The value planted here is a PERFECTLY VALID serialized header map, and + // that is the point: `resolveSecretField` answers `null` for anything + // that is not a `secret:` ref, so the map is unreadable through the + // supported channel no matter how well-formed it looks at rest. + await driver.update('sys_webhook', row.id, { + [WEBHOOK_HEADERS_FIELD]: JSON.stringify({ 'X-Team': 'crm' }), + }); + + await expectHeadersGenuinelyStored(engine, stores); + expectParkedNotDelivered(await driveOnce(engine)); + }); + + it('refuses when the row is deleted between the cache read and the dereference', async () => { + const { engine } = await buildEngine(); + await bootstrapDeclaredWebhooks(engine, metadataWith([headerBearingWebhook()])); + + // The snapshot the enqueuer's refresh loop holds while it dereferences + // each row's credentials, one at a time. + const [snapshot] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } }); + expect(snapshot[WEBHOOK_HEADERS_FIELD]).toBe(SECRET_MASK); + + await engine.delete('sys_webhook', { where: { id: snapshot.id } }); + + // Pinned at the seam rather than through the enqueuer because the race + // is a property of the seam: `resolveSecretField` opens `if (!row) + // return null`, and the caller holding the snapshot cannot tell that + // `null` apart from "this webhook was authored without headers". + await expect( + resolveWebhookHeaders(engine, snapshot as any, 'sys_webhook'), + ).rejects.toThrow(/resolved to nothing/); + }); + + it('refuses on an engine with no encrypted channel whose column holds an unusable value', async () => { + // The third arm of the same seam, and the one with no analogue on the + // signing side at all: with no `resolveSecretField` the column IS the + // serialized map, so reading it verbatim is correct — but the parse can + // still fail, and that answered `undefined` too. + const engineWithoutChannel = { + async find() { return []; }, + } as any; + + await expect( + resolveWebhookHeaders( + engineWithoutChannel, + { id: 'whk_1', name: 'crm_hook', [WEBHOOK_HEADERS_FIELD]: '{oops' }, + 'sys_webhook', + ), + ).rejects.toThrow(/not a flat JSON object/); + + // …and the same engine still returns a well-formed verbatim map, so + // this refusal did not break the no-crypto deployment shape. + await expect( + resolveWebhookHeaders( + engineWithoutChannel, + { id: 'whk_1', name: 'crm_hook', [WEBHOOK_HEADERS_FIELD]: '{"X-Team":"crm"}' }, + 'sys_webhook', + ), + ).resolves.toEqual({ 'X-Team': 'crm' }); + }); + + it('a webhook authored with NO headers still arms and delivers — the refusal is not a blanket', async () => { + const { engine } = await buildEngine(); + await bootstrapDeclaredWebhooks( + engine, + metadataWith([declaredWebhook({ headers: undefined })]), + ); + + // The control's precondition is the exact mirror of the anti-vacuity one + // above: NOTHING is stored, so `undefined` is the legitimate authored + // fact and not a swallowed failure. Its signing secret is left declared + // on purpose — the header refusal must not spill onto the sibling + // credential that resolves perfectly well. + const [viaApi] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } }); + expect(viaApi[WEBHOOK_HEADERS_FIELD] ?? null).toBeNull(); + + const { calls, rows, errors } = await driveOnce(engine); + expect(calls).toHaveLength(1); + // `success`, not `pending`: the dispatcher ran, so this control asserts + // a delivery that COMPLETED — the working feature a blanket refusal + // would have turned into a parked `dead` row. + expect(rows[0].status).toBe('success'); + expect(errors).toHaveLength(0); + // …and it is still signed: the header rule did not touch the key path. + const expected = createHmac('sha256', SECRET).update(calls[0].body).digest('hex'); + expect(calls[0].headers['X-Objectstack-Signature']).toBe(`sha256=${expected}`); + }); + + it('a webhook whose stored map resolves normally still delivers every header', async () => { + // The other half of the control, and the one that would catch a fix + // written as "refuse whenever headers are stored": the healthy path has + // to keep working end to end, credential entry included. + const { engine, stores } = await buildEngine(); + await bootstrapDeclaredWebhooks(engine, metadataWith([headerBearingWebhook()])); + + await expectHeadersGenuinelyStored(engine, stores); + const { calls, rows, errors } = await driveOnce(engine); + expect(calls).toHaveLength(1); + expect(calls[0].headers.Authorization).toBe(BEARER); + expect(calls[0].headers['X-Team']).toBe('crm'); + expect(rows[0].status).toBe('success'); + expect(errors).toHaveLength(0); + }); +});