Skip to content

fix(oauth): treat naive datetime values as UTC when converting to timestamps - #1970

Open
kwy404 wants to merge 3 commits into
slackapi:mainfrom
kwy404:fix-naive-datetime-timestamp
Open

kwy404 wants to merge 3 commits into
slackapi:mainfrom
kwy404:fix-naive-datetime-timestamp

Conversation

@kwy404

@kwy404 kwy404 commented Sep 24, 2026

Copy link
Copy Markdown

Summary

SQL-based installation stores save expiration and install times as naive UTC datetimes, but _timestamp_to_type converted naive datetimes with datetime.timestamp(), which assumes local time. On servers not running in UTC this shifted bot_token_expires_at and user_token_expires_at by the local offset, so token rotation could skip an already expired token. Naive datetimes are now treated as UTC, matching how naive ISO strings are already handled.

Fixes #1731

Testing

Added a naive datetime case to test_timestamp_to_type in tests/slack_sdk/oauth/installation_store/test_internals.py; it fails before the change on a machine outside UTC and passes after. The oauth test suites (sync and async), ruff format --check, ruff check and scripts/run_mypy.sh were run locally.

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.sh after making the changes.

@kwy404
kwy404 requested a review from a team as a code owner September 24, 2026 23:43
@salesforce-cla

Copy link
Copy Markdown

Thanks for the contribution! Before we can merge this, we need @kwy404 to sign the Salesforce Inc. Contributor License Agreement.

@AmyScript AmyScript left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice catch, normalizing to UTC with .replace(tzinfo=timezone.utc) is exactly right. A couple of small things:

On the test (test_internals.py)

One thing to flag: this case expects the same value as the existing tz-aware one, so if CI runs in UTC it'll pass even without the fix. Could we force a non-UTC timezone so it fails on main? Something like:

@mock.patch.dict(os.environ, {"TZ": "America/New_York"})
def test_naive_datetime_is_treated_as_utc(self):
    time.tzset()  # POSIX only
    try:
        assert _timestamp_to_type(datetime(2023, 11, 28, 22, 9, 7), int) == 1701209347
    finally:
        del os.environ["TZ"]
        time.tzset()

On the string branch (internals.py)

Quick question: does the isinstance(ts, str) branch just below have the same issue? If it parses to a naive datetime and calls .timestamp(), it'd hit the same local-time bug. Might be worth normalizing it the same way for consistency.

Nit: could be nice to mention this in the changelog, since it slightly changes behavior for anyone who was relying on naive = local time.

@kwy404

kwy404 commented Sep 25, 2026

Copy link
Copy Markdown
Author

Thanks for the review!

  • Test: good catch, the parametrized case passed on main whenever the runner was in UTC. I replaced it in 1c008a4 with test_timestamp_to_type_naive_datetime_is_utc, which sets TZ=America/New_York and calls time.tzset(). On main that datetime comes out as 1701227347 (5 hours off) instead of 1701209347, so it fails without the fix. It is skipped where time.tzset does not exist (Windows); CI runs on Ubuntu.
  • String branch: it already goes through _from_iso_format_to_datetime, which appends +00:00 when the string has no offset, so naive strings are parsed as UTC. The "2023-11-28 22:11:19" case in the same test covers that.
  • Changelog: as far as I can tell the release notes are generated from merged PRs (.github/release.yml) and there is no changelog file to update, but happy to add a note wherever the maintainers prefer.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Token rotation failed to refresh the token in a certain time period when using SQLAlchemyInstallationStore

2 participants