fix: Correct timestamp unit conversion in signature verification#1579
fix: Correct timestamp unit conversion in signature verification#1579
Conversation
Original prompt from Mark Tran
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughWalkthroughAdds bidirectional timestamp tolerance checks to signature verification, converts parsed microsecond timestamps to milliseconds, and adds tests covering old, future-dated, and within-tolerance timestamps, including webhook-level rejection for out-of-tolerance signatures. Changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
devin-ai-integration
Bot
changed the title
Greptile SummaryThis PR fixes a unit mismatch in Confidence Score: 5/5Safe to merge — the fix is correct, well-tested, and introduces no new risk. No P0 or P1 findings. The unit conversion (μs ÷ 1000 → ms) correctly aligns both operands of the tolerance comparison. The bidirectional bounds check and the three new test scenarios (too-old, future, within-tolerance) all exercise the right arithmetic. JavaScript's Number safely handles current Unix microsecond values (~1.7 × 10¹⁵, well below Number.MAX_SAFE_INTEGER ≈ 9 × 10¹⁵). No files require special attention. Important Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/webhooks/webhooks.spec.ts (1)
175-176: ⚡ Quick winMake this replay test independent from default tolerance values.
Set
toleranceexplicitly inoptionsso this assertion only validates replay behavior, not whichever default is configured later.Proposed patch
- const options = { payload, sigHeader, secret }; + const options = { payload, sigHeader, secret, tolerance: 180000 };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhooks/webhooks.spec.ts` around lines 175 - 176, The replay test currently constructs options as { payload, sigHeader, secret } which relies on external defaults for tolerance; update the test to include an explicit tolerance field (e.g., { payload, sigHeader, secret, tolerance: <number> }) so the assertion validates replay behavior only and not whatever default tolerance may change; modify the options object used in the test (where options is declared) to include the chosen numeric tolerance value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/webhooks/webhooks.spec.ts`:
- Around line 175-176: The replay test currently constructs options as {
payload, sigHeader, secret } which relies on external defaults for tolerance;
update the test to include an explicit tolerance field (e.g., { payload,
sigHeader, secret, tolerance: <number> }) so the assertion validates replay
behavior only and not whatever default tolerance may change; modify the options
object used in the test (where options is declared) to include the chosen
numeric tolerance value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c1b7a82-ac8d-4148-a469-a1407ad901b7
📒 Files selected for processing (3)
src/common/crypto/signature-provider.spec.tssrc/common/crypto/signature-provider.tssrc/webhooks/webhooks.spec.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
devin-ai-integration
Bot
changed the title
Convert parsed timestamps to the correct unit before comparing against Date.now() in SignatureProvider.verifyHeader. Also add a bidirectional bounds check and tests for the timestamp validation logic. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
Bot
force-pushed
the
devin/1777619733-fix-signature-replay-protection
branch
from
cffa97e to
a98807d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/crypto/signature-provider.ts`:
- Around line 32-42: Reject malformed non-numeric timestamps before doing the
tolerance check: validate the incoming timestamp string (the timestamp variable)
is a pure integer string (e.g. /^\d+$/) or otherwise convert safely and check
Number.isFinite/Number.isInteger, and if invalid throw
SignatureVerificationException immediately; then continue to compute timestampMs
(convert microseconds to milliseconds) and compare against now ± tolerance using
the existing timestampMs and tolerance variables. Ensure the validation is
placed before the parseInt/Math.floor step so malformed t= values cannot bypass
the replay-protection logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: daeb3aac-81a5-423e-8924-161e19fa345f
📒 Files selected for processing (3)
src/common/crypto/signature-provider.spec.tssrc/common/crypto/signature-provider.tssrc/webhooks/webhooks.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/webhooks/webhooks.spec.ts
| // WorkOS emits microsecond timestamps; convert to milliseconds for comparison | ||
| const timestampMs = Math.floor(parseInt(timestamp, 10) / 1000); | ||
| const now = Date.now(); | ||
|
|
||
| if (timestampMs < now - tolerance) { | ||
| throw new SignatureVerificationException( | ||
| 'Timestamp outside the tolerance zone', | ||
| ); | ||
| } | ||
|
|
||
| if (timestampMs > now + tolerance) { |
There was a problem hiding this comment.
Reject malformed timestamps before comparing against the tolerance window.
parseInt() will coerce partially numeric input and returns NaN for invalid input, so a bad t= value can slip past the replay-protection check instead of failing fast. Please validate the timestamp format up front and reject non-numeric values before doing the range comparison.
🔧 Suggested fix
- const timestampMs = Math.floor(parseInt(timestamp, 10) / 1000);
+ if (!/^\d+$/.test(timestamp)) {
+ throw new SignatureVerificationException(
+ 'Invalid timestamp',
+ );
+ }
+ const timestampMs = Math.floor(Number(timestamp) / 1000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // WorkOS emits microsecond timestamps; convert to milliseconds for comparison | |
| const timestampMs = Math.floor(parseInt(timestamp, 10) / 1000); | |
| const now = Date.now(); | |
| if (timestampMs < now - tolerance) { | |
| throw new SignatureVerificationException( | |
| 'Timestamp outside the tolerance zone', | |
| ); | |
| } | |
| if (timestampMs > now + tolerance) { | |
| // WorkOS emits microsecond timestamps; convert to milliseconds for comparison | |
| if (!/^\d+$/.test(timestamp)) { | |
| throw new SignatureVerificationException( | |
| 'Invalid timestamp', | |
| ); | |
| } | |
| const timestampMs = Math.floor(Number(timestamp) / 1000); | |
| const now = Date.now(); | |
| if (timestampMs < now - tolerance) { | |
| throw new SignatureVerificationException( | |
| 'Timestamp outside the tolerance zone', | |
| ); | |
| } | |
| if (timestampMs > now + tolerance) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/crypto/signature-provider.ts` around lines 32 - 42, Reject
malformed non-numeric timestamps before doing the tolerance check: validate the
incoming timestamp string (the timestamp variable) is a pure integer string
(e.g. /^\d+$/) or otherwise convert safely and check
Number.isFinite/Number.isInteger, and if invalid throw
SignatureVerificationException immediately; then continue to compute timestampMs
(convert microseconds to milliseconds) and compare against now ± tolerance using
the existing timestampMs and tolerance variables. Ensure the validation is
placed before the parseInt/Math.floor step so malformed t= values cannot bypass
the replay-protection logic.
1 participant
Description
Fixes a timestamp unit mismatch in
SignatureProvider.verifyHeader. The parsed timestamp was being compared directly againstDate.now()without accounting for the difference in precision. This corrects the comparison and adds a bidirectional bounds check.Also adds tests covering the timestamp validation logic.
Files changed
src/common/crypto/signature-provider.ts— fix timestamp comparisonsrc/common/crypto/signature-provider.spec.ts— timestamp validation testssrc/webhooks/webhooks.spec.ts— additional webhook testDocumentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
No documentation changes required.
Link to Devin session: https://app.devin.ai/sessions/d21d8fbacb954248b5c3f746110a2092
Requested by: @marktran
Summary by CodeRabbit
Bug Fixes
Tests