Fix timedelta date headers (e.g. Response.expires) to be UTC-based (fixes #430) - #491
Fix timedelta date headers (e.g. Response.expires) to be UTC-based (fixes #430)#491apoorvdarshan wants to merge 1 commit into
Conversation
serialize_date() added a timedelta to naive local time (_now()) but then serialized the result as GMT (usegmt=True, via calendar.timegm), so setting a date header such as Response.expires from a timedelta produced a value off by the machine's UTC offset. Add the delta to UTC now instead, matching Response._cache_expires, which already uses utcnow(). A new hook point (_utcnow) mirrors the existing _now hook. Adds a regression test that serializes a zero timedelta under a non-UTC timezone and asserts the header matches UTC now, and updates the existing serialize_date test to compute its expected value from utcnow() so it is timezone-independent. Signed-off-by: Apoorv Darshan <ad13dtu@gmail.com>
|
Thank you for this — the diagnosis in your PR description was exactly right, and the tz-sensitive test approach was the thing that made the bug demonstrable. There were three overlapping PRs open against #473 and #430 (this one, #475 and #480), all touching the same lines, so none could be merged alongside the others. I've combined them into #496, where your commit is preserved with you as the author. Two adjustments worth flagging:
Verified end-to-end under Closing here since the work continues in #496 — it's open for review, not merged yet. |
serialize_date() added the delta to _now(), which was datetime.now and so naive *local* time. The result is then fed to calendar.timegm (which reads its argument as UTC) and serialized with usegmt=True, so the header came out off by the machine's UTC offset. Adapted from #491 for this branch: - Rather than adding a _utcnow = datetime.utcnow hook, _now is now the utcnow() helper introduced alongside the #473 deprecation work. #491's original form would have reintroduced the deprecated call this branch removes, in the same module. - Repointing _now fixes the second occurrence of the same bug, in parse_date_delta(), which resolved delta seconds against local time and made e.g. Response.retry_after return an instant off by the UTC offset. Covered by test_parse_date_delta_is_utc_based. Keeping the single _now hook means the existing _NowRestorer tests continue to apply to both call sites.
Fixes #430
Root cause
serialize_date()insrc/webob/datetime_utils.pyhandles atimedeltaby adding it to_now(), where_now = datetime.nowreturns naive local time:The local wall-clock time is then fed to
calendar.timegm(which treats its argument as UTC) and serialized withusegmt=True. So a local time is mislabeled as GMT, and the resulting header is off by the machine's UTC offset.Response.expires = <timedelta>goes through this path, as does anything that sets a date header from atimedelta.Response.cache_expiresalready does the right thing by usingdatetime.utcnow().Reproduction
Running under a non-UTC timezone (here
America/New_York, UTC-4 in July), setting a zero timedelta should yield anExpiresequal to "now" in UTC:Actual (wrong) output:
Response.expiresthen reads back the wrong value:This matches the original report from CEST (off by +2h).
Corrected output (same moment, after the fix):
Verified skew is 0 seconds under
America/New_York,Europe/Berlin,Asia/Kolkata, andUTC.Fix
In the
timedeltabranch ofserialize_date, add the delta to UTC now instead of naive local time, mirroringResponse._cache_expires:A
_utcnow = datetime.utcnowhook point is added next to the existing_nowhook, keeping the same test-seam style already used in the module. The change is a single line in the affected branch;_nowandparse_date_deltaare left untouched to keep the fix localized to the reported behavior.Tests
test_serialize_date_timedelta_is_utc_based, which runs under a non-UTC timezone (TZ=America/New_York, restored afterward) and asserts the serialized zero-delta header falls within a one-second window ofdatetime.utcnow(). It fails on the unpatched code (header is 4 hours behind) and passes after the fix.test_serialize_dateto compute its expected value fromutcnow()instead ofnow(), so it asserts the correct (timezone-independent) behavior rather than enshrining the bug.2299 passed, 1 xfailed.black --check,isort --check-only, andflake8are clean on the changed files.Disclosure: prepared with AI assistance; reviewed and verified locally.