Skip to content

[security] Webhook HMAC signing secrets are persisted in cleartext on every sys_http_delivery row #7722

Closed

Description

@huangyiirene

Impact

The shared secret that authenticates ObjectStack to a webhook receiver is stored verbatim, per delivery attempt, in a table readable over the ordinary data API. Anyone who can read sys_http_delivery recovers the signing key for every subscriber and can mint payloads the receiver accepts as genuine — the signature is the receiver's only proof of origin.

Admin-gated on this deployment, so this is defense-in-depth rather than privilege escalation. But the blast radius is external: it compromises the trust boundary at systems ObjectStack does not control, and rotating a leaked key means re-coordinating with every receiver operator.

Root cause (located)

sys_http_delivery carries the secret as a plain column alongside headers_json; the outbox writer persists it with the attempt rather than resolving it at send time.

The platform already has the machinery to avoid this and uses it elsewhere: sys_setting stores a sec_ handle with the ciphertext in sys_secret (aes-256-gcm). And the same QA run proved the sibling datasource path gets it right — an inline secret on POST /api/v1/datasources is split out server-side, and a byte-level scan of every table and column of the running DB found no occurrence of it.

So this is an inconsistency between two sibling subsystems, not a missing capability — which is what makes it tractable.

Reproduction

  1. Author a webhook subscriber with a signing secret and trigger a delivery.
  2. Read the delivery row over the ordinary data API as an admin: GET /api/v1/data/sys_http_delivery.
  3. The authored secret comes back verbatim on the attempt row.

Found as an adjacent observation while verifying HMAC correctness: the runner recomputed HMAC-SHA256(raw body, <secret>) against the delivered x-objectstack-signature — exact match, so signing itself is correct — and then noticed the secret was readable from the delivery table.

Acceptance criteria

  • The signing secret is not recoverable from sys_http_delivery (or any delivery/outbox row) over the data API, for any persona.
  • Signing still works end-to-end: a delivered payload's x-objectstack-signature still verifies against the subscriber's secret, asserted by recomputing the HMAC over the raw body — not by asserting "no error".
  • A regression guard reads the delivery table directly after a real delivery and asserts the secret's bytes are absent (the byte-level-scan shape the datasource path already passes).

Suggested shape (not prescriptive)

Store a handle rather than the value on the delivery row and resolve it at send time, matching the sys_setting / sys_secret split. Failing that, at minimum keep it off the delivery record entirely — it belongs to the subscriber, not to each attempt — and exclude the column from the data-API projection.

Worth checking in the same pass whether any other outbox/delivery table copies a credential the same way.

Note on target:<major>

Not applied here — that label has a single producer and the maintainer has not ruled on this card. Flagging the tension for that ruling rather than pre-empting it: this is admin-gated (the #7522 precedent for an admin-gated cleartext read was not boarded), but unlike #7522 its blast radius reaches outside the deployment, which is an argument for boarding it. Maintainer's call.

Provenance & disclosure

Held back from the public run card #7690 pending disclosure handling (D1 precedent); the private write-up is docs/qa/platform-checklist/FOLLOW-UPS.md §1a entry D12 (branch claude/platform-test-checklist-ocwugl @ 1ce13eb). Filed publicly under the maintainer's standing instruction of 2026-08-11 (verbatim): 「有问题就立卡片」.

Source

QA run #7690 (webhook-lifecycle, adjacent observation; framework 92f26f75), private write-up FOLLOW-UPS.md §1a D12. The webhook item itself passes — signature computation, custom headers, per-attempt durability, timeout enforcement and the retry ladder were all proven correct; this finding does not change that verdict.

Activity

  1. added
    bugSomething isn't working
    pm:queueReady for the PM dispatch loop
    domain:servicesRetired 2026-10-22, alias of domain:server (services surface); in-flight cards keep it until landed
    pm:dispatchedDispatched to a dev agent by /pm-dispatch
    and removed
    pm:queueReady for the PM dispatch loop
    on Aug 11, 2026
  2. self-assigned this
    on Aug 11, 2026
  3. huangyiirene commented on Aug 11, 2026

    @huangyiirene
    CollaboratorAuthor

    Claim: PM loop round 1
    Session: session_01JVfWSQN9RDDGwdVSmMc29x
    Branch: claude/issue-7722-webhook-secret-cleartext
    Worktree: cloud session's own clone (mode:cloud)
    Domain: domain:services
    File surface: packages/plugins/plugin-webhooks/src/** (auto-enqueuer.ts carries signingSecret onto the enqueue at ~:387/:483; webhook-outbox-plugin.ts owns sys_http_delivery; sys-webhook.object.ts / schema.ts for the column/projection), packages/services/service-messaging/src/http-dispatcher.ts (send-time HMAC). Read-only consumption of the existing sys_secret/sec_ handle machinery expected; if the fix requires changing the secret store itself, stop on breach and report.
    Container & model: M (security, cross-file redesign of where the secret lives), mode:cloud, model: opus
    Serial constraints cleared: none — lane in-flight was 0 at claim; no open PR touches plugin-webhooks or service-messaging (same-day churn on both paths: version-packages rc only).


    Generated by Claude Code

  4. huangyiirene commented on Aug 11, 2026

    @huangyiirene
    CollaboratorAuthor
    {
      "issue": 7722,
      "premise_still_valid": true,
      "premise_evidence": "origin/main@7a8476f: packages/services/service-messaging/src/objects/http-delivery.object.ts:126 declared `signing_secret: Field.text({ label: 'HMAC Secret', maxLength: 256 })`; sql-http-outbox.ts:109 wrote `signing_secret: input.signingSecret` verbatim on every enqueue; the object declares `enable.apiMethods: ['get','list']`, so the column is readable over the ordinary data API. Both producers fed it cleartext — plugin-webhooks/src/auto-enqueuer.ts:387 and :483 (`signingSecret: sub.secret`) and service-automation/src/builtin/http-nodes.ts:133 (Flow `http` node).",
      "status": "draft_pr_open",
      "branch": "claude/issue-7722-webhook-secret-cleartext",
      "pr": 7798,
      "pr_url": "https://github.com/objectstack-ai/objectstack/pull/7798",
      "commit": "500944c",
      "shape_chosen": "Delivery rows carry the SIGNATURE, never the key. The body is fixed at enqueue and replayed byte-for-byte by retries and redeliver(), so the HMAC has exactly one correct value for the row's life: the outbox computes it once at enqueue from EnqueueHttpInput.signingSecret and persists only `signature` (`sha256=<hex>`), the same value the receiver is handed on the wire and one-way in the key. The secret is consumed and dropped.",
      "shape_rejected": {
        "handle_resolved_at_send_time": "The issue's first suggestion (sys_setting/sys_secret split, datasource-binder shape). Rejected on evidence: the datasource binder exists because a live connection pool must recover CLEARTEXT at runtime; a delivery never needs the key back, only one HMAC over one fixed body. A handle would still have minted a sys_secret row per delivery attempt — per-attempt ciphertext on a telemetry table with 30d retention and no reaper for sys_secret — plus a decrypt per retry, to reach the same end state. It also could not be wired without a composition-root change (packages/cli/serve.ts owns crypto-provider injection), which is outside the declared surface."
      },
      "surface_touched": {
        "declared": ["packages/plugins/plugin-webhooks", "packages/services/service-messaging"],
        "outside_declared": ["content/docs/automation/webhooks.mdx", ".changeset/webhook-delivery-signature-not-secret.md"],
        "outside_declared_justification": "The mdx documents the renamed column (§3.2 table) and the signing model (§6) — leaving it would ship a doc naming a column that no longer exists. The changeset is a required local gate. No change to the secret store, packages/spec, packages/objectql, or any producer package.",
        "files": [
          "packages/services/service-messaging/src/http-sender.ts",
          "packages/services/service-messaging/src/http-outbox.ts",
          "packages/services/service-messaging/src/sql-http-outbox.ts",
          "packages/services/service-messaging/src/memory-http-outbox.ts",
          "packages/services/service-messaging/src/objects/http-delivery.object.ts",
          "packages/services/service-messaging/src/index.ts",
          "packages/services/service-messaging/src/translations/{en,es-ES,ja-JP,zh-CN}.objects.generated.ts",
          "packages/services/service-messaging/package.json (devDeps: objectql, driver-sql — for the at-rest integration test)",
          "packages/services/service-messaging/src/http-outbox.test.ts",
          "packages/services/service-messaging/src/http-signature-at-rest.integration.test.ts (new)",
          "packages/plugins/plugin-webhooks/src/webhook-signing-secret.test.ts (new)",
          "content/docs/automation/webhooks.mdx",
          ".changeset/webhook-delivery-signature-not-secret.md"
        ],
        "no_change_needed": "packages/plugins/plugin-webhooks/src/auto-enqueuer.ts — it hands `signingSecret` to the outbox, which is still the correct producer contract; what changed is that the outbox consumes rather than copies it."
      },
      "acceptance_evidence": {
        "byte_scan_after_real_delivery": "http-signature-at-rest.integration.test.ts — real ObjectQL + @objectstack/driver-sql on better-sqlite3 :memory:, real DDL via syncSchemas(), production SqlHttpOutbox + HttpDispatcher. After a real delivery it runs `SELECT *` over every column of every delivery row (not a projection of declared fields, so a stale physical column could not hide) and asserts the secret's bytes are absent. The scan is itself guarded — it must also see the URL and the signature — so an empty dump can never be what makes it pass. Cases: webhook, flow, redelivery, unsigned.",
        "hmac_recomputed_over_raw_body": "Same file plus plugin-webhooks/src/webhook-signing-secret.test.ts (full chain from a data.record.created event through the real AutoEnqueuer). Each asserts `X-Objectstack-Signature === 'sha256=' + HMAC-SHA256(raw body received, secret)`, and that the signed body is the real event rather than an empty one an HMAC would also match. Also upgraded the pre-existing http-outbox.test.ts case from `toMatch(/^sha256=/)` to the same recompute.",
        "round_trip_pin": "The signature is computed BEFORE the row is written and the body is rebuilt AFTER a full stringify -> text column -> parse -> stringify round-trip, so the recompute assertion doubles as the guard that a changed byte breaks verification instead of shipping an unverifiable delivery. Payload fixture carries nesting, unicode, a float, an empty string.",
        "reverse_verification": "Reinstating the pre-fix column + `signing_secret: input.signingSecret` write fails 3 of the 4 at-rest cases, each on the byte-scan assertion (`AssertionError: expected '…' not to contain 'whsec_7722_do_not_persist_me'`). The 4th (unsigned delivery) legitimately passes either way. Temporary edits reverted before commit; `git grep signing_secret` over src is clean apart from one explanatory comment."
      },
      "gates": {
        "service_messaging_suite": "pass — 18 files / 206 tests",
        "plugin_webhooks_suite": "pass — 4 files / 35 tests",
        "typecheck_dependents_closure": "pass — turbo typecheck --filter=...@objectstack/service-messaging, 77 tasks",
        "build_closure": "pass — turbo build over the dependents closure of both packages, 66 tasks",
        "check_docs_audit_scope": "pass",
        "changeset": "present — patch × service-messaging, plugin-webhooks",
        "ci": "in_progress — not polled (PM owns CI convergence, ready-flip and landing)"
      },
      "migration_note": "The `signing_secret` column is no longer declared, so an existing database keeps it as an unmapped column still holding the old cleartext until dropped. The platform's own path covers this: `os migrate plan` reports the `drop_column` op (schema-drift `unmapped_column`, category `destructive`, so never applied unattended). Those rows also age out on the table's existing 30-day telemetry retention. Documented in the changeset and PR body, with an explicit 'rotate any exposed secret' instruction.",
      "other_outbox_credential_sweep": {
        "result": "none found",
        "method": "grep over every *.object.ts for a field named *secret*/*password*/*api_key*/*credential*/*signing* on objects matching outbox|delivery|queue|job — zero hits. sys_notification_delivery / receipt / inbox_message carry no credential column. The identity tables (sys_account, sys_session, sys_oauth_*) hold credentials by definition and are the auth subsystem's own channel, not outbox rows."
      },
      "out_of_scope_findings_filed": [
        {
          "issue": 7799,
          "url": "https://github.com/objectstack-ai/objectstack/issues/7799",
          "title": "[security] The webhook signing secret is stored in cleartext in `sys_webhook.definition_json`",
          "assignee": "unassigned",
          "why_not_here": "Not a delivery/outbox row, so outside #7722's acceptance criteria, and the fix cannot use this PR's trick — the subscriber row holds the secret precisely because FUTURE deliveries must be signed with it, so it needs a real sys_secret binding plus the schema, seeder, Studio form and row migration that implies. Folding it in would have breached the declared surface and the no-expansion instruction."
        }
      ],
      "open_questions": [
        {
          "q": "Should `signature` be exposed over the data API at all?",
          "decided": "Yes, left readable. It is a one-way function of (body, secret) and is transmitted to the receiver in a header on every delivery, so a reader learns nothing they could not observe on the wire — while it stays genuinely useful for debugging 'was this delivery signed, and with what'. Excluding it would have cost the byte-scan test its 'guard the guard' anchor for no security gain."
        },
        {
          "q": "Rotation semantics for rows already enqueued.",
          "decided": "A row keeps the signature matching the body it carries; rotating a subscriber's secret changes what SUBSEQUENT deliveries are signed with. This preserves the outbox's existing byte-for-byte replay contract (redeliver() already promised URL/payload untouched) and is documented in webhooks.mdx §6. A resolve-at-send-time design would have silently changed this — pending rows would start signing with the new key."
        },
        {
          "q": "devDeps added to service-messaging (@objectstack/objectql, @objectstack/driver-sql).",
          "decided": "Accepted — dev-only, and the card's evidence bar ('reads the delivery table directly after a REAL delivery') is not reachable with the package's existing recording-fake engines. Precedent: service-automation, plugin-approvals, plugin-auth, plugin-reports all devDepend on the same pair for integration tests."
        }
      ],
      "blocked": false
    }

    Generated by Claude Code

  5. added a commit that references this issue on Aug 11, 2026
  6. huangyiirene commented on Aug 11, 2026

    @huangyiirene
    CollaboratorAuthor

    Addendum — patch round 1 (PR #7798, branch claude/issue-7722-webhook-secret-cleartext)

    {
      "issue": 7722,
      "pr": 7798,
      "patch_commit": "25e08c7",
      "scope": "The reported ESLint-job failure only — nothing else on the PR was touched.",
      "problem": "check:engine-double-contract, 2 pinned problems, both at packages/plugins/plugin-webhooks/src/webhook-signing-secret.test.ts:56 — the fake engine's delete() and update() accepted every call shape, including the ones ObjectQL refuses (#4434's failure mode).",
      "fix": "Both verbs now open with the producer's own predicates — `assertEngineDeleteDispatch(options)` and `assertEngineUpdateDispatch(data, options)` — imported from '@objectstack/metadata-core' (the predicate's home since #5619, and the right side of the edge here), added as a devDependency of plugin-webhooks, which lacked it. The direct pin applied cleanly; no baseline entry was needed, so scripts/engine-double-contract.baseline.json is untouched.",
      "reproduced_locally_before_fix": "node scripts/check-engine-double-contract.mjs → 'check-engine-double-contract: 2 problem(s).', both naming this file at line 56 — same two the CI job reported.",
      "gates_rerun": {
        "check_engine_double_contract": "pass — 'OK — 152 pinned, 133 in the DEBT ledger, 2 exempt' (was 2 problems, both this file)",
        "plugin_webhooks_suite": "pass — 4 files / 35 tests",
        "eslint_changed_file": "pass — `eslint packages/plugins/plugin-webhooks/src/webhook-signing-secret.test.ts --no-inline-config`, exit 0"
      },
      "unchanged": "Fix semantics, the at-rest byte scan, and the HMAC-recompute assertions are as reported in the first report — the pinned verbs are inert members this test never calls; pinning them changes what the double would REFUSE, not what the test exercises.",
      "ci": "not polled — PM owns CI convergence, the ready-flip and landing."
    }

    Generated by Claude Code

  7. huangyiirene commented on Aug 11, 2026

    @huangyiirene
    CollaboratorAuthor

    ACCEPT — PR #7798 (services seat review, session session_01JVfWSQN9RDDGwdVSmMc29x).

    What shipped: delivery rows carry the signature, never the key — the outbox computes sha256=<hex> once at enqueue (the body is fixed for the row's life and replayed byte-for-byte) and drops the secret; http-sender owns both halves of the signing contract so signer and transport cannot drift. Both producers (webhook fan-out AND the Flow http node — the report found the second producer the card didn't name) stop persisting cleartext with zero call-site changes.

    Review notes:

    • The dev rejected the issue's suggested handle-shape with evidence and I endorse the rejection: a delivery never needs the key back (one HMAC over one fixed body), a handle would have minted per-attempt sys_secret ciphertext rows on a 30-day telemetry table with no reaper, and wiring it needed a composition-root change outside the declared surface. This is the premise-first discipline working — the issue's own second option ("keep it off the delivery record entirely") was the stronger shape.
    • Acceptance criteria met with the exact evidence the card demanded: byte-scan SELECT * over every physical column after a REAL delivery (guard-the-guard anchored — the scan must also see URL + signature), HMAC recomputed over the raw received body (not "no error"), reverse verification 3/4 red on reinstated pre-fix (4th = unsigned, legitimately bidirectional).
    • Declared-surface additions, accepted with declaration: webhooks.mdx (documents this PR's own renamed column — leaving it would ship a doc naming a dead column), regenerated locale files (产物随源走), dev-only deps for the integration test (precedented in four sibling packages).
    • Patch round (1): the new webhook-signing-secret.test.ts engine double missed the assertEngineDelete/UpdateDispatch pins (check:engine-double-contract, ESLint job) — my dispatch didn't name that gate family, so the round is on the PM's ledger, not the dev's. Fixed exactly as prescribed on the same claim; all 27 checks now green.
    • Rotation semantics decision (enqueued rows keep the signature matching their body; rotation affects subsequent deliveries) preserves the outbox's documented byte-for-byte replay contract — endorsed, documented in §6.
    • Out-of-scope finding [security] The webhook signing secret is stored in cleartext in sys_webhook.definition_json #7799 (subscriber's secret in sys_webhook.definition_json — needs a real sys_secret binding, different fix class) correctly filed separately; awaiting triage.
    • Migration posture: unmapped column until os migrate plan's destructive drop_column is applied by hand; 30-day retention ages the rows; "rotate any exposed secret" instruction in changeset + PR body.

    Marking ready and arming auto-merge now.


    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

Labels

bugSomething isn't workingdomain:servicesRetired 2026-10-22, alias of domain:server (services surface); in-flight cards keep it until landedpm:dispatchedDispatched to a dev agent by /pm-dispatchsecurity

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions