Skip to content

Commit 80d7425

Browse files
committed
fix(plugin-webhooks): park a subscription whose stored signing secret resolves to nothing (#8542)
`resolveWebhookSecret` returned `undefined` for two different facts — "the author configured this webhook unsigned" and "a key IS stored and nothing came back" — and `AutoEnqueuer.attachSecret` acts on the first reading. So the second silently became the first: the subscription armed and every delivery went out unauthenticated while `sys_webhook` kept reading `active: true`. That is the #7799 signing invariant failing OPEN, beside two adjacent modes that fail closed and loud. Fixed at the seam, not at each caller: presence is already decidable there (a set secret comes back from the generic read path as the engine's mask), so a stored key that does not resolve now raises `WebhookSecretUnresolvableError` and reaches `attachSecret` exactly the way a throwing resolver already did — park, durable record (#8069), say-once `error` with the ADR-0112 pair. The redeliver guard (#8069/#8541) keeps its contract in both directions: an unresolvable key is still refused with its own reason, anything else still propagates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk
1 parent 116c0d9 commit 80d7425

5 files changed

Lines changed: 364 additions & 16 deletions

File tree

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
---
2+
"@objectstack/plugin-webhooks": patch
3+
---
4+
5+
fix(plugin-webhooks): a stored signing secret that cannot be recovered parks the subscription instead of arming it and delivering UNSIGNED (#8542)
6+
7+
A webhook whose `sys_webhook.signing_secret` held a value that did not resolve
8+
was treated as **authored unsigned**. The subscription armed, every matching
9+
record change was delivered, and the HMAC signature — the receiver's only proof
10+
the delivery came from us — was silently absent. Nothing logged, nothing
11+
dropped, and `GET /api/v1/data/sys_webhook` kept reporting `active: true` with
12+
the secret column masked, so both the operator and the Setup UI still read
13+
"this webhook is signed".
14+
15+
The cause was one return value carrying two facts. `resolveWebhookSecret`
16+
answered `undefined` both for *"the author configured this webhook unsigned"*
17+
legitimate, `secret` is optional on the envelope — and for *"a key is stored and
18+
nothing came back"*. Its caller acts on the first reading, so the second became
19+
the first. That is the #7799 signing invariant failing **open**, immediately
20+
beside two adjacent failure modes that fail closed and loudly: a resolver that
21+
throws, and an engine with no encrypted-field channel, both of which drop the
22+
subscription and report at `error`.
23+
24+
Three states reach the silent path, all confirmed against a real engine:
25+
26+
- the `sys_webhook` row is deleted between the dispatcher's cache read and the
27+
per-row dereference;
28+
- the column holds something that is not a `secret:` ref — reachable only
29+
through a write that bypasses the engine (a column edited in SQL, a dump
30+
restored without its `sys_secret` rows, a seed script writing at driver
31+
level). The engine's own write path defends the two obvious routes: an echoed
32+
read-mask is dropped and cleartext is re-encrypted;
33+
- the stored value decrypts to an **empty string** — reachable through the
34+
ordinary data API, which accepts `signing_secret: ""`, encrypts it like any
35+
other value, and leaves the column holding a perfectly valid ref.
36+
37+
The fix is at the seam, so no consumer has to re-derive the rule: presence is
38+
already decidable there (a set secret comes back from the generic read path as
39+
the engine's mask, an unset one as `null`), and a stored key that does not
40+
resolve now raises rather than answering `undefined`. It therefore reaches
41+
`AutoEnqueuer.attachSecret` exactly the way a throwing resolver already did —
42+
the subscription is parked, the discarded event lands in `sys_http_delivery`
43+
with a cause (#8069), and the operator gets the existing remedy-bearing
44+
say-once `error` carrying `INTERNAL_ERROR` / `500` (ADR-0112).
45+
46+
**Unchanged:** a webhook authored with no secret at all still arms and delivers
47+
unsigned — that is a legitimate authored configuration, and it is pinned as the
48+
control for this change. The redelivery guard (#8069) keeps its behaviour in
49+
both directions: a stored-but-unresolvable key is still refused with its own
50+
reason, and any other failure still propagates, because "we could not check"
51+
must never read as "allowed".
52+
53+
**What an operator sees after upgrading.** A webhook that was quietly delivering
54+
unsigned stops delivering and starts reporting. If the deliveries were meant to
55+
be signed, re-save the secret so the column holds a fresh ref. If the webhook
56+
was meant to be unsigned, **clear** the field to `null` — an empty secret is not
57+
the same thing as no secret, and only the second one means "unsigned".

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,15 @@ export class AutoEnqueuer {
498498
* origin (#7722, #7799): a webhook that stops arriving is visible and gets
499499
* investigated, while one that keeps arriving unsigned is invisible and
500500
* teaches the receiver to accept unauthenticated traffic.
501+
*
502+
* [#8542] Case 3 means what it says only because the seam was fixed to say
503+
* it. `resolveWebhookSecret` used to answer `undefined` for BOTH "no key is
504+
* stored" and "a key is stored and did not come back", so this method read
505+
* the second as the third and armed the subscription — the invariant above
506+
* failing OPEN, silently, on the producer path. Nothing here changed: the
507+
* seam now raises for that case, so it lands in the `catch` below exactly
508+
* the way a throwing resolver already did, and the drop, the say-once
509+
* `error` and the #8069 park all apply to it unchanged.
501510
*/
502511
private async attachSecret(sub: CachedSubscription, row: any): Promise<boolean> {
503512
try {

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

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,26 @@
3636
*
3737
* ## The narrow fail-open this closes deliberately
3838
* Case 2 is checked as *"a value is stored but nothing came back"*, not as
39-
* *"the resolver threw"*. `resolveWebhookSecret` returns `undefined` — the same
40-
* value it uses for "authored unsigned" — when the column holds something that
41-
* is not a resolvable ref, so a `try/catch` alone would treat an unrecoverable
42-
* key as a legitimately unsigned webhook and allow the replay. Presence is
43-
* decidable from the masked read even though the value is not, so the guard
44-
* asks the question it can actually answer.
39+
* *"the resolver threw"*. Presence is decidable from the masked read even
40+
* though the value is not, so the guard asks the question it can actually
41+
* answer.
42+
*
43+
* [#8542] That reasoning has since moved DOWN into `resolveWebhookSecret`,
44+
* which now raises `WebhookSecretUnresolvableError` instead of returning the
45+
* same `undefined` it uses for "authored unsigned". The reason it had to move:
46+
* the ENQUEUE path had the identical ambiguity and no way to see it — and there
47+
* it failed open, arming the subscription and delivering unsigned. One seam,
48+
* one rule, so a consumer cannot forget to re-derive it. This guard keeps its
49+
* own presence check because the refusal REASON it returns is written from the
50+
* subscription row, and keeps its behaviour byte for byte: an unresolvable key
51+
* is refused with the text below, and anything else still propagates.
4552
*/
4653

4754
import type { IDataEngine } from '@objectstack/spec/contracts';
4855
import {
4956
WEBHOOK_OBJECT,
5057
WEBHOOK_SECRET_FIELD,
58+
isWebhookSecretUnresolvable,
5159
resolveWebhookSecret,
5260
} from './webhook-secret.js';
5361

@@ -91,7 +99,21 @@ export function createWebhookRedeliverGuard(
9199
&& subscription[WEBHOOK_SECRET_FIELD] !== '';
92100
if (!storesSecret) return undefined;
93101

94-
const plaintext = await resolveWebhookSecret(engine, subscription as { id: string }, subscriptionsObject);
102+
// [#8542] The seam now RAISES for the case this guard used to detect on
103+
// its own — the enqueue path needed the same distinction and could only
104+
// get it from a throw (its `catch` is what parks the subscription), so
105+
// the rule moved down one level instead of being written twice. This
106+
// guard's contract is unchanged in both directions, which is the point:
107+
// a stored-but-unresolvable key still returns the refusal REASON below
108+
// (case 2), and any OTHER failure still propagates, because "we could
109+
// not check" must never read as "allowed" (case 3, handled by
110+
// `assertRedeliverAllowed`).
111+
let plaintext: string | undefined;
112+
try {
113+
plaintext = await resolveWebhookSecret(engine, subscription as { id: string }, subscriptionsObject);
114+
} catch (err) {
115+
if (!isWebhookSecretUnresolvable(err)) throw err;
116+
}
95117
if (plaintext) return undefined;
96118

97119
return (

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

Lines changed: 177 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,11 @@ import { AutoEnqueuer } from './auto-enqueuer.js';
4141
import { bootstrapDeclaredWebhooks } from './bootstrap-declared-webhooks.js';
4242
import { migrateLegacyWebhookSecrets } from './migrate-webhook-secrets.js';
4343
import { SysWebhook } from './sys-webhook.object.js';
44-
import { WEBHOOK_SECRET_FIELD, __objectqlSecretWireForms } from './webhook-secret.js';
44+
import {
45+
WEBHOOK_SECRET_FIELD,
46+
__objectqlSecretWireForms,
47+
resolveWebhookSecret,
48+
} from './webhook-secret.js';
4549
import { WEBHOOK_HEADERS_FIELD } from './webhook-headers.js';
4650

4751
/**
@@ -951,3 +955,175 @@ describe('fail-closed and re-arm, extended to headers (#7986 × #7799/#8022)', (
951955
expect(calls[0].headers['X-Objectstack-Signature']).toBe(`sha256=${expected}`);
952956
});
953957
});
958+
959+
// ---------------------------------------------------------------------------
960+
// #8542 — the same invariant, failing in the OPPOSITE direction.
961+
//
962+
// Every fail-closed pin above is reached by making the resolver THROW. There is
963+
// a second way for a stored key not to come back, and it used to be silent:
964+
// `resolveSecretField` answers `null`, `resolveWebhookSecret` folded that onto
965+
// the `undefined` it uses for "authored unsigned", and `attachSecret` acted on
966+
// that reading — the subscription ARMED and every delivery went out
967+
// unauthenticated while `sys_webhook` kept reading `active: true`. Nothing
968+
// logged, nothing dropped, no `sys_http_delivery` row to find. The receiver's
969+
// only proof of origin simply stopped being attached.
970+
//
971+
// That is why these pins are written on the WIRE and on the durable record
972+
// rather than on the resolver's return value: the defect's entire signature is
973+
// a request that arrives looking normal.
974+
//
975+
// ⚠️ Every pin here first asserts that a secret is genuinely STORED (the row
976+
// reads back as the mask, and the operator sees `active: true`). Without that
977+
// precondition a fixture that had quietly lost its secret would exercise only
978+
// the legitimate-unsigned arm and pass against a completely unfixed tree — the
979+
// control at the end of this block is that arm, deliberately kept separate.
980+
// ---------------------------------------------------------------------------
981+
982+
describe('a stored signing secret that resolves to nothing (#8542)', () => {
983+
/** Drive one create event through the PRODUCTION wiring; report all three surfaces. */
984+
async function driveOnce(engine: any) {
985+
const realtime = new FakeRealtime();
986+
const outbox = new MemoryHttpOutbox();
987+
const errors: Array<{ msg: string; meta: any }> = [];
988+
const enqueuer = new AutoEnqueuer(engine, realtime, enqueueVia(outbox), {
989+
// The 60s escape hatch held shut, as in the #8022 pins: whatever the
990+
// first cache build concluded is what these assertions see.
991+
refreshIntervalMs: 0,
992+
logger: {
993+
error: (msg: string, _e?: unknown, meta?: unknown) => { errors.push({ msg, meta: meta as any }); },
994+
debug: () => {}, warn: () => {},
995+
},
996+
});
997+
await enqueuer.start();
998+
await realtime.publish(recordEvent('contact', { id: 'c1', name: 'Ada' }));
999+
await new Promise((r) => setTimeout(r, 0));
1000+
await new Promise((r) => setTimeout(r, 0));
1001+
const { impl, calls } = makeFetch();
1002+
await new HttpDispatcher({ nodeId: 'n1', outbox, fetchImpl: impl, partitionCount: 1 }).tick();
1003+
await enqueuer.stop();
1004+
return { calls, rows: await outbox.list(), errors };
1005+
}
1006+
1007+
/**
1008+
* The anti-vacuity precondition. Asserts the state the whole card is about:
1009+
* a secret IS stored, everything an operator can read says so, and the
1010+
* webhook is armed. A test that reached the assertions below WITHOUT this
1011+
* state would be testing the legitimate unsigned arm and would pass on an
1012+
* unfixed tree.
1013+
*/
1014+
async function expectSecretGenuinelyStored(engine: any, stores: any) {
1015+
const [viaApi] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } });
1016+
expect(viaApi[WEBHOOK_SECRET_FIELD]).toBe(SECRET_MASK);
1017+
expect(viaApi.active).toBe(true);
1018+
const atRest = Array.from(stores.get('sys_webhook')!.values())[0] as any;
1019+
expect(atRest[WEBHOOK_SECRET_FIELD]).toBeTruthy();
1020+
return viaApi;
1021+
}
1022+
1023+
/** What a fail-closed outcome has to look like on all three surfaces. */
1024+
function expectParkedNotDelivered(result: Awaited<ReturnType<typeof driveOnce>>) {
1025+
// ── The invariant, on the wire: nothing arrives, and in particular
1026+
// nothing arrives UNSIGNED. Asserting the absence of the signature
1027+
// header alone would pass on a tree that delivers, which is the defect.
1028+
expect(result.calls).toHaveLength(0);
1029+
// ── #8069: the discarded event still leaves a durable trace.
1030+
expect(result.rows).toHaveLength(1);
1031+
expect(result.rows[0]).toMatchObject({ status: 'dead', attempts: 0 });
1032+
expect(result.rows[0].signature).toBeUndefined();
1033+
// ── #8022/#8043: one say-once `error`, carrying the ADR-0112 pair a
1034+
// consumer branches on, and naming the credential so this cannot be
1035+
// confused with the header map's identical-looking drop (#7986).
1036+
expect(result.errors).toHaveLength(1);
1037+
expect(result.errors[0].meta).toMatchObject({
1038+
code: 'INTERNAL_ERROR',
1039+
status: 500,
1040+
field: WEBHOOK_SECRET_FIELD,
1041+
});
1042+
}
1043+
1044+
it('refuses to arm when the stored secret was emptied through the ordinary data API', async () => {
1045+
const { engine, stores } = await buildEngine();
1046+
await bootstrapDeclaredWebhooks(engine, metadataWith([declaredWebhook()]));
1047+
const [row] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } });
1048+
1049+
// The trigger that needs no privileged access at all — measured, not
1050+
// assumed. The engine accepts an empty string for a `secret` field,
1051+
// encrypts it like any other, mints a real `sys_secret` row, and leaves
1052+
// the column holding a perfectly VALID ref. Every read path then
1053+
// reports a secret is set, and the dereference answers ''.
1054+
await engine.update('sys_webhook', { [WEBHOOK_SECRET_FIELD]: '' }, { where: { id: row.id } });
1055+
1056+
await expectSecretGenuinelyStored(engine, stores);
1057+
const result = await driveOnce(engine);
1058+
expectParkedNotDelivered(result);
1059+
// The remedy this state specifically needs, since "re-save the secret"
1060+
// is not the only fix and an operator who wanted it unsigned has to be
1061+
// told the difference between an empty secret and no secret.
1062+
expect(result.rows[0].error).toMatch(/could not be decrypted/);
1063+
expect(result.errors[0].msg).toMatch(/NO delivery/);
1064+
});
1065+
1066+
it('refuses to arm when the column no longer holds a resolvable ref', async () => {
1067+
const { engine, stores, driver } = await buildEngine();
1068+
await bootstrapDeclaredWebhooks(engine, metadataWith([declaredWebhook()]));
1069+
const [row] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } });
1070+
1071+
// Written BELOW the engine deliberately, because that is the only route
1072+
// measured to reach this state: the engine's own write path DROPS an
1073+
// echoed mask and RE-ENCRYPTS cleartext, so neither lands. What does
1074+
// land here is a dump restored without its sys_secret rows, a column
1075+
// edited in SQL, or a seed script writing at driver level.
1076+
await driver.update('sys_webhook', row.id, { [WEBHOOK_SECRET_FIELD]: 'whsec_pasted_by_hand' });
1077+
1078+
// …and the row still reads back as the mask, so nothing an operator can
1079+
// see distinguishes this from the healthy webhook it was a moment ago.
1080+
await expectSecretGenuinelyStored(engine, stores);
1081+
expectParkedNotDelivered(await driveOnce(engine));
1082+
});
1083+
1084+
it('refuses when the row is deleted between the cache read and the dereference', async () => {
1085+
const { engine } = await buildEngine();
1086+
await bootstrapDeclaredWebhooks(engine, metadataWith([declaredWebhook()]));
1087+
1088+
// The snapshot the enqueuer's refresh loop holds while it dereferences
1089+
// each row's credentials, one at a time.
1090+
const [snapshot] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } });
1091+
expect(snapshot[WEBHOOK_SECRET_FIELD]).toBe(SECRET_MASK);
1092+
1093+
await engine.delete('sys_webhook', { where: { id: snapshot.id } });
1094+
1095+
// Pinned at the seam rather than through the enqueuer because the race
1096+
// is a property of the seam: `resolveSecretField` opens `if (!row)
1097+
// return null`, and the caller holding the snapshot cannot tell that
1098+
// `null` apart from "this webhook was authored unsigned".
1099+
await expect(resolveWebhookSecret(engine, snapshot as any)).rejects.toThrow(
1100+
/resolved to nothing/,
1101+
);
1102+
});
1103+
1104+
it('a webhook authored unsigned still arms and delivers — the refusal is not a blanket', async () => {
1105+
const { engine } = await buildEngine();
1106+
await bootstrapDeclaredWebhooks(
1107+
engine,
1108+
metadataWith([declaredWebhook({ secret: undefined })]),
1109+
);
1110+
1111+
// The control's precondition is the exact mirror of the anti-vacuity
1112+
// one above: NOTHING is stored, so `undefined` is the legitimate
1113+
// authored fact and not a swallowed failure. Its header map is left
1114+
// declared on purpose — the signing refusal must not spill onto the
1115+
// sibling credential that resolves perfectly well (#7986).
1116+
const [viaApi] = await engine.find('sys_webhook', { where: { name: 'crm_hook' } });
1117+
expect(viaApi[WEBHOOK_SECRET_FIELD] ?? null).toBeNull();
1118+
1119+
const { calls, rows, errors } = await driveOnce(engine);
1120+
expect(calls).toHaveLength(1);
1121+
expect(calls[0].headers['X-Objectstack-Signature']).toBeUndefined();
1122+
expect(calls[0].headers['X-Team']).toBe('crm');
1123+
// `success`, not `pending`: the dispatcher ran, so this control asserts
1124+
// a delivery that COMPLETED — the working feature a blanket refusal
1125+
// would have turned into a parked `dead` row.
1126+
expect(rows[0].status).toBe('success');
1127+
expect(errors).toHaveLength(0);
1128+
});
1129+
});

0 commit comments

Comments
 (0)