Skip to content

fix(webhook): restore provider contracts and readiness - #90479

Open
aviyashchin wants to merge 2 commits into
NousResearch:mainfrom
aviyashchin:fix/webhook-provider-contracts
Open

aviyashchin wants to merge 2 commits into
NousResearch:mainfrom
aviyashchin:fix/webhook-provider-contracts

Conversation

@aviyashchin

Copy link
Copy Markdown

What

  • Restore provider-specific signature verification for configured webhook routes.
  • Add authenticated GET readiness for known routes.
  • Deduplicate repeated provider payloads by stable delivery identifiers.
  • Preserve multiplex-profile validation on readiness routes.

Why

Generic HMAC-only handling rejects valid provider callbacks whose documented signature formats differ, while missing readiness and payload-level deduplication make production subscriptions hard to verify and allow provider retries to enqueue the same event twice.

Validation

  • scripts/run_tests.sh tests/gateway/test_webhook_adapter.py tests/gateway/test_webhook_session_close.py tests/gateway/test_webhook_route_toolsets.py tests/gateway/test_webhook_deliver_only.py tests/gateway/test_webhook_signature_rate_limit.py tests/gateway/test_webhook_integration.py tests/gateway/test_webhook_dynamic_routes.py
  • 71 passed, 0 failed on macOS arm64
  • Production acceptance separately confirmed public readiness returns 200 and unsigned POSTs return 401; no production payload contents are included here.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/webhook Webhook / API server area/auth Authentication, OAuth, credential pools 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
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Reviewed the diff. Three solid restorations in one: (1) the payload-ID dedup fallback is carefully bounded — route-scoped via JSON array encoding, scalar-only with bool/empty rejection, and strictly below trusted delivery headers, with tests proving both single-invocation retries and header-priority preservation plus an explicit no-secret/no-payload-leak assertion; (2) provider signature support (Linear/Attio/`X-Signature`) keeps the commit-to-V2 semantics intact — malformed V2 still never downgrades to body-only V1 — and `removeprefix("sha256=")` normalization matches what most emit; any attacker-supplied extra header can only cause additional strict comparisons, so mixed-header games fail closed; (3) the GET readiness endpoint deliberately touches none of the POST pipeline (no auth/rate/dedup/agent), verified by state-preservation assertions.

One design point to make explicit:

  • gateway/platforms/webhook.py:~527 (_handle_webhook_readiness) is an unauthenticated route-name oracle. A 200-vs-404 difference lets anyone probe whether `/webhooks/` exists. Since the POST path treats knowing the route name as necessary-but-insufficient (signature still required), the leak is low-severity — but route names sometimes embed tenant hints. Consider documenting "treat route_name as a credential component" alongside the endpoint, or returning a constant-status/generic body for unknown routes too (404 with identical shape still distinguishes). The profile-multiplex variant at least gates on configured profiles first, which is good.

Nit: the V2 comment block lost some of its original rationale (the mixed-header downgrade attack walkthrough) in this refactor — that reasoning is exactly what stops a future cleanup from re-nesting the V2 check inside `if generic_sig:`; worth preserving somewhere.

No blocking issues found.

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

Labels

area/auth Authentication, OAuth, credential pools comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists 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/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants