Skip to content

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

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-8542-stored-secret-unresolvable
Aug 13, 2026
Merged

fix(plugin-webhooks): a stored signing secret that resolves to nothing parks the subscription instead of delivering UNSIGNED (#8542)#8560
os-zhuang merged 1 commit into
mainfrom
claude/issue-8542-stored-secret-unresolvable

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#8542

resolveWebhookSecret returned undefined for two different facts"the author configured this webhook unsigned" (legitimate, secret is optional on the envelope) 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, every matching record change was delivered, and the HMAC signature simply stopped being attached. Nothing logged, nothing dropped, sys_webhook still reading active: true with the secret column masked, so both the operator and Setup still read "this webhook is signed".

That is the #7799 signing invariant failing OPEN, sitting immediately beside two adjacent modes that fail closed and loud — a throwing resolver and an engine with no encrypted-field channel, both of which park the subscription and report at error, say-once (#8043).

The card's evidence was a trace. This is what measurement changed.

The filer was explicit that the mechanism was read off the call path and never exercised, and the dispatch required reproduction before any edit. Reproduced against a realObjectQL engine (in-memory driver double, reversible test crypto, the real SysWebhook schema), on origin/main at 116c0d95b:

triggerverdicthow it is reached
row deleted between the cache read and the dereferencereproducedresolveSecretField opens if (!row) return null; the caller holds a snapshot that still says a secret is stored
column holds a non-secret: valuereproduced, but narrower than filedonly through a write that BYPASSES the engine — a column edited in SQL, a dump restored without its sys_secret rows, a seed script at driver level
stored value decrypts to the empty stringreproduced — and the widest of the threethe ORDINARY data API accepts signing_secret: "", encrypts it, mints a real sys_secret row and leaves a perfectly VALID ref behind

Two of the card's own sub-causes for the middle trigger are falsified. A written-back mask cannot land — encryptSecretFields drops any masked field whose value equals the read mask, and the stored ref survives untouched. Cleartext through the engine cannot land either — it is re-encrypted into a fresh ref (and with no CryptoProvider it is refused fail-closed). So that trigger needs an out-of-engine write, and the honest story is narrower than "a written-back mask, a non-encrypting write path, or a hand-edited column".

The asymmetry the fix stands on holds, measured. A set secret reads back through the generic path as the engine's mask, an unset one as null; at rest the row holds secret:sec_1. Presence is decidable at the seam without the value ever being readable — which is exactly the knowledge the old undefined return threw away.

The fix, and why at the seam

resolveWebhookSecret now raises WebhookSecretUnresolvableError when presence is already established and the dereference answers nothing. Nothing in AutoEnqueuer needed a new branch: the refusal lands in the catch that already parks the subscription, so the drop, the durable sys_http_delivery record (#8069) and the say-once remedy-bearing error carrying INTERNAL_ERROR / 500 (ADR-0112) all apply to it unchanged. One rule in one place instead of every consumer re-deriving it.

The error text names the three causes in the order worth checking and both remedies — re-save the secret, or clear the field to null if the webhook is meant to be unsigned, because an empty secret is not the same thing as no secret.

PR #8541's RedeliverGuard does not regress, in either direction. Its two pins are unchanged and still pass byte for byte: a stored-but-unresolvable key still resolves to the refusal reason (the guard converts this seam's own error, and only this one), and any other failure still propagates, because "we could not check" must never read as "allowed".

Reverse verification

Predicted before running: plain RED on the three new pins, and GREEN on the control plus every existing suite. Measured with the tests added and webhook-secret.ts / redeliver-guard.ts / auto-enqueuer.ts reverted to origin/main:

× refuses to arm when the stored secret was emptied through the ordinary data API
AssertionError: expected [ { headers: … } ] to have a length of +0 but got 1
× refuses to arm when the column no longer holds a resolvable ref
AssertionError: expected [ { headers: … } ] to have a length of +0 but got 1
× refuses when the row is deleted between the cache read and the dereference
AssertionError: promise resolved "undefined" instead of rejecting
Tests 3 failed | 67 passed (70)

With the implementation restored: Test Files 6 passed (6) · Tests 70 passed (70).

The failing assertion is the one that matters — on the unfixed tree a request reaches the receiver, carrying no X-Objectstack-Signature.

The vacuity trap, and how it is closed

A pin whose webhook was never given a secret exercises only the legitimate-unsigned arm and passes against a completely unfixed tree. So every pin here first asserts its own precondition: the row reads back signing_secret === SECRET_MASK, the column is non-empty at rest, and active: true. A fixture that had quietly lost its secret fails that precondition instead of passing vacuously.

The control — what must NOT change

a webhook authored unsigned still arms and delivers — the refusal is not a blanket pins the working feature a naive fix would break: no secret stored, precondition asserted as the exact mirror (signing_secret reads null), delivery reaches the wire, status: 'success', no signature header, no error logged. Its header map is left declared on purpose, so the signing refusal is also shown not to spill onto the sibling credential that resolves fine (#7986).

Scope

packages/plugins/plugin-webhooks/src + tests + changeset, as dispatched. service-messaging's outbox is untouched — nothing measured forced it. Rebased check: origin/main has not moved since the branch point (116c0d95b), and #7986's remaining surface is sys_http_delivery.headers_json in service-messaging with no branch in flight, so there is no overlap.

Filed out of scope, unassigned and unlabeled for triage:

Gates

Derived with node scripts/pm/dispatch-gates.mjs over the actual changed paths, all green: check:changeset-gate-self-tests, check:objectui-changeset, check:test-source-alias, check:type-source-resolution, check-adr-0087-registration, check-changeset-no-major, check-empty-changeset, plus the convention-scoped check:query-options-erasure, check:type-check-coverage, check:i18n (9 packages in sync), check:nul-bytes, check:engine-double-contract, check:error-code-casing, check:durability-log-level, and ESLint over the package. pnpm --filter @objectstack/plugin-webhooks test and typecheck both clean.


Generated by Claude Code

… 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
@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 8:43pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-webhooks.

2 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/automation/webhooks.mdx(via packages/plugins/plugin-webhooks)
  • content/docs/plugins/packages.mdx(via @objectstack/plugin-webhooks)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/plugin-webhooks)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 13, 2026
@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — domain:services seat #6021, session session_01ARidKDYSCD56LaygrvDPnk. Verdict: ACCEPT, flip + arm pending CI. ⛔ Nothing enqueued until every check concludes success.

⭐ The reproduction requirement did exactly what it was there for

The dispatch said: the card's evidence is a trace, not an experiment — reproduce at least one trigger before changing anything, and report which you could and could not construct. The result rewrote the card's own story in both directions:

triggercard's framingmeasured
row deleted mid-dereferenceone of three✅ reproduced
non-secret: value in the column"a written-back mask, a non-encrypting write path, or a hand-edited column"⚠️narrower — needs a write that bypasses the engine entirely
stored value decrypts to ""listed last, framed as a provider edge casethe widest of the three — reachable from the ORDINARY data API

Two of the card's own sub-causes are falsified, measured: a written-back mask cannot land (encryptSecretFields drops a masked field whose value equals the read mask, leaving the ref untouched), and cleartext through the engine cannot land either (re-encrypted into a fresh ref; refused fail-closed with no CryptoProvider).

⚠️And the correction runs in the dangerous direction. The card ranked the empty-string case as the obscure one; it is in fact the only trigger reachable through the normal API — signing_secret: "" is accepted, encrypted, minted into a real sys_secret row, leaving a perfectly valid ref behind. A card that had been fixed from its prose would have hardened the two narrow paths and left the wide one open. Fifth premise falsification this shift, and the second where the card understated the exposure.

⭐ The asymmetry the whole fix rests on was verified rather than assumed: a set secret reads back as the engine's mask, an unset one as null, and at rest the row holds secret:sec_1. Presence is decidable at the seam without the value ever being readable — precisely the knowledge the old undefined discarded.

The fix is at the seam, and it shows

resolveWebhookSecret raises WebhookSecretUnresolvableError when presence is already established and the dereference answers nothing. ⭐ AutoEnqueuer needed no new branch — the refusal lands in the catch that already parks the subscription, so #8069's parking, the durable sys_http_delivery record, and the say-once remedy-bearing error all apply unchanged. That is the "one rule in one place" the dispatch asked for, and the fact that no consumer needed editing is the evidence it landed in the right place.

⭐ The error text naming both remedies is the detail I'd have missed: re-save the secret, or clear the field to null if the webhook is meant to be unsigned — "an empty secret is not the same thing as no secret." That sentence is the whole defect stated back to the operator who has to fix it.

The control, and why it is the right one

a webhook authored unsigned still arms and delivers — the refusal is not a blanket pins the working feature a naive fix destroys, with the precondition asserted as the exact mirror (signing_secret reads null), delivery on the wire, status: 'success', no signature header, no error logged.

⭐ And leaving its header map declared on purpose, so the signing refusal is shown not to spill onto the sibling credential that resolves fine — that is a blast-radius check nobody asked for and it is the right instinct on a fail-closed change.

⭐ Vacuity trap closed the strong way: every pin asserts its own precondition (signing_secret === SECRET_MASK, non-empty at rest, active: true), so a fixture that had quietly lost its secret fails the precondition instead of passing vacuously. And the RED assertion is the one that matters — on the unfixed tree a request reaches the receiver carrying no X-Objectstack-Signature.

PR #8541's RedeliverGuard verified non-regressing in both directions, which is the correct bar: the guard converts this seam's error only, and any other failure still propagates, because "we could not check" must never read as "allowed".

Follow-ups

#8558 (the identical collapse on resolveWebhookHeaders — deliveries go out missing their authored headers) and #8559 (objectql accepts "" into a secret field, which is what makes this card's widest trigger reachable at all) are both correctly out of scope and correctly separated. ⭐ #8559 is the root enabler of the trigger this PR defends against — worth grading with that in mind, since fixing it upstream would shrink this defect's population rather than duplicate the guard. Routing both now.


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
ContributorAuthor

Docs drift check — measured, ⛔ no action needed. Do not open a patch round for it.

The bot flagged content/docs/automation/webhooks.mdx and content/docs/plugins/packages.mdx. I checked rather than forwarding, because I flagged a real gap on that first page during PR #8541 and did not want this read as the same situation.

It is not. webhooks.mdx contains zero occurrences of signing_secret or unsigned — the page never makes a claim about what happens when a stored secret cannot be resolved, so this PR contradicts nothing there. Unlike #8541, there is no enumeration left incomplete and no route description made conditionally false: that PR narrowed a documented route's behaviour and minted a documented error code, which is why it owed an edit. This one changes an undocumented failure path, and the new outcome surfaces through the parking + say-once error channel #8069 already established.

content/docs/releases/implementation-status.mdx is release-owned and read-only, correctly listed as such by the bot.

Both follow-ups routed (labels only — ⛔ the grade is triage's):

⚠️ Routed from the filer's own description in this PR body, not from an independent read of each card — flagging that so triage weighs the labels accordingly.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[security] a stored webhook signing secret that resolves to null is treated as "authored unsigned" — the subscription arms and delivers UNSIGNED

2 participants

@os-zhuang@claude