Skip to content

fix(spec): make the §5.3 verification example reject truncated signatures - #93

Merged
ucekmez merged 2 commits into
mainfrom
fix/spec-webhook-verification-example
Sep 1, 2026
Merged

fix(spec): make the §5.3 verification example reject truncated signatures#93
ucekmez merged 2 commits into
mainfrom
fix/spec-webhook-verification-example

Conversation

@ucekmez

@ucekmez ucekmez commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

PR 1 of a stacked series fixing findings from the 2026-08 EEP protocol audit. Each PR in the series is based on the previous one so the diffs stay reviewable in isolation and never conflict; GitHub retargets the base automatically as they merge. Not for merge without review — opened for discussion.


The normative webhook-verification example in SPECIFICATION.md §5.3 is unsafe as written:

const incoming = webhookSignature.replace('v1,', '');
return timingSafeEqual(Buffer.from(expected), Buffer.from(incoming));

Two defects:

  1. timingSafeEqual throws RangeError when buffer lengths differ. An attacker sending a truncated webhook-signature header triggers an unhandled exception — HTTP 500 — instead of a clean 401. The input is entirely attacker-controlled.
  2. It parses a single signature token. Standard Webhooks transmits space-delimited multiples (v1,old v1,new) during secret rotation, so this code rejects valid traffic for the full duration of every rotation.

The example also silently omits the 60-second timestamp check that requirement 4 in the same section mandates.

@eep-dev/signer (src/index.ts:86-95) and its Python sibling already get all three right, as does the compliance-cli verifier. Only the specification — the thing implementers copy from — taught the unsafe version.

What changed

  • §5.3 example rewritten: length guard before the constant-time compare, iterate every space-delimited token, enforce the timestamp tolerance.
  • Two new receiving-platform MUSTs making the length guard and multi-signature acceptance explicit rather than implied.
  • Pointer to @eep-dev/signer so the path of least resistance is importing the reference implementation, not re-deriving it.
  • New conformance fixture signature/truncated-signature — a strict prefix of a valid signature — registered in manifest.json and wired into both the TypeScript and Python harnesses.
  • Regression tests asserting returns false, does not throw for truncated and over-long signatures across @eep-dev/signer, eep-signer, and compliance-cli.

Scope

  • Spec / schema only
  • TypeScript package(s)
  • Python package(s)
  • Tests / CI
  • Docs / examples

Checklist

  • I read CONTRIBUTING.md and CODE_OF_CONDUCT.md.
  • Tests added or updated where appropriate.
  • Breaking change? No. This corrects prose and example code to match behaviour the reference implementations already ship. No wire-format change.
  • Documentation updated for user-visible behavior.

Verification

Run locally against this branch:

Suite Result
tests/ (vitest) 164 passed
@eep-dev/signer 54 passed
@eep-dev/compliance-cli 57 passed
eep-signer-python (pytest) 16 passed
tests/cross-impl/test_conformance_fixtures.py 19 passed
node scripts/codegen-schema-types.mjs --check no drift

Notes for reviewers

The fixture is deliberately a prefix of the correct signature rather than random bytes — random bytes of the right length would exercise signature_mismatch but never reach the length-guard path that this finding is about.

No schema under schemas/v0.1/ changed, so the drift gate is a no-op here.

…ures

The normative webhook-verification example in SPECIFICATION.md §5.3
called `timingSafeEqual` on two buffers without checking their lengths.
Node throws `RangeError: Input buffers must have the same byte length`
when they differ, so an attacker-supplied truncated `webhook-signature`
header produced an unhandled exception — surfacing as HTTP 500 rather
than an authentication failure. The same example also parsed a single
signature token, so it rejected valid traffic for the whole duration of
any `delivery_secret` rotation, during which publishers send multiple
space-delimited signatures.

The reference libraries (`@eep-dev/signer`, `eep-signer`) already handle
both cases correctly. Only the specification — which is what
implementers copy from — taught the unsafe version.

Changes:
- Rewrite the §5.3 example: parse every space-delimited token, guard on
  buffer length before the constant-time compare, and enforce the 60s
  timestamp tolerance the surrounding text already mandates but the
  example omitted entirely.
- Promote both behaviours to explicit receiving-platform MUSTs, and
  point readers at `@eep-dev/signer` rather than a hand-rolled compare.
- Add conformance fixture `signature/truncated-signature` (a strict
  prefix of a valid signature) with the manifest entry and the
  TypeScript + Python harness cases.
- Add regression tests asserting "returns false, does not throw" for
  truncated and over-long signatures in `@eep-dev/signer`, `eep-signer`
  and the compliance-cli verifier.

No wire-format change: this corrects prose and example code to match
the behaviour the reference implementations already ship.

Refs: EEP audit 2026-08 finding A8
Signed-off-by: Ugur Cekmez <ucekmez@gmail.com>
Copilot AI lite review requested due to automatic review settings August 26, 2026 18:29

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

`pull_request.branches` was limited to `main`, so a PR based on another
branch got no checks whatsoever. That is exactly the situation where a
reviewer most needs them: a stacked change is only reviewable in
isolation if CI has actually run against it.

Stacking PRs is the practical way to ship a series of related changes
without resolving the same conflicts N times, and it should not cost the
series its test coverage. Pushes to `main` are unaffected.

Signed-off-by: Ugur Cekmez <ucekmez@gmail.com>
@ucekmez
ucekmez merged commit b040e2d into main Sep 1, 2026
21 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.

2 participants