Skip to content

Commit f46e987

Browse files
huangyiireneclaude
andauthored
fix(plugin-webhooks): re-arm an encrypted-secret webhook when the CryptoProvider registers (#8022) (#8043)
* fix(plugin-webhooks): re-arm an encrypted-secret webhook when the CryptoProvider registers (#8022) A webhook whose signing secret is encrypted was unsubscribed for ~60s after every restart: a record change in that window produced no delivery and no `sys_http_delivery` row, while the row still read `active: true`. Not a race — an ordering. Plugins run inside `kernel:ready`, which `runtime.start()` completes, and every host wires `setCryptoProvider` only after `runtime.start()` returns. So `AutoEnqueuer`'s first cache build always preceded the capability it needs, correctly dropped every secret-bearing subscription (#7799 fail-closed), and nothing re-read until the periodic refresh. `ObjectQL.onCryptoProviderChange(listener)` reports the registration; the auto-enqueuer subscribes before its first build and rebuilds when it fires, without joining the in-flight pre-registration refresh. Feature-detected, as `resolveSecretField` already was. The fail-closed drop itself is unchanged. It now reports at `error` with the consequence and the fix, carries the ADR-0112 `INTERNAL_ERROR`/500 pair, and is said once per outage rather than every refresh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuYKU5d8xPfHSZyASNsyia * test(plugin-webhooks): drop the mid-flight re-arm case — it passes on main too (#8022) Every harness that can inject the CryptoProvider registration while the first cache build is in flight also moves the secret resolution to after it, so the naive `() => this.refresh()` passes the test as well. A case that cannot separate the two revisions is a false green, so it is removed rather than kept as coverage it does not provide. The guard in `rearmAfterCryptoRegistered` stays, with the reasoning stated where it lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuYKU5d8xPfHSZyASNsyia * fix(plugin-webhooks): prune the dropped-webhook report set on refresh (#8022) The set that makes the fail-closed drop report once per outage instead of once per refresh kept every id it ever saw. Two consequences: it grows for the life of the process, and a webhook deactivated while broken and later reactivated still broken has its first report suppressed as a repeat. Each refresh now forgets ids the read no longer returns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FuYKU5d8xPfHSZyASNsyia --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 22f0daa commit f46e987

5 files changed

Lines changed: 421 additions & 9 deletions

File tree

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
---
2+
"@objectstack/plugin-webhooks": patch
3+
"@objectstack/objectql": patch
4+
---
5+
6+
fix(plugin-webhooks): a webhook holding an encrypted signing secret re-arms the moment the CryptoProvider registers, instead of ~60s later (#8022)
7+
8+
For roughly **60 seconds after every server start**, a webhook whose
9+
`signing_secret` is encrypted (the population #7799 created) was **not
10+
subscribed**. A record change in that window produced no delivery **and no
11+
`sys_http_delivery` row at all** — no dead letter, no retry, no durable trace
12+
that anything was missed — while `GET /api/v1/data/sys_webhook/` kept reading
13+
`active: true`, so the webhook looked armed in Setup the whole time. It
14+
self-healed at the next periodic cache refresh, which is why it was invisible to
15+
anyone not watching that window.
16+
17+
**The fail-closed behaviour is unchanged and is not the bug.** Dropping a
18+
subscription whose stored key cannot be recovered — rather than delivering it
19+
unsigned — is #7799's whole point and still holds: the signature is the
20+
receiver's only proof of origin, and a webhook that stops arriving gets
21+
investigated while one that keeps arriving unsigned teaches the receiver to
22+
accept unauthenticated traffic. What was wrong is that a fail-closed drop
23+
outlived its own cause.
24+
25+
**The ordering.** It was never a race that sometimes went the other way. Plugins
26+
run inside `kernel:ready`, which `runtime.start()` completes; the host's
27+
composition root calls `engine.setCryptoProvider(...)` only *after*
28+
`runtime.start()` returns (`packages/cli/src/commands/serve.ts`,
29+
`packages/verify/src/harness.ts`). So `AutoEnqueuer`'s first subscription-cache
30+
build reliably preceded the capability it needs, dropped every secret-bearing
31+
row on what it could see, and nothing re-read until the periodic refresh.
32+
33+
`ObjectQL` now reports the registration (`onCryptoProviderChange(listener)`,
34+
fired after the provider is in place), and the auto-enqueuer subscribes
35+
**before** its first build and rebuilds the cache when it fires. Re-arming is
36+
immediate and event-driven — no polling, and no shorter-but-still-present
37+
window. The re-arm deliberately does not join an in-flight refresh: the build
38+
most likely running at that moment is the pre-registration one, and joining it
39+
would report success having re-armed nothing.
40+
41+
The channel is feature-detected, as `resolveSecretField` already was — this
42+
plugin takes no dependency on `@objectstack/objectql`. An engine without it keeps
43+
the previous behaviour, with the periodic refresh as the backstop.
44+
45+
**The drop is also no longer quiet.** A subscription dropped for an unresolvable
46+
key now reports at `error` with the consequence and the fix stated in the
47+
message, and carries an ADR-0112 `code`/`status` pair (`INTERNAL_ERROR`/500) in
48+
its metadata — the same pair the seeder's refusal for the same cause already
49+
carried. Per AGENTS.md it is said **once** per outage per webhook rather than
50+
every refresh cycle, and a webhook that recovers and breaks again is loud again.
51+
52+
Unaffected, and verified still true: the secret's bytes appear nowhere in
53+
`sys_webhook` or in a delivery row, deliveries carry `signature` and never the
54+
key (#7722), and a delivery whose key exists only as ciphertext after a restart
55+
still produces the byte-identical HMAC receivers already verify.

packages/objectql/src/engine.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1666,6 +1666,16 @@ export class ObjectQL implements IObjectQLEngine {
16661666
// persists cleartext). Injected by the host via setCryptoProvider().
16671667
private cryptoProvider?: ICryptoProvider;
16681668

1669+
// [#8022] Listeners notified when a crypto provider is (re)registered.
1670+
// Server-side consumers that dereference a secret at BOOT — the webhook
1671+
// auto-enqueuer's subscription cache is the one this was built for — run
1672+
// inside `kernel:ready`, which every host completes BEFORE its composition
1673+
// root injects a provider. Their first read therefore fails closed against a
1674+
// capability that is about to exist, and without a notification the only way
1675+
// back is to poll. The engine is the sole party that knows the moment it
1676+
// arrives, so the notification belongs here.
1677+
private readonly cryptoProviderListeners = new Set<() => void>();
1678+
16691679
// [ADR-0105 D2 / #3623] Posture accessor for driver-scope widening under the
16701680
// `group` posture. Injected by SecurityPlugin via setTenancyPostureProvider();
16711681
// absent = equality scoping (fail toward isolation).
@@ -4618,10 +4628,51 @@ export class ObjectQL implements IObjectQLEngine {
46184628
* Mirrors the Settings subsystem's ICryptoProvider wiring; the host (e.g.
46194629
* `serve`) injects `LocalCryptoProvider` in dev and a KMS/Vault-backed
46204630
* provider in production.
4631+
*
4632+
* Notifies {@link onCryptoProviderChange} listeners AFTER the provider is in
4633+
* place, so a listener that immediately re-reads a secret sees the new
4634+
* capability rather than the state that made it fail (#8022).
46214635
*/
46224636
setCryptoProvider(provider: ICryptoProvider): void {
46234637
this.cryptoProvider = provider;
46244638
this.logger.info('CryptoProvider configured for secret fields');
4639+
// A listener is a re-arm, never part of this call's contract: one that
4640+
// throws must not fail the host's composition root, and must not stop the
4641+
// listeners behind it from re-arming.
4642+
for (const listener of [...this.cryptoProviderListeners]) {
4643+
try {
4644+
listener();
4645+
} catch (err) {
4646+
this.logger.warn('CryptoProvider registration listener failed', {
4647+
error: (err as Error)?.message ?? String(err),
4648+
});
4649+
}
4650+
}
4651+
}
4652+
4653+
/**
4654+
* [#8022] Observe crypto-provider registration.
4655+
*
4656+
* Exists for consumers that must dereference a `secret` field on a schedule
4657+
* they do not control — the boot path. `secret` reads are fail-closed by
4658+
* design (#7799), which is correct, but "no provider" at boot is a
4659+
* *transient* state on every host: `kernel:ready` runs plugins, and only
4660+
* after `runtime.start()` returns does the composition root call
4661+
* {@link setCryptoProvider}. A consumer whose cache was built in that gap is
4662+
* wrong until it rebuilds, and polling is the only alternative to being told.
4663+
*
4664+
* Fires on every registration, including a later replacement (a KMS provider
4665+
* swapped in over the dev one) — a listener that re-reads is correct in both
4666+
* cases, and a re-read is cheap next to signing with a key from the wrong
4667+
* provider.
4668+
*
4669+
* @returns an unsubscribe function; call it when the listener's owner stops.
4670+
*/
4671+
onCryptoProviderChange(listener: () => void): () => void {
4672+
this.cryptoProviderListeners.add(listener);
4673+
return () => {
4674+
this.cryptoProviderListeners.delete(listener);
4675+
};
46254676
}
46264677

46274678
/**

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

Lines changed: 136 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,14 @@
33
import type { IDataEngine, IRealtimeService, RealtimeEventPayload } from '@objectstack/spec/contracts';
44
import type { WebhookTriggerType } from '@objectstack/spec/automation';
55
import type { EnqueueHttpInput } from '@objectstack/service-messaging';
6-
import { WEBHOOK_SECRET_FIELD, readLegacySecret, resolveWebhookSecret } from './webhook-secret.js';
6+
import {
7+
WEBHOOK_SECRET_FIELD,
8+
WEBHOOK_SECRET_REFUSAL_CODE,
9+
WEBHOOK_SECRET_REFUSAL_STATUS,
10+
onCryptoProviderChange,
11+
readLegacySecret,
12+
resolveWebhookSecret,
13+
} from './webhook-secret.js';
714

815
/**
916
* The authored trigger vocabulary, taken from the spec rather than restated
@@ -116,6 +123,16 @@ export class AutoEnqueuer {
116123
private refreshTimer: ReturnType<typeof setInterval> | undefined;
117124
private running = false;
118125
private refreshing: Promise<void> | undefined;
126+
/** [#8022] Detach for the engine's crypto-registration listener. */
127+
private unbindCryptoListener: (() => void) | undefined;
128+
/**
129+
* [#8022] Webhook ids currently dropped for an unresolvable signing key.
130+
* Held so the loud first report is said ONCE per outage (AGENTS.md
131+
* "Degradation log levels": *say it once, at the first degradation*) and
132+
* again if the same webhook breaks after recovering — not once per row per
133+
* refresh, forever.
134+
*/
135+
private readonly droppedForSecret = new Set<string>();
119136

120137
constructor(
121138
private readonly engine: IDataEngine,
@@ -135,6 +152,17 @@ export class AutoEnqueuer {
135152
if (this.running) return;
136153
this.running = true;
137154

155+
// [#8022] Bound BEFORE the first build, not after: on every host the
156+
// composition root wires the CryptoProvider after `runtime.start()`
157+
// returns, i.e. after the `kernel:ready` handler that runs this method
158+
// — so the registration we need to hear about can land at any point
159+
// from here on, including while the await below is still in flight.
160+
// Subscribing first makes that unmissable; subscribing after the
161+
// refresh would reintroduce the same race in miniature.
162+
this.unbindCryptoListener = onCryptoProviderChange(this.engine, () =>
163+
this.rearmAfterCryptoRegistered(),
164+
);
165+
138166
await this.refresh();
139167

140168
// Main subscription: every data event → match → enqueue.
@@ -167,9 +195,38 @@ export class AutoEnqueuer {
167195
if (this.subId) await this.realtime.unsubscribe(this.subId);
168196
if (this.subIdSelfHeal) await this.realtime.unsubscribe(this.subIdSelfHeal);
169197
if (this.refreshTimer) clearInterval(this.refreshTimer);
198+
this.unbindCryptoListener?.();
170199
this.subId = undefined;
171200
this.subIdSelfHeal = undefined;
172201
this.refreshTimer = undefined;
202+
this.unbindCryptoListener = undefined;
203+
}
204+
205+
/**
206+
* [#8022] The engine just gained a CryptoProvider — rebuild the cache so
207+
* subscriptions dropped for an unresolvable signing key re-arm now, instead
208+
* of at the next periodic refresh up to {@link refreshIntervalMs} away.
209+
*
210+
* It deliberately does NOT call {@link refresh} directly. `refresh()`
211+
* coalesces onto an in-flight build, and the build most likely to be in
212+
* flight right now is the one from `start()` — the very build whose rows
213+
* were read while there was no provider. Joining it would return "refreshed"
214+
* having re-armed nothing, which is this issue with an extra step. So: let
215+
* whatever is running finish, then read again.
216+
*/
217+
private rearmAfterCryptoRegistered(): void {
218+
const inFlight = this.refreshing ?? Promise.resolve();
219+
void inFlight
220+
// A failed in-flight refresh already logged; it must not stop the
221+
// re-arm, which is the whole point of this callback.
222+
.catch(() => undefined)
223+
.then(() => (this.running ? this.refresh() : undefined))
224+
.catch((err) =>
225+
this.logger.warn?.(
226+
'[webhook-auto-enqueuer] re-arm after CryptoProvider registration failed',
227+
err,
228+
),
229+
);
173230
}
174231

175232
/**
@@ -220,6 +277,17 @@ export class AutoEnqueuer {
220277
this.subscriptions.clear();
221278
for (const [k, v] of next) this.subscriptions.set(k, v);
222279

280+
// [#8022] Forget rows this refresh no longer sees — deleted, or
281+
// deactivated. Otherwise the set grows for the life of the process, and
282+
// a webhook turned off while broken and later turned back on still
283+
// broken would have its first report suppressed as a repeat.
284+
if (this.droppedForSecret.size > 0) {
285+
const live = new Set(rows.map((r) => String(r?.id)));
286+
for (const id of this.droppedForSecret) {
287+
if (!live.has(id)) this.droppedForSecret.delete(id);
288+
}
289+
}
290+
223291
this.logger.debug?.('[webhook-auto-enqueuer] cache refreshed', {
224292
objects: this.subscriptions.size,
225293
rows: rows.length,
@@ -258,15 +326,13 @@ export class AutoEnqueuer {
258326
const stored = await resolveWebhookSecret(this.engine, row, this.subscriptionsObject);
259327
if (stored) {
260328
sub.secret = stored;
329+
// Recovered — a later break is a new outage and gets said loudly
330+
// again rather than being swallowed as a repeat.
331+
this.droppedForSecret.delete(sub.id);
261332
return true;
262333
}
263334
} catch (err) {
264-
this.logger.warn?.(
265-
`[webhook-auto-enqueuer] webhook '${sub.name}' holds an encrypted signing secret that ` +
266-
`could not be decrypted — the subscription is DROPPED rather than delivered unsigned ` +
267-
`(#7799). Deliveries resume once the sys_secret row and CryptoProvider are reachable.`,
268-
{ id: sub.id, field: WEBHOOK_SECRET_FIELD, err: (err as Error)?.message ?? err },
269-
);
335+
this.reportDrop(sub, err);
270336
return false;
271337
}
272338

@@ -281,9 +347,72 @@ export class AutoEnqueuer {
281347
);
282348
sub.secret = legacy;
283349
}
350+
this.droppedForSecret.delete(sub.id);
284351
return true;
285352
}
286353

354+
/**
355+
* [#8022] Report a subscription dropped for an unresolvable signing key.
356+
*
357+
* ## Why `error`, and why only the first time
358+
* AGENTS.md decides the level with one question: *after the degradation,
359+
* does the system still look normal from the outside while something the
360+
* system claims is happening is not?* Here the answer is yes, and it is the
361+
* whole defect — `GET /api/v1/data/sys_webhook` keeps reading
362+
* `active: true`, Setup keeps showing the webhook armed, and every matching
363+
* record change is discarded with no delivery and no `sys_http_delivery`
364+
* row to find afterwards. That is a durability degradation wearing a
365+
* functional degradation's clothes, so it owes the two things an `error`
366+
* owes: the consequence, concretely, and the fix.
367+
*
368+
* Said ONCE per outage per webhook, per the same section. The cache is
369+
* rebuilt every {@link refreshIntervalMs}; an unfixed misconfiguration would
370+
* otherwise print this line every 60s forever, which is how an `error`
371+
* channel becomes unreadable — the failure mode that made the founding
372+
* incident's `warn` invisible. Repeats drop to `debug`; a recovery clears
373+
* the id, so a re-break is loud again.
374+
*
375+
* ADR-0112: `code` + `status` travel in the meta so a consumer branches on
376+
* the pair, not on message text. Same pair the seeder's refusal carries for
377+
* the same underlying cause.
378+
*/
379+
private reportDrop(sub: CachedSubscription, err: unknown): void {
380+
const meta = {
381+
id: sub.id,
382+
webhook: sub.name,
383+
field: WEBHOOK_SECRET_FIELD,
384+
code: WEBHOOK_SECRET_REFUSAL_CODE,
385+
status: WEBHOOK_SECRET_REFUSAL_STATUS,
386+
err: (err as Error)?.message ?? err,
387+
};
388+
if (this.droppedForSecret.has(sub.id)) {
389+
this.logger.debug?.(
390+
`[webhook-auto-enqueuer] webhook '${sub.name}' is still dropped for an unresolvable ` +
391+
'signing secret (#7799/#8022)',
392+
meta,
393+
);
394+
return;
395+
}
396+
this.droppedForSecret.add(sub.id);
397+
const message =
398+
`[webhook-auto-enqueuer] webhook '${sub.name}' holds an encrypted signing secret that ` +
399+
'could not be decrypted — the subscription is DROPPED rather than delivered unsigned ' +
400+
'(#7799), so every matching record change is discarded with NO delivery and NO ' +
401+
'sys_http_delivery row, while the row keeps reading active:true in Setup. Fix: register a ' +
402+
'CryptoProvider (engine.setCryptoProvider — LocalCryptoProvider in dev, KMS/Vault in ' +
403+
'production) with the same key the secret was written under, and make sure the sys_secret ' +
404+
'row is reachable; the subscription re-arms on registration (#8022) and at the next ' +
405+
'periodic refresh.';
406+
// The logger surface is a subset of console/kernel logger — `error` is
407+
// optional on it, so fall back rather than silently losing the report
408+
// on a logger that only implements `warn`.
409+
if (typeof this.logger.error === 'function') {
410+
this.logger.error(message, err, meta);
411+
} else {
412+
this.logger.warn?.(message, meta);
413+
}
414+
}
415+
287416
private parseRow(row: any): CachedSubscription | null {
288417
if (!row?.id || !row?.url) return null;
289418
// `triggers` is now authored as a multi-select (stored as an array), but

0 commit comments

Comments
 (0)