Repository navigation
Conversation
…e returns seconds fs._toUnixTimestamp tested its argument with the global isFinite, which converts the argument to a number first. A Date passed that test and came back unchanged, or as the current time when it was before 1970. null, booleans, arrays and boxed primitives also came back unchanged. Check typeof time === "number" first, which is what Node's NumberIsFinite(time) does. A Date now reaches the isDate branch and returns getTime() / 1000. Every other value that is not a number or a numeric string throws ERR_INVALID_ARG_TYPE.
|
Updated 1:19 PM PT - Oct 26th, 2026
✅ @robobun, your commit ac5f7f6563799c556c79dd93efe7d052f191cf34 passed in 🧪 To try this PR locally: bunx bun-pr 45274That installs a local version of the PR into your bun-45274 --bun |
|
Status: reproduced, fix pushed in this PR (#45274). Reproduced with |
|
No actionable comments were generated in the recent review. ℹ️ Recent review info
Walkthrough
ChangesUnix Timestamp Validation
Suggested reviewers: Priority: Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title names the main change: fs._toUnixTimestamp no longer converts its argument, so a Date returns seconds. It matches the diff, which replaces the global isFinite check with a typeof number chec… |
| Description check | |
The description follows the template. It has a 'Problem' and 'Fix' section that covers the 'What does this PR do?' content, and it has a 'Verified' section with test commands and results that covers '… |
Full details: Title check
-
Explanation
The title names the main change: fs._toUnixTimestamp no longer converts its argument, so a Date returns seconds. It matches the diff, which replaces the global isFinite check with a typeof number check before $isFinite. The title is 86 characters, which is longer than ideal, but it is specific and accurate.
Full details: Description check
-
Explanation
The description follows the template. It has a 'Problem' and 'Fix' section that covers the 'What does this PR do?' content, and it has a 'Verified' section with test commands and results that covers 'How did you verify your code works?'. The template headings 'What does this PR do?' and 'How did you verify your code works?' are not used verbatim. The description is long and contains a draft note, extensive notes, and internal process remarks (such as self-review counts and references to other PRs) that do not belong in a PR description. These are not missing required information, so the check passes.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/node/fs.ts— Any script that patches Date.prototype.getTime now changes what fs._toUnixTimestamp returns for every valid Date; on the base branch that method was never reached for a valid Date. src/js/node/fs.ts:1012 now sends every Date past the number branch, so src/js/node/fs.ts:1020 calls the user-overridable time.getTime() for all of them. Node uses the DatePrototypeGetTime primordial here, and src/js code must not route through user-replaceable methods. Fix: capture Date.prototype.getTime at module load (as src/js/internal/test_runner/mock_timers.ts:24 does) and invoke it with $call, so a tampered or mocked prototype cannot alter the result.Why this was flagged
Trigger: any Date argument to fs._toUnixTimestamp, arriving from user code or from vendored Node tests such as test/js/node/test/parallel/test-fs-utimes.js:49 and :51, in a process where Date.prototype.getTime was replaced (test mocks, polyfills, instrumentation). On the base branch, $isFinite(date) coerced the Date and returned it unchanged, so src/js/node/fs.ts:1020 ran only for an invalid Date; after the diff, src/js/node/fs.ts:1012 excludes all non-numbers and every valid Date reaches time.getTime() at :1020. The call is a normal property lookup, so a replaced getTime returns whatever user code decides, and the dismissal's claim that no runtime path depends on it ignores that the function is a public export exercised by Node's own test suite. The sibling test file asserts valueOf is never invoked (test/js/node/fs/fs-to-unix-timestamp.test.ts:69) yet the Date path still invokes user-overridable code. Remedy: use a module-load-captured DatePrototypeGetTime with $call.
Verification: nit — triggered when any code in the process replaces or shadows
Date.prototype.getTimeand then a valid Date reachesfs._toUnixTimestamp. src/js/node/fs.ts:1012 now excludes every Date, so each valid Date reaches src/js/node/fs.ts:1020return time.getTime() / 1000;, a user-replaceable method. On the base, line 1020 ran only for an invalid Date.
Node's toUnixTimestamp calls the DateNow and DatePrototypeGetTime primordials. Capture both when node:fs loads and call those. A Date subclass that overrides getTime, a replaced Date.prototype.getTime and a replaced Date.now (for example node:test mock timers) no longer change the result.
…ounixtimestamp-date
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, self-contained port of Node's toUnixTimestamp with a test that runs under both runtimes.
What was reviewed:
- Branch order and semantics in
src/js/node/fs.tsagainst Node'slib/internal/fs/utils.js:typeof === "number" && $isFiniteis equivalent toNumberIsFinite($isFinitemaps to the coercing global insrc/codegen/replacements.ts), and theisDate/DatePrototypeGetTime.$call/DateNowcapture matches the primordials pattern used elsewhere insrc/js. - Callers:
_toUnixTimestampis only exported fromnode:fs(theutimesfamily parses times natively), so the behavior change is confined to the helper; the vendoredtest-fs-timestamp-parsing-error.jsandtest-fs-utimes.jsusages remain valid sinceStats.mtimeis a realDateInstance. - The new test's tamper cases (replaced
Date.now, own/overridden/replacedgetTime) each fail without the corresponding captured primordial, and no existing test asserts the old pass-through behavior.
Extended reasoning...
The diff changes one exported helper in src/js/node/fs.ts (about eight lines) to use a typeof guard before $isFinite and to call Date.now and Date.prototype.getTime captured at module load, plus a new 115-line node:test file. It touches no security-sensitive surface; the helper is only exported and not used by utimes/futimes/lutimes, which parse times natively. The implementation matches Node's source line for line, the test file follows an existing precedent in test/js/node/fs for node:test-based compat tests, and the changed paths are not covered by CODEOWNERS. The earlier COMMENTED review was followed by a commit adding the primordial captures, and nothing in the timeline indicates an outstanding CHANGES_REQUESTED objection.
There was a problem hiding this comment.
LGTM — the one-line comment from the latest push settles the last open nit, and the fix now matches Node's toUnixTimestamp exactly.
What was reviewed:
- The
typeof time === "number" && $isFinite(time)guard is equivalent to Node'sNumberIsFiniteand makes theisDatebranch reachable;DateNow/DatePrototypeGetTime.$callare captured at load, so a replacedDate.now/getTimecannot change the result. - Confirmed
_toUnixTimestamphas no internal callers insrc/js/(only the export atsrc/js/node/fs.ts:1365); utimes/futimes/lutimes parse times natively, so no sibling site shares the pattern. - Checked that
$ERR_INVALID_ARG_TYPEformats bigint and symbol arguments (the newly reachable inputs) inErrorCode.cppwithout throwing a bare TypeError. - The new test restores
Date.now/Date.prototype.getTimeinfinallyand asserts exact code/class on every rejected input, so it cannot pass vacuously.
Extended reasoning...
The diff changes ~10 lines in src/js/node/fs.ts (a typeof guard in _toUnixTimestamp plus two load-time captures of Date.now and Date.prototype.getTime) and adds a 115-line node:test-based test file following the pattern of sibling files in test/js/node/fs/. It touches no security-sensitive surface; _toUnixTimestamp is an internal helper exported only for Node's vendored tests and has no callers inside src/js/. The change is small, self-contained, mirrors Node's reference implementation line for line, and the bug hunt ran dry; the paths are not covered by CODEOWNERS and the only prior reviews were COMMENTED states whose last open item (comment length at line 1014) was addressed by commit 667d598.
robobun
changed the title
…ounixtimestamp-date
…xTimestamp The captures of Date.now and Date.prototype.getTime ran when node:fs loads. A program that deleted or replaced the global Date before that made node:fs fail to load, and a replaced built-in stayed captured after it was restored. fs._toUnixTimestamp now calls one native, made at its first call, that gives the clock's time or the time value of a Date. node:fs reads nothing from Date when it loads. The isDate capture goes too: the native does that check.
|
Update: the native fix that the last paragraph describes is not in this PR. See #45274 (comment) for the final shape. I converted this PR to a draft. Do not merge the head dacbecf. A later test found a defect in it. The two module-scope captures ( const D = Date;
delete globalThis.Date;
await import("node:fs"); // this head: ReferenceError: Date is not defined. main and node v26.3.0: loadsbun:test A fix is built and verified locally. |
Take the native read of the clock and of a Date's time value out again. fs._toUnixTimestamp calls Date.now() and time.getTime() as on main, and only tests the type before $isFinite. The tests keep the cases that need no replaced built-in. Rows for an invalid Date, a Date of another realm, an object with Date.prototype and a Proxy of a Date are new. test-fs-utimes.js is the copy of Node v26.3.0. It takes the wanted time of a negative number before the call, so the check that this fix makes real does not depend on how long the call takes.
robobun
changed the title
|
This PR is back to its first shape: the one-line type check in What changed since my draft note above:
What the PR does not do: a replaced or overridden The PR stays a draft until CI on this head is done. |
Draft: it waits for CI on ac5f7f6. Nothing else is owed.
Problem
fs._toUnixTimestamp(new Date(5000))returns theDateobject. Node v26.3.0 returns5.null,trueand[]come back unchanged. Node throwsERR_INVALID_ARG_TYPE.$isFinite(time)atsrc/js/node/fs.ts:1010is the globalisFinite, which converts its argument. Node's line isNumberIsFinite(time).Fix
typeof time === "number"before$isFinite(time). The pair equalsNumber.isFinite, so aDatereaches theisDatebranch.test/js/node/fs/fs-to-unix-timestamp.test.ts(new: 2 of 4 tests fail onmain, 4 pass under Node). Alsotest-fs-utimes.js, 30 runs.REVIEW.md:55here, because that text is the maintainers'.Background
fs._toUnixTimestampis Node's internaltoUnixTimestamp, exported for tests. No user reported this fault: the case is Node parity.test-fs-utimes.jssends times through it. Two of its seven cases, the numeric ones, compared seconds with milliseconds and could not fail. This PR takes Node v26.3.0's copy of the file.Date, so that replaced built-ins cannot change the result. It is out: it conflicts with bindgen: deleteb.anyand.variadic#46789, and nobody asked for it.Downsides
getTimeorDate.nowalso changes the result.typeofcheck per call and 28 B of embeddednode:fssource. No native code changes.Notes
Node source.
toUnixTimestampis lib/internal/fs/utils.js#L779-L796. The export is lib/fs.js#L3361-L3362.Repro.
main(449877f)41 values. "now" is the current time in seconds. The message text is not compared (see #38657). On
main,{valueOf(){return 7}}also runs the user'svalueOf.fs._toUnixTimestamp(x)main(449877f)10001.50-0-1-5.5NaNInfinity-Infinity'1000''-1'''' ''abc''Infinity''0x10''1e3'' 12 'new Date(5000)new Date(0)new Date(1500)new Date(-5000)new Date(NaN)new MyDate(5000)(a subclass)vm.runInNewContext("new Date(5000)")Object.create(Date.prototype)nullundefinedtruefalse[][7][1,2]{}new Number(12)new String('12')12nSymbol(){valueOf(){return 7}}{valueOf(){throw}}function(){}test-fs-utimes.js. Lines 48 to 53 of the old copy computefs._toUnixTimestamp(mtime) - fs._toUnixTimestamp(stats.mtime), and line 67 asserts that the result is less than 2.Datecases, both calls return theDateonmain, so the check compares milliseconds. It is real there, and 1000 times stricter than Node's.maincompares seconds with milliseconds. With a wanted time of 5000 and a real mtime of 1000 it computes -995000, and the assertion passes. This branch and node v26.3.0 compute 4000.mtime = -1, which means "now". The old copy reads that "now" after the call, so with this fix the check becomes a race against a 2 s bound. Node v26.3.0's copy reads it before the call (getExpectedMtime). The PR takes that copy verbatim. It passes onmain, on this branch (30 of 30 runs on the debug ASAN build) and under node.What the fix does not cover. With the type check, every valid
Datereachestime.getTime(), and a negative number reachesDate.now()as before. User code can replace both. Node calls theDatePrototypeGetTimeandDateNowprimordials, which it saves before user code runs. A grid of 27 states of theDatebuilt-ins, each in 3 orders against the first load ofnode:fs, in 81 fresh processes with 38 cells each:main(449877f)The 311 are a
Datesubclass that overridesgetTime, an owngetTimeproperty and an owngetTimeaccessor (81 each, in every state), and the cells whereDate.noworgetTimeis replaced or deleted at the time of the call. Againstmain: 1578 cells are as Node on both, 1189 moved to Node's answer, 311 are as Node on neither, and 0 are right onmainand wrong on this branch.node:fsloads in all 81 processes.How the PR got here. Two earlier heads tried to close that gap.
Date.nowandDate.prototype.getTimeat the top offs.ts, after a review nit. They read the globalDatewhennode:fsloads. WithDatedeleted,undefinedor spied (bun:testspyOn(globalThis, "Date")) before the first load,node:fsdid not load.mainnever had that defect.timeValueOrNow, read where no user code runs. It matched Node in all 3078 cells. A self-review then said to take it out: it adds ab.anyunion while bindgen: deleteb.anyand.variadic#46789 deletesb.any(a trial merge conflicts innode_fs_binding.rs), no issue or user asks for it, andREVIEW.md:66and the PR process notes ask for the narrowest change. It is kept on the branchrobobun/3179519b/fs-tounixtimestamp-native-read(c0126ec) with its tests and measurements. internal/primordials: Node.js parity, generated and engine-backed #35567 would supply$DateNowand$DatePrototypeGetTimefor all ofsrc/js; it has had no push since 2026-08-16.Upstream. nodejs/node#66634 ("fs: preserve negative utimes timestamps", open, approved, semver-major) changes what a negative time means in Node. Bun follows v26, where a negative number means "now".
The forms that take a time.
utimes,lutimesandfutimes(sync, callback, promises andFileHandle.utimes) do not use this helper. They parse times intime_like::from_js(src/runtime/node/time_like.rs). With 14 values,main, this branch and node give the same mtime or error code in all 9 forms.Not fixed here, and handed to separate work.
Dateat module scope onmain(node/v8.ts:13,node/inspector.ts:12,node/_http_server.ts:123,internal/test_runner/mock_timers.ts:24,internal/repl/node-primordials.js:32,internal/util/inspect.js:62-64). With no globalDateat their first load,node:v8,node:inspector,node:http,node:https,node:testandnode:repldo not load. Node loads them.$isFiniteforNumberIsFinite:$isJSArraystands where Node hasArrayIsArrayinchild_process.ts, so a Proxy of an array is rejected asargsorstdiowhere Node accepts it.$isFiniteitself has no other use insrc/js.Other open PRs. This branch merges without a conflict with #46789 and #45338 (
git merge-tree). Against #38657 and #35541 it adds no conflict that they do not already have withmain.Measurements.
bundle-modules.ts --debug=OFF): +28 B againstmain(InternalModuleRegistryConstants.bin2,714,997 to 2,715,025,node/fs.js30,980 to 31,008). The 28 B are the texttypeof time === "number" &&.node:fstop level (BUN_JSC_dumpGeneratedBytecodes): 987 instructions and 5070 bytecode bytes, the same asmain.typeofcheck, only for an argument that is not a string.Tests.
The test file uses
node:testandnode:assert, asfs-read-buffer-before-offset.test.tsandfs-write-offset-bound.test.tsdo, so the same file runs under Node.Other runs on this branch (debug ASAN build):
test/js/node/test/parallel/test-fs-utimes.js(the new copy),test-fs-timestamp-parsing-error.js,test-fs-utimes-y2K38.js,test-fs-promises.js: exit 0.test/js/node/fs/fs.test.ts -t utimes: 9 pass, 1 skip.tsc --noEmit -p src/js/tsconfig.json: exit 0.bun lintandbun fmt --checkon the changed files: no problems.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file