🐛 constant-time HMAC security fix

[ianardee]

Description

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Requires a change to the official Guide documentation.

[Copilot]

🟡 Changes recommended

Several unit tests currently assert on unresolved Promises (so they can’t fail when parsing breaks), and the new HMAC validation logic should be made more explicit/robust to avoid unexpected throws.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens LocalResponse HMAC verification by switching to a constant-time signature comparison and aligns both v1 and v2 LocalResponse implementations on a shared BaseLocalResponse, with expanded parsing/HMAC tests to cover new validation behavior.

Changes:

• Replace direct string equality with crypto.timingSafeEqual in BaseLocalResponse.isValidHmacSignature(), and add input validation for empty/null inputs.
• Rename and migrate v1/v2 LocalResponse to extend BaseLocalResponse.
• Expand v2 local response tests and add new v1 local response tests (string/file/buffer + prediction loading cases).

File summaries

File	Description
tests/v2/parsing/localResponse.spec.ts	Expands v2 local response tests for HMAC validation, error cases, and raw payload/string checks.
tests/v1/parsing/localResponse.spec.ts	Adds v1 local response tests covering load modes, HMAC checks, and prediction loading.
src/v2/parsing/localResponse.ts	Updates v2 LocalResponse to extend the new shared base class.
src/v1/parsing/localResponse.ts	Updates v1 LocalResponse to extend the new shared base class.
src/parsing/baseLocalResponse.ts	Introduces constant-time HMAC compare, adds constructor input validation, and exposes toString().

Review details

Suppressed comments (1)

src/parsing/baseLocalResponse.ts:110

• The secretKey/signature validation mixes || with a ternary without parentheses, which is hard to read and can be misinterpreted. Also, if a non-string/non-buffer object with a truthy length is passed as secretKey (e.g. {length: 1} via any), this check passes and getHmacSignature() can throw, even though isValidHmacSignature() returns a boolean type. Consider making the validation explicit and guarding the allowed runtime types before computing the HMAC.

• Files reviewed: 4/5 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

There are verified correctness issues in the new HMAC validation guard (operator precedence) and in the new v1 tests (missing await for asDict()), plus an unrelated pre-push hook behavior change not reflected in the PR description.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/parsing/baseLocalResponse.ts:110

• The secretKey validation mixes || with a ternary without parentheses; due to operator precedence this expression is evaluated as (!secretKey || typeof secretKey === "string") ? ... : ..., which can incorrectly treat some non-string keys as strings (and vice-versa). This can make isValidHmacSignature return incorrect results for certain inputs.

• Files reviewed: 5/6 changed files
• Comments generated: 1
• Review effort level: Lite