Skip to content

feat(webhooks): sign deliveries under web-bot-auth when a registration asks - #1860

Merged
bokelley merged 4 commits into
nextfrom
claude/issue-1859-webhook-wba
Nov 8, 2026
Merged

bokelley merged 4 commits into
nextfrom
claude/issue-1859-webhook-wba

Conversation

@bokelley

@bokelley bokelley commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Closes #1859.

At AdCP 3.3 a webhook registration can omit authentication and name a signing_profile. WebhookSender could not produce web-bot-auth, so a seller built on the SDK, including its task-webhook outbox, could not honor a buyer registered for it. This PR adds the sender, carries each registration's profile through every outbox to every attempt, and refuses profiles the seller does not advertise.

Sender

  • WebhookSender.from_wba(private_key, *, agent_origin) signs every attempt afresh with sign_wba_request: a new 64-byte nonce and created, the key's thumbprint as keyid, and Signature-Agent naming the origin. Ed25519 only.
  • sender.with_web_bot_auth(key, agent_origin=...) adds web-bot-auth to an adcp/webhook-signing/v1 sender. Each delivery is signed under the profile its registration selected, and resend keeps the original's profile.
  • New signing_profiles / signing_algorithms properties.
  • sender.sign_web_bot_auth_directory(other_keys=[...]) signs the origin's key directory. The directory belongs to the origin, so it also lists request-signing and rotating keys.
  • challenge_webhook_destination(..., signing_profile=) signs proof-of-control under the config's profile.
  • Signature-Agent can no longer be set through extra_headers, for either profile.

Outboxes

  • PgTaskWebhookOutbox captures push_notification_config.signing_profile at issue, stores it in the encrypted registration and delivery row, and signs every attempt under it. Rows with no stored profile require adcp/webhook-signing/v1. A registration naming the default is stored exactly like one naming none.
  • PgNotificationOutbox persists required_signing_profile, which was previously dropped. A non-default profile uses envelope version 2.
  • Reporting notifications add an optional ReportingNotificationSubscription.signing_profile. When unset it's left out of the fingerprint, so existing subscriptions and queued deliveries are byte-identical.
    • ReportingSigningMaterial(..., web_bot_auth=ReportingWebBotAuthKey(key, origin)) supplies the key, and the worker builds a from_wba sender for each attempt.
    • ReportingProductionSigning(..., web_bot_auth=True) opts production reporting in and advertises both profiles.
    • Without that flag, a web-bot-auth subscriber is skipped with a warning rather than blocking the account's other subscribers.

Validation

  • Before issuing a task, dispatch refuses with INVALID_REQUEST (field push_notification_config.signing_profile) a push registration requiring a profile outside the request's effective webhook_signing.profiles. That set comes from the capabilities resolved for the request, including a tenant's get_adcp_capabilities_for_request override. The check applies to every publisher: fixed-sender, sender_resolver and externally managed outboxes. A fixed sender is also checked for a key under the profile.
    • Implementation: _handoff_webhook_kwargs → _invoke_platform_method → _project_invocation_result → _project_handoff / _project_workflow_handoff, as an optional webhook_signing_profiles keyword.
  • At boot, validation requires a key for every advertised profile (omitted profiles means adcp/webhook-signing/v1) and every sender algorithm to be advertised. Both outboxes' resolver checks require every algorithm a resolved sender signs with.

Known gap: rolling deploys

Delivery rows stored with web-bot-auth are quarantined by pods running the previous SDK rather than signed under the wrong profile. That holds for the task outbox, the notification outbox (envelope v2) and the reporting worker.

One case is not covered: a task registered on an upgraded pod and completed on a pod still on the previous SDK is signed under adcp/webhook-signing/v1. The old pod ignores the registration's profile. I found no stored format that makes an old pod refuse without also breaking its task completion. docs/webhooks/web-bot-auth.md says to finish the rollout before listing web-bot-auth in webhook_signing.profiles.

Tests

  • New: tests/test_webhook_sender_wba.py and tests/test_reporting_webhook_wba.py.
    • Each WBA attempt is verified end to end with verify_wba_request against the published directory, sharing one replay store (an attempt answered 503 followed by its retry).
    • Also covered: dual-profile selection, the envelope round trips, and the resolver-mode advertised-profile check.
  • Added to tests/test_decisioning_webhook_emit.py: a tenant-scoped capabilities override admits a profile the static capabilities lack, and other tenants are refused with no task issued.
  • Added to tests/test_webhook_challenge.py: the challenge is signed under the config's profile.
  • Updated tests/test_task_webhook_outbox_pg.py for the new registration field.
  • Locally, make lint and make typecheck-all pass. Full suite, last run: 21,774 passed, 1 failed.

Timing-sensitive failures in parallel runs. Across three full runs under pytest -n auto, a different test failed each time:

  • test_branch_parity_absorbs_registry_variance (timing budget)
  • test_real_mcp_dispatch_reports_the_exact_pinned_wire_key (event timing)

Both pass when run alone. The second also fails on unmodified next when its file runs in parallel. Neither is in code this PR touches.

Also in this PR

task_webhook_outbox.py gets # nosec B608 / # nosec B311 annotations on existing lines. The pre-commit bandit hook reports them for any change to that file, and the file fails it identically on next. The SQL interpolates a table name validated against _SAFE_IDENTIFIER_RE, and random is only used for retry jitter.

Docs: new docs/webhooks/web-bot-auth.md; updated reporting-production.md, reporting-operation-id-3.2.md and the 3.3 inventory.

🤖 Generated with Claude Code

…n asks

AdCP 3.3 lets a webhook registration omit authentication and name a
signing_profile. WebhookSender could not produce web-bot-auth, so a seller
built on the SDK, including its task-webhook outbox, could not honor a
buyer registered for it.

Sender:
- WebhookSender.from_wba(private_key, agent_origin=...) signs each attempt
  afresh with sign_wba_request (new 64-byte nonce and created, thumbprint
  keyid, Signature-Agent naming the origin).
- with_web_bot_auth() adds web-bot-auth to an adcp/webhook-signing/v1
  sender; each delivery is signed under its registration's profile, and
  resend keeps the original's profile.
- signing_profiles / signing_algorithms; Signature-Agent is reserved from
  extra_headers.
- sign_web_bot_auth_directory() signs the origin's key directory.
- challenge_webhook_destination signs under the config's signing_profile.

Outboxes:
- PgTaskWebhookOutbox stores push_notification_config.signing_profile with
  the encrypted registration and delivery and signs every attempt under it.
- PgNotificationOutbox persists required_signing_profile (envelope v2 only
  for non-default profiles, so earlier pods quarantine instead of
  mis-signing).
- The reporting notification worker signs web-bot-auth subscriptions with
  ReportingWebBotAuthKey; ReportingProductionSigning(web_bot_auth=True)
  advertises both profiles. Unset profiles keep fingerprints byte-stable.

Validation:
- Dispatch refuses, with INVALID_REQUEST before the task is issued, a push
  registration requiring a profile outside the request's effective
  webhook_signing.profiles (tenant-scoped overrides included), for every
  publisher.
- Boot validation requires a key for every advertised profile and every
  sender algorithm to be advertised.

Also annotates the pre-existing bandit B608/B311 findings in
task_webhook_outbox.py (validated table identifier; retry jitter), which
the pre-commit hook reports for any change to that file.

Closes #1859

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — additive Web Bot Auth webhook signing feature (AdCP 3.3, #1859).

What I checked:

  • Semver signal: feat(webhooks): is correct. New optional signing_profile, new web-bot-auth enum value, and new public exports (WEBHOOK_SIGNING_PROFILES, ReportingWebBotAuthKey, from_wba, with_web_bot_auth) are all additive — no removed/narrowed public surface, so no major-bump requirement is triggered.
  • Wire-shape forward-compat: task-outbox stores the ADCP default as a bare body (byte-identical to pre-profile rows); non-default profiles go in an encrypted/v2 envelope that an older pod quarantines rather than mis-signs. The _open_delivery_body XOR and notification _open version/profile checks round-trip old envelopes unchanged; reporting subscription fingerprints preserved when signing_profile is unset.
  • Rolling-deploy gap (task registered on new pod, completed on old) is documented and mitigated by not advertising the profile until rollout completes.
  • Test coverage is thorough, including end-to-end verify_wba_request against the published directory.

Decision trace: No critical/high findings (row 1 n/a). gated_paths false (row 2 n/a). high_risk is true but all reasons are (modified) with only ONE medium finding — row 5 requires a medium on a modified high-risk file; the single medium is in notifications.py (reporting/production, not a high-risk path), so row 5 does not fire. The medium is not data-loss/schema/infra (row 4 n/a). No deletions (row 3 n/a). No prior decision (row 6 n/a). No team gate (row 7 n/a). Only 1 medium finding, fewer than 3 (row 8 n/a). Falls through to row 9 → approve.

Medium findings (non-blocking):

  • src/adcp/reporting/production/notifications.py:133 — web-bot-auth reporting subscription skipped with warn-only, no alerting path.

Medium findings

  • src/adcp/reporting/production/notifications.py:133 — web-bot-auth reporting subscription skipped with warn-only, no alerting path

…the outboxes

The outboxes read the new WebhookSender.signing_algorithms (and the task
outbox signing_profiles) directly. A sender-like object without them - the
Postgres conformance tests' MagicMock senders, and adopters' own test
doubles - failed the check and its rows were quarantined.

Both outboxes and the boot validator now share _sender_signing_algorithms
and _sender_signing_profiles, which fall back to _auth.alg and, for an
RFC 9421 sender, adcp/webhook-signing/v1, as the boot check already did.
The Postgres conformance mock senders also declare both properties.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean, behavior-preserving refactor with no blocking findings.

This delta moves _sender_algorithms verbatim from webhook_emit.py into webhook_sender.py as _sender_signing_algorithms, adds _sender_signing_profiles, and routes call sites in task_webhook_outbox.py, notification_outbox_pg.py, and webhook_emit.py through these duck-typed helpers. Legacy/supervisor senders now fall back fail-closed instead of raising AttributeError. The local import in webhook_emit.py is required by the existing TYPE_CHECKING-only WebhookSender import (circular dep). New test covers legacy-double, unsigned, and dual real-sender paths.

Decision trace:

  • Row 1 (critical/high): no findings — skip.
  • Row 2 (gated_paths): false — skip.
  • Row 3 (deleted high-risk): all high-risk files are (modified), none deleted — skip.
  • Row 4 (medium data-loss/schema/infra): no findings — skip.
  • Row 5 (modified high-risk + medium finding): no medium findings in this run — skip.
  • Row 6 (sticky escalate): prior decision was approve, not escalate — skip.
  • Row 7 (no-auto-approve team): no team match — skip.
  • Row 8 (3+ medium): zero findings — skip.
  • Row 9: approve.

The prior-run medium (notifications.py:133, warn-only web-bot-auth skip) is outside this delta and did not recur. high_risk is true only via (modified) decisioning files, which the reviewer cleared with no medium-or-worse concerns — presumed safe per the high-risk heuristic guidance. review_decision is APPROVED.

A web-bot-auth subscription that production signing cannot sign is skipped
so it does not block the account's other subscribers. That subscriber then
receives no notifications, so log the skip at error level (the level the
outboxes use for a quarantined delivery) with the subscriber and account
ids, instead of a warning that can be lost under volume.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean observability fix, no blocking findings.

This is a subsequent review on top of a prior approve. The single new commit (db7756b) upgrades the skipped web-bot-auth subscription log from warn to error and adds subscriber_id and account_id to the message (notifications.py:37-43). Both fields are valid, non-credential references on ReportingNotificationSubscription (routing.py:60-61), the test asserts the error level and both ids (test_reporting_webhook_wba.py:247-251), and the doc matches. A quarantined subscriber is now alertable — a net improvement.

Decision-table walk:

  • No critical/high findings → row 1 does not fire.
  • gated_paths is false → row 2 N/A (and review_decision is APPROVED anyway).
  • high_risk is true but all high-risk reasons are (modified) with zero medium-or-higher findings on those files — no (deleted) (row 3 N/A), no medium on a modified high-risk file (row 5 N/A).
  • No medium findings at all → rows 4, 8 N/A.
  • Prior decision was approve, not escalate → row 6 N/A.
  • No no-auto-approve team match → row 7 N/A.
  • Falls through to row 9 → approve.

No credentials surfaced in logs, no public-surface/semver signal change, no generated-code hand-edits, no CI-gate tampering.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean pass, no blocking or medium findings.

Checked against the decision table top-down:

  • Rows 1 (critical/high): no findings.
  • Row 2 (gated_paths): gated_paths is false; review_decision is APPROVED regardless.
  • Row 3 (deleted high-risk path): all high-risk reasons are (modified), none (deleted).
  • Rows 4–5 (medium + schema/infra or modified high-risk): no medium findings.
  • Row 6 (sticky escalate): prior decision was approve, not escalate.
  • Row 7 (no-auto-approve team): no team match.
  • Row 8 (≥3 medium): zero medium findings.
  • Row 9: approve.

The PR threads web-bot-auth webhook signing through WebhookSender (additive public surface, correct feat prefix — no breaking semver signal). Reviewer verified the per-attempt resign path has no transport leak, both PG outbox envelopes round-trip byte-identically for pre-feature rows (rolling-deploy quarantine holds), retries re-sign afresh rather than replaying stored signatures, and timeouts + allowed_destination_ports are threaded to the worker. high_risk is true only because decisioning/** and webhook_auth.py were modified, but the reviewer found no medium-or-higher concerns on those files, so the modifications are presumed safe. Consistent with the prior clean approve on an earlier head.

No blocking findings. No escalation triggers.

@bokelley
bokelley merged commit 6ec940d into next Nov 8, 2026
41 checks passed
@bokelley
bokelley deleted the claude/issue-1859-webhook-wba branch November 8, 2026 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant