Repository navigation
2.0.0: injective canonical string, Web Crypto, adapters - #16
Merged
Merged
Conversation
…ding
The v1 string `v1:{ts}:{nonce}:{payload}` put two free-form fields next
to each other with an unescaped delimiter. One signed message could be
re-split into several (nonce, payload) pairs that all verified, so a
replay cache keyed on the nonce never fired.
v2 is `v2.{timestamp}.{nonce}.{payload}`. The timestamp must be a
non-negative safe integer and the nonce must match
`^[A-Za-z0-9_-]{1,64}$`, both at sign and verify time, so the first
three fields parse unambiguously and the payload is whatever follows
the third dot. A fast-check suite checks that the encoding has a left
inverse and that no alternative split survives the nonce grammar.
v1 is not accepted any more. The `version` option and `DEFAULT_VERSION`
are gone; `SIGNATURE_VERSION` is exported instead. Test vectors are
recomputed for v2 and a dots-and-digits vector is added.
Header timestamps now have to be decimal digits, and malformed
timestamps and nonces carry their own error codes.
Signatures are now `v2={hex}`. The verifier reads the version from the
signature instead of assuming one, and refuses any version it does not
implement, so a future scheme change has somewhere to live and an old
scheme cannot be replayed against a newer receiver.
The digest must be exactly 64 lower-case hex characters. Buffer.from
with 'hex' stops silently at the first bad pair, which let a valid
signature with junk appended, or a folded duplicate header
("sigA, sigB"), verify against the first 32 bytes. Anything that is not
the exact wire form is rejected before any HMAC work.
parseSignature, formatSignature and SIGNATURE_PATTERN are exported.
`secret: string` becomes `secrets: string | string[]` on signWebhook, verifyWebhook and the adapter options. Signing uses the first entry. Verification accepts a signature made with any entry. Every candidate is evaluated, HMAC and constant-time compare both, whether or not an earlier one already matched, so the time taken depends only on how many secrets are configured. Empty lists and empty entries are rejected up front.
…with 401 The Fastify and NestJS adapters fell back to JSON.stringify when handed a parsed body, which is the exact mistake the README warns about. All three adapters now go through resolveRawBody: a Buffer or string is used as is, anything else throws a configuration error that names the raw-body option for that framework. The NestJS guard reads request.rawBody (populated by `rawBody: true`) before request.body. Verification failures used to map to 401, 400 or 409 depending on which check failed. A 409 told an unauthenticated caller that its signature had been accepted and only the nonce was stale. Every WebhookError is now a 401 with the same body; the specific error is still handed to onError. The reasoning is recorded on WebhookError's JSDoc for anyone writing their own handler.
The canonical string is injective, but the value handed to createHmac was a JS string and UTF-8 encoding is not: every unpaired surrogate becomes the same three bytes. The adapters made that reachable from the wire. resolveRawBody decoded the Buffer that express.raw() hands over, so the bodies 7b ff 7d and 7b fe 7d arrived as one string and a signature over the first verified the second. `payload` is now `string | Uint8Array` on both sign and verify. buildCanonicalBytes concatenates the UTF-8 prefix with the payload bytes: a string is UTF-8 encoded, a Uint8Array is copied through untouched. buildCanonicalString stays for the readable form the test vectors are written against, but nothing signs from it any more, and resolveRawBody returns the Buffer as it is. Secrets take the same type, because a binary key hit the same problem: two keys that differ only outside valid UTF-8 produced one signature. Strings are still UTF-8 encoded, so existing callers are unaffected; normalizeSecrets now returns the key material as Buffers. The digests are unchanged. Splitting a well-formed string at the third dot and encoding the halves gives the same bytes as encoding the whole, so every fixed vector still verifies against the value it always had.
The timestamp and the nonce had runtime guards; the payload did not,
and a template literal stringifies whatever it is handed. A JS caller
signing ['a'], null, {} or undefined got the signature for 'a', 'null',
'[object Object]' or 'undefined', so two calls that meant different
things produced one digest. TypeScript catches this, but the library
ships to JS callers too.
buildCanonicalBytes now throws a TypeError for anything that is not a
string or a Uint8Array, and buildCanonicalString for anything that is
not a string, bytes included: it renders text and cannot show a byte
sequence without losing it.
The hook sent a 401 and then resolved to undefined. Fastify's contract for an async hook that has already responded is to return the reply; without it the lifecycle continues into the route handler, which runs its side effects and only then fails with FST_ERR_REP_ALREADY_SENT. request.webhookVerified stays false, so a handler that checks it is safe, but one that trusts the hook is not. The missing-header branch had the same shape and gets the same fix. The success path returns undefined so the lifecycle carries on as intended.
Every property was over buildCanonicalString, a string to string map
that is injective and was injective even while the bytes handed to the
HMAC were not. The suite could not have caught that, and the generator
could not reach the input that shows it either: over 5000 samples the
grapheme unit produced 0 ill-formed strings, so an unpaired surrogate
was never encoded.
The main property is now over signWebhook(...).signature: distinct
(timestamp, nonce, payload) triples must give distinct signatures for a
fixed secret. Payloads are strings from a unit that includes lone
surrogates (1895 of 5000 samples are ill-formed) and byte arrays, one
arm of which draws only lead bytes that are invalid UTF-8 on their own,
so two spellings of the same decoded text are common rather than
impossible. The second triple is built from the first with some fields
overridden: two independently drawn triples practically never share a
timestamp and a nonce, so without that the payload could never be the
only difference and the property would pass whatever the encoder did.
Checked by reintroducing the decode-then-encode step in
buildCanonicalBytes; the property fails on [248,248] against [248,249]
and passes once it is removed.
Timestamps are drawn from a plausible epoch range as well as the old
nat({max: MAX_SAFE_INTEGER}), which biases so hard towards small values
that a real unix second almost never appears.
Also added: a string payload and its UTF-8 bytes must sign identically,
so the two payload types stay interchangeable for text.
The "distinct triples never produce the same canonical string" property
is dropped. The left-inverse property already implies it, and it cost
2000 runs to say the same thing. The re-split properties are unchanged.
Six cheap items from the audit, none of them exploitable on their own.
The timestamp header now has to match ^(0|[1-9]\d*)$. '000{ts}' parsed
to the same number as '{ts}' and verified, so the signature covered a
spelling of the timestamp that was not the one on the wire, and
anything downstream logging or deduping on the raw header saw a value
nobody had authenticated.
Hex digests are accepted in either case and lower-cased before they are
decoded. A Go or Python sender formatting with %X used to get the same
401 as a forgery, with nothing in it to say why.
Duplicate headers are refused instead of silently taking the first
value. Node folds duplicates into one comma-joined string, which
already failed closed at the signature grammar, but a framework that
hands over an array had the rest of the values dropped without a word.
extractHeaders returns the whole message now, so the adapters carry one
branch for a missing header and a duplicated one rather than two.
A nonceValidator that throws answers 401 like every other verification
failure. It used to map to 500, which is the status oracle the uniform
401 was built to remove; the store being down is not the caller's
business. The original error travels as the cause so onError can log
it.
normalizeSecrets collapses duplicates and refuses more than 16 distinct
entries. Every entry costs an HMAC on every request whether or not an
earlier one matched, and 500 copies of one secret cost 500 of them.
The Express adapter routes the raw-body configuration error through the
same handler as a verification failure. It was thrown out of the
middleware, so Express answered 500 from its own error handler and
onError never fired. It still answers 500 — a misconfigured route that
quietly returned 401 forever would be worse — but the integrator now
hears about it.
The Express middleware called next() inside the fulfilled handler of a .then().catch(fail) chain, so the whole rest of the route was inside the webhook's own catch. A route handler that threw synchronously was reported to onError as though the signature had failed, answered with a 500 written over whatever the handler had already sent, and never reached Express's error middleware. The two-argument form keeps the failure path to verification only, and a throw from next() is handed to next(err), which is where Express expects a middleware's synchronous throw to end up. onError is the integrator's code and can throw. In Express that left the request unanswered and raised an unhandledRejection; in Fastify the hook rejected with the reply unsent; in Nest the guard threw the logger's error instead of the HttpException, so a failed log turned a 401 into a 500. All three now go through reportError, which contains it: there is nowhere to report a failure of the reporter, and the caller still gets the uniform response.
resolveRawBody tested Buffer.isBuffer, which is false for a Uint8Array, so a body that was already exactly the bytes we sign was refused with the configuration error that tells the integrator to go and set up a raw body parser they have in fact set up correctly. Buffer is a subclass of Uint8Array, so the wider check keeps every Buffer working and the payload type has accepted Uint8Array since the signing path moved to bytes. An ArrayBuffer, a DataView and a parsed object are still refused: none of them is a byte sequence we can sign without guessing at a view.
Express routed the raw-body configuration error through reportError and the shared status and body mapping. Fastify let it reject the hook with the reply unsent, and Nest threw a plain Error that its exception filter renders as a bare 500. Three adapters, three answers to one mistake, and in two of them the integrator's onError never heard about it. resolveRawBody now sits inside the try in both, so all three report the error, answer 500 with the generic body, and leave webhookVerified alone. The Nest comment that described the escape as deliberate is gone; it was describing the bug.
Node lower-cases the header names it parses, so configuring signatureHeader as 'X-Webhook-Signature' looked up a key that is never present and answered 400 for a header the sender had sent. Header names are case-insensitive on the wire and the option now behaves that way.
buildCanonicalString is no longer exported. It looks like the thing you hash and it is not: it is the readable form, and passing it to createHmac is the bug the v2 work exists to fix. It stays in src for the test vectors and for anything that needs to show what was signed, and buildCanonicalBytes is the exported one. formatSignature loses its version parameter and always emits SIGNATURE_VERSION. It let a caller put a version on the wire that nothing in this library will ever accept, and the 401 that followed would have looked like a receiver bug.
Every vector was text, so the whole vector suite would have passed against the version that decoded the body before signing it. The new one carries the bytes 7b ff 7d and a digest computed outside the library, so a regression to string-based hashing changes it. A byte payload has no string form, so `canonical` is now optional and the payload is `string | Uint8Array`. The string builder skips the vector that has no string form and the byte builder derives the expected value from the prefix and the bytes. The six text vectors are untouched, digests included.
An empty list and a list with a blank entry in it both threw "secrets must not be empty", so `secret: 'x'` against the README's old singular option, and a rotation list with one env var unset, produced the same message for two unrelated mistakes. The per-entry case now says what an entry has to be.
"defense" was the only genuine variant in src. The -ize endings stay: Oxford spelling has them, and they match the identifiers.
Accepting [0-9a-fA-F] and lower-casing before the compare gave one digest 2^64 spellings on the wire. The verifier read them all as the same signature, but nothing else does: a replay cache keyed on the header, a rate limiter, a log line compared against another, a deduplicating proxy all treat the header as an opaque token and would see distinct values for one accepted request. A signature has to have exactly one wire form, and the sender is the side that can fix its formatting. The kindness to a sender using %X was worth less than the canonical form. It bought a working request for a misconfigured sender at the cost of every receiver-side identity check on the header, and a sender that gets it wrong now fails on its first delivery rather than after a cache starts missing. The vectors file says the digest is lower-case hex and that a sender in another language must format with %x.
Three things that have to be true before the Web Crypto work starts. The matrix still tested Node 18, which has no globalThis.crypto unless you pass --experimental-global-webcrypto. Every commit after this one would have left that job red for the rest of the branch. 20 went end-of-life on 30 Apr 2026, so 22 and 24 are what is left; 26 joins them when it becomes LTS on 28 Oct 2026. engines follows the same line: >=22 is a support-policy floor, not a security one. tsup 8 defaults removeNodeProtocol to true and rewrote our import to a bare `crypto`, in both dist/index.js and dist/index.cjs. That is why listing 'node:crypto' in external never did anything: esbuild saw `crypto` by the time it resolved the import, and the entry matched nothing. A bare builtin does not resolve on Deno, and a bundler pointed at the browser has no opinion to fall back on, so the rewrite goes off permanently rather than just for the duration of this migration. The dead external goes with it. Action pins move to v7 because v4 is two majors behind and the Node 24 image is only in the newer setup-node.
The seven fixed vectors already pin the digests, and a digest is not the
contract. A rewrite could hash correctly and still resolve {valid:false}
instead of rejecting, lose the error codes the adapters map to 401, drop the
cause off a nonce-store failure, or move the nonce grammar check behind the
HMAC. All of that survives a vectors-only check.
So this pins the shape as well: every vector through sign and through verify,
one assertion per error class and code, rejection rather than a falsy result,
the cause on a wrapped validator failure, the four ordering steps, the secret
messages that have to stay in front of the crypto layer, and the adapter
answers - 401 with one body for a verification failure, 500 for a
misconfiguration.
Written in await-form throughout, including where nothing is asynchronous
yet, so the file does not have to be edited by the commit it exists to
police. The argument-validation assertions deliberately stay in
expect(() => ...).toThrow() form: signWebhook validates eagerly and returns
a promise rather than being marked async, so a misconfiguration is still
catchable where the caller wrote the call. If this file has to change there
later, that decision was undone and the change is the bug.
Checked it fails: deleting the nonce grammar check and reporting a store
failure as a replay turns three of these red.
Buffer is the one Node type left in the signatures, and everything that uses it here is four small operations: UTF-8 encode, join, hex out, hex in. Uint8Array.prototype.toHex and Uint8Array.fromHex would have done it, and they are not available: they arrived in Node 25, so 24 - the current Active LTS - does not have them and neither do the lines below it. Detecting them would mean shipping two codecs and running the fallback everywhere that matters, so there is one. atob/btoa are out for the same reason and for speaking latin-1 code units rather than bytes. fromHex is strict where Buffer.from(str, 'hex') is not. That one stops at the first pair it cannot read and hands back what it managed, which is how a signature with junk appended, or a folded duplicate header value, decodes to a plausible digest instead of being refused. Even length, lower case, nothing else, or it throws. bytesEqual is not constant time and says so where it is defined. It exists to collapse a configured secret list, which is the receiver's own data; comparing a presented MAC gets the blinded path in the next commits. No consumer yet - this commit only adds the file and its tests.
Buffer was in four public signatures - buildCanonicalBytes' return, ParsedSignature.digest, formatSignature's parameter and normalizeSecrets' return - plus the rawBody option on the Fastify and Nest adapters. On Deno, Bun or Workers there is no Buffer to hand back, so those types were promises the library could only keep on Node. Uint8Array everywhere instead. The emitted .d.ts files now contain the word Buffer zero times, which is the check worth running rather than reading the source. rawBody widens rather than narrows: a Buffer is a Uint8Array, so a caller whose request type says Buffer still satisfies the new signature and has nothing to do. Two things change behind the types as well. parseSignature decodes through the strict codec, so it can no longer accept a digest the pattern let past. And the secret list dedupes by comparing bytes instead of keying a Map on base64, which drops a second copy of every secret that the Map would have kept alive - and would have needed a base64 encoder we deliberately do not have. signer and verifier still go through node:crypto here. createHmac and timingSafeEqual both take a Uint8Array, so this commit is types and encoding only and the seven vectors do not move.
The layer signer and verifier will sit on, with nothing in it that only Node has. No node:crypto import, guarded or otherwise: a literal 'node:crypto' specifier gets resolved at bundle time by esbuild, wrangler, Vite and Metro whether or not the code around it can run, so wrapping one in try/catch protects nobody and breaks every bundled Workers and React Native build. getSubtle() is a plain guard instead, and when it fires it says what to do - the flag to drop, and the one line that installs the global. The compare is a Double-HMAC blind rather than subtle.verify. Constant-time HMAC verification is required by the Web Crypto editor's draft only (w3c/webcrypto PR #553, 26 Mar 2026); it is in neither the 2017 Recommendation nor the Level 2 FPWD, and there is no web-platform-test for it, which is how Node ran a plain memcmp in crypto_hmac.cc for years - CVE-2026-21713, fixed on 24 Mar 2026 in v20.20.2, v22.22.2, v24.14.1 and v25.8.2. We do not get to choose our users' patch level, so the compare has to hold on a runtime that never got the fix. Blinding both operands under a key drawn per call and thrown away leaves a leaky memcmp with nothing to leak: it can only say how two random-looking digests agree. Defence in depth, not a response to a demonstrated exploit. getRandomValues plus importKey rather than generateKey, because HMAC generateKey needs an IoContext on Workers and throws outside a request, and because getRandomValues is the only synchronous member of Crypto and is present everywhere. Two signs and no verify per compare, which keeps the call count a verify makes a flat multiple of the secret count. Keys import with both usages: a key imported with ['sign'] alone makes subtle.verify throw on every runtime. One cast, in bufferSource. TypeScript models BufferSource as excluding SharedArrayBuffer-backed views, so a plain Uint8Array does not satisfy it while the algorithms - which take a copy of the bytes - do not care. The seven pinned digests come back byte-identical through hmacSha256, which is the whole point of the commit.
Web Crypto has no synchronous HMAC. Every operation in the IDL returns a promise and no future level changes that, so signWebhook has to return one too. That is a breaking change and it goes in the changelog at A/4. What it deliberately is not is an async function. An async function cannot throw synchronously, so marking this one would quietly convert every argument mistake - an empty secret list, a nonce outside the grammar, a payload that is neither bytes nor text - into a rejected promise. Anyone who wrote try/catch around a misconfiguration would stop catching it and get an unhandled rejection instead, which is a second breaking change hiding inside the first. Validating eagerly and returning hmacSha256(...).then(...) keeps configuration errors where the caller wrote the call and makes only the HMAC asynchronous. Every expect(() => signWebhook(...)).toThrow(...) in the suite is therefore untouched - the diff contains no line matching toThrow and none matching expect(() =>. The call sites that read .signature gained an await, which is the change consumers will make too. Properties 1 and 2 move to fc.asyncProperty for now; the next-but-one commit restates them over buildCanonicalBytes, which is synchronous and a stronger assertion than "two digests differ". Measured cost of the interim form: 153ms to 303ms for the property file. verifier.ts still goes through node:crypto. It is the next commit.
The last two node:crypto callers go: createHmac becomes hmacSha256 and
timingSafeEqual becomes blindedEqual. src now has no node: module specifier
at all - a static test walks the tree and asserts it, because nothing else
in the toolchain would notice one coming back, and the specifier is what
breaks a bundled Workers or React Native build rather than the code path
around it. dist has no Node builtin import in either format.
The loop keeps the property it had: every configured secret is evaluated
whether or not an earlier one matched, results OR-ed at the end, no break.
A break would tell an attacker which position in the rotation their forgery
got nearest to.
The constant-work test is rewritten around what is now observable. It spies
subtle.sign and subtle.verify and asserts three signs per secret - one
expected MAC plus two to blind that MAC and the presented digest - and no
verify at all, for a match at the front, in the middle, at the end, and
nowhere. Four scenarios, one number, so the assertion fails if the loop ever
learns to stop early. vi.mock('node:crypto') is deleted in the same commit
that made it meaningless.
Properties 1 and 2 were about the encoding, not the MAC, and the previous commit had them running 3000 signWebhook calls at 2000 and 1000 runs to say so. They now assert over buildCanonicalBytes directly, which is synchronous and strictly stronger: "distinct triples give distinct canonical bytes" implies "distinct signatures" and would also catch a digest collision, which the old form could not distinguish from an encoding collision. Property 5 is the only one that genuinely needs the round trip and stays async. Then the guard. A forgotten await on fc.assert(fc.asyncProperty(...)) does not fail: the test is reported as passed and has checked nothing. Removing one await here and running the file proves it - 8 tests, all green, nothing verified. Biome 1.9.4 has no noFloatingPromises (it is a 2.x type-aware nursery rule; the installed binary answers "Unrecognized option"), tsc has no equivalent, and adding ESLint to a Biome-only repo for one rule is worse than the problem. So: a small test that reads the test sources, finds every fc.assert whose property is asynchronous, and requires an await in front. Against the same mutation it fails and names the file and line. It carries its own fixtures so it can show it sees a mistake rather than asserting it would. restoreMocks in the vitest config for the other half: crypto.subtle is one object per process, so a spy left behind by one file would still be counting in the next. Property file 303ms to 195ms; whole suite 1.19s against 1.14s at 7f8e769.
Everything up to here was verified on Node, which is the one runtime the migration was not for. This runs the published artefact - dist, not src, so whatever tsup did to the specifiers is in scope - on all three, through one plain script. node:assert/strict is the only testing API the three share, and vitest is Node-only, so a suite was never going to do this. Seven vectors signed and verified, a forgery rejected with the right code, a rotation where the retiring secret is the one that matches, and the synchronous throw on an unusable secret - that last one because signWebhook returning a promise from a plain function is a decision worth checking somewhere other than the Node suite. Ran locally: Node 24.20.0 - smoke ok, 7 vectors Deno 2.9.6 - smoke ok, 7 vectors Bun 1.4.2 - smoke ok, 7 vectors Zero flags on all three; Deno needed no permissions. The CI job pins denoland/setup-deno by commit because its "v2" is a branch rather than an immutable tag. Workers is not here: @cloudflare/vitest-plugin, which does run a plain library suite inside real workerd, needs vitest >= 4.1 and this repo is on 2. That is its own upgrade, not a bullet inside this one.
Comments only. The blinded compare is the one part of this migration a reader could reasonably think is over-engineered, so the comment now scopes it honestly rather than implying every runtime needed it. Every other target was read at source and is already constant time - workerd and Chromium CRYPTO_memcmp, Firefox NSS_SecureMemcmp, WebKit's openssl and gcrypt ports constantTimeMemcmp, Deno aws-lc, Node after b36d5a3d. The blind defends against unpatched Node and nothing else, and no remote exploit of the underlying bug turned up in any source. The dissent is recorded with it: Miller on the threat models differing, armfazh on this being an abuse of the API. Both are reasonable, and the reason they do not win is that we ship a library and do not pick our users' patch level. The citation is to the editor's draft by name, never /TR/, because the mandate is in neither the 2017 Recommendation nor the Level 2 FPWD. Also written down: the JS fold is not itself constant time and cannot be (CT-Wasm POPL 2019, Pornin ePrint 2025/435, and nodejs/node#38226 measuring t up to 37.9 on the native primitive when unrelated JS moved), which is the reason its inputs are blinded rather than secret. And the known-bad list as a list, so nobody re-adds a hex '===', a per-runtime conditional export or a three-year-stale vendored sha256 in a tidy-up.
The smoke job pinned setup-deno and then took setup-bun on a floating ref, which is the worse half of both options: it looks pinned and is not. oven-sh/setup-bun@v2 is a tag rather than a branch, so it differs from the setup-deno case in form but not in effect - a tag can be repointed, and neither ref tells you which code will run. Both are now commits with the ref they came from in a trailing comment. Refs resolved today, each read back from the API rather than taken on trust: oven-sh/setup-bun v2 -> 0c5077e51419868618aeaa5fe8019c62421857d6 denoland/setup-deno v2 -> 22d081ff2d3a40755e97629de92e3bcbfa7cf2ed actions/checkout v7 -> 3d3c42e5aac5ba805825da76410c181273ba90b1 actions/setup-node v7 -> 820762786026740c76f36085b0efc47a31fe5020 checkout and setup-node stay on their tags. They are first-party actions under the same org as the runner, the repo has no secrets for a workflow to exfiltrate, and pinning them would mean a dependabot entry for every bump. Their resolved commits are recorded above so a future reader can tell what this branch was tested against.
getSubtle throws it, so a consumer can hit it, and until now the only ways to catch it specifically were matching the message or reading .name off an Error - both of which break the next time the wording changes. It is now reachable by name like every other error class this library throws. CHANGELOG item for A/4: new public export, additive, nothing to act on. It is exported from crypto.js rather than moved into errors.js, and the comment at the export says why. Everything in errors.js is a WebhookError, and the adapters answer a WebhookError with 401 and anything else with 500. A runtime with no Web Crypto is the receiver being broken, not a signature being wrong, so it has to stay outside that hierarchy - and next to the one function that throws it, rather than in the file where somebody tidying up would reasonably make it extend the base class. public-api.test.ts asserts the negative directly, since that is the property a refactor would break. The smoke script imports it too. It cannot be triggered on Node, Deno or Bun - they all have Web Crypto, which is the point of the port - but it is the export most likely to be lost to a bundler or an export-condition mistake, being the only one re-exported from a different module. So all three runtimes now check it arrived and is still not a WebhookError.
The guard looked at each fc.assert, decided it was asynchronous if
fc.asyncProperty appeared in the next 200 characters, and required the six
characters before it to be "await ". Every part of that was wrong in a way
the four review cases show:
const p = fc.asyncProperty(...); fc.assert(p) missed - no property near
the assert
a comment longer than 200 chars between them missed - outside the window
a sync assert on the line before an async one blamed for its neighbour
await Promise.all([fc.assert(...), ...]) flagged, though correct
Two false negatives and two false positives, and the false positives are the
worse half: a guard that cries wolf gets its one assertion deleted.
It now scans fc.asyncProperty sites and asks whether anything in the
enclosing statement awaits them. The statement is found by walking backwards
with bracket depth, so it steps over balanced groups - a previous argument, a
preceding callback body - and continues outward through the ( and [ that
enclose the call, which is how the Promise.all form is reached from inside.
Comments are blanked first, so prose between the two calls neither hides the
property nor supplies the word await; only block comments and comments that
own their line, because // inside a string is a real thing in these files
(https://example.com is in the vectors) and eating to end of line there could
swallow a genuine await.
Scanning from the property is also what makes the hoisted case visible at
all. It is stricter than strictly necessary - hoisting and then awaiting the
assert is correct and still reported - and that is the trade: resolving it
would mean following assignments, and inlining the property costs nothing.
Reported position moves from the assert to the property, which is the site
being scanned. Re-checked against the mutation the guard exists for: removing
the real await in canonical.property.test.ts still fails it, now naming
line 192.
It called getSubtle twice and asserted the two results matched, which is true of a cached value and a re-read one alike. It would have passed against the implementation it exists to rule out. It now swaps globalThis.crypto for a stand-in mid-test, asserts getSubtle follows it rather than the object it first saw, and restores in a finally so a failing expect cannot leave a fake SubtleCrypto behind for the rest of the file. Why it matters: on Workers the module body runs outside a request, so a SubtleCrypto captured at import belongs to an isolate that may be gone by the time a request uses it. Checked it fails: adding a `cached ??=` to getSubtle turns this red - along with the two guard tests, which had been carrying the property on their own by accident.
The dedupe compared every new entry against every kept one and only checked the cap after the whole list had been walked, so a caller who passed ten thousand secrets got the full quadratic scan done for them before being told sixteen was the limit. The cap now applies as each distinct entry is added. The count comes out of the message with it. "got 500" was only ever true because the loop had run to the end; keeping it while stopping early would have printed "got 17" for a list of any size, which is worse than not saying. The limit is the useful half and it is what the message states. Behaviour is otherwise unchanged: sixteen distinct entries still pass, seventeen still throw, and forty copies of one secret still collapse to one - duplicates never increment the count, so they cannot trip the cap. The existing test matches on /more than 16/ and needed no edit.
src annotated getSubtle as SubtleCrypto and importHmacKey as CryptoKey. Neither is a global under lib: ["ES2022"]; both resolve through @types/node, and only from the 25 line. On 22 - which is this package's own engines floor - all three of SubtleCrypto, CryptoKey and BufferSource are undeclared, so the code compiled solely because a devDependency happened to be two majors ahead of the runtime the package claims to support. That is the sort of thing that stays invisible until someone matches their types to their engines. Adding "DOM" to lib would supply the names and also admit every browser-only API into src without anything noticing, which is the worse trade for a library whose entire point is running outside a browser. So the three types are read off globalThis.crypto instead - the object the module actually calls. It cannot drift from the implementation, it needs no lib change, and it compiles on every @types/node in range. BufferSource was already being derived this way; this extends the same trick to the other two. No public surface moves: getSubtle and importHmacKey are internal, and the emitted .d.ts is unchanged.
engines says >=22 and the types said ^25, so the build was checked against declarations for a runtime two majors past the oldest one supported. Now 22, resolving 22.20.1. The bump is what turned up the SubtleCrypto and CryptoKey dependency fixed in the commit before this one; with that in place typecheck, test and build are all clean on the 22 line. Worth doing in that order rather than reaching for "lib": ["DOM"] to make the error go away. npm reflowed the "files" array on install; biome put it back.
The comment sat above the length XOR and talked about length mismatches, so it read as though that term were the defence against them. It is not - the blinding is. Operands of different lengths become two different 32-byte digests and fail on content, with no early return and nothing leaked about which was longer. What the term is for is a future change of digest size: without it a shorter run could compare equal to a prefix of a longer one. Under SHA-256 it is a constant and costs nothing. Worth saying, because a reader who believed the old comment might conclude the blinding was redundant and take it out. Also hoists the getSubtle call in hmacSha256, which was fetching the same object twice per signature. Cosmetic.
Both directory scanners built a filesystem path by taking .pathname off a file URL. That leaves percent-encoding in place, so a checkout under a directory with a space in its name gives readdirSync a path containing %20 and it fails on a directory that does not exist. It happens to work here because this path has no characters needing escapes, which is the kind of thing that holds until someone clones into "My Projects". fileURLToPath is the conversion that exists for this and it also gets the drive-letter and UNC cases right on Windows.
The comment explained that both v2 refs are movable, which invited the obvious question about checkout and setup-node sitting on v7. It now states the actual rule: SHA pins are for the third-party actions, and the GitHub-owned ones ride their major tags by choice. One sentence, no pins changed.
Add automated release workflow using npm trusted publishing (OIDC) for supply-chain
security. Workflow triggers on GitHub release creation and supports manual dry-run
via workflow_dispatch. Includes version guard to fail if package.json version
doesn't match the release tag, and previews the package contents in the job summary.
One-time npm setup required:
1. Log in to npmjs.com with the account owner
2. Navigate to Account Settings → Publishing access → Trusted publisher
3. Add GitHub Actions with these settings:
- Owner: JosephDoUrden
- Repository: webhook-hmac-kit
- Workflow name: release.yml
4. Save. The workflow will now authenticate via OIDC without a token.
The runner upgrade step ensures npm ≥11.5.1 (required for OIDC; Node 24 ships
npm 10.x). Provenance attestation is automatic.
The whole document still described v1: the colon-delimited canonical
string, a singular secret, a version option, a synchronous signWebhook,
and an Express example that decodes the body with toString('utf-8') before
verifying it — the exact pattern the injectivity fix removed. None of it
matched the code on this branch.
Rewritten for the dot-delimited wire format, secrets as a list with
rotation, the three adapters, the Web Crypto runtime requirement and its
--no-experimental-global-webcrypto escape hatch, the Nest exception
mismatch, the Double-HMAC compare and why it exists, and a threat model
that states what the signature does and does not cover now that the
payload is bytes end to end.
package.json still said 1.0.0 and there was no entry at all for a branch that changes the wire format, the public API and the adapter contract. Two prior review passes each produced an exhaustive diff of what changed; merged the two lists, deduplicated, and grouped as Breaking / Added / Changed / Fixed / Build. The Fixed section leads with the canonical-string injectivity bug, written out honestly: the signer and verifier UTF-8-encoded the canonical string before hashing it, UTF-8 encoding of a JS string is not injective, and two different payloads could produce one signature.
Supported versions still listed 1.x, and there was no mention of why verification compares signatures the way it does. Marked 2.x as supported and 1.x as not (different wire format, no backports), and added the Double-HMAC-blind rationale: the Web Crypto constant-time requirement is editor's-draft-only, Node shipped a plain memcmp until CVE-2026-21713, and this library's own compare does not depend on which patch level a caller runs. Reporting route and out-of-scope items carried over unchanged.
It still described the 1.0.0 shape: Node >=18, no adapters, a colon canonical string, a single secret string, and no mention of Web Crypto at all. Updated the architecture section to match the current code, and added the guidance that only exists because of mistakes made on this branch: why there's no node: fallback, why signWebhook stays a plain function instead of async, what the await-guard and the migration oracle each protect, and why test/vectors.ts is the wire contract and not just another test file.
Wire format, public API and adapter contract all changed on this branch. package.json still read 1.0.0 the whole time. files and exports are untouched; the description didn't mention Node 18 or a synchronous API so it needed no wording change, just the version.
Four fixes addressing coordinator feedback:
1. Drop workflow_dispatch dry-run input (boolean type safety issue):
- workflow_dispatch now always runs npm publish --dry-run --provenance
- Real publish only on release: published
- Removes unreachable code path where string 'true' was compared to boolean input
2. Fix script injection in version check:
- Pass github.event_name and github.ref_name via env: block
- Use $EVENT_NAME and $TAG in shell (no direct ${{ }} interpolation in run:)
3. Pin npm upgrade to npm@11 (not @latest):
- npm 11.5.1+ required for OIDC; Node 24 ships npm 10.x
- Comment clarifies the 11.5.1+ floor
4. Fix comment inconsistency: Node 22 → Node 24 (job uses Node 24)
Simplify version check: on dispatch, just print version instead of verifying
against itself. Single npm pack --dry-run call (output only to summary).
The Fixed section had the UTF-8/surrogate collapse in front, found and fixed on this branch. The actual headline bug for 2.0.0 is older and worse: the original audit against 1.0.0 found that the colon-delimited v1 canonical string let a payload containing a colon re-split into a different (nonce, payload) pair with a byte-identical signature, so a single captured message replayed past a correctly implemented nonce cache — three accepted deliveries against a working Set-backed validator. That's what the dot delimiter and the dot-free nonce grammar actually fixed. Reordered so it leads, kept the UTF-8 item as the second, unrelated fix it is, left everything else in the section alone.
Quick Start signed with an inline crypto.randomUUID() and an inline timestamp, then verified against headers that nothing in the example had set, and computed the timestamp a second time to describe it. Pulled both into named consts, computed once, and named the three headers they go into so the sender and receiver sides read as the same request. Corrected the "both functions are async" line: signWebhook is a plain function that validates synchronously and returns a promise; verifyWebhook is async throughout. Corrected the test-vectors line: the canonical string in test/vectors.ts exists for each text payload, not for the byte one. Gave the Web Crypto remedy as ESM (the package is "type": "module"), mentioning the CommonJS form in one clause instead of leading with it. Documented the Nest useFactory requirement: WebhookGuard takes its options as a constructor argument, so registering it as a bare class provider (which is what WebhookModule.forRoot does today) leaves Nest unable to construct it. Left the adapter code alone and showed the useFactory wiring a reader needs instead. Also swept every em dash out of the file per the register, replaced with a full stop, colon or comma depending on what the sentence needed.
WebCryptoUnavailableError told a caller to fix a missing global with
require('node:crypto').webcrypto, in a package published as "type":
"module". Leads with the ESM form now, import { webcrypto } from
'node:crypto'; globalThis.crypto ??= webcrypto, and names the CommonJS
form in the closing clause for anyone importing this library from CJS.
That ESM sample is real import syntax as text, so it trips the same
node: specifier scan that exists to keep this module bundler-safe. Scan
now strips the one known example line before checking, the same way it
already special-cased the old require() text, instead of loosening the
pattern everywhere. Updated the message-text assertion to check for both
forms.
The entry was comparing against the git-main state rather than what npm 1.0.0 actually shipped: dist/index.* only, no adapters. The adapters merged to main later and were never published, so their behaviour (the uniform 401, the response body shape, the parsed-body refusal, duplicate headers, header lower-casing) is new to npm, not a break from it. Moved all of it into Added, with a note up top stating the real baseline. The four Buffer-to-Uint8Array bullets in Breaking, and the matching two in Changed, described types that never shipped either (buildCanonicalBytes, ParsedSignature.digest, formatSignature, normalizeSecrets, and every adapter, are all new in this release). Folded into one line under Added. Corrected the secret and version bullets against what the code actually does: a bare `secret` now fails with the each-secret message, not the empty-list one, and 1.0.0's version option fed the canonical string directly rather than being ignored. Corrected the buildCanonicalString bullet: the break is that it is no longer exported, not an arity change. Added the breaks the diff had missed: DEFAULT_VERSION's removal, the tightened timestamp validation, signWebhook now validating timestamp and nonce (1.0.0 checked only the secret), the two new WebhookErrorCode members breaking an exhaustive switch, and the verification cost change. Added one clause to the re-split fix: the same bug silently truncated the parsed payload for anything that wasn't JSON. Also swept every em dash out of the file, including the 1.0.0 section.
npm run test:coverage failed outright: @vitest/coverage-v8 was never added as a dependency, and CI never calls the script so nothing caught it. Removed the script from package.json and the line documenting it in CLAUDE.md rather than adding the dependency, since nothing else in this release needed coverage numbers.
The register calls for none. Replaced each with a full stop, colon or comma depending on the sentence, no wording changes beyond that.
Still called the library "production-ready" without saying what it does, and said nothing about Web Crypto or the adapters. Replaced with a description that names the actual feature set.
Same register cleanup as the rest of this pass. Replaced each with a comma or colon, no wording changes beyond that.
CHANGELOG said the tightened timestamp validation throws a TypeError everywhere; that's only true on the sign side. verifyWebhook throws WebhookTimestampError / WEBHOOK_TIMESTAMP_INVALID instead, so the two paths are now named separately. README and SECURITY.md both had a comma splice right after citing the patched Node versions and the editor's-draft PR, joining what should be two sentences. Split both. Moved the five adapter bug fixes (Fastify's unreturned reply, the Nest raw-body escape, Express's then/catch, resolveRawBody's Uint8Array rejection, and an onError that could swallow its own throw) out of Fixed and under the Added adapters entry as a short list of what got caught before any of it reached npm. None of that code was ever published, so it was never a bug an npm consumer hit. Fixed now holds only what changed for someone who had 1.0.0 installed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The v1 canonical string joined the fields with a colon that the nonce and payload could both contain, so one signed message could be re-split into a different nonce and payload and replayed past a correct nonce cache. This is the fix, plus everything the fix pulled in.
Wire format is now v2.{ts}.{nonce}.{payload} as bytes, nonce limited to [A-Za-z0-9_-]{1,64}, signature header is v2=<64 lower-case hex>. Secrets are a list so you can rotate. Every failure is a 401 with one body, the reason goes to onError.
Crypto moved from node:crypto to Web Crypto, one code path, so it runs on Node 22+, Workers, Deno and Bun. signWebhook returns a promise now. Compare is a blinded HMAC compare rather than trusting the runtime, see SECURITY.md for why.
README, CHANGELOG and SECURITY rewritten. 322 tests. Smoke on Node 24, Deno 2.9.6 and Bun 1.4.2 against dist.
Breaking, so 2.0.0. Release goes through the new release.yml once trusted publishing is set up on npm.