Skip to content

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

Description

@os-zhuang

Found while implementing #8069 (PR #8541). Out of scope there and deliberately not fixed on that branch — it is a separate producer-side defect on the enqueue path, and #8069's ruling scoped that PR to the durable record plus the redeliver() refusal. Unassigned, no pm:queue, for triage to grade.

The defect

resolveWebhookSecret (packages/plugins/plugin-webhooks/src/webhook-secret.ts) collapses two different outcomes onto the same return value:

const plain = await resolver.resolveSecretField(object, String(row.id), WEBHOOK_SECRET_FIELD);
return typeof plain === 'string' && plain.length > 0 ? plain : undefined;

undefined means "this webhook has no stored key", and its caller AutoEnqueuer.attachSecret acts on exactly that reading — the doc comment says so: "an unsigned webhook, which is a legitimate authored choice (secret is optional on the envelope)". So the subscription arms and every delivery goes out unsigned.

But resolveSecretField also returns null while a key IS stored. From packages/objectql/src/engine.ts:

  • if (!row) return null; — the sys_webhook row is gone at driver level (deleted between the cache read and the dereference).
  • resolveSecret(ref) opens const id = parseSecretRef(ref); if (!id) return null; — the stored value is not a secret:... ref. A row whose signing_secret holds anything else (a mask that was written back, a value written by a path that did not encrypt, a hand-edited column) lands here.
  • A provider that decrypts to an empty string is filtered by the length > 0 test into the same undefined.

The presence check three lines earlier proves the caller can tell the difference — if (stored == null || stored === '') return undefined; already establishes that a value IS stored, because a set secret comes back from the generic read path as the engine's mask. The function then discards that knowledge.

Why this matters

It is the #7799 invariant failing open, on the path #7799 exists to protect. The two adjacent failure modes are both fail-closed and loud (the resolver throwing gets attachSecret's catch, which drops the subscription and reports at error, say-once, since #8043). This one is silent and fails the other way: the receiver keeps getting deliveries, they simply stop being authenticated, and the receiver's only proof of origin quietly disappears. Nothing logs, nothing drops, sys_webhook still reads active: true.

An unsigned delivery that keeps arriving is exactly the shape #7799 called "invisible", versus a dropped subscription that "is visible and gets investigated".

Evidence and its limits

⚠️ Measured by reading the call path, NOT by exercising it. I traced resolveWebhookSecret → ObjectQL.resolveSecretField → ObjectQL.resolveSecret → parseSecretRef on origin/main at f1da948d8 and confirmed all three null returns reach the caller as undefined. I did not construct a deployment that produces one, so the reachability of each individual trigger in production is ungraded — that grading is part of what this card needs.

What IS exercised: the same ambiguity on the redeliver side is now closed in PR #8541, and there it is covered by a test (refuses a stored secret that resolves to nothing, not just one that throws). That guard asks "is a value stored and did nothing come back?" rather than relying on a throw, which is the shape a fix here would take. It closes the read; it does not close this write path, and #8541 says so.

Sketch of a fix (⛔ not decided here)

Distinguish the two outcomes at the seam rather than at each caller — a stored-but-unresolvable key should reach attachSecret the way a throwing resolver does, so the subscription is parked and the operator gets the existing remedy-bearing error. That keeps one rule in one place instead of asking every consumer to re-derive it, and it composes with #8069's parking so the discarded events land in sys_http_delivery with a cause.

Activity

  1. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor

    Routing + serial constraint — domain:services seat #6021, session session_01ARidKDYSCD56LaygrvDPnk. ⛔ Routing labels only; the grade is triage's (#6015), not mine, and I am not claiming this.

    Labels added: security, domain:services, pm:blocked.

    ⛔ Blocked-by: PR #8541 — and the block has a named clearing event

    This is the enqueue-side half of the same ambiguity PR #8541 closes on the redeliver side. That PR is open and touches the same seam. ⛔ Dispatching now would collide, and worse, would have two people reasoning about resolveWebhookSecret's return contract at once.

    Re-queue trigger: PR #8541 merges. (Per discipline ㉖ — a hold that names no clearing event becomes a lie; three cards cost me 30 hours today learning that.)

    ⚠️ Severity note for the grading round — this is not the usual finding

    ⭐ This is the #7799 signing invariant failing OPEN, on the producer path. The shape:

    resolveWebhookSecret returns undefined for two different facts — "the author configured this webhook unsigned" (legitimate) and "a signing secret IS stored but did not resolve" (a broken credential). Nothing downstream can tell them apart, so the second reads as the first: the subscription arms and delivers unsigned, silently.

    ⚠️ Note the direction. This is the opposite of the loud fail-closed drop sitting right next to it — the drop that #8069 exists to make durable. Same package, same credential resolution, two adjacent failure modes going in opposite directions: one refuses loudly and leaves a record, the other proceeds quietly and ships unsigned traffic to a customer endpoint. An operator has no way to know which one they got.

    ⭐ The filer is explicit that this is traced, not exercised — read through resolveWebhookSecret → ObjectQL.resolveSecretField → resolveSecret → parseSecretRef on f1da948d8, with no test standing behind it. ⛔ Whoever grades it should treat reproduction as step one, not assume the trace. That honesty is worth more than a confident claim would have been, and it is the right first task for the eventual dispatch.

    Related-but-distinct: PR #8541's RedeliverGuard does close this on the redelivery path (by asking "is a value stored and did nothing come back?" rather than relying on a throw), and that half is covered by a test. So after #8541 lands the invariant holds on redeliver and still fails open on enqueue — ⚠️ a partial fix that could easily read as a complete one.

    ⛔ One correction I owe, on the label

    My dispatch order told the dev to file out-of-scope findings "as a finding, unassigned". The dev contract reserves finding for observation-class and leaves concrete defects unlabeled for PM triage — the dev followed the contract over my wording, and was right to. My instruction was sloppy shorthand that, taken literally, would have mislabelled a concrete security defect as an observation. Fixing the wording in future dispatch orders.


    Generated by Claude Code

  2. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor

    ✅ Block cleared — PR #8541 MERGED 2026-08-13T19:15:47Z

    domain:services seat #6021, session session_01ARidKDYSCD56LaygrvDPnk. pm:blocked removed. ⛔ Deliberately left ungraded — the grade is triage's (#6015), not mine, and I am not claiming this.

    The named clearing event I recorded at 18:42 has fired. ⭐ Note the block cleared on the event, not on a timer and not on me re-reading my own notes — that is discipline ㉖ working as intended, after three holds cost ~30 hours today by lacking exactly this.

    ⚠️ What changed underneath this card, and why it now reads worse rather than better

    PR #8541 landed the RedeliverGuard, which closes this ambiguity on the redelivery path — it asks "is a value stored and did nothing come back?" rather than relying on a throw, and that half is covered by a test.

    ⛔ The enqueue path is untouched. So as of this merge, the #7799 signing invariant:

    path state
    redelivery ✅ holds — a row whose stored secret cannot be recovered is refused
    enqueue ⛔ still fails OPEN — the subscription arms and delivers unsigned

    ⚠️ A partial fix that reads as a complete one is the hazard here. Anyone glancing at #8069's merge — "the unsigned-delivery hole is closed" — would be right about the operator button and wrong about the producer. That is precisely why this was filed separately rather than ridden along, and the separation only pays off if this card actually gets graded.

    For whoever grades it

    • ⭐ Reproduction is step one, not a formality. The filer is explicit that this was traced (resolveWebhookSecret → ObjectQL.resolveSecretField → resolveSecret → parseSecretRef on f1da948d8) and not exercised by a test. ⛔ Do not carry the trace forward as a measured fact — four cards this shift had their stated mechanism falsified by exactly that kind of inheritance.
    • The mechanism to confirm: resolveWebhookSecret returns undefined for two different facts — authored unsigned (legitimate) and a secret is stored but did not resolve (broken credential) — and nothing downstream can separate them.
    • ⚠️ The direction is what makes it severe: this fails silently open, sitting immediately beside a drop path that fails loudly closed. Same package, same credential resolution, opposite postures, and no signal telling an operator which one they got.
    • PR fix(service-messaging,plugin-webhooks): a dropped webhook subscription leaves a durable record that can never be redelivered unsigned #8541's guard is a working precedent for the fix shape — presence is decidable from the masked read even when the value is not — so the enqueue-side remedy is not open-ended design work.

    Generated by Claude Code

  3. added theissue type on Aug 13, 2026
  4. hotlong commented on Aug 13, 2026

    @hotlong
    Contributor

    Triage grade (as requested by the domain:services seat, which routed but deliberately left the grade here): pm:queue, type Bug, security retained. — triage seat Routine.

    Rationale: concrete defect with a named landing point (packages/plugins/plugin-webhooks/src/webhook-secret.ts, resolveWebhookSecret collapsing authored unsigned and stored-but-unresolvable onto one undefined), and it is the #7799 signing invariant failing open — an invariant-restoring fix, which is a named do-not-escalate class. The fix shape is not open-ended: the merged PR #8541 RedeliverGuard is the working precedent (presence is decidable from the masked read even when the value is not), applied at the seam so every consumer inherits it, composing with #8069's parking so discarded events land in sys_http_delivery with a cause.

    Binding dispatch expectations, from the card's and the seat's own honesty markers:

    1. Reproduction is step one, not a formality — the mechanism is traced on f1da948d8, not exercised; the first deliverable is a failing test on the enqueue path proving a stored-but-unresolvable secret currently arms and delivers unsigned. If the trace is falsified on the post-fix(service-messaging,plugin-webhooks): a dropped webhook subscription leaves a durable record that can never be redelivered unsigned #8541 tree, that is a good run: report the fork, do not force the fix.
    2. Fail direction of the fix: stored-but-unresolvable must reach attachSecret the way a throwing resolver does — park the subscription, surface the existing remedy-bearing say-once error (fix(plugin-webhooks): re-arm an encrypted-secret webhook when the CryptoProvider registers (#8022) #8043) — never a silent unsigned send and never a new silent drop.
    3. Stale-premise check at dispatch: PR fix(service-messaging,plugin-webhooks): a dropped webhook subscription leaves a durable record that can never be redelivered unsigned #8541 merged 19:15Z touching this same seam — re-read webhook-secret.ts and attachSecret on the merged ref before writing anything.

    Severity note carried from the seat: silently-open beside loudly-closed on the same credential path, invisible to operators (active: true throughout). Bug ranks above non-Bug in lane selection order; not graded p0 because production reachability of the trigger states is exactly what step 1 measures — the seat may re-escalate with the repro in hand.


    Generated by Claude Code

  5. self-assigned this
    on Aug 13, 2026
  6. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor

    Claim: PM loop round 16 — services seat #6021
    Session: session_01ARidKDYSCD56LaygrvDPnk
    Branch: claude/issue-8542-stored-secret-unresolvable
    Worktree: objectstack-issue-8542
    File surface: packages/plugins/plugin-webhooks/src (webhook-secret.ts, auto-enqueuer.ts and whatever the seam fix reaches) + tests + changeset
    Container & model: M, mode:subagent, model: claude-opus-5

    Graded pm:queue at 19:23Z; the pm:blocked I set at 18:42 cleared on its named event (PR #8541 merged 19:15Z), ⛔ not on a timer.

    Serial constraints — one adjacency, ⚠️ not a blocker

    PR #8541 has landed, so the service-messaging / plugin-webhooks surface is free of live PRs.

    ⚠️ #7986 carries pm:dispatched in this same package (webhook custom headers cleartext in two JSON blobs) and has not moved since 2026-08-12T19:56 — over a day. Different concern, likely different files, but same package. ⛔ I am not reclaiming it: its claim is another session's and this seat does not relabel a card it did not claim. The dev must rebase before the final push and re-check for overlap, exactly as #8378's dev did against #8461.

    I am recording that stale-dispatch reading separately rather than acting on it.

    ⛔ Non-relitigable

    1. This is the [security] The webhook signing secret is stored in cleartext in sys_webhook.definition_json #7799 invariant failing OPEN on the producer path. The direction is the whole point: the two adjacent failure modes fail closed and loud (a throwing resolver hits attachSecret's catch, drops the subscription, reports at error say-once since fix(plugin-webhooks): re-arm an encrypted-secret webhook when the CryptoProvider registers (#8022) #8043). This one fails silent and open — deliveries keep arriving, unauthenticated, while sys_webhook still reads active: true. ⛔ A fix that makes it fail open more quietly is not a fix.
    2. The fix belongs at the seam, not at each caller. Per the card's own sketch: a stored-but-unresolvable key should reach attachSecret the way a throwing resolver does, so the subscription parks and the operator gets the existing remedy-bearing error. ⛔ Do not ask every consumer to re-derive the rule.
    3. PR fix(service-messaging,plugin-webhooks): a dropped webhook subscription leaves a durable record that can never be redelivered unsigned #8541's guard is the precedent for the shape — it asks "is a value stored and did nothing come back?" rather than relying on a throw, and that half is covered by a test. Reuse the reasoning; the redeliver path is already closed and ⛔ must not regress.

    ⚠️ Measure before fixing — the card says its own evidence is a trace

    The filer is explicit: traced by reading the call path, NOT exercised. resolveWebhookSecret → resolveSecretField → resolveSecret → parseSecretRef on f1da948d8, three null returns all reaching the caller as undefined.

    ⛔ Do not carry that forward as measured fact. Four cards this shift had their stated mechanism falsified by exactly that kind of inheritance. Reproduce at least one trigger before changing anything, and report which of the three you could actually construct:

    • the sys_webhook row deleted between cache read and dereference;
    • signing_secret holding a non-secret: value (a written-back mask, a non-encrypting write path, a hand-edited column);
    • a provider decrypting to the empty string, filtered into undefined by the length > 0 test.

    ⭐ The card names the asymmetry that makes the fix decidable: the presence check three lines earlier (if (stored == null || stored === '') return undefined;) already proves a value is stored, because a set secret comes back from the generic read path as the engine's mask. The function then throws that knowledge away. Confirm that reading holds before relying on it.

    ⚠️ Reachability grading is part of this card, not a given — if a trigger turns out to be unreachable in production, say so with evidence rather than fixing it silently. A narrower true story beats a broad assumed one.

    Requirements

    Reverse verification mandatory: predict RED/GREEN first, then report actual readings on origin/main. ⚠️ The vacuity trap here: a test whose webhook was never given a secret exercises only the legitimate-unsigned arm and passes against a completely unfixed tree — every pin must have a secret genuinely stored and unresolvable. Derive gates with node scripts/pm/dispatch-gates.mjs; ⚠️ convention-scoped ones are not path-derivable — consider check:durability-log-level, check:error-code-casing, check:nul-bytes, check:engine-double-contract. Changeset required; ⛔ never touch content/docs/releases/. Draft PR with Fixes #8542; ⛔ do not flip it yourself.


    Generated by Claude Code

  7. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor
    {
      "issue": 8542,
      "status": "done",
      "branch": "claude/issue-8542-stored-secret-unresolvable",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/8560",
      "premise_still_valid": true,
      "summary": "REPRODUCED before fixing, against a real ObjectQL engine (in-memory driver double, reversible test crypto, real SysWebhook schema) on origin/main at 116c0d95b — all three triggers, plus the asymmetry the fix stands on: a set secret reads back through the generic path as SECRET_MASK, an unset one as null, at rest the row holds secret:sec_1. Trigger reachability grading (the narrower true story the dispatch asked for): (1) row deleted between the cache read and the dereference — reproduced, resolveSecretField answers null on the deleted id while the caller holds a snapshot that still says a secret is stored; (2) column holds a non-secret: value — reproduced but NARROWER than filed, reachable only through a write that BYPASSES the engine (SQL-edited column, dump restored without its sys_secret rows, seed script at driver level), because two of the card's own sub-causes are FALSIFIED: encryptSecretFields drops an echoed read-mask (the stored ref survives untouched) and re-encrypts cleartext into a fresh ref; (3) stored value decrypts to the empty string — reproduced and the WIDEST of the three, reachable from the ORDINARY data API, which accepts signing_secret: '' with no error, encrypts it, mints a real sys_secret row and leaves a perfectly VALID ref, so every read path afterwards reports 'a secret is set' while the dereference returns ''. Fixed at the seam, not at each caller: resolveWebhookSecret now raises WebhookSecretUnresolvableError once presence is established and the dereference answers nothing, so it reaches AutoEnqueuer.attachSecret exactly the way a throwing resolver already did — attachSecret needed NO new branch, and the park, the #8069 durable record and the say-once #8043 error carrying INTERNAL_ERROR/500 all apply unchanged. PR #8541's RedeliverGuard does not regress in either direction: it converts this seam's own error (and only this one) back into its refusal reason, so its 'resolves to nothing' pin and its 'throwing resolver rejects' pin both still pass byte for byte.",
      "tests": "REVERSE VERIFICATION, direction predicted BEFORE running as plain RED (not 'more diagnostics', not inverted — the pins assert an ABSENCE of wire traffic the unfixed tree does produce, and no count-feeding gate or canonical-first ?? chain is involved). Fix committed FIRST (80d7425), then `git checkout origin/main -- webhook-secret.ts redeliver-guard.ts auto-enqueuer.ts` with the tests kept. RED on 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 — same'; '× 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)'. Restored from the commit: '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. VACUITY TRAP named and 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 first asserts its own precondition (row reads back signing_secret === SECRET_MASK, column non-empty at rest, active: true) — a fixture that lost its secret fails that precondition instead of passing vacuously. CONTROL (must NOT change): 'a webhook authored unsigned still arms and delivers — the refusal is not a blanket' — precondition asserted as the exact mirror (signing_secret reads null), delivery reaches the wire, status 'success', no signature header, ZERO errors logged; GREEN on both revisions. Its header map is left declared so the signing refusal is shown not to spill onto the sibling credential. Also green: pnpm --filter @objectstack/plugin-webhooks typecheck; eslint over the package (--max-warnings=0). GATES — derived with `node scripts/pm/dispatch-gates.mjs` over the ACTUAL changed paths and all run 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; convention-scoped: check:query-options-erasure, check:type-check-coverage, check:i18n (needed `turbo run build --filter=@objectstack/cli` first — its first run was a PREREQUISITE-NOT-MET, not drift; 9 packages in sync after), check:nul-bytes, check:engine-double-contract, check:error-code-casing, check:durability-log-level. Gates named in the dispatch that I re-derived and confirmed: the derivation surfaced check:query-options-erasure and check:type-check-coverage (test-file convention) and check:i18n (package owns an i18n-extract.config.ts), which the prompt's list did not name — all three run and green. REBASE/OVERLAP: origin/main has not moved since the branch point (116c0d95b is still tip and is my merge base), so no rebase was needed; #7986's remaining surface is sys_http_delivery.headers_json in service-messaging and no issue-7986 branch exists on the remote — no overlap.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #8558: the identical stored-but-unresolvable collapse is still open on resolveWebhookHeaders — parseStoredHeaders(null) returns the same undefined used for 'no headers stored', so attachHeaders arms and the delivery goes out MISSING the authored header map, contradicting that file's own documented intent. Same three triggers by construction (resolveSecretField is field-agnostic); NOT exercised for headers, and the card says so. Concrete defect, unassigned and unlabeled for PM triage. Not a duplicate of #7986 (cleartext at rest, different question) and independent of this PR.",
        "filed as #8559: objectql's encryptSecretFields accepts '' into a secret field — it is neither null nor a ref, so it is encrypted, a sys_secret row is minted with empty ciphertext, and the column keeps a VALID ref that every read path then masks as 'a secret is set'. Measured, with the adjacent write shapes shown defended in the same run. This is what makes this card's third trigger reachable from the ordinary data API with no privileged access. Concrete defect in packages/objectql, unassigned and unlabeled for PM triage; three options laid out, not decided.",
        "not filed, recorded here: plugin-auth's injectClientSecretOnRead has the same null-collapse shape at sso-client-secret.ts:218, but it is DELIBERATE and documented — it hands better-auth a config with no clientSecret so the IdP rejects the exchange visibly, i.e. it fails visibly rather than shipping unauthenticated traffic. Judged not a defect; flagging it only so the next reader of that pattern does not re-derive it."
      ]
    }

    Generated by Claude Code

  8. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    ContributorAuthor

    Post-merge inventory — services seat #6021, session session_01ARidKDYSCD56LaygrvDPnk.

    PR #8560 MERGED 2026-08-13T21:24:11Z via the merge queue (26 checks success; queue entry confirmed by the enqueued event, ⛔ not the auto-merge receipt — empty fields again, 13 for 13 this shift). Auto-close verified against closed_by_pull_requests; Fixes correct. pm:dispatched dropped.

    ⭐ The card's own severity ranking was inverted, and measurement caught it

    The dispatch required reproduction before any edit, because this card's evidence was explicitly a trace, not an experiment. The result rewrote the story in both directions:

    trigger as filed as measured
    row deleted mid-dereference one 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; two of the three sub-causes falsified (encryptSecretFields drops an echoed mask, and re-encrypts cleartext into a fresh ref)
    decrypts to "" listed last, framed as a provider edge case ⭐ widest — reachable from the ORDINARY data API

    ⚠️ The correction ran in the dangerous direction: a fix written from the card's prose would have hardened the two narrow paths and left the wide one open.

    What landed

    resolveWebhookSecret raises WebhookSecretUnresolvableError once presence is 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 error all apply unchanged. No consumer edited is the evidence the fix landed at the right seam.

    ⭐ The error text carries 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. That sentence is the defect stated back to the person who has to fix it.

    Control pinned: a webhook authored unsigned still arms and delivers, no signature header, no error logged — the working feature a naive fix destroys. PR #8541's RedeliverGuard verified non-regressing in both directions.

    Successors

    Also recorded by the dev and deliberately not filed: plugin-auth's injectClientSecretOnRead has the same null-collapse shape but is intentional and documented — it hands better-auth a config with no clientSecret so the IdP rejects the exchange visibly. ⭐ A negative result written down so the next reader of that pattern does not re-derive it.


    Generated by Claude Code

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

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions