fix(plugin-webhooks): park a subscription whose stored header map resolves to nothing (#8558)

.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".

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<boolean> {
  try {

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}`);
 }
 
 /**