Skip to content

refactor(webhook): establish canonical intake authority - #90995

Open
andrexibiza wants to merge 1 commit into
NousResearch:mainfrom
andrexibiza:convergence/webhook-authority-spine-90989
Open

refactor(webhook): establish canonical intake authority#90995
andrexibiza wants to merge 1 commit into
NousResearch:mainfrom
andrexibiza:convergence/webhook-authority-spine-90989

Conversation

@andrexibiza

@andrexibiza andrexibiza commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Refs #90989.

Purpose

This is the first implementation slice of the webhook surface-convergence issue. It creates the domain authority that the existing webhook campaign lanes can compose onto instead of continuing to derive provider, delivery identity, event identity, and intake provenance independently.

Exact topology

  • base: 5e32e3aecd2070e8245d9ae2c9ee257f547fcb71
  • head: a9af5efcfeb6d81ae7be04421c7fcc8e872440fa
  • tree: f174a69ead1a509be956207581eaccf927fe6665
  • one commit
  • two files
  • no campaign receipts, contributor churn, UI/router spillover, or webhook.py god-file rewrite

The branch was rebuilt after main advanced during PR creation. The current head overlays only the two webhook-authority blobs onto the exact new main tree; no intervening upstream files are dropped.

What this establishes

Canonical provider registry

gateway/platforms/webhook_contract.py owns provider namespaces and the wire facts required by intake: aliases, provider-native delivery-ID headers, event headers, compatibility-detection headers, payload identity keys, allowed verifier modes, and default verifier mode.

Provider identity and cryptographic verifier identity are related but deliberately not conflated. For example, an explicitly declared Chatwoot route can remain in the chatwoot provider namespace while using a registered generic HMAC verifier. Impossible provider/verifier combinations fail closed.

One route/provider binding decision

WebhookRouteConfig.bind() normalizes profile, provider, signature mode, enabled state, and event allowlist once. Explicit route authority cannot subsequently be replaced by attacker-controlled request headers.

Undeclared historical routes retain one explicit compatibility bridge (infer_legacy_provider) so migration can be staged without silently breaking existing installs. The compatibility result is recorded as provenance and is not a downstream authorization mechanism.

Provider-native retry identity

resolve_delivery_identity() consumes only the already-bound provider namespace:

  • GitHub: X-GitHub-Delivery
  • Svix: svix-id
  • GitLab: event/webhook/idempotency UUID headers
  • Standard Webhooks: webhook-id / Idempotency-Key
  • Chatwoot: declared delivery header, then declared payload ID
  • Stripe: declared payload ID
  • generic: X-Request-ID

There is no timestamp fallback. If no stable provider identity exists, deduplication authority is absent and the envelope receives a unique trace/session identity instead.

Payload id is accepted only for explicitly declared providers, so an attacker cannot turn an undeclared/generic route into Stripe or Chatwoot by shaping the JSON body.

Immutable intake envelope

WebhookEnvelope carries the facts that downstream execution should consume exactly once:

  • normalized route/profile
  • provider + verifier/auth provenance
  • provider-scoped event type
  • stable delivery identity when one exists
  • unique trace identity
  • SHA-256 of the exact raw body
  • recursively immutable payload
  • profile/route/provider-scoped idempotency key

This creates the intended HTTP/domain boundary: later composition can move from aiohttp.Request to WebhookEnvelope without re-deriving security or identity facts.

Focused verification surface

tests/gateway/test_webhook_contract.py covers immutable registry behavior, alias normalization, unknown provider/verifier rejection, provider/verifier disagreement, valid provider + generic-verifier composition, attacker-header resistance, provider-scoped event extraction, bounded legacy inference, provider-native identity precedence, absence of dedupe authority without a stable ID, timestamp non-identity, explicit-only payload IDs, scoped idempotency keys, profile mismatch, raw-body provenance, and recursive envelope immutability.

Fresh hosted CI/Docker/Nix is required on a9af5efc...; no sibling or historical green is inherited.

Composition boundary

This PR intentionally does not rewrite the current WebhookAdapter handler yet. Doing that before the current Task 10/auth/session candidates converge would recreate the same multi-owner conflict #90989 is intended to eliminate.

The next convergence slice should wire current intake/auth onto this contract, preserving #90236's body-hash/idempotency semantics and consuming #85318's verifier rather than copying either implementation again.

Centralize route/provider/verifier binding, provider-native retry identity, provider-scoped event extraction, and immutable intake-envelope construction behind one domain authority. Keep legacy header inference isolated to undeclared compatibility routes and never use timestamps as delivery identity.

Refs NousResearch#90989
Interlocks NousResearch#90236 and NousResearch#85318.
@andrexibiza
andrexibiza force-pushed the convergence/webhook-authority-spine-90989 branch from 298dc23 to a9af5ef Compare August 20, 2026 17:58
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery platform/webhook Webhook / API server sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head hosted verification

Verified the published head a9af5efcfeb6d81ae7be04421c7fcc8e872440fa directly rather than inheriting any sibling or historical status:

  • CI 32400604659success
  • Docker Build, Test, and Publish 32400603905success
  • Nix flake check 32400603870success

The later Label-rerun records are skipped/cancelled orchestration noise; they do not supersede the successful exact-head product workflows above. This closes the PR body's outstanding “fresh hosted CI/Docker/Nix required” gate for the canonical webhook intake-authority slice. The composition boundary with #90236/#85318/#90304/#85640 remains exactly as documented; this comment does not claim terminal campaign integration.

Copy link
Copy Markdown
Contributor Author

@teknium1 convergence review requested on exact head a9af5efcfeb6d81ae7be04421c7fcc8e872440fa.

Revalidated now: GitHub reports open/non-draft/mergeable. Exact-head CI 32400604659, Docker 32400603905, and Nix 32400603870 are successful.

This should remain the canonical webhook domain/intake spine: provider namespace, route/provider binding, provider-native delivery identity, raw-body provenance, and immutable WebhookEnvelope. It deliberately does not replace #90236's executable Task 10 idempotency/raw-body owner or #85318's verifier authority. The next composition should consume those authorities rather than grow another adapter-local derivation path.

Copy link
Copy Markdown
Contributor Author

Architecture interlock locked in #91913

This PR remains the active webhook delivery spine in the new machine-readable authority contract.

The exact role is now tested as:

The conformance test fails if another PR displaces #90995 as the spine or if documentation/final assembly moves ahead of effect settlement.

Contract PR: #91913
Exact contract head: 170a3a0e67034abd7d6a2c69a16c292b4781720f.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

The security reasoning here is sound and well-documented: payload-derived delivery IDs accepted only for declared providers (resolve_delivery_identity gating on route.provider_declared) kills the attacker-supplied-id spoofing path, and once a route is bound, headers can never re-select the provider (test_explicit_provider_cannot_be_reselected_by_attacker_headers pins exactly the right invariant). The recursively frozen envelope is a nice touch.

Concrete points:

  1. enabled coercion in WebhookRouteConfig.bind (webhook_contract.py, enabled=route.get("enabled", True) is not False) — this treats any non-False value as enabled: "false" (string), 0, None, [] all enable the route. Everywhere else this module fails closed on malformed input (provider must be non-empty, events must be a sequence). For consistency, either validate the type —

    enabled_raw = route.get("enabled", True)
    if not isinstance(enabled_raw, bool):
        raise WebhookContractError(f"route {route_name!r} enabled must be a boolean")

    — or at minimum add a test documenting that enabled: "false" silently enables. Given this contract module explicitly exists to stop config ambiguity from becoming authorization behavior, the loose coercion stands out.

  2. infer_legacy_provider order vs Idempotency-Key — gitlab's legacy_detection_headers includes the bare Idempotency-Key, which is a completely generic header many webhook senders set. A legacy route receiving an unrelated provider's delivery with an Idempotency-Key header gets bound to gitlab with default signature mode gitlab and will fail verification. That fails closed (good), but the diagnostic story ("why is my generic hook classified as gitlab?") is confusing. Since inference is compatibility-only, consider dropping Idempotency-Key from detection or moving it after standard_webhooks.

  3. session_identity returns the raw provider value while idempotency_key namespaces by profile/route/provider. If any future consumer uses session_identity as a dedupe key across routes, two different routes delivering the same GitHub delivery ID collide. Consider either namespacing it too, or adding a docstring warning that it is only unique within one route.

  4. Scope question — this PR adds the authority module + tests but no consumer yet; the existing webhook adapter presumably still does its own header inference. Assuming the wiring lands as a focused follow-up (the module docstring reads like phase 1 of a series), it would help to link that follow-up here so the new module doesn't sit unwired.

The test suite is genuinely strong — immutability, alias canonicalization, precedence, and both fail-closed paths are all covered.

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

Labels

comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have platform/webhook Webhook / API server sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants