fix(time): correct Chromium/Firefox/Safari timestamp conversions#586
Merged
fix(time): correct Chromium/Firefox/Safari timestamp conversions#586
Conversation
Replaces broken per-browser helpers that used time.Local (causing up to 8h + historical LMT drift for UTC+8 machines), truncated sub-second precision via integer division, and returned arbitrary sentinel dates (2049-01-01, 9999-12-13) for out-of-range values. - Chromium timeEpoch: single UnixMicro subtraction with the canonical offset 11644473600000000 (matches base::Time::kTimeTToMicrosecondsOffset). - Firefox: splits the catch-all timestamp() into firefoxMicros/Millis/Seconds, matching each column's actual unit; callers drop manual division. - Safari: adds .UTC() and preserves fractional seconds from CFAbsoluteTime. - All helpers return zero time.Time for non-positive input and clamp values outside [year 1, year 9999] so JSON export cannot panic on sentinel values like INT64_MAX. Unit tests cover UTC invariance (with t.Setenv TZ), precision preservation, and JSON round-trip safety. Validated end-to-end on the Windows regression sandbox: 726 cookies, 0 non-UTC entries, 0 LMT drift, 716 preserved microsecond precision. Fixes #239, #240, #522
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #586 +/- ##
==========================================
+ Coverage 70.89% 71.25% +0.35%
==========================================
Files 61 61
Lines 3274 3294 +20
==========================================
+ Hits 2321 2347 +26
+ Misses 761 757 -4
+ Partials 192 190 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
binarycookies and the Keychain shim both return time.Time in Local, so Safari-sourced cookie.expire_at / cookie.created_at / password.created_at leaked +08:00 offsets while Chromium/Firefox exports were already UTC. Observed 115/12353 non-UTC cookies on a real macOS profile; apply .UTC() at the assignment site to match the rest of the codebase.
There was a problem hiding this comment.
Pull request overview
This PR fixes timezone-dependent and precision-loss issues in browser timestamp conversions (Chromium base::Time, Firefox PRTime/Date.now/seconds, Safari Core Data), standardizing outputs to UTC, preserving sub-second precision, and preventing JSON export crashes from out-of-range sentinel values.
Changes:
- Chromium: replace local-time epoch math with a UTC
UnixMicroconversion using the canonical 1601→1970 microsecond offset and add unit tests. - Firefox: split timestamp conversion into unit-specific helpers (µs/ms/s), update all call sites to stop manual division, and add unit tests for precision/UTC/clamping.
- Safari: make Core Data timestamp conversion UTC, preserve fractional seconds, treat zero/negative as “no timestamp”, and add tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| browser/chromium/chromium.go | Rewrites Chromium epoch conversion to a UTC microsecond-based conversion with year clamping. |
| browser/chromium/chromium_test.go | Adds tests for UTC invariance, precision retention, epoch boundary, and JSON-safe clamping. |
| browser/firefox/firefox.go | Introduces unit-specific Firefox timestamp helpers (µs/ms/s) and clamps out-of-range years. |
| browser/firefox/firefox_test.go | Adds tests covering precision preservation, UTC invariance, unit consistency, and JSON-safe clamping. |
| browser/firefox/extract_password.go | Switches login timeCreated conversion to the new ms helper (removes manual division). |
| browser/firefox/extract_history.go | Switches last_visit_date conversion to the new µs helper (removes manual division). |
| browser/firefox/extract_download.go | Switches dateAdded to µs and JSON endTime to ms helpers (removes manual division). |
| browser/firefox/extract_cookie.go | Switches cookie expiry to seconds helper and creationTime to µs helper (removes manual division). |
| browser/firefox/extract_bookmark.go | Switches bookmark dateAdded conversion to the new µs helper (removes manual division). |
| browser/safari/safari.go | Updates Core Data timestamp conversion to UTC with fractional-second preservation and year clamping. |
| browser/safari/safari_test.go | Adds focused unit tests for Core Data conversion correctness, UTC, and fractional seconds. |
| browser/safari/extract_history_test.go | Updates existing Core Data timestamp test to match the new “zero maps to zero time” behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+223
to
+227
| } | ||
| whole := int64(seconds) | ||
| frac := seconds - float64(whole) | ||
| nanos := int64(frac * 1e9) | ||
| t := time.Unix(whole+coreDataEpochOffset, nanos).UTC() |
Comment on lines
+318
to
+323
| // clampJSON maps years outside time.Time.MarshalJSON's [0, 9999] window | ||
| // to the zero time, so JSON export can't crash on sentinel inputs. | ||
| func clampJSON(t time.Time) time.Time { | ||
| if t.Year() < 1 || t.Year() > 9999 { | ||
| return time.Time{} | ||
| } |
Comment on lines
+754
to
+757
| // Location must be UTC regardless of the test runner's TZ. Set a | ||
| // non-UTC local zone via t.Setenv so the assertion catches any | ||
| // accidental time.Local usage. | ||
| t.Setenv("TZ", "Asia/Shanghai") |
Comment on lines
+369
to
+370
| // Verify no helper leaks time.Local, regardless of runner TZ. | ||
| t.Setenv("TZ", "Asia/Shanghai") |
| } | ||
|
|
||
| func TestCoredataTimestamp_AlwaysUTC(t *testing.T) { | ||
| t.Setenv("TZ", "Asia/Shanghai") |
- Use require.NoError so a MarshalJSON failure aborts the subtest before the follow-up comparison runs on garbage. - Use assert.JSONEq for the JSON-encoded zero-time literal; matches semantic rather than byte-for-byte.
- safari: guard float64 seconds against out-of-range conversion to int64 (Go spec makes those implementation-dependent); bound by maxCoreDataSeconds so year clamp at the end becomes unnecessary. - firefox: correct clampJSON's documented range to [1, 9999] so the comment matches the predicate. - tests: swap assert.Equal for assert.Same on UTC location checks, so any leaked time.Local is caught via pointer identity regardless of the runner's TZ (t.Setenv was ineffective since time.Local is cached at process init).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
timeEpoch, Firefoxtimestamp, and SafaricoredataTimestampall usedtime.Local, which shifted results by 8h on UTC+8 machines and further drifted by the historical LMT offset (5m 43s) on macOS/Asia/Shanghai.timestamptruncated sub-second precision because every caller did/ 1000000integer division before passing in.INT64_MAXfor "never expires") produced years past 9999 that crashedtime.Time.MarshalJSONat export.Fix
time.UnixMicro(us - 11644473600000000).UTC(), using the canonical offset frombase::Time::kTimeTToMicrosecondsOffset.firefoxMicros/firefoxMillis/firefoxSeconds, one per storage unit; callers no longer divide manually..UTC()on output plus fractional-second preservation.time.Time{}for non-positive input and clamp years outside[1, 9999]to zero.Test plan
go test ./...greent.Setenv("TZ", "Asia/Shanghai")), precision preservation, andMarshalJSONround-tripClose #239
Close #240
Close #522