fix(crypto): reject malformed hex in timingSafeEqualHex

[jaso0n0818]

Summary

• timingSafeEqualHex now fails closed on empty, odd-length, or non-hex operands (fixes webhook signature weakness flagged in review)
• Keeps sha256/base64url helper coverage from the original test additions
• Adds verifyGitHubSignature cases for malformed sha256= digests

Review feedback addressed

• No longer asserts that unequal malformed hex strings (abc vs def) compare equal
• Superagent security concern about verifyGitHubSignature delegating to weak comparison is resolved at the source

Test plan

• npm test -- test/unit/crypto.test.ts

[ghost]

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal	Result	Evidence	Action
Gate result	⚠️ Skipped	#661 is no longer open.	No action.

---

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

[superagent-security]

P0: Test asserts timing-safe hex comparison returns true for unequal malformed inputs

Test expects timingSafeEqualHex("abc","def") to be true, but the underlying hexToBytes returns empty arrays for odd-length strings, making any two malformed odd-length hex strings compare equal.

Fix hexToBytes to explicitly reject malformed input and update the test to expect false for unequal odd-length strings.

AI prompt

Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="test/unit/crypto.test.ts">
<violation number="1" location="test/unit/crypto.test.ts:18">
<priority>P0</priority>
<title>Test asserts timing-safe hex comparison returns true for unequal malformed inputs</title>
<evidence>The test currently expects timingSafeEqualHex("abc", "def") to return true. However, the underlying hexToBytes function in src/utils/crypto.ts returns an empty Uint8Array when hex.length % 2 !== 0. Consequently, timingSafeEqualHex receives two zero-length arrays, skips the comparison loop, and returns true. This means any two odd-length hex strings—including completely different values—compare as equal, breaking the security guarantees of the function.</evidence>
<recommendation>Fix hexToBytes to throw an error (or return a sentinel value handled by timingSafeEqualHex) when given malformed/odd-length hex input, so that timingSafeEqualHex never silently returns true for unequal malformed strings. Then update the test to expect false (or an exception) for timingSafeEqualHex("abc", "def").</recommendation>
</violation>
</file>

[JSONbored]

PR #661 — test(crypto): cover sha256, hex compare, and base64url helpers
Verdict: REQUEST CHANGES
Resolves issue: N/A (test-only)
CI: validate/test/lint/workers success, BUT Superagent Security Scan = action_required (real concern, not informational). gittensory/Context neutral.

Highlights:

• Otherwise-reasonable coverage for sha256Hex (format + determinism), base64UrlEncode (string + bytes, no padding), and the equal/unequal/length-mismatch branches of timingSafeEqualHex.

Concerns / required changes:

• BLOCKER — P0 STILL PRESENT on current head (verified test/unit/crypto.test.ts line 19 + src/utils/crypto.ts): the test asserts timingSafeEqualHex("abc","def") === true. Both inputs are odd-length, so hexToBytes returns an empty Uint8Array for each; equal length (0) + empty loop -> result 0 -> true. This codifies as "expected" that two DIFFERENT malformed hex strings compare equal. Since verifyGitHubSignature delegates to timingSafeEqualHex, it locks in a webhook-signature-verification weakness.
• Required fix: malformed/odd-length hex must NOT compare equal — either hexToBytes signals invalidity (and timingSafeEqualHex returns false on invalid input), or the test asserts === false and the implementation is corrected to match. Do not merge a test that ratifies the broken behavior.

Completeness vs issue: As pure coverage it is broad, but it actively asserts insecure behavior as correct, so it must not be merged in its current form.

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.60%. Comparing base (22e4583) to head (69b4e41).

Files with missing lines	Patch %	Lines
src/utils/crypto.ts	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
- Coverage   96.60%   96.60%   -0.01%     
==========================================
  Files         109      109              
  Lines       14895    14898       +3     
  Branches     5384     5386       +2     
==========================================
+ Hits        14390    14392       +2     
  Misses        104      104              
- Partials      401      402       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JSONbored]

Good:

• Adds useful crypto helper coverage around sha256, base64url, normal equality, unequal hex, and length mismatch.
• Normal CI/Codecov are green.

Bad:

• Existing owner review requested changes, still unresolved.
• Superagent Security Scan is ACTION_REQUIRED.
• Test asserts timingSafeEqualHex("abc", "def") === true, codifying that two different malformed odd-length hex strings compare equal.
• verifyGitHubSignature delegates to timingSafeEqualHex, so this locks in webhook-signature weakness.

Change requests:

• Make malformed/odd-length hex compare false.
• Update implementation and test together; do not merge a test that ratifies the broken behavior.
• Add explicit invalid hex/empty hex coverage once semantics are fixed.

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[jaso0n0818]

Addressed the review feedback:

• timingSafeEqualHex now fails closed on empty, odd-length, or non-hex operands (no more equal comparison for malformed digests like abc vs def).
• Added verifyGitHubSignature cases for malformed sha256= digests.
• Rebased on latest main; validate + unit tests green.

Ready for another look when you have a moment.

[superagent-security]

⚠️ Contributor trust inconclusive. Click here for more info: Superagent Dashboard

[JSONbored]

Hi there, codecov CI is still failing due to inadequate test coverage, can you please resolve this. Thanks!

[ghost]

reviewbot · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Changed files: src/utils/crypto.ts, test/unit/crypto.test.ts

Suggested action: ✅ Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR tightens hex handling by making hexToBytes return null for malformed input and updates timingSafeEqualHex to reject such inputs, adding comprehensive tests for crypto utilities and edge‑case signature handling.

Suggestions

• Consider adding a brief comment to hexToBytes explaining the null return contract.
• If any internal modules import hexToBytes directly, update them to handle the possible null return.

Worth double-checking

• Other code paths that might call hexToBytes could now receive null instead of an empty Uint8Array; verify no such callers exist.

Reviewer B · mistral-small-3.1-24b-instruct — recommends ✅ merge
The PR adds validation to reject malformed hex strings in the timingSafeEqualHex function and includes corresponding tests. The changes are clean and well-targeted.

Suggestions

• Consider adding a comment above the hexToBytes function to explain the validation logic, especially the check for Number.isNaN(byte).

Worth double-checking

• Verify that the new validation logic covers all edge cases for malformed hex strings.
• Ensure that the tests adequately cover both valid and invalid hex string inputs.