Skip to content

fix: Correct timestamp unit conversion in signature verification #1579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
marktran wants to merge 1 commit into from
+78 −1
Closed

fix: Correct timestamp unit conversion in signature verification #1579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/common/crypto/signature-provider.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,55 @@ describe('SignatureProvider', () => {
});
});

describe('replay protection', () => {
it('rejects a timestamp that is too old', async () => {
const oldTimestamp = (Date.now() - 300000) * 1000; // 5 minutes ago in microseconds
const unhashedString = `${oldTimestamp}.${JSON.stringify(payload)}`;
const oldSignatureHash = crypto
.createHmac('sha256', secret)
.update(unhashedString)
.digest()
.toString('hex');
const sigHeader = `t=${oldTimestamp}, v1=${oldSignatureHash}`;
const options = { payload, sigHeader, secret, tolerance: 180000 };

await expect(signatureProvider.verifyHeader(options)).rejects.toThrow(
'Timestamp outside the tolerance zone',
);
});

it('rejects a future-dated timestamp', async () => {
const futureTimestamp = (Date.now() + 300000) * 1000; // 5 minutes in the future in microseconds
const unhashedString = `${futureTimestamp}.${JSON.stringify(payload)}`;
const futureSignatureHash = crypto
.createHmac('sha256', secret)
.update(unhashedString)
.digest()
.toString('hex');
const sigHeader = `t=${futureTimestamp}, v1=${futureSignatureHash}`;
const options = { payload, sigHeader, secret, tolerance: 180000 };

await expect(signatureProvider.verifyHeader(options)).rejects.toThrow(
'Timestamp outside the tolerance zone',
);
});

it('accepts a timestamp within tolerance', async () => {
const recentTimestamp = (Date.now() - 60000) * 1000; // 1 minute ago in microseconds
const unhashedString = `${recentTimestamp}.${JSON.stringify(payload)}`;
const recentSignatureHash = crypto
.createHmac('sha256', secret)
.update(unhashedString)
.digest()
.toString('hex');
const sigHeader = `t=${recentTimestamp}, v1=${recentSignatureHash}`;
const options = { payload, sigHeader, secret, tolerance: 180000 };

const result = await signatureProvider.verifyHeader(options);
expect(result).toBeTruthy();
});
});

describe('when in an environment that supports SubtleCrypto', () => {
it('automatically uses the subtle crypto library', () => {
// tslint:disable-next-line
Expand Down
12 changes: 11 additions & 1 deletion src/common/crypto/signature-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,17 @@ export class SignatureProvider {
);
}

if (parseInt(timestamp, 10) < Date.now() - tolerance) {
// 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) {
Comment on lines +32 to +42
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 1, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
// 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.

throw new SignatureVerificationException(
'Timestamp outside the tolerance zone',
);
Expand Down
18 changes: 18 additions & 0 deletions src/webhooks/webhooks.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,24 @@ describe('Webhooks', () => {
);
});
});

describe('with a replayed old webhook', () => {
it('rejects a valid signature whose timestamp exceeds the tolerance', async () => {
const oldTimestamp = (Date.now() - 300000) * 1000; // 5 minutes ago in microseconds
const oldUnhashedString = `${oldTimestamp}.${JSON.stringify(payload)}`;
const oldSignatureHash = crypto
.createHmac('sha256', secret)
.update(oldUnhashedString)
.digest()
.toString('hex');
const sigHeader = `t=${oldTimestamp}, v1=${oldSignatureHash}`;
const options = { payload, sigHeader, secret };

await expect(workos.webhooks.constructEvent(options)).rejects.toThrow(
SignatureVerificationException,
);
});
});
});

describe('verifyHeader', () => {
Expand Down
Loading