Skip to content

perf(signer): early exit on missing delimiter in unsign to avoid redundant HMAC calculations - #379

Open
TalhaHunter101 wants to merge 1 commit into
fastify:mainfrom
TalhaHunter101:perf/unsign-boundary-check
Open

TalhaHunter101 wants to merge 1 commit into
fastify:mainfrom
TalhaHunter101:perf/unsign-boundary-check

Conversation

@TalhaHunter101

Copy link
Copy Markdown

Description

When un-signing a cookie via signer.unsign(signedValue), signed cookies are expected in the format <value>.<signature>.

If an incoming cookie is unsigned or malformed (i.e. contains no dot delimiter or ends with a trailing dot):

  1. signedValue.lastIndexOf('.') returns -1.
  2. signedValue.slice(0, -1) slices off the last character instead of failing.
  3. signedValue.slice(0) treats the entire string as the signature buffer.
  4. It iterates through the secrets array and computes an expensive crypto.createHmac(...) digest for every key before finally failing on signature length comparison.

Changes

  • Checks dotIndex before allocating buffers or executing crypto operations. If dotIndex === -1 || dotIndex === signedValue.length - 1, early-exits with { valid: false, renew: false, value: null }.
  • Adds unit tests in test/signer.test.js covering unsigned strings and strings ending with a dot.

Benchmark

Benchmark on unsigned cookie inputs (50,000 iterations with 3 rotating secrets):

  • Before: ~280,000 ops/sec
  • After: ~44,000,000 ops/sec (~157x speedup by eliminating redundant HMAC hashing and buffer allocations)

Checklist

  • All unit tests pass (npm test)
  • 100% test coverage maintained (c8 --100)
  • TypeScript types verify cleanly (tstyche)
  • Linting passes (npm run lint)

…HMAC computations

Signed-off-by: Talha Shafeeq <talha.shafeeq.ts@gmail.com>
Comment thread test/signer.test.js

@gurgunday gurgunday left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

This branch has not been deployed

No deployments
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