fix: SignatureVerifier.is_valid raises ValueError on non-numeric time… - #1968
Shubham-Padkonde wants to merge 2 commits into
Conversation
…stamp is_valid called int(timestamp) without catching ValueError, so any request with a non-numeric X-Slack-Request-Timestamp header (e.g. "not-a-number", an empty string, or a float like "1.5") caused an unhandled exception rather than a clean False return. Fix: wrap the int() conversion in a try/except (ValueError, TypeError) and return False on failure, consistent with how None timestamps are already handled. Add two tests covering the new behaviour: - test_is_valid_non_numeric_timestamp: direct is_valid() calls with invalid timestamp strings - test_is_valid_request_non_numeric_timestamp_header: end-to-end call via is_valid_request() with a crafted header
WilliamBergamin
left a comment
WilliamBergamin
left a comment
There was a problem hiding this comment.
Thanks for this @Shubham-Padkonde! 🙌
I reproduced this, a present but non-numeric X-Slack-Request-Timestamp header ("not-a-number", "", "1.5") makes is_valid raise ValueError instead of returning False. In bolt-python that bubbles out of the request verification middleware as a 500 rather than a clean 401. It fails closed either way, so there's no bypass, but returning False is the right behavior and the try/except is the minimal fix.
Left a few small nits. One more ask: could you fill in the PR template? Please tick the slack_sdk.signature and tests categories and the Requirements boxes, and confirm you've run ./scripts/run_validation.sh.
| # A non-integer X-Slack-Request-Timestamp header must cause is_valid to | ||
| # return False rather than raising ValueError from int(). |
There was a problem hiding this comment.
nit: The test name already says what this comment says, so I think we can drop it.
| # A non-integer X-Slack-Request-Timestamp header must cause is_valid to | |
| # return False rather than raising ValueError from int(). |
| self.assertFalse(verifier.is_valid(self.body, "1.5", self.valid_signature)) | ||
| self.assertFalse(verifier.is_valid(self.body, "", self.valid_signature)) | ||
|
|
||
| def test_is_valid_request_non_numeric_timestamp_header(self): |
There was a problem hiding this comment.
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.
| ) | ||
| 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)) |
There was a problem hiding this comment.
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.
|
Addressed in af94129: removed both redundant comments and added a deterministic replay-window test covering exactly 300 seconds, 301 seconds, and a future timestamp outside the window. All 15 signature tests and project lint pass. Prepared with Codex assistance. |
This branch has not been deployed
2 participants
Summary
Return False instead of raising for non-numeric request timestamps. Regression coverage also preserves the five-minute replay window.
Testing
./scripts/run_validation.sh tests/slack_sdk/signaturepasses all 15 tests in the isolated Windows Python 3.13 environment. Project lint passes for 455 files. This was a targeted validation run, not the full SDK suite; the script skips mypy/format on Python 3.13.Category
slack_sdk.web.WebClient (sync/async) (Web API client)
slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
slack_sdk.socket_mode (Socket Mode client)
slack_sdk.signature (Request Signature Verifier)
slack_sdk.oauth (OAuth Flow Utilities)
slack_sdk.models (UI component builders)
slack_sdk.scim (SCIM API client)
slack_sdk.audit_logs (Audit Logs API client)
slack_sdk.rtm_v2 (RTM client)
/docs(Documents)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements
I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've run
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.Prepared with OpenAI Codex assistance. The full-validation and contributor-agreement checkboxes are left unchecked; the scoped validation above is the work actually performed.