Skip to content

Conversation

@bdesoky
Copy link
Contributor

@bdesoky bdesoky commented Dec 3, 2025

Ticket: ANT-1352

Context: during local v3 auth testing I ran into the error server response outside response validity time window, possible man-in-the-middle-attack which happened because the response timestamp logged was 1764777803921 and the Date.now() timestamp that verifyResponse used was 1764777803898. So, the response timestamp was slightly ahead, even though verifyResponse is called after. This is likely server-client clock skew, this fix adds a forward validity check of 60s to accommodate this.

Description

This pull request updates the HMAC utility functions to support a more flexible validity window for timestamp verification, allowing both backward and forward checks. It also adds an optional useOriginalPath parameter to key functions for more control over HMAC subject calculation, and expands the test suite to cover the new validity window logic.

HMAC validity window improvements:

  • The verifyResponse function now checks if the timestamp is within a 5-minute backward window and a 1-minute forward window, improving support for minor clock drift between systems.
  • The test suite adds cases for forward validity window checks, ensuring that timestamps up to 1 minute in the future are considered valid, and those beyond are invalid. [1] [2]

API flexibility enhancements:

  • The calculateRequestHMAC, calculateRequestHeaders, and verifyResponse functions now accept an optional useOriginalPath parameter to keep them in line with calculateHMACSubject, allowing callers to specify whether to use the original URL path when calculating the HMAC subject. [1] [2] [3]

Issue Number

ANT-1352

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Unit test

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My code compiles correctly for both Node and Browser environments
  • I have commented my code, particularly in hard-to-understand areas
  • My commits follow Conventional Commits and I have properly described any BREAKING CHANGES
  • The ticket or github issue was included in the commit message as a reference
  • I have made corresponding changes to the documentation and on any new/updated functions and/or methods - jsdoc
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@bdesoky bdesoky requested review from a team as code owners December 3, 2025 16:38
@bdesoky
Copy link
Contributor Author

bdesoky commented Dec 3, 2025

not sure why CI is failing, looks like its coming from

224 passing (5s)
@bitgo/sdk-coin-eth:   1 failing
@bitgo/sdk-coin-eth:   1) ETH:
@bitgo/sdk-coin-eth:        TSS Transaction Verification
@bitgo/sdk-coin-eth:          consolidationToBaseAddress verification
@bitgo/sdk-coin-eth:            should reject consolidation when recipient does not match base address:
@bitgo/sdk-coin-eth:      AssertionError: expected [Promise] to be rejected
@bitgo/sdk-coin-eth:       at Assertion.fail (/home/runner/work/BitGoJS/BitGoJS/node_modules/should/cjs/should.js:275:17)
@bitgo/sdk-coin-eth:       at /home/runner/work/BitGoJS/BitGoJS/node_modules/should/cjs/should.js:1747:16
@bitgo/sdk-coin-eth:       at async Context.<anonymous> (test/unit/eth.ts:961:9)

which seems unrelated to these changes 🤔

@bdesoky bdesoky merged commit 3075ec1 into master Dec 4, 2025
14 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.

3 participants