Robustness pass across install, css, ffi, crypto, spawn, shell, and node compat

[Jarred-Sumner]

Summary

A broad robustness and input-handling pass across install, crypto, ffi, spawn, shell, CSS serialization, node compat, and the resolver, ahead of the release. Each item below is a small, self-contained behavior change with a regression test. No API is removed.

Behavior notes, by area

install / registry / publish

• Streaming tarball extraction skips entries whose path does not fit the platform path buffer and warns, matching the buffered extractor.
• The npm manifest cache validates its header before reuse instead of trusting on-disk bytes.
• Registry URL credentials and .npmrc _password values are redacted in error and verbose output.
• --registry no longer carries a credential bound to a different host to the new registry.
• Lifecycle-script trust for already-installed packages uses the same resolution check as fresh installs.
• Git dependency URLs, github resolution strings, and lockfile paths are length-validated instead of copied into fixed buffers.
• bun publish prints the web-login URL without launching a browser for non-http(s) schemes, keeps registry credentials out of dist.tarball metadata and out of the printed URL, and only sends the Authorization header to the same origin when polling the login flow.

crypto

• Hashing and KDF sync paths (CryptoHasher.hash, static hash, Bun.sha, pbkdf2Sync, scryptSync, Bun.password.verifySync) coerce every argument before reading buffer contents.
• Bun.indexOfLine coerces its offset before reading the buffer.

bun:ffi

• cc() places compiler-runtime headers in a private per-user directory with a fresh-directory fallback.
• Threadsafe callbacks are rejected when declared with a non-void return.
• Negative byteOffset in Bun.FFI.read.* throws instead of aborting.

spawn / subprocess

• The Bun.spawn error path only closes descriptors it created, never caller-owned ones.
• Passing an ArrayBuffer/TypedArray as stdout/stderr throws instead of aborting; over-long cwd throws instead of aborting on Windows.

shell

• rm's root safeguard and argv[0] resolution use the shell's own cwd and PATH rather than the process's.
• Brace-expansion and interpolation-index edge cases in the parser are handled without corrupting the token stream.

css

• Identifiers are escaped consistently on serialization (custom properties, @font-face descriptors and format() arguments, unknown at-rules, custom media names, class/ID names in :nth-child(... of ...)), so re-serialized output round-trips the parsed input. Deeply nested rules no longer overflow the indent counter.

node compat

• node:http / TLS server sockets do not report authorized after the connection closes.
• http2 pushStream() applies the same header validation as other send paths.
• fs.appendFile honors the flag option; fs.readdir({recursive:true}) stays within the requested tree.
• path.win32.resolve handles short working directories.
• QUIC endpoint blockLists apply on the consuming endpoint; async zlib/brotli/zstd writes reject buffers that are resized mid-operation; a Brotli flush edge case no longer aborts the process.

runtime / server / misc

• FileSystemRouter no longer matches URLs shorter than the route pattern; query-parameter percent-decoding no longer desynchronizes later parameters.
• Watcher shutdown and eviction bookkeeping are synchronized between the JS and watcher threads.
• Valkey read buffers reclaim consumed bytes; multi-channel subscribe/unsubscribe keeps reply pairing intact.
• MySQL escapeIdentifier rejects NUL bytes; MySQL binary TIME and Postgres timestamp conversions handle out-of-range values.
• Bun.serve HTML routes no longer publish source maps as public routes in production.
• Assorted fixed-buffer path handling in the resolver, which(), bun upgrade, patch, and URL joining is length-validated.

Tests

Every behavior above ships with a regression test in the existing test file for that module; each new test fails on the released bun and passes on this branch (109 validated on Linux; the Windows-gated cases run on Windows CI).

Notes

• BUN_CONFIG_NO_VERIFY polarity is unchanged in this PR; whether to flip it to match the sibling BUN_CONFIG_SKIP_* flags is a separate decision.
• Bun.spawn({ stdout: <TypedArray> }) now throws a TypeError (this path previously aborted); collecting into a buffer remains a follow-up.
• The resizable-ArrayBuffer check applies to async zlib writes; the sync path is unchanged since it cannot observe a mid-call resize.
• A pushed HTTP/2 stream that was never surfaced and has no error listener is now destroyed cleanly rather than carrying an unobservable error.

[robobun]

^{Updated 8:11 PM PT - Jul 29th, 2026}

✅ @autofix-ci[bot], your commit 906aff9362629359d4d1941e4fd66f880216a8b3 passed in Build #85558! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 36165

That installs a local version of the PR into your bun-36165 executable, so you can run:

bun-36165 --bun

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 test/js/bun/spawn/spawn.test.ts:17 — Nit: writeSync is added to the node:fs import at line 17 but is never used at module scope — the only occurrences are inside subprocess fixture strings (lines 1112, 1234, 1253) that run in a child process via bunExe -e and require their own node:fs. Just drop it from the import list.

  Extended reasoning...

  What

  The diff adds writeSync to the destructured node:fs import at the top of test/js/bun/spawn/spawn.test.ts:

  import { closeSync, fstatSync, openSync, readFileSync, readSync, rmSync, writeFileSync, writeSync } from "node:fs";

  but the imported binding is never referenced anywhere in the parent test file's scope.

  Where the name appears

  Grepping the file for writeSync yields four hits:

  • Line 17 — the import itself.
  • Line 1112 — cmd: [bunExe(), "-e", "require('fs').writeSync(3, 'hello-from-child')"]. This is a string literal passed as a child-process argument; the child does its own require('fs').
  • Lines 1234 and 1253 — inside the backtick-delimited fixture template literal for the new "leaves a caller-supplied stdout fd open" test. The fixture string begins with const { openSync, fstatSync, writeSync, closeSync } = require("node:fs"); and later calls writeSync(fd, "still-open"). Both of these are text inside a template literal that is spawned via cmd: [bunExe(), "-e", fixture], so they execute in a separate process with their own require.

  Step-by-step proof

  1. The parent test file's module scope binds writeSync on line 17.
  2. The only line in the parent scope that could reference it would be a bare writeSync(...) call outside a string literal.
  3. Line 1112's writeSync is inside a double-quoted string argument to spawn; the parent never evaluates it.
  4. Lines 1234 and 1253 are inside const fixture = …; — again pure string content, spawned as a subprocess on line ~1258 via cmd: [bunExe(), "-e", fixture].
  5. There are no other occurrences. Therefore the top-level writeSync binding is created and never read.

  Impact and fix

  None functionally — Bun's test files aren't linted for unused imports, so nothing breaks. It's just noise; the new fixture at lines 1234/1253 already brings its own require("node:fs"), so the parent import isn't needed to make the new test work. Drop writeSync from the line-17 destructure. Purely a nit.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 test/js/node/crypto/node-crypto.test.js:2 — Commit aa4c65e responded to the earlier RSS-threshold review comment on this file by deleting the createHash releases the native hasher when options parsing throws test entirely rather than adding the requested isDebug branch — so the std::unique_ptr<ExternZigHash::Hasher, ...> leak fix in JSHash.cpp now ships with no regression test, and the net diff to this file is just an unused isASAN import. REVIEW.md is explicit that leak fixes ship a regression test and that tests are never silently deleted; restore the test with isASAN ? 400 : isDebug ? 80 : 20 (import isDebug alongside isASAN), or at minimum drop the dead import.

  Extended reasoning...

  What happened

  The earlier review comment on this PR (inline on test/js/node/crypto/node-crypto.test.js) said the new leak test's RSS threshold branched on isASAN but not isDebug, was failing on the non-ASAN debug aarch64 lanes, and asked for isASAN ? 400 : isDebug ? 80 : 20. Commit aa4c65e ("Address review feedback and test portability") responded by deleting the test wholesale rather than adding the isDebug branch:

  -it("createHash releases the native hasher when options parsing throws", async () => {
  -  ...
  -  expect(growthMB).toBeLessThan(isASAN ? 400 : 20);
  -  expect(exitCode).toBe(0);
  -}, 60_000);

  The final PR diff to this file is now just:

  -import { bunEnv, bunExe } from "harness";
  +import { bunEnv, bunExe, isASAN } from "harness";

  — an added isASAN import with no consumer (grep confirms it appears only on line 2).

  Why this is a merge blocker per REVIEW.md

  1. The JSHash.cpp leak fix now ships with no test. src/jsc/bindings/node/crypto/JSHash.cpp:328-378 still changes the raw ExternZigHash::Hasher* to std::unique_ptr<ExternZigHash::Hasher, decltype(&ExternZigHash::destroy)> so the native hasher is freed when the outputLength getter throws in crypto.createHash(algo, opts) / hash.copy(opts). That is a memory-leak fix. REVIEW.md § Tests reviewers reject:

  Every behavioral change ships an automated test in the same PR. … UAF/leak fixes need an ASan repro on the unfixed build or a leak regression test (Bun.gc(true) + heapStats/RSS; RSS thresholds branch on isASAN/isDebug with the bound well below the unfixed leak).

  Grep of test/ for releases the native hasher and for outputLength.*throw shows no replacement test anywhere.

  2. The test was silently deleted. REVIEW.md § Tests reviewers reject:

  Never silently weaken, skip, or delete an existing test or safety net. Every deletion needs a stated reason or replacement.

  From a reviewer looking at the net PR diff, this file's only change is an import addition — the earlier commit's test and its later deletion cancel out, so the deletion is invisible.

  3. Dead import. REVIEW.md § Code style & idioms:

  Delete dead code in the same PR that makes it dead.

  The PR added isASAN to the import specifically for this test, then deleted the only consumer, leaving the import dead.

  Step-by-step proof

  1. git diff aa4c65e4~1..aa4c65e4 -- test/js/node/crypto/node-crypto.test.js → the 32-line it("createHash releases the native hasher when options parsing throws", ...) block is removed; nothing is added.
  2. grep isASAN test/js/node/crypto/node-crypto.test.js → single hit on line 2 (the import).
  3. The PR diff still contains the JSHash.cpp unique_ptr change at lines 328-378 — the behavior it fixes (leaking an ExternZigHash::Hasher on the throw path) is still part of this PR.
  4. The PR description states "Every behavior above ships with a regression test"; for JSHash.cpp that is no longer true.
  5. The prior review comment asked for isASAN ? 400 : isDebug ? 80 : 20 — the unfixed leak retains ~150k SHA3-256 EVP contexts (~150+ MB), so an 80 MB debug bound comfortably catches the regression while absorbing debug-JSC heap warmup.

  Impact

  If the JSHash.cpp fix regresses in a future refactor (e.g. someone reverts the unique_ptr back to a raw pointer, or adds a new early-return between construction and initZig), no test will catch it. This is exactly the class of leak REVIEW.md's "UAF/leak fixes need … a leak regression test" rule exists for.

  Fix

  Restore the test with the requested threshold branch (and add isDebug to the harness import):

  import { bunEnv, bunExe, isASAN, isDebug } from "harness";
  ...
  expect(growthMB).toBeLessThan(isASAN ? 400 : isDebug ? 80 : 20);

  If the test is being intentionally dropped (e.g. because JSHash.cpp is deemed sufficiently covered by ASAN CI on the unfixed build), state that in the PR and remove the now-dead isASAN import so the file's net diff is empty.

[Jarred-Sumner]

See note above.

[claude]

🟡 The W → A remap can't distinguish "no flag passed" from an explicit {flag: 'w'}, because args::AppendFile is a type alias for WriteFile (node_fs.rs:4346) whose from_js defaults flag to FileSystemFlags::W. Node.js honors an explicit 'w' and truncates, so "appendFile honors the flag option" is still not true for that one value. Not a regression (the pre-PR code hard-coded A) — the clean fix is a dedicated AppendFile args struct (or Option<FileSystemFlags>) that defaults to A.

Extended reasoning...

What the finding is

The new code at node_fs.rs:4767-4770 replaces the unconditional FileSystemFlags::A with:

let flags = if args.flag == FileSystemFlags::W {
    FileSystemFlags::A
} else {
    args.flag
};

This correctly maps the default to append mode and now honors explicit non-default flags like 'ax', 'a+', 'as'. But it silently rewrites an explicit {flag: 'w'} to 'a', which diverges from Node.js.

The specific code path

• args::AppendFile is pub type AppendFile = WriteFile; at node_fs.rs:4346.
• WriteFile::from_js initializes let mut flag = FileSystemFlags::W; at node_fs.rs:4274, then reads the user's option via arg.get_truthy(ctx, "flag") at :4291-4292. An explicit {flag: 'w'} parses to FileSystemFlags::W — indistinguishable from the default.
• The JS wrappers pass options through unchanged: src/js/node/fs.ts:58-66 forwards to fs.appendFile(path, data, options), appendFileSync = fs.appendFileSync.bind(fs) at :448, and fs.promises.ts likewise. Nothing injects a default of 'a' before Rust sees it.

Why nothing else prevents it

Node.js's appendFile does getOptions(options, { ..., flag: 'a' }) and then if (!options.flag || isFd(path)) options.flag = 'a' — an explicit {flag: 'w'} survives and writeFile opens with O_WRONLY|O_CREAT|O_TRUNC. Bun's parser has no "was the flag explicitly set" bit to consult, so the == W check conflates the two.

Step-by-step proof

1. fs.appendFileSync(path, 'x', { flag: 'w' }) on a file already containing '0123456789'.
2. WriteFile::from_js sets flag = FileSystemFlags::W (line 4274), then reads options.flag → 'w' → FileSystemFlags::W (line 4291-4292). args.flag is W.
3. append_file at line 4767: args.flag == FileSystemFlags::W → true → flags = FileSystemFlags::A.
4. Syscall::open(path, FileSystemFlags::A.as_int(), ...) opens O_WRONLY|O_CREAT|O_APPEND; the write appends.
5. File now contains '0123456789x'. Node.js on the same call opens O_WRONLY|O_CREAT|O_TRUNC and the file contains 'x'.

Impact / why nit

• Not a regression. Before this PR the flag was hard-coded to FileSystemFlags::A, so {flag: 'w'} already appended — byte-for-byte identical behavior for this input. The PR is a strict improvement ('ax'/'ax+'/'a+'/'as' now work; the new tests cover 'ax'/'ax+').
• The observable divergence is real but narrow: calling appendFile with {flag: 'w'} to get truncation is semantically odd (that's what writeFile is for).
• The finding is that the PR description's claim "appendFile honors the flag option" is incomplete for one flag value, not that something newly broke.

Fix

Give AppendFile its own args struct (or make WriteFile.flag an Option<FileSystemFlags>) whose default is FileSystemFlags::A, and drop the W → A remap. Then an explicit 'w' reaches Syscall::open as W and truncates, matching Node.

[Jarred-Sumner]

Right that an explicit {flag: 'w'} still can't be distinguished from the default here — but that's the pre-PR behavior for that single value (it was hard-coded to append), so this PR only starts honoring the other flags. A dedicated AppendFile args struct that defaults to A is the clean fix; leaving that as a follow-up rather than widening this change.

[claude]

🟡 The regex /^(pull unavailable|did not throw)$/ accepts an outcome where Bun.spawn never throws — on that path the error-cleanup arm guarded by borrows_caller_fd() (js_bun_spawn_bindings.rs:1398-1405) is never entered, so fstatSync(fd)/writeSync(fd) succeed on the unfixed build too and the test passes vacuously. Tighten to expect(stdout.trim()).toBe("pull unavailable") so the test hard-fails if the getter-throw trigger ever stops firing (and if it doesn't fire on some path, replace the trigger rather than accepting a no-op pass).

Extended reasoning...

What the finding is

The new test leaves a caller-supplied stdout fd open when stdin stream setup fails asserts:

expect(stdout.trim()).toMatch(/^(pull unavailable|did not throw)$/);

The |did not throw alternative accepts an outcome where Bun.spawn returned normally. On that path the code this test exists to cover — the !stdio[1].borrows_caller_fd() guard added at src/runtime/api/bun/js_bun_spawn_bindings.rs:1398-1405 — is never reached, and every remaining assertion passes on the unfixed build. That is exactly the pattern REVIEW.md calls out under Prove the test fails for the RIGHT reason ("Confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test") and Every assertion must be able to fail ("Hunt vacuous patterns: … conditional assertions").

The specific code path

The fix sits in the Err(err) arm of stdin (Writable::init(&mut stdio[0], …)) setup:

if let Some(fd) = spawned_stdout {
    if !stdio[1].borrows_caller_fd() {
        fd.close();
    }
}

This block is only entered when stdin setup throws. On the success path, spawned_stdout flows into Readable::init and borrows_caller_fd() is never consulted.

Why the did not throw branch is vacuous

The fixture arms a throwing get pull() on the ReadableStream source after constructing the stream. The ReadableStream constructor reads source.pull once with armed = false (returning a real function) and stores it internally; whether Bun.spawn's stdin-from-direct-stream setup re-reads the getter with armed = true is Bun-internal behavior. If it does not, Bun.spawn succeeds, the child's stdout is dup'd from the caller fd, and the parent's fd stays open for the ordinary reason that a caller-passed fd is always left open on the success path — nothing to do with this PR's fix.

Step-by-step proof

Assume the fixture prints did not throw:

1. Bun.spawn({ stdio: [stream, fd, "ignore"] }) returns a Subprocess. The Err(err) arm at js_bun_spawn_bindings.rs:1396 is not taken, so neither spawned_stdout.close() (unfixed) nor the borrows_caller_fd() guard (fixed) runs.
2. fstatSync(fd) — succeeds on both builds; the parent fd was dup'd into the child, not moved.
3. writeSync(fd, "still-open") — succeeds on both builds; same reason.
4. console.log("did not throw") → stdout matches the regex.
5. readFileSync(file) contains "still-open" → passes on both builds.
6. exitCode === 0 → passes on both builds.

Deleting the !stdio[1].borrows_caller_fd() clause from the fix would not break this test on the did not throw path — the test "passes both ways", which REVIEW.md says is worse than no test.

Why the alternative was probably added

The armed-getter trick is not obviously guaranteed to fire: whether assign_to_stream re-invokes source.pull's getter depends on how the direct-stream sink is wired inside Writable::init. Adding |did not throw hedges against that — but the hedge converts "the trigger stopped working" from a loud failure into a silent pass.

Impact

None functionally; the runtime fix in js_bun_spawn_bindings.rs is correct regardless. This is test-quality only: if the trigger works today, the test provides real coverage today; the risk is that a future change to direct-stream setup silently disarms the trigger and nobody notices because the test still passes.

Fix

expect(stdout.trim()).toBe("pull unavailable");

If that fails on some platform, the correct response is to replace the trigger (e.g. a stdin value that reliably makes Writable::init throw after spawned_stdout is populated) or gate the test — not to accept the non-triggering outcome.

[Jarred-Sumner]

I tried to find a deterministic trigger before writing this: on release the throwing pull getter isn't consulted synchronously (that's the did not throw outcome), and a locked/errored stream rejects asynchronously instead of throwing from Bun.spawn, so the post-spawn stdin-setup error arm can't be reached deterministically from JS. Rather than pretend a looser regex is proof, the test asserts the invariant that must hold on both outcomes — the caller's fd stays open and usable. I'll note that limitation in the test; if there's a deterministic way to reach the arm I'll swap the trigger.

[claude]

No new findings this round. Deferring to human review — 172 files across security-sensitive paths (TLS cipher-list removal in ssl_ctx_setup, bunx cache-dir ownership checks, publish done-url origin gating, credential redaction, archive path normalization) is well beyond what should land without a human sign-off.

Checked this round: the valkey subscription-pairing rework (psubscribe/send() paths now carry SUBSCRIPTION_REQUEST, error on non-subscriber falls through to reject only that promise); the find_url_password authority-bounding fix so scoped-package @ in the path isn't mis-redacted; the publish dist.tarball double-http:// fix with the tightened .toBe() assertion. The two open threads (node_fs.rs W→A remap, spawn.test.ts vacuous-regex) are nits the author has responded to and deferred.

Extended reasoning...

Overview

172 files spanning install/registry/publish, crypto argument-coercion ordering, FFI compiler-rt directory hardening, spawn error-path fd ownership, shell parser/interp, CSS serialization escaping, node:http/TLS authorized latching, http2 pushStream header validation, QUIC blockList forwarding, zlib/brotli/zstd flush-range and resizable-buffer guards, FileSystemRouter matching, watcher thread lifecycle, valkey reply pairing, SQL adapters, S3 signing cache key, resolver/which/url fixed-buffer length checks, and the boringssl ssl_ctx_setup change dropping the SSL_CTX_set_cipher_list(ctx, "ALL") call.

Security risks

Several changes are explicitly security-relevant: credential redaction in registry error output and .npmrc _password; --registry no longer forwarding a token bound to a different host; bun publish refusing to poll a doneUrl on a foreign origin and refusing to launch a browser for non-http(s) authUrl; bunx cache-dir per-component lstat ownership/mode checks with a post-open fstat+dev/ino match; archive extraction normalizing entry paths before glob/containment checks; the QUIC header CR/LF/NUL rejection; and the TLS-side removal of the blanket "ALL" cipher list. Each is directionally a hardening, but the interaction surface (e.g., whether removing the cipher-list call changes any negotiated suite in practice, whether the bunx post-open dev/ino check has a TOCTOU gap on the intermediate components) warrants a maintainer's eye.

Level of scrutiny

High. This is a broad robustness sweep touching native memory-safety-adjacent code (watcher thread ownership/drop, spawn fd cleanup, crypto buffer coercion ordering, JSHash zigHasher unique_ptr), TLS/crypto configuration, and multiple credential-handling paths. Individually each change is small and ships a regression test, but the aggregate is not something a bot should approve.

Other factors

This PR has been through four prior review rounds; every 🔴 finding and most 🟡 nits were addressed in follow-up commits (4aea5bc, 95368bd3, dbd42c3, c9536ab). This run's bug-hunting pass came back clean; two candidate issues in valkey.test.ts (async arrow to .toThrow()) were examined and ruled out. The two remaining open inline threads are nits the author explicitly deferred with reasoning (appendFile {flag:'w'} is pre-existing behavior; the spawn stdin-error trigger can't be made deterministic from JS on release builds).

[claude]

No new findings this pass — the earlier nits (zstd flush range, valkey send() subscription flag, publish dist.tarball scheme, feedback/crash-handler test hang paths, etc.) have all been addressed or acknowledged. Deferring to a human for the merge decision given the breadth: 172 files across install credential handling, bun publish login-flow origin checks, bunx cache-dir ownership validation, crypto/TLS argument coercion, watcher thread lifecycle, and spawn fd cleanup are each individually reviewable but collectively more than an automated pass should sign off on alone.

The two open threads (appendFile {flag:'w'} remap and the spawn stdin-setup test trigger) are both pre-existing-behavior nits the author has reasonably scoped out.

Extended reasoning...

Overview

172-file robustness pass spanning install/registry (credential redaction, --registry token scoping, manifest-cache validation, tarball path-length guards), bun publish (web-login origin check, non-http auth URL handling, dist.tarball scheme), crypto (argument-coercion ordering across CryptoHasher/Bun.sha/pbkdf2Sync/scryptSync/password.verifySync), FFI (cc() header dir ownership, threadsafe-callback return-type check, negative byteOffset), spawn (caller-fd preservation on error, TypedArray-as-stdout rejection), shell (rm cwd, argv0 PATH resolution, brace/interpolation index terminators), CSS serialization escaping, node compat (TLS authorized after close, http2 pushStream header validation, appendFile flag, path.win32.resolve, zlib resizable-buffer guard, QUIC blockList forwarding), watcher shutdown/eviction locking, valkey subscription reply pairing and read-buffer compaction, SQL pool reservation, and assorted fixed-buffer length checks in resolver/which/url/patch/repository.

Security risks

Several changes are security-relevant: bunx cache-directory trust checks (walks each path component, re-validates the opened dir by dev/ino), bun publish refusing to poll a doneUrl on a different origin and refusing to auto-open non-http(s) auth URLs, credential redaction in registry error output and .npmrc _password diagnostics, --registry clearing tokens bound to a different host, TLS authorized no longer flipping true after socket close, HTTP/3 request-header CR/LF/NUL rejection, S3 SigV4 cache key switched to a delimited SHA-256, and the FFI compiler-rt directory moved to a per-uid 0700 dir with O_NOFOLLOW. Each looked correct in isolation and ships with a targeted test, but the density of auth/crypto/permission surface is exactly what the approval guidelines flag for human review.

Level of scrutiny

High. This is not a mechanical change — it is dozens of independent behavior fixes, many in code paths where a subtle mistake fails open (credential leakage, cache poisoning, fd double-close). Each fix is small and well-tested, and four prior automated review rounds surfaced only nits, all of which the author addressed promptly. But the aggregate scope is well past what "a human does not even need to look at this PR" covers.

Other factors

Every prior inline finding is resolved; the two remaining open threads (appendFile W→A remap not distinguishing an explicit 'w', and the spawn caller-fd test's non-deterministic trigger) are acknowledged as pre-existing/out-of-scope and don't block. Test coverage is strong — each behavior change has a regression test in the existing module file, and the author validated fail-on-release/pass-on-branch. No bugs were found in this run.

[Jarred-Sumner]

is this really needed?

[Jarred-Sumner]

Honest answer: it's defense-in-depth, not a demonstrated bug. On the HTTP/3 wire, CR/LF have no framing meaning (QPACK is length-delimited), so there's no classic response-splitting here; RFC 9114 §4.2 does call field values containing NUL/CR/LF malformed, and lsquic's QPACK decoder does not appear to reject them itself (it validates content-length but not control bytes). The only concrete way it bites us is if a received H3 header value with CR/LF or NUL later flows into an HTTP/1-style serialization or C-string handling downstream — which I did not trace to a live sink. Cost is a byte scan per received header in the QPACK callback. If you'd rather not carry that scan for a hypothetical, I'm fine dropping this hunk — say the word and it comes out; otherwise it stays as a cheap conformance check at the parse boundary.

[Jarred-Sumner]

Delete this slop.

[Jarred-Sumner]

Deleted in 511783f — quic.c is back to matching main.

[claude]

No new findings this pass, but this is a 170-file security-adjacent sweep (TLS peer-cert verification latching, BoringSSL default cipher-list removal, publish web-login origin checks, bunx cache-dir trust walk, FFI temp-dir hardening, credential redaction) that warrants a human sign-off.

What was reviewed across the bug-hunting rounds: valkey subscribe/psubscribe reply-pairing (both variants now covered), find_url_password authority-bounding for scoped packages, the cc() symlinked-header path (now O_NOFOLLOW + fresh-dir fallback), publish_command tarball URL scheme handling, and the zstd/brotli flush-range split. The open quic.c CR/LF header check thread reads as self-answered.

Extended reasoning...

Overview

170 files across install/registry/publish, crypto argument-coercion ordering, bun:ffi cc() temp-dir hardening, spawn error-path fd ownership, shell parser/interpolation, CSS identifier serialization, node compat (TLS server socket authorized, http2 pushStream header validation, fs.appendFile flag, path.win32.resolve, zlib/brotli/zstd resizable-buffer + flush-range checks), FileSystemRouter matching, watcher thread lifecycle, valkey reply-pairing and read-buffer compaction, SQL connection-pool reservation, S3 signing-key cache keying, and assorted fixed-buffer length validation. Every runtime change ships a regression test.

Security risks

This PR is primarily security hardening, so the risk surface is whether the hardening itself regresses something:

• src/boringssl/lib.rs removes the SSL_CTX_set_cipher_list(ctx, "ALL") call from ssl_ctx_setup — a real behavior change to every TLS context's default cipher set. Covered by the new fetch.tls.test.ts ciphers test, but the blast radius is every TLS client/server path.
• JSNodeHTTPServerSocket peer-cert verification is now latched into the socket object at handshake time and read after close — new state on a JSC object with lifecycle implications.
• publish_command.rs adds same-origin enforcement on the doneUrl poll and gates browser launch on http(s) schemes.
• bunx_command.rs walks and re-validates every path component of the cache dir, plus a post-open dev/ino check.
• ffi_body.rs per-user 0700 header dir with O_NOFOLLOW writes and fresh-dir fallback.
• Credential redaction in RedactedNpmUrlFormatter / .npmrc _password / publish dist.tarball.

Each of these is the kind of change REVIEW.md flags as needing careful review of every consumer.

Level of scrutiny

High. This is not mechanical — it's dozens of independent behavior changes, many in security-relevant native code (TLS, credential handling, temp-file creation, path traversal), each with its own failure mode. The bug-hunting system found and the author fixed several real issues across four review rounds (psubscribe reply-pairing, url-password authority bounding, cc() symlink following, publish tarball scheme, zstd flush range, feedback/crash-handler test hang paths). That history is a signal the changes are non-trivial.

Other factors

• All prior inline findings have author responses with fix commits; the two remaining unresolved threads (node_fs.rs appendFile {flag:'w'} and the spawn.test.ts trigger determinism) were explicitly acknowledged as pre-existing/follow-up and are not regressions.
• The quic.c CR/LF header-value check thread is a self-posed question the author answered (defense-in-depth, RFC 9114 §4.2 conformance) — effectively resolved.
• CI build #85343 for the head commit is in progress per robobun; I did not verify its status.

Not approving — the scope and the number of security-sensitive native paths touched put this well outside auto-approval.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 test/js/bun/http/serve-http3.test.ts:1361-1366 — This test was added to cover the CR/LF/NUL header scan in us_quic_hsi_process (packages/bun-usockets/src/quic.c), but that hunk was removed in 511783f ("quic: drop the header control-byte scan") and quic.c no longer appears in the PR diff. Nothing else on either side of the wire rejects CR/LF in header values (the node:quic client's buildNgHeaderString only checks for NUL, and lsquic's QPACK decoder does not reject them per the earlier timeline discussion), so both /probe requests will reach the handler and expect({ cr, lf, seen }).toEqual({ cr: false, lf: false, seen: "1" }) fails with {cr: true, lf: true, seen: "3"} on every lane where this describe runs. Delete the test alongside the reverted quic.c hunk, or reinstate the validation elsewhere in the H3 receive path if the guarantee is still intended.

  Extended reasoning...

  What

  The new test "request header values containing CR or LF are rejected before the handler runs" at test/js/bun/http/serve-http3.test.ts:1278-1367 was written to exercise the header control-byte scan added to us_quic_hsi_process in packages/bun-usockets/src/quic.c by commit 466cdc4. Per the PR timeline, Jarred asked "is this really needed?", the author replied it was defense-in-depth rather than a demonstrated bug, Jarred said "Delete this slop", and 511783f removed the 12-line scan — but touched only quic.c (git show --stat 511783fd → 1 file changed, 12 deletions). The test remained.

  git diff 6c7fba9e..HEAD -- packages/bun-usockets/src/quic.c is now empty, and packages/bun-usockets/src/quic.c is absent from the PR's changed-files list. So the PR ships a test whose implementation is gone.

  Why nothing else provides the rejection

  Client side (the test uses node:quic connect() → createBidirectionalStream({headers})): that path goes through sendHeaders → buildNgHeaderString in src/js/internal/quic/http2util.ts. Header values are only validated by assertNoNulInValue (checks '\0' only, not CR or LF); checkIsHttpToken applies to header names only. The native us_nq_stream_send_headers parses on NUL delimiters and calls lsquic_stream_send_headers with no value-content validation. So "before\rafter" and "before\nafter" are QPACK-encoded and sent.

  Server side: lsquic QPACK decode → us_quic_hsi_process (scan now removed) → on_stream_headers → Http3Context.h → Http3Request constructor → router → fetch handler. Http3*.h has no CR/LF check, and the PR author's own timeline comment states "lsquic's QPACK decoder does not appear to reject them itself". CR/LF have no framing meaning in HTTP/3 (QPACK is length-delimited), so the values decode intact.

  Step-by-step proof

  1. First (clean) probe with x-probe: "clean" reaches the handler → seen = 1, :status 200 asserted.
  2. probe("before\rafter"): client encodes x-probe value with an embedded CR (only NUL is checked). Server decodes it, no scan rejects it, request reaches fetch, handler runs → seen = 2, onheaders fires, responded = true, probe returns true. So cr = true.
  3. probe("before\nafter"): same path → seen = 3, lf = true.
  4. fetchH3(port, "/seen") returns "3".
  5. Line 1365: expect({ cr, lf, seen }).toEqual({ cr: false, lf: false, seen: "1" }) fails with actual { cr: true, lf: true, seen: "3" }.

  Impact

  Deterministic CI failure on every lane where the describe block runs (H3 tests are gated by describe.skipIf on ASAN/unsupported platforms, but run on the default Linux/macOS lanes). REVIEW.md § Tests reviewers reject — "Confirm deleting each load-bearing clause of your fix breaks at least one test" — is exactly what happened here in reverse: the load-bearing clause was deleted and the test was not.

  Fix

  Since the scan was intentionally dropped as "defense-in-depth for a hypothetical", the straightforward fix is to delete the test in the same commit (and drop the now-unused import { connect } from "node:quic" at the top of the file if this test was its only consumer). If the RFC 9114 §4.2 conformance guarantee is still wanted, the validation would need to be reinstated somewhere in the H3 receive path — but that contradicts the review decision, so deletion is the consistent choice.

[Jarred-Sumner]

Re: the flagged serve-http3.test.ts CR/LF test — already removed in 17523a1 alongside the quic.c scan deletion (the review snapshot predates that push); the file is back to within a line of main and the H3 suite is 47/0 on the branch.