fix(plugin-webhooks): a stored signing secret that resolves to nothing parks the subscription instead of delivering UNSIGNED (#8542)

.changeset/webhook-stored-secret-unresolvable.md

@@ -0,0 +1,57 @@
+---
+"@objectstack/plugin-webhooks": patch
+---
+
+fix(plugin-webhooks): a stored signing secret that cannot be recovered parks the subscription instead of arming it and delivering UNSIGNED (#8542)
+
+A webhook whose `sys_webhook.signing_secret` held a value that did not resolve
+was treated as **authored unsigned**. The subscription armed, every matching
+record change was delivered, and the HMAC signature — the receiver's only proof
+the delivery came from us — was silently absent. Nothing logged, nothing
+dropped, and `GET /api/v1/data/sys_webhook` kept reporting `active: true` with
+the secret column masked, so both the operator and the Setup UI still read
+"this webhook is signed".
+
+The cause was one return value carrying two facts. `resolveWebhookSecret`
+answered `undefined` both for *"the author configured this webhook unsigned"* —
+legitimate, `secret` is optional on the envelope — and for *"a key is stored and
+nothing came back"*. Its caller acts on the first reading, so the second became
+the first. That is the #7799 signing invariant failing **open**, immediately
+beside two adjacent failure modes that fail closed and loudly: a resolver that
+throws, and an engine with no encrypted-field channel, both of which drop the
+subscription and report at `error`.
+
+Three states reach the silent path, all confirmed against a real engine:
+
+- the `sys_webhook` row is deleted between the dispatcher'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 engine's own write path defends the two obvious routes: an echoed
+ read-mask is dropped and cleartext is re-encrypted;
+- the stored value decrypts to an **empty string** — reachable through the
+ ordinary data API, which accepts `signing_secret: ""`, encrypts it like any
+ other value, and leaves the column holding a perfectly valid ref.
+
+The fix is at the seam, so no consumer has to re-derive the rule: presence is
+already decidable there (a set secret comes back from the generic read path as
+the engine's mask, an unset one as `null`), and a stored key that does not
+resolve now raises rather than answering `undefined`. It therefore reaches
+`AutoEnqueuer.attachSecret` 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).
+
+**Unchanged:** a webhook authored with no secret at all still arms and delivers
+unsigned — that is a legitimate authored configuration, and it is pinned as the
+control for this change. The redelivery guard (#8069) keeps its behaviour in
+both directions: a stored-but-unresolvable key is still refused with its own
+reason, and any other failure still propagates, because "we could not check"
+must never read as "allowed".
+
+**What an operator sees after upgrading.** A webhook that was quietly delivering
+unsigned stops delivering and starts reporting. If the deliveries were meant to
+be signed, re-save the secret so the column holds a fresh ref. If the webhook
+was meant to be unsigned, **clear** the field to `null` — an empty secret is not
+the same thing as no secret, and only the second one means "unsigned".

packages/plugins/plugin-webhooks/src/auto-enqueuer.ts

@@ -498,6 +498,15 @@ export class AutoEnqueuer {
  * origin (#7722, #7799): a webhook that stops arriving is visible and gets
  * investigated, while one that keeps arriving unsigned is invisible and
  * teaches the receiver to accept unauthenticated traffic.
+ *
+ * [#8542] Case 3 means what it says only because the seam was fixed to say
+ * it. `resolveWebhookSecret` used to answer `undefined` for BOTH "no key is
+ * stored" and "a key is stored and did not come back", so this method read
+ * the second as the third and armed the subscription — the invariant above
+ * failing OPEN, silently, on the producer path. Nothing here changed: the
+ * seam now raises for that case, 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 attachSecret(sub: CachedSubscription, row: any): Promise<boolean> {
  try {

packages/plugins/plugin-webhooks/src/redeliver-guard.ts

@@ -36,18 +36,26 @@
  *
  * ## The narrow fail-open this closes deliberately
  * Case 2 is checked as *"a value is stored but nothing came back"*, not as
- * *"the resolver threw"*. `resolveWebhookSecret` returns `undefined` — the same
- * value it uses for "authored unsigned" — when the column holds something that
- * is not a resolvable ref, so a `try/catch` alone would treat an unrecoverable
- * key as a legitimately unsigned webhook and allow the replay. Presence is
- * decidable from the masked read even though the value is not, so the guard
- * asks the question it can actually answer.
+ * *"the resolver threw"*. Presence is decidable from the masked read even
+ * though the value is not, so the guard asks the question it can actually
+ * answer.
+ *
+ * [#8542] That reasoning has since moved DOWN into `resolveWebhookSecret`,
+ * which now raises `WebhookSecretUnresolvableError` instead of returning the
+ * same `undefined` it uses for "authored unsigned". The reason it had to move:
+ * the ENQUEUE path had the identical ambiguity and no way to see it — and there
+ * it failed open, arming the subscription and delivering unsigned. One seam,
+ * one rule, so a consumer cannot forget to re-derive it. This guard keeps its
+ * own presence check because the refusal REASON it returns is written from the
+ * subscription row, and keeps its behaviour byte for byte: an unresolvable key
+ * is refused with the text below, and anything else still propagates.
  */
 
 import type { IDataEngine } from '@objectstack/spec/contracts';
 import {
  WEBHOOK_OBJECT,
  WEBHOOK_SECRET_FIELD,
+ isWebhookSecretUnresolvable,
  resolveWebhookSecret,
 } from './webhook-secret.js';
 
@@ -91,7 +99,21 @@ export function createWebhookRedeliverGuard(
  && subscription[WEBHOOK_SECRET_FIELD] !== '';
  if (!storesSecret) return undefined;
 
- const plaintext = await resolveWebhookSecret(engine, subscription as { id: string }, subscriptionsObject);
+ // [#8542] The seam now RAISES for the case this guard used to detect on
+ // its own — the enqueue path needed the same distinction and could only
+ // get it from a throw (its `catch` is what parks the subscription), so
+ // the rule moved down one level instead of being written twice. This
+ // guard's contract is unchanged in both directions, which is the point:
+ // a stored-but-unresolvable key still returns the refusal REASON below
+ // (case 2), and any OTHER failure still propagates, because "we could
+ // not check" must never read as "allowed" (case 3, handled by
+ // `assertRedeliverAllowed`).
+ let plaintext: string | undefined;
+ try {
+ plaintext = await resolveWebhookSecret(engine, subscription as { id: string }, subscriptionsObject);
+ } catch (err) {
+ if (!isWebhookSecretUnresolvable(err)) throw err;
+ }
  if (plaintext) return undefined;
 
  return (

packages/plugins/plugin-webhooks/src/webhook-secret-at-rest.test.ts

@@ -41,7 +41,11 @@ import { AutoEnqueuer } from './auto-enqueuer.js';
 import { bootstrapDeclaredWebhooks } from './bootstrap-declared-webhooks.js';
 import { migrateLegacyWebhookSecrets } from './migrate-webhook-secrets.js';
 import { SysWebhook } from './sys-webhook.object.js';
-import { WEBHOOK_SECRET_FIELD, __objectqlSecretWireForms } from './webhook-secret.js';
+import {
+ WEBHOOK_SECRET_FIELD,
+ __objectqlSecretWireForms,
+ resolveWebhookSecret,
+} from './webhook-secret.js';
 import { WEBHOOK_HEADERS_FIELD } from './webhook-headers.js';
 
 /**
@@ -951,3 +955,175 @@ describe('fail-closed and re-arm, extended to headers (#7986 × #7799/#8022)', (
  expect(calls[0].headers['X-Objectstack-Signature']).toBe(`sha256=${expected}`);
  });
 });
+
+// ---------------------------------------------------------------------------
+// #8542 — the same invariant, failing in the OPPOSITE direction.
+//
+// Every fail-closed pin above is reached by making the resolver THROW. There is
+// a second way for a stored key not to come back, and it used to be silent:
+// `resolveSecretField` answers `null`, `resolveWebhookSecret` folded that onto
+// the `undefined` it uses for "authored unsigned", and `attachSecret` acted on
+// that reading — the subscription ARMED and every delivery went out
+// unauthenticated while `sys_webhook` kept reading `active: true`. Nothing
+// logged, nothing dropped, no `sys_http_delivery` row to find. The receiver's
+// only proof of origin simply stopped being attached.
+//
+// That is why these pins are written on the WIRE and on the durable record
+// rather than on the resolver's return value: the defect's entire signature is
+// a request that arrives looking normal.
+//
+// ⚠️ Every pin here first asserts that a secret is genuinely STORED (the row
+// reads back as the mask, and the operator sees `active: true`). Without that
+// precondition a fixture that had quietly lost its secret would exercise only
+// the legitimate-unsigned arm and pass against a completely unfixed tree — the
+// control at the end of this block is that arm, deliberately kept separate.
+// ---------------------------------------------------------------------------
+
+describe('a stored signing secret that resolves to nothing (#8542)', () => {
+ /** 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: whatever the
+ // first cache build concluded is what these assertions see.
+ 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. Asserts the state the whole card is about:
+ * a secret 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 testing the legitimate unsigned arm and would pass on an
+ * unfixed tree.
+ */
+ async function expectSecretGenuinelyStored(engine: any, stores: any) {
+ const [viaApi] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } });
+ expect(viaApi[WEBHOOK_SECRET_FIELD]).toBe(SECRET_MASK);
+ expect(viaApi.active).toBe(true);
+ const atRest = Array.from(stores.get('sys_webhook')!.values())[0] as any;
+ expect(atRest[WEBHOOK_SECRET_FIELD]).toBeTruthy();
+ return viaApi;
+ }
+
+ /** What a fail-closed outcome has to look like on all three surfaces. */
+ function expectParkedNotDelivered(result: Awaited<ReturnType<typeof driveOnce>>) {
+ // ── The invariant, on the wire: nothing arrives, and in particular
+ // nothing arrives UNSIGNED. Asserting the absence of the signature
+ // header alone would pass on a tree that delivers, which is the defect.
+ expect(result.calls).toHaveLength(0);
+ // ── #8069: the discarded event still leaves a durable trace.
+ expect(result.rows).toHaveLength(1);
+ expect(result.rows[0]).toMatchObject({ status: 'dead', attempts: 0 });
+ expect(result.rows[0].signature).toBeUndefined();
+ // ── #8022/#8043: one say-once `error`, carrying the ADR-0112 pair a
+ // consumer branches on, and naming the credential so this cannot be
+ // confused with the header map's identical-looking drop (#7986).
+ expect(result.errors).toHaveLength(1);
+ expect(result.errors[0].meta).toMatchObject({
+ code: 'INTERNAL_ERROR',
+ status: 500,
+ field: WEBHOOK_SECRET_FIELD,
+ });
+ }
+
+ it('refuses to arm when the stored secret was emptied through the ordinary data API', async () => {
+ const { engine, stores } = await buildEngine();
+ await bootstrapDeclaredWebhooks(engine, metadataWith([declaredWebhook()]));
+ const [row] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } });
+
+ // The trigger that needs no privileged access at all — measured, not
+ // assumed. The engine accepts an empty string for a `secret` field,
+ // encrypts it like any other, mints a real `sys_secret` row, and leaves
+ // the column holding a perfectly VALID ref. Every read path then
+ // reports a secret is set, and the dereference answers ''.
+ await engine.update('sys_webhook', { [WEBHOOK_SECRET_FIELD]: '' }, { where: { id: row.id } });
+
+ await expectSecretGenuinelyStored(engine, stores);
+ const result = await driveOnce(engine);
+ expectParkedNotDelivered(result);
+ // The remedy this state specifically needs, since "re-save the secret"
+ // is not the only fix and an operator who wanted it unsigned has to be
+ // told the difference between an empty secret and no secret.
+ expect(result.rows[0].error).toMatch(/could not be decrypted/);
+ expect(result.errors[0].msg).toMatch(/NO delivery/);
+ });
+
+ it('refuses to arm when the column no longer holds a resolvable ref', async () => {
+ const { engine, stores, driver } = await buildEngine();
+ await bootstrapDeclaredWebhooks(engine, metadataWith([declaredWebhook()]));
+ const [row] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } });
+
+ // Written BELOW the engine deliberately, because that is the only route
+ // measured to reach this state: the engine's own write path DROPS an
+ // echoed mask and RE-ENCRYPTS cleartext, so neither lands. What does
+ // land here is a dump restored without its sys_secret rows, a column
+ // edited in SQL, or a seed script writing at driver level.
+ await driver.update('sys_webhook', row.id, { [WEBHOOK_SECRET_FIELD]: 'whsec_pasted_by_hand' });
+
+ // …and the row still reads back as the mask, so nothing an operator can
+ // see distinguishes this from the healthy webhook it was a moment ago.
+ await expectSecretGenuinelyStored(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([declaredWebhook()]));
+
+ // 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_SECRET_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 unsigned".
+ await expect(resolveWebhookSecret(engine, snapshot as any)).rejects.toThrow(
+ /resolved to nothing/,
+ );
+ });
+
+ it('a webhook authored unsigned still arms and delivers — the refusal is not a blanket', async () => {
+ const { engine } = await buildEngine();
+ await bootstrapDeclaredWebhooks(
+ engine,
+ metadataWith([declaredWebhook({ secret: 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 header map is left
+ // declared on purpose — the signing refusal must not spill onto the
+ // sibling credential that resolves perfectly well (#7986).
+ const [viaApi] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } });
+ expect(viaApi[WEBHOOK_SECRET_FIELD] ?? null).toBeNull();
+
+ const { calls, rows, errors } = await driveOnce(engine);
+ expect(calls).toHaveLength(1);
+ expect(calls[0].headers['X-Objectstack-Signature']).toBeUndefined();
+ expect(calls[0].headers['X-Team']).toBe('crm');
+ // `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);
+ });
+});