Repository navigation
windows: run Bun inside an AppContainer (lowbox token) - #33119
Conversation
…hNameByHandle Inside an AppContainer (lowbox token) the DOS volume-name translation is denied (the mount manager device cannot be opened), so every GetFinalPathNameByHandleW(VOLUME_NAME_DOS) call fails with ERROR_ACCESS_DENIED while VOLUME_NAME_NT and VOLUME_NAME_NONE still work. That single failure breaks running a script by path, bun install, bun build, bunx, and every relative openat whose path contains a dot. On that specific failure, rebuild the DOS path from the NT device form plus a device->drive map learned from paths whose DOS and NT spellings are both known to the process (the cwd and the executable directory), verified against VOLUME_NAME_NONE so a junction cannot poison an entry. Also prefix the ConPTY pipe names with LOCAL\ so Bun.Terminal works in an AppContainer; outside one the prefix is just part of the name.
The package installer and the isolated-install linker called the raw GetFinalPathNameByHandleW, so inside an AppContainer they still failed with EPERM (failed opening node_modules/package dir) after the central wrapper learned to survive the mount-manager denial. Add a raw-call- compatible variant that applies the same NT-device fallback and keeps the \\?\ prefix, and use it at those call sites.
Picks up oven-sh/libuv#5: LOCAL\ internal pipe names with bounded, randomized access-denied retries, console read cancellation that works when input injection is denied, sandbox-rewritten junction readback, EACCES for namespace-denied pipe binds, and assorted error-reporting fixes (uv_pipe translation, EMFILE, realpath GetLastError, stat out-of-bounds).
The resolver builds DirInfo for every ancestor of the requested directory starting at the drive root, and treated any open failure as fatal for the whole resolution. Sandboxed processes (e.g. a Windows AppContainer) can read their granted project tree but not the drive root or profile directories above it, so 'bun run' failed with 'error loading current directory' even though the cwd itself was readable. Treat EPERM/EACCES on an ancestor like an opaque, empty directory and keep walking; nothing above the readable tree can contribute a package.json or node_modules. Errors on the requested directory itself remain fatal.
A sandboxed process (e.g. a Windows AppContainer) is denied the DOS volume-name translation, which broke all four realpath entry points differently: - fs.realpathSync.native / fs.realpath.native / fs.promises.realpath route through uv_fs_realpath, which always fails EPERM there. On that error, resolve off an opened handle instead: open with backup semantics and read the final path back via get_fd_path, which carries the lowbox-aware GetFinalPathNameByHandle fallback. Other errors and the fallback's own failures still report the original realpath error. - fs.realpathSync / fs.realpath walk the path component by component with lstat, starting at the drive root, and drive roots and profile directories carry no ACE for a sandboxed token. Treat an EPERM/EACCES component as a plain, hard directory (not a link, not a pipe or socket) instead of failing the whole walk; everything the process can actually traverse still resolves normally.
A failed pipe listen threw a generic invalid-arguments TypeError
('Failed to listen at ...') with no code, errno, or syscall. Callers
need to distinguish EADDRINUSE (name taken; retry with another name)
from EACCES (pipe namespace denied, e.g. a sandboxed process binding
outside \.\pipe\LOCAL\; renaming will never help).
Propagate the libuv error through the named-pipe listening context and
build a Node-shaped SystemError (code/errno/syscall/path) from it,
falling back to the generic error only for failures with no system
error code.
Spawns with an ignored stdio slot substitute an anonymous pipe when the NUL device is denied (Windows Server AppContainers).
The kernel silently rewrites junctions created by a sandboxed process (e.g. a Windows AppContainer) into untrusted mount points that nothing can traverse, while creation still reports success. The isolated linker's junction fallback then produced a node_modules full of dead links with a green install summary. - Probe the junction after creating it in symlink_or_junction; if traversal reports ERROR_UNTRUSTED_MOUNT_POINT, remove it and fail with EACCES so the installer reports the package instead of pretending it linked. - Map ERROR_UNTRUSTED_MOUNT_POINT (448) to EACCES generally; reads through such a junction surfaced EUNKNOWN before.
Covers the host configuration a sandboxed launch needs (ACL grants, network capabilities, window-station access for services, loopback exemption) and the platform behaviors that cannot be configured away (LOCAL\ pipe namespace, no true symlinks, quarantined junctions, inaccessible home directory, limited process visibility).
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Windows AppContainer support across lowbox path resolution, realpath and resolver fallback behavior, named-pipe error shaping, install path handling, and documentation. It also updates the pinned libuv commit and Windows errno mappings. ChangesWindows AppContainer Support
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/deps/libuv.ts`:
- Around line 13-19: Shorten the header comment in the libuv dependency note so
it fits the repository’s 3-line limit and stays concise. Condense the long
dependency/context explanation around the branch/SHA details into a brief
summary in the same comment block, and move any extra background into the PR
description. Keep the intent clear while trimming the text near the existing
libuv version note.
In `@src/runtime/socket/Listener.rs`:
- Around line 282-286: The newly added explanatory comments in Listener.rs
exceed the repo’s 3-line comment limit and need to be condensed. Trim the
comment block around the syscall failure handling in the relevant
listener/socket code so it stays within 3 lines while preserving only the
essential note about surfacing coded syscall failures like node:net; if more
rationale is needed, move it to the PR description instead.
In `@src/sys/windows/mod.rs`:
- Around line 3817-3822: The lowbox fallback in the GetFinalPathNameByHandleW
wrapper is missing the required NUL terminator, unlike the raw success path.
Update the fallback path in the Windows path helper (the branch using
lowbox_dos_name_fallback in the GetFinalPathNameByHandleW wrapper) to write a
trailing NUL at the returned length before returning, so callers like
PackageInstall::init_install_dir and install_from_link can safely inspect
buf[returned_len]. Ensure the terminator is written in-bounds after copying PFX
and computing the final length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 378466a1-ea5b-4945-86ee-61f27fe81b33
📒 Files selected for processing (14)
docs/docs.jsondocs/runtime/windows-appcontainer.mdxscripts/build/deps/libuv.tssrc/errno/windows_errno.rssrc/install/PackageInstall.rssrc/install/isolated_install/Installer.rssrc/js/node/fs.tssrc/resolver/resolver.rssrc/runtime/api/bun/Terminal.rssrc/runtime/node/node_fs.rssrc/runtime/socket/Listener.rssrc/sys/lib.rssrc/sys/windows/mod.rssrc/windows_sys/externs.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/runtime/windows-appcontainer.mdx (2)
84-96: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the smoke test module-format agnostic.
smoke.jsusesrequire(...), so it fails immediately in any project with"type": "module". That gives a false AppContainer failure before the snippet reaches the paths this page is trying to validate. Rename it tosmoke.cjsor switch the snippet to ESM imports.Suggested doc fix
-// smoke.js — run with: bun smoke.js (inside the container) -const { spawnSync } = require("child_process"); +// smoke.cjs — run with: bun smoke.cjs (inside the container) +const { spawnSync } = require("child_process");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/runtime/windows-appcontainer.mdx` around lines 84 - 96, The smoke test snippet is not module-format agnostic because it uses CommonJS require calls in smoke.js, which will fail in projects with "type": "module" before the AppContainer checks run. Update the example around the smoke.js snippet to either rename it to smoke.cjs or rewrite it to use ESM imports while preserving the same spawnSync, realpathSync, and net.createServer behavior.
98-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid hard-coding an unverified Bun release floor.
Bun ≥ 1.4.xis not established anywhere in the supplied PR context, and this wording will age badly if the change lands in a different release line or gets backported. Prefer wording this as “a build that includes the patched libuv/AppContainer changes” until the release target is confirmed. As per coding guidelines, "Be humble & honest — NEVER overstate what you got done or what actually works in commits, PRs or in messages to the user."Suggested doc fix
-If `spawn+pipes` hangs or the pipe server reports `EADDRINUSE` on a fresh -name, the Bun build predates the AppContainer support (Bun ≥ 1.4.x with the -patched libuv is required). +If `spawn+pipes` hangs or the pipe server reports `EADDRINUSE` on a fresh +name, the Bun build predates the AppContainer support in this PR. Use a build +that includes the patched libuv/AppContainer changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/runtime/windows-appcontainer.mdx` around lines 98 - 100, The runtime docs currently hard-code an unverified Bun version floor in the AppContainer troubleshooting note, which should be removed. Update the wording in the Windows AppContainer section to refer to “a build that includes the patched libuv/AppContainer changes” instead of naming a specific release, and keep the guidance tied to the spawn+pipes and EADDRINUSE behavior described there.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/runtime/windows-appcontainer.mdx`:
- Around line 84-96: The smoke test snippet is not module-format agnostic
because it uses CommonJS require calls in smoke.js, which will fail in projects
with "type": "module" before the AppContainer checks run. Update the example
around the smoke.js snippet to either rename it to smoke.cjs or rewrite it to
use ESM imports while preserving the same spawnSync, realpathSync, and
net.createServer behavior.
- Around line 98-100: The runtime docs currently hard-code an unverified Bun
version floor in the AppContainer troubleshooting note, which should be removed.
Update the wording in the Windows AppContainer section to refer to “a build that
includes the patched libuv/AppContainer changes” instead of naming a specific
release, and keep the guidance tied to the spawn+pipes and EADDRINUSE behavior
described there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7be7de16-651d-410a-a508-f06ebafebfa2
📒 Files selected for processing (1)
docs/runtime/windows-appcontainer.mdx
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ast error The raw GetFinalPathNameByHandleW always NUL-terminates and returns the length excluding the terminator, and two of the installer call sites read buf[len] on a pooled buffer to decide whether to append a path separator, so the fallback has to write that NUL too or a stale backslash silently concatenates the package name onto its parent. Also set the thread error back to ERROR_ACCESS_DENIED before the raw-shape wrapper returns 0, since the fallback's own successful queries clobber it and callers map GetLastError into the errno they report.
…tive path Treating a component the walk may not lstat as a plain, hard directory was fail-open: a denied component can hide a junction or symlink, so realpath would return the unresolved spelling while traversal through the link still worked - defeating the canonical realpath-plus-prefix containment pattern for any non-elevated process on Windows, sandboxed or not. When a component lstat fails with EPERM/EACCES, resolve the whole path through the handle-based native realpath instead (which follows the true chain, including in sandboxes via the lowbox-aware fallback), and rethrow the original error if the native resolution also fails. Fail closed, never sideways.
…nction probe Two follow-ups for sandboxed (and merely read-only) configurations: - openat requests FILE_WRITE_ATTRIBUTES even for O_RDONLY, so files in a tree granted read-execute only - the normal shape for a sandboxed process's project tree - failed ACCESS_DENIED even though reading is permitted; entry loading failed loudly and package.json resolution silently fell back to index.js. Retry pure read-only opens without the write-attributes bit when the first attempt is denied. - The junction traversability probe now runs only when the process token is an AppContainer (memoized TokenIsAppContainer query): the kernel rewrite it detects cannot happen outside one, so normal installs skip the extra open per junction.
Node-shaped errors carry the negated errno (the canonical fill_system_error_common does the same); the pipe-listen SystemError carried it positive. Strengthen the named-pipe listen test to pin the full error shape (code/errno/syscall) instead of swallowing the exception.
The installer treats EPERM/EACCES rename failures on Windows as benign in-use collisions, so an untrusted-mount-point failure during the staging rename would silently discard the staged store entry. ELOOP - an unresolvable link - is both more accurate for a quarantined junction and outside the collision heuristic, so the failure propagates. Reads through a quarantined junction now report ELOOP instead of EACCES.
The fallback path buffer is large and only needed when uv_fs_realpath is denied; keeping it inline reserved that stack space on every realpath call.
The tolerance is cross-platform; an execute-only (0o111) ancestor is the CI-runnable shape of the sandboxed drive-root case.
- Loopback: same-container processes can reach each other; only processes outside the container are isolated. - Junctions: quarantine applies to client Windows builds (some Server builds leave the rewritten junctions traversable), and reads through a quarantined junction report ELOOP. - realpath resolves through inaccessible ancestors via the native path rather than treating them as opaque. - Replace the placeholder version guidance and shorten the sidebar title. - Pin libuv at the head with the exact-fill console read fixes, the bind disambiguation hardening, the non-inheritable NUL-fallback pipe end, and the drive-root stat fix.
fs.realpath, fs.realpathSync, fs.realpath.native and fs.promises.realpath now behave exactly as Node does inside an AppContainer: the component walk's drive-root lstat and uv_fs_realpath's mount-manager query are denied and surface as-is. Bun's own resolver and install paths resolve through get_fd_path / the system-volume GetFinalPathNameByHandleW fallback and are unaffected. Drops fs.isInsideAppContainer(), sys::realpath_handle, the node_fs handle-based retry, and the Node-parity test that pinned the removed gate. The AppContainer integration test now asserts the denial instead.
The T0 fd_path_raw_w in bun_core calls kernel32 directly and cannot reach the bun_sys lowbox fallback (bun_core sits below bun_sys); its only callers are bun link / bun unlink, whose global link directory is outside any container grant anyway.
| // A directory the user can read but not write (RX-only grant) must still be | ||
| // descended by the scanner: read-only directory opens used to also request | ||
| // FILE_WRITE_ATTRIBUTES and fail ACCESS_DENIED. Elevated tokens bypass the | ||
| // ACL; the precondition is probed and the test skips visibly then. |
There was a problem hiding this comment.
🟡 The 4-line header comment here says "read-only directory opens used to also request FILE_WRITE_ATTRIBUTES" — but glob scans open with O::DIRECTORY, which routes to open_dir_at_windows_nt_path (never the file-open arm where FILE_WRITE_ATTRIBUTES was dropped); what an RX-only grant denies there is FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY, and this test exercises the new retry wrapper (src/sys/lib.rs:6957-6978) that drops those bits — matching the PR description's own "directory opens retry without create bits ... Fixes Bun.Glob" bullet. Separately, this same 4-line block and the 5-line block at test/js/bun/net/named-pipe-listen-error.test.ts:29-33 are both over the CLAUDE.md 3-line cap (neither test file was named in any prior comment-length thread, so these read as missed in the f25f021 pass). Comment accuracy/style only, zero runtime impact.
Extended reasoning...
What the issue is
Two related nits on new-in-PR test comments:
(a) Factual accuracy — the header at test/js/bun/glob/scan.test.ts:1130-1133 reads:
// A directory the user can read but not write (RX-only grant) must still be
// descended by the scanner: read-only directory opens used to also request
// FILE_WRITE_ATTRIBUTES and fail ACCESS_DENIED. Elevated tokens bypass the
// ACL; the precondition is probed and the test skips visibly then.It names FILE_WRITE_ATTRIBUTES as the access bit that used to fail an RX-only directory open. That's the file-open change in this PR; the glob-scan fix is the directory-open create-bits retry.
(b) Length — CLAUDE.md rule 13 caps code comments at 3 lines. This same block is 4 lines, and test/js/bun/net/named-pipe-listen-error.test.ts:29-33 is 5:
// The collision must surface as a Node-shaped system error, not a
// generic TypeError: code/errno/syscall identify EADDRINUSE so
// callers can react (retry another name) - and distinguish it from
// EACCS (pipe namespace denied, e.g. sandboxed processes binding
// outside \.\pipe\LOCAL\, where renaming never helps).The specific code path (accuracy claim)
GlobWalker::openat(src/glob/GlobWalker.rs:203) callsbun_sys::openat(fd, path, O::DIRECTORY | O::RDONLY, 0).openat_windows_implat src/sys/lib.rs:7260 sees(flags & O::DIRECTORY) != 0and routes toopen_dir_at_windows_nt_path— the file-open arm whereFILE_WRITE_ATTRIBUTESwas dropped (line ~7277) is never reached for a directory open.open_dir_at_windows_nt_path_impl(src/sys/lib.rs:6989-7008) buildsbase_flags = STANDARD_RIGHTS_READ | FILE_READ_ATTRIBUTES | FILE_READ_EA | SYNCHRONIZE | FILE_TRAVERSEand, when!read_only, addsFILE_ADD_FILE | FILE_ADD_SUBDIRECTORY.FILE_WRITE_ATTRIBUTESappears nowhere in the directory-open mask.- The fix that makes this glob test pass is the new retry wrapper at src/sys/lib.rs:6957-6978, whose own comment states: "Plain opens request FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY; a read-only ACL grant ... denies that. Retry read-only so creates fail at create time instead."
- The PR description agrees verbatim: "Windows directory opens retry without create bits on denial … Fixes
Bun.Glob/recursive readdir descents into read-only directories."
So the test header conflates the two unconditional Windows changes in this PR: it names the access bit from the file-open change while describing the directory-open retry the test actually exercises.
Why this looks like an oversight rather than a deliberate exception
Length: the author has already accepted and applied CLAUDE.md rule 13 six-plus times in this PR's review cycle (0929cea, 6ca60b2, 6139543, f25f021 — the last literally titled "review: trim two 4-line comments to 3"). None of the prior resolved comment-length threads (resolver.rs, Listener.rs, sys/lib.rs openat_windows_impl, node_fs.rs, windows/mod.rs, libuv.ts, fs.ts, sys/lib.rs open_dir_at_windows_nt_path) named either of these two test-file locations, so these read as simply missed rather than deliberate exceptions.
Accuracy: per REVIEW.md "Comments carry only durable non-obvious content" / "Verify semantics empirically, never from names", a future reader auditing why this test exists would grep the named bit (FILE_WRITE_ATTRIBUTES), land in openat_windows_impl's file-open arm (the unrelated change), and misattribute what this test protects — which matters if the open_dir_at_windows_nt_path retry is ever revisited.
Step-by-step proof
CLAUDE.mdrule 13: "Keep code comments to 3 lines max — Comments must be concise."- Diff hunk
@@ -1125,3 +1126,57 @@in scan.test.ts: 4 consecutive+// ...lines at 1130-1133. - Diff hunk
@@ -26,6 +26,27 @@in named-pipe-listen-error.test.ts: 5 consecutive+// ...lines at 29-33. - src/sys/lib.rs:7260 —
if (flags & O::DIRECTORY) != 0 { return open_dir_at_windows_nt_path(...) }→ glob'sO::DIRECTORY | O::RDONLYnever reaches theFILE_WRITE_ATTRIBUTESchange. - src/sys/lib.rs:6959-6961 — retry-wrapper comment names
FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY, notFILE_WRITE_ATTRIBUTES. - PR description Windows directory opens retry without create bits bullet: "Fixes
Bun.Glob/recursive readdir descents" — the intended attribution for this test.
Impact and fix
Zero runtime impact — comment accuracy/style only. The scan.test.ts block can be rewritten to 3 lines with the correct bit in one edit, e.g.:
// An RX-only directory must still be descended: directory opens used to also
// request FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY (denied under RX) and fail EPERM.
// Elevated tokens bypass the ACL; the precondition is probed and skipped visibly.The named-pipe-listen-error.test.ts block just needs trimming to 3 lines.
There was a problem hiding this comment.
Thanks — the accuracy fix in 2304480 (FILE_ADD_FILE instead of FILE_WRITE_ATTRIBUTES) addresses (a). The length nit (b) is still outstanding: the scan.test.ts block at :1130-1133 is still 4 lines and the named-pipe-listen-error.test.ts block at :29-33 is still 5. e.g. for scan.test.ts:
// An RX-only directory must still be descended: directory opens used to request
// FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY (denied under RX) and fail ACCESS_DENIED.
// Elevated tokens bypass the ACL; the precondition is probed and skipped visibly.and for named-pipe-listen-error.test.ts:
// The collision must surface as a Node-shaped system error (code/errno/syscall),
// not a generic TypeError, so callers can distinguish EADDRINUSE (retry another
// name) from EACCES (pipe namespace denied — renaming never helps).NtCreateFile with a RootDirectory handle checks the target directory's ACL for child creates and renames, not the handle's access mask, so requesting these bits on the directory open grants nothing and only narrows where the open is admitted (RX-only trees, Program Files, sandboxed project dirs). Verified empirically: a handle opened without them still creates files, subdirectories, and accepts rename-into. Removes the read_only option (now a no-op) and the retry-on-denial wrapper it motivated. Neither libuv nor Zig std request these bits.
| // No FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY: child creates via RootDirectory | ||
| // check the directory's ACL, not this handle's access mask, so requesting | ||
| // them only narrows where this open is admitted. |
There was a problem hiding this comment.
🟡 Commit c2c66a7 (tip) replaced the retry-with-read_only: true approach with an unconditional drop of FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY and deleted the read_only field from WindowsOpenDirOptions, but four references to the old mechanism were missed: three code comments — src/resolver/fs.rs:1432-1433 ("iterable + read_only so the resulting handle has FILE_LIST_DIRECTORY"), src/libarchive/lib.rs:1421-1423 ("is selected by setting read_only"), src/sys/dir.rs:437-438 ("opened without read_only … unlike the read-only open_dir_* iteration helpers") — and the PR description's third Changes bullet, which still says "On EPERM/EACCES … retry with read_only: true; the original error is preserved if the retry also fails", describing a retry mechanism and struct field that no longer exist. (The still-open comment on scan.test.ts:1133 also references "the new retry wrapper (src/sys/lib.rs:6957-6978)", now gone; its accuracy point stands but the suggested wording should reference the unconditional drop.) Per REVIEW.md "One source of truth; update every consumer atomically" and CLAUDE.md #11. Comment/description accuracy only, zero runtime impact.
Extended reasoning...
What the issue is
Commit c2c66a7 ("win: drop FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY from directory opens") is the tip of this branch and changed the approach for the third unconditional Windows change: instead of retrying with read_only: true on EPERM/EACCES, open_dir_at_windows_nt_path now simply never requests FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY at all (src/sys/lib.rs:6956-6974, comment: "No FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY: child creates via RootDirectory check the directory's ACL, not this handle's access mask"), and the read_only field was deleted from WindowsOpenDirOptions (src/sys/lib.rs:6515). This PR updates every struct-literal call site (nine read_only: deletions across seven files), but four references to the old mechanism were missed in the sweep.
The specific stale references
(a) Three code comments still name the removed read_only field:
- src/resolver/fs.rs:1432-1433 — "On Windows this must go through
open_dir_at_windows_awith iterable + read_only so the resulting handle has FILE_LIST_DIRECTORY". This file is in the diff;read_only: truewas removed from the struct literal at line 1446, but the comment 13 lines above it was not updated. - src/libarchive/lib.rs:1421-1423 — "Access mask (
STANDARD_RIGHTS_READ | FILE_READ_ATTRIBUTES | FILE_READ_EA | SYNCHRONIZE | FILE_TRAVERSE) is selected by settingread_only, andFILE_OPEN_IFviaOpenOrCreate". This file is in the diff;read_only: truewas removed from the struct literal at line 1425, but the comment immediately above still says the mask is selected by setting it. - src/sys/dir.rs:437-438 — "the handle is opened without
read_onlyso the caller may create/rename children — unlike the read-onlyopen_dir_*iteration helpers". Not in the diff, but describes a field and a distinction that no longer exist: after c2c66a7 no directory-open handle requestsFILE_ADD_FILE|FILE_ADD_SUBDIRECTORY, so "opened withoutread_only" no longer implies "may create children", and the "unlike the read-only helpers" contrast is gone.
(b) The PR description's third Changes bullet still reads: "Windows directory opens retry without create bits on denial … On EPERM/EACCES for a read-intent open (OnlyOpen, !can_rename_or_delete), retry with read_only: true; the original error is preserved if the retry also fails." — describing a retry mechanism and a struct field that no longer exist. There is no retry-on-denial wrapper in the final diff; the bits are simply never requested.
(c) Knock-on to a still-open review comment — the still-open inline comment on test/js/bun/glob/scan.test.ts:1133 says the test "exercises the new retry wrapper (src/sys/lib.rs:6957-6978) that drops those bits". That comment's factual-accuracy point (the test header names FILE_WRITE_ATTRIBUTES instead of FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY) still stands, but its suggested wording should now reference the unconditional drop, not a retry.
Why this looks like an oversight rather than a deliberate exception
REVIEW.md states "One source of truth; update every consumer atomically … grep the whole repo", and the author has already accepted and applied comment-accuracy sweeps 6+ times in this PR's review cycle (0929cea, 6ca60b2, 6139543, f25f021, plus the previously-resolved 2026-07-20T10:54:43Z description-accuracy comment covering the 220395f realpath drop). The three code-comment sites are the only remaining WindowsOpenDirOptions-context read_only references (a grep for read_only in src/**/*.rs finds these three plus unrelated hits: GlobalCache::read_only, open_file_read_only, CriticalSection::begin_read_only). The PR-description bullet is the same class as the previously-resolved comment on the 220395f realpath sections, but for a different bullet made stale by a later commit (c2c66a7) that landed after that thread was resolved.
Not a duplicate
- Previous comment Fix errors in
bun bun(broke after threading) #15 covered theFILE_WRITE_ATTRIBUTEScomment inPackageInstall.rs(different file, different field, already fixed in this PR's diff). - The resolved 2026-07-20T10:54:43Z description-accuracy comment covered the
fs.realpathsections made stale by 220395f; this is the directory-opens bullet made stale by c2c66a7, which is the tip and landed after that thread was resolved. - The still-open scan.test.ts:1133 comment targets the test-header comment, not the PR description or the three source comments; it is mentioned here only because its own suggested wording now references the removed retry wrapper.
Step-by-step proof
git log --oneline -1→c2c66a70 win: drop FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY from directory opens(tip commit).- Diff at src/sys/lib.rs:6515 —
- pub read_only: bool,removes the field fromWindowsOpenDirOptions. - Diff at src/sys/lib.rs:6956-6974 — the
read_only_flag = if options.read_only { 0 } else { FILE_ADD_FILE | FILE_ADD_SUBDIRECTORY }branch is replaced by a 3-line comment + unconditionalbase_flagswith no create bits and no retry logic. - src/resolver/fs.rs:1433 — reads verbatim "iterable + read_only so the resulting handle has FILE_LIST_DIRECTORY"; the diff hunk for this file removes
read_only: true,from :1446 but leaves the comment. - src/libarchive/lib.rs:1422 — reads verbatim "is selected by setting
read_only"; the diff hunk for this file removesread_only: true,from :1425 but leaves the comment. - src/sys/dir.rs:437-438 — reads verbatim "opened without
read_onlyso the caller may create/rename children — unlike the read-onlyopen_dir_*iteration helpers"; not in the diff, so untouched by the sweep. - PR description third Changes bullet — reads verbatim "On
EPERM/EACCESfor a read-intent open (OnlyOpen,!can_rename_or_delete), retry withread_only: true; the original error is preserved if the retry also fails" — none ofread_only, "retry", or "the original error is preserved" exist in the final diff.
Impact and fix
Zero runtime impact — comment and description accuracy only. Suggested edits:
- src/resolver/fs.rs:1432-1433 — drop "+ read_only": "…with
iterableso the resulting handle has FILE_LIST_DIRECTORY + FILE_DIRECTORY_FILE". - src/libarchive/lib.rs:1421-1423 — the access-mask sentence is now the unconditional
base_flags, so: "Access mask is the read-onlybase_flagsset;FILE_OPEN_IFviaOpenOrCreate." (or simply drop the first clause). - src/sys/dir.rs:436-439 — the create/rename distinction no longer exists at the directory-open level; drop the "opened without
read_only… unlike the read-only helpers" sentence, keeping the "On Windows the flags select the access mask:iterateaddsFILE_LIST_DIRECTORY" part. - PR description third bullet — replace with: "Windows directory opens no longer request
FILE_ADD_FILE | FILE_ADD_SUBDIRECTORY: child creates viaRootDirectorycheck the directory's ACL, not the handle's access mask, so requesting them only narrowed where the open was admitted. FixesBun.Glob/recursive readdir descents into RX-only directories." - When addressing the still-open scan.test.ts:1133 comment, phrase the fix as "directory opens no longer request
FILE_ADD_FILE|FILE_ADD_SUBDIRECTORY" rather than "the new retry wrapper".
There was a problem hiding this comment.
Thanks — the PR description's third bullet and the scan.test.ts header (2304480) are now accurate, but the three code comments in part (a) were missed and still reference the removed read_only field:
- src/resolver/fs.rs:1433 — still reads "iterable + read_only so the resulting handle has FILE_LIST_DIRECTORY"; drop "+ read_only".
- src/libarchive/lib.rs:1422 — still reads "is selected by setting
read_only"; the mask is now the unconditionalbase_flagsset. - src/sys/dir.rs:437-438 — still reads "opened without
read_onlyso the caller may create/rename children — unlike the read-onlyopen_dir_*iteration helpers"; that distinction no longer exists (no directory-open handle requestsFILE_ADD_FILE|FILE_ADD_SUBDIRECTORYnow, and child creates go by the directory ACL regardless).
Comment accuracy only, zero runtime impact.
One conflict in src/runtime/socket/Listener.rs: main's #33119 added a ListenPipeError enum for Windows named-pipe listen errors where this PR had a uv_errno_out out-param. Took main's enum approach (it uses the standard jsc::SystemError shape and net.ts formatListenError reshapes the code/address/port on the JS side regardless).
My merge 37263b0 took main's #33119 ListenPipeError approach over this PR's uv_errno_out out-param. Main's path builds jsc::SystemError with errno = -(SystemErrno as c_int), which is the UV errno on POSIX but the cross-platform SystemErrno ordinal (-98 for EADDRINUSE) on Windows, not UV_EADDRINUSE (-4091). RoundRobinHandle extracts err.errno, passes it through uvTranslateSysError (no-op for n<=0), and the worker's ExceptionWithHostPort(-98) surfaces 'Unknown system error -98'. Keep the raw listen_rc.int() alongside the bun_sys::Error in ListenPipeError::Sys and use that for jsc::SystemError.errno, matching what the PR's pre-merge out-param carried and what Node reports on err.errno. Fixes test-cluster-eaccess.js and cluster.test.ts 'cluster pipe listen error carries no port suffix' on Windows.
Measured 2026-09-06 against Bun 1.4.2: contained `bun install` installs, `bunx` runs, and relative-path reads and writes work. Every one of those failed on 1.3.1, which is what yesterday's entry was written from. Bun added AppContainer support in oven-sh/bun#33119, merged 2026-07-20 and shipped from 1.4.0. The mechanism described yesterday was right -- older Bun keeps a working-directory descriptor captured at startup that an AppContainer will not honour, which is why absolute paths worked and relative ones did not, and why Node was unaffected. The scope was wrong: this was never "Bun does not work inside the Windows sandbox at all", it was "Bun before 1.4.0 does not", and the fix already existed upstream while I was root-causing it. Found while checking whether the upstream issue was worth commenting on. It is closed, and the PR that closed the Windows half is titled "windows: run Bun inside an AppContainer (lowbox token)" -- which would have been worth reading before spending an evening on the mechanism. The CHANGELOG edit that made this correction also deleted forty lines of the Unreleased section on its first attempt, including today's egress fix. TestAppVersionMatchesNewestChangelogEntry caught it because the version headings went with them.
Measured 2026-09-06 against Bun 1.4.2: contained `bun install` installs, `bunx` runs, and relative-path reads and writes work. Every one of those failed on 1.3.1, which is what yesterday's entry was written from. Bun added AppContainer support in oven-sh/bun#33119, merged 2026-07-20 and shipped from 1.4.0. The mechanism described yesterday was right -- older Bun keeps a working-directory descriptor captured at startup that an AppContainer will not honour, which is why absolute paths worked and relative ones did not, and why Node was unaffected. The scope was wrong: this was never "Bun does not work inside the Windows sandbox at all", it was "Bun before 1.4.0 does not", and the fix already existed upstream while I was root-causing it. Found while checking whether the upstream issue was worth commenting on. It is closed, and the PR that closed the Windows half is titled "windows: run Bun inside an AppContainer (lowbox token)" -- which would have been worth reading before spending an evening on the mechanism. The CHANGELOG edit that made this correction also deleted forty lines of the Unreleased section on its first attempt, including today's egress fix. TestAppVersionMatchesNewestChangelogEntry caught it because the version headings went with them.
3 participants
Makes Bun usable inside a Windows AppContainer (lowbox token), the sandbox used by packaged apps and embedders that launch worker processes with
CreateAppContainerProfile+PROC_THREAD_ATTRIBUTE_SECURITY_CAPABILITIES. Depends on the libuv-side fixes in oven-sh/libuv#7 (cherry-pick of upstream libuv/libuv#5181's AppContainer pipe-namespace fix) and oven-sh/libuv#8 (fs stat/realpath bounds and tty exact-fill correctness fixes), both merged into thebunbranch and pinned here.The four unconditional changes below apply everywhere; the two AppContainer-only changes are gated on
bun_sys::windows::is_app_container()(a cachedGetTokenInformation(TokenIsAppContainer)probe) and are no-ops outside a container.Changes
Resolver ancestor-directory tolerance (cross-platform, unconditional).
bun run <script>failed witherror loading current directorywhen any ancestor directory on the path to cwd was unreadable: the resolver builds aDirInfofor every ancestor starting at the drive root, and a sandboxed token (or an execute-only0o111unix directory, Android/data, etc.) denies that listing. A permission-denied ancestor is now treated as an opaque empty directory, the same treatment the existingENOTDIRtolerance applies; errors on the requested directory itself stay fatal. Also fixes #28220 and #30859.Windows
O_RDONLYopen no longer requestsFILE_WRITE_ATTRIBUTES(Windows, unconditional). Theopenatbase access mask unconditionally includedFILE_WRITE_ATTRIBUTES, so opening a fileO_RDONLYon a tree with an RX-only ACL grant (Program Files, read-only shares, the normal sandbox project-tree shape) failedEPERM. The mask now matches libuv'sfs__open(O_RDONLY->GENERIC_READonly; write modes already include it viaGENERIC_WRITE);fs.futimescontinues to work via libuv'sReOpenFile(FILE_WRITE_ATTRIBUTES)at futimes time. Unskipstest-module-readonly.js.Windows directory opens no longer request
FILE_ADD_FILE | FILE_ADD_SUBDIRECTORY(Windows, unconditional).NtCreateFilewith aRootDirectoryhandle checks the target directory's ACL for child creates and renames, not the handle's access mask, so these bits grant nothing and only narrow where the open is admitted. Dropping them letsBun.Glob/recursive readdir/fs.opendirdescend RX-only directories (Program Files, read-only shares, sandboxed project trees); creating children through the handle still works where the ACL allows it. Removes the now-vestigialWindowsOpenDirOptions.read_onlyfield.Named-pipe listen failures surface as Node-shaped errors (Windows, unconditional). They were a codeless
ERR_INVALID_ARG_TYPETypeError; now anErrorwithcode/errno/syscall/pathset, matching the POSIX unix-socket listen path. Fixes #30265.AppContainer-only (gated on
is_app_container(); no-ops outside):GetFinalPathNameByHandleW(VOLUME_NAME_DOS)is denied on every handle inside an AppContainer because the DOS-name translation opens the mount manager. For handles on the system volume, reconstruct the DOS name as<system-drive>:+ theVOLUME_NAME_NTtail (the system directory carries anALL APPLICATION PACKAGES:(RX)ACE by Windows default, so its device name is resolvable from any lowbox); handles on any other volume surface the original denial. Applies to both the typedbun_syswrapper and a raw-ABI drop-in. This is what keeps Bun's resolver andbun installworking inside a container; user-facingfs.realpathgoes through libuv and is left at Node parity (failsEPERM).Bun.TerminalConPTY internal pipe names: insertLOCAL\into the\\.\pipe\...name inside a container (the only namespace an AppContainer may create server pipes under), matching libuv's conditional insert.deps:pins oven-sh/libuvf6e75a7e(=bunbranch after #7 and #8). Behavioural delta at this pin: libuv's internal pipe names gainLOCAL\inside a container (upstream #5181);uv_fs_statof files the OS holds exclusively at a drive root (theC:\pagefile.sysclass) reports the real stats instead ofENOENT;uv_fs_realpathpreserves the real error instead of masking asEBADF; console line reads don't tear characters on an exact-fill allocation and reportUV_ENOBUFSfor allocations too small to convert into.Known limitations
"ignore"stdio opens theNULdevice, whose default ACL denies AppContainer tokens; grant the device ACL to the container SIDs from an elevated context per boot, or use"inherit"stdin.fs.realpath(all variants) failsEPERMinside a container, as it does under Node.js; Bun's own module resolution does not go through it.Tests
test/js/bun/windows/appcontainer.test.tslaunches bun inside a real AppContainer in the regular Windows CI lanes (bun:ffi lowbox launcher, no admin needed) and asserts the sandbox-only behaviours (piped-stdio spawn,LOCAL\pipe namespace,fs.realpathdenial, fork + IPC); hosts that cannot run sandboxed children skip visibly.resolver-permission-denied-ancestor.test.tscovers the ancestor tolerance on unix with an execute-only directory. The globscan.test.tsRX-only case andnamed-pipe-listen-error.test.tserror-shape assertions cover the unconditional Windows changes.