Skip to content

fix: SignatureVerifier.is_valid raises ValueError on non-numeric time… #1968

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

Open
Shubham-Padkonde wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: SignatureVerifier.is_valid raises ValueError on non-numeric time… #1968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdfac64
fix: SignatureVerifier.is_valid raises ValueError on non-numeric time…
Shubham-Padkonde Sep 24, 2026
af94129
test: pin signature replay-window boundaries
Shubham-Padkonde Sep 24, 2026
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
6 changes: 5 additions & 1 deletion slack_sdk/signature/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,11 @@ def is_valid(
if timestamp is None or signature is None:
return False

if abs(self.clock.now() - int(timestamp)) > 60 * 5:
try:
ts = int(timestamp)
except (ValueError, TypeError):
return False
if abs(self.clock.now() - ts) > 60 * 5:
return False

calculated_signature = self.generate_signature(timestamp=timestamp, body=body)
Expand Down
29 changes: 29 additions & 0 deletions tests/slack_sdk/signature/test_signature_verifier.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import unittest
from unittest.mock import Mock

from slack_sdk.signature import SignatureVerifier

Expand Down Expand Up @@ -98,6 +99,26 @@ def test_is_valid_none(self):
self.assertFalse(verifier.is_valid(self.body, None, None))
self.assertFalse(verifier.is_valid(None, None, None))

def test_is_valid_non_numeric_timestamp(self):
verifier = SignatureVerifier(
signing_secret=self.signing_secret,
clock=MockClock(),
)
self.assertFalse(verifier.is_valid(self.body, "not-a-number", self.valid_signature))
self.assertFalse(verifier.is_valid(self.body, "1.5", self.valid_signature))
self.assertFalse(verifier.is_valid(self.body, "", self.valid_signature))

@WilliamBergamin WilliamBergamin Sep 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit (optional): Could we add a case showing that a valid numeric timestamp outside the 5-minute window still returns False? That would pin down that the new try block didn't change the replay check. Something like verifier.is_valid(self.body, "1531420618", self.valid_signature) with a clock that's well past it.


def test_is_valid_request_non_numeric_timestamp_header(self):

@WilliamBergamin WilliamBergamin Sep 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: is_valid_request just normalizes the headers and hands off to is_valid, so this test runs the same code path as the one above. I'm fine keeping it as the end-to-end check. If you do, can the comment on lines 113–114 go too? Same reason as above.

verifier = SignatureVerifier(
signing_secret=self.signing_secret,
clock=MockClock(),
)
bad_headers = {
"X-Slack-Request-Timestamp": "not-a-number",
"X-Slack-Signature": self.valid_signature,
}
self.assertFalse(verifier.is_valid_request(self.body, bad_headers))

def test_invalid_signing_secret(self):
with self.assertRaises(ValueError):
SignatureVerifier("")
Expand All @@ -117,3 +138,11 @@ def test_invalid_signing_secret_reassignment(self):
with self.assertRaises(ValueError):
verifier.signing_secret = None
self.assertEqual(verifier.signing_secret, self.signing_secret)

def test_is_valid_rejects_numeric_timestamp_outside_replay_window(self):
clock = Mock()
verifier = SignatureVerifier(signing_secret=self.signing_secret, clock=clock)
for elapsed, expected in ((300, True), (301, False), (-301, False)):
with self.subTest(elapsed=elapsed):
clock.now.return_value = int(self.timestamp) + elapsed
self.assertEqual(verifier.is_valid(self.body, self.timestamp, self.valid_signature), expected)