Skip to content

fix: use default JSON separators for webhook HMAC signing#152

Merged
EmmaLouise2018 merged 3 commits intomainfrom
EmmaLouise2018/fix-issue-151
Mar 11, 2026
Merged

fix: use default JSON separators for webhook HMAC signing#152
EmmaLouise2018 merged 3 commits intomainfrom
EmmaLouise2018/fix-issue-151

Conversation

@EmmaLouise2018
Copy link
Contributor

@EmmaLouise2018 EmmaLouise2018 commented Mar 11, 2026

Summary

  • Signing fix: get_adcp_signed_headers_for_webhook() was computing HMAC over compact JSON (separators=(",", ":")) but callers send the body via json= kwarg which uses default separators (with spaces), causing signature mismatches on receivers. Changed to json.dumps() with default separators, aligning with the JS reference implementation's JSON.stringify() behavior.
  • Raw body verification: _verify_webhook_signature and handle_webhook now accept an optional raw_body parameter. When provided, the raw HTTP body bytes are used directly for HMAC verification instead of re-serializing, preventing cross-language JSON serialization mismatches (e.g. JS compact vs Python spaced).
  • Unix timestamps: Changed X-AdCP-Timestamp from ISO 8601 to Unix seconds, matching the AdCP spec and JS reference implementation. get_adcp_signed_headers_for_webhook now accepts str | int | None and defaults to int(time.time()).
  • Official test vectors: Added TestHMACTestVectors class validating all 10 vectors from adcontextprotocol/adcp PR #1383 — signing, verification via raw_body, and rejection of tampered signatures all pass.

Fixes #151

Test plan

  • All 70 webhook tests pass (38 existing + 32 new from 10 test vectors x 4 test paths, minus 8 expected skips)
  • All 10 official AdCP HMAC test vectors validate correctly
  • raw_body verification works with compact, spaced, pretty-printed, and unicode JSON
  • Wrong signatures are correctly rejected for all vectors
  • Lint passes

get_adcp_signed_headers_for_webhook() was signing over compact JSON
(no spaces) but callers send the body via json= kwarg which uses
default separators (with spaces), causing HMAC verification failures
on receivers. Aligns with JS reference implementation's JSON.stringify().

Fixes #151
@EmmaLouise2018 EmmaLouise2018 requested a review from bokelley March 11, 2026 01:34
Copy link
Contributor

@bokelley bokelley left a comment

Choose a reason for hiding this comment

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

The signing fix is correct — json.dumps() with default separators now matches what requests.post(json=...) puts on the wire. Two issues remain:

1. Verification still re-serializes (cross-language breakage)

_verify_webhook_signature in client.py still does:

payload_bytes = json.dumps(payload).encode("utf-8")

If a JS sender sends {"key":"value"} (no spaces, JSON.stringify output), the Python verifier will re-serialize as {"key": "value"} (with spaces) → signature mismatch. This needs the same raw-body treatment as the JS PR (#314) — accept raw body bytes and use them directly for verification, with json.dumps() as a fallback.

2. Timestamp format doesn't match the spec

The test uses an ISO 8601 timestamp:

header_timestamp = "2025-01-15T10:00:00Z"

But the spec (and the JS client) uses Unix seconds:

X-ADCP-Timestamp: <unix timestamp in seconds>

The signing function get_adcp_signed_headers_for_webhook should be checked too — if it's generating ISO timestamps in the header, every JS receiver will reject it because parseInt("2025-01-15T10:00:00Z", 10) returns 2025, which will fail the 5-minute window check.

Summary

  • ✅ Signing serialization fix is correct
  • ❌ Verification needs raw body support
  • ❌ Timestamp format needs to be Unix seconds, not ISO 8601

Addresses PR review feedback:
- _verify_webhook_signature now accepts optional raw_body param to avoid
  re-serializing the payload, preventing cross-language JSON mismatches
- Timestamp format changed from ISO 8601 to Unix seconds to match the
  AdCP spec and JS reference implementation
- Added test for raw_body signature verification path

Fixes #151
@EmmaLouise2018
Copy link
Contributor Author

Both issues addressed in 4a663e5:

1. Verification raw body support_verify_webhook_signature, _handle_mcp_webhook, and handle_webhook now accept an optional raw_body: bytes | str | None parameter. When provided, the raw HTTP body is used directly for HMAC verification instead of re-serializing, so a JS sender's JSON.stringify output (or any other serializer) will verify correctly. Falls back to json.dumps() when raw_body is not provided for backward compatibility. Added a dedicated test that signs over compact JSON and verifies via raw_body.

2. Unix timestampsget_adcp_signed_headers_for_webhook now accepts str | int | None for timestamp and defaults to str(int(time.time())). The X-AdCP-Timestamp header is now Unix seconds (e.g. "1773185740"), matching the spec and JS client. All tests updated accordingly.

Add TestHMACTestVectors class with parametrized tests against all 10
official test vectors from adcontextprotocol/adcp PR #1383. Validates
signing, verification via raw_body, and rejection of tampered signatures.
@EmmaLouise2018 EmmaLouise2018 merged commit 2223f6e into main Mar 11, 2026
8 checks passed
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.

Bug: get_adcp_signed_headers_for_webhook signs compact JSON but wire body uses default JSON

2 participants