Tighten ffi pointer bounds, sparse archive extraction, and the Windows default trust store#31581
Tighten ffi pointer bounds, sparse archive extraction, and the Windows default trust store#31581Jarred-Sumner wants to merge 8 commits into
Conversation
…tore on Windows On Windows, the bundled BoringSSL's built-in default cert file/dir paths resolve against the drive root, which is not a meaningful location there. Only honor SSL_CERT_FILE/SSL_CERT_DIR when explicitly set; Linux/macOS behavior is unchanged.
|
Updated 4:50 PM PT - May 29th, 2026
✅ @Jarred-Sumner, your commit 0532a928b0b2a62ef9445e13b3c2df587e482962 passed in 🧪 To try this PR locally: bunx bun-pr 31581That installs a local version of the PR into your bun-31581 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughWindows OpenSSL CA-store loading now reads OpenSSL default cert-file/dir helpers; libarchive gains sparse-file awareness (FFI + extraction changes + regression test); FFI pointer offset validation tightened with corresponding tests. ChangesWindows Certificate Store Initialization
Sparse File Support in Archive Extraction
FFI Pointer Boundary Validation
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
test/js/node/tls/test-use-system-ca.test.ts (1)
133-140: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
expect(stderr).toBe("")for consistency with the second test.The second test (line 171) asserts stderr is empty, but this test doesn't. Since both use
bunEnv(which setsBUN_DEBUG_QUIET_LOGS=1), stderr should be empty on success. Adding the assertion helps catch unexpected debug output or regressions.Suggested change
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); // The planted certificate must not be in the default trust store, so the // fetch has to fail certificate verification. expect(stdout).not.toContain("FETCH_OK"); expect(stdout).toContain("FETCH_ERR"); expect(stdout).toMatch(/SELF_SIGNED|CERT|UNABLE_TO_VERIFY/i); + expect(stderr).toBe(""); expect(exitCode).toBe(0);Based on learnings: "In oven-sh/bun Jest/Bun test files under
test/js/that spawn subprocesses usingbunEnvfrom theharnessmodule, it's safe and intentional to assertexpect(stderr).toBe("")unconditionally."🤖 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 `@test/js/node/tls/test-use-system-ca.test.ts` around lines 133 - 140, The test captures subprocess output into stdout, stderr, and exitCode via proc (const [stdout, stderr, exitCode] = ...); add an assertion expect(stderr).toBe("") after those captures (mirroring the second test) so unexpected debug/diagnostic output is detected; this is safe because the test uses bunEnv/bun debug quiet logs and keeps behavior consistent with the other test that already asserts stderr is empty.
🤖 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 `@test/js/node/tls/test-use-system-ca.test.ts`:
- Around line 133-140: The test captures subprocess output into stdout, stderr,
and exitCode via proc (const [stdout, stderr, exitCode] = ...); add an assertion
expect(stderr).toBe("") after those captures (mirroring the second test) so
unexpected debug/diagnostic output is detected; this is safe because the test
uses bunEnv/bun debug quiet logs and keeps behavior consistent with the other
test that already asserts stderr is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0b00a10-975c-4217-8d67-75d5e1bec66d
📒 Files selected for processing (1)
test/js/node/tls/test-use-system-ca.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
All three issues I flagged are addressed and the current diff looks correct, but since this touches the Windows default TLS trust store and untrusted-archive extraction I'd like a human to sign off — particularly on the C:\etc\ssl\cert.pem test approach the description already calls out.
Extended reasoning...
Overview
Three independent hardening fixes: (1) bun:ffi ptr()/get_ptr_slice() now reject out-of-range byteOffset values via saturating_add/saturating_sub + unsigned_abs() and a two-sided bounds check; (2) Bun.Archive extraction skips Linux fallocate for sparse entries and (on non-Windows only) ftruncates to the declared real size so holes stay holes; (3) on Windows, us_get_default_ca_store() no longer calls X509_STORE_set_default_paths() (which would consult the compiled-in /etc/ssl/... paths that resolve to user-writable drive-root locations) and instead only honors explicit SSL_CERT_FILE/SSL_CERT_DIR env vars. Each fix has a regression test.
Prior review status
I raised three inline issues on earlier revisions (i64::MIN negation overflow in ptr_(), the Windows ftruncate allocating the full declared size on NTFS, and the same negation overflow in get_ptr_slice()). All three are now fixed in the current diff and the author resolved each thread with a clear explanation. The bug-hunting pass on the latest revision found nothing new.
Security risks
This is the crux of why I'm not auto-approving. The root_certs.cpp change alters how the default CA trust store is assembled on Windows — a change in the wrong direction here could either silently trust attacker-planted roots or break TLS verification entirely. The change looks correct to me (it strictly removes an implicit, attacker-influenceable trust source while preserving the explicit env-var override and the bundled root set), but trust-store initialization is exactly the category where a second pair of human eyes is warranted. The archive change handles untrusted tar input and the ffi change touches raw-pointer arithmetic; both are hardening in the safe direction and look right.
Level of scrutiny
Medium-high. Individually each hunk is small and mechanical, but the surfaces are security-sensitive (crypto trust store, untrusted-archive extraction, FFI pointer bounds). The author also explicitly asked reviewers to weigh in on whether the Windows regression test — which creates and removes <drive-root>\etc\ssl\cert.pem on the runner — is acceptable on persistent CI machines or should be reworked. That's a judgment call I shouldn't make on the author's behalf.
Other factors
The one CI failure (streams-leak.test.ts on x64-baseline) is unrelated to anything this PR touches. Tests cover the new behavior on the platforms where it applies; Windows sparse-file handling is explicitly deferred to a follow-up and the code is gated accordingly.
2 participants
Three small hardening fixes in
bun:ffi,Bun.Archive, and the Windows TLS trust store. Continuation of the recent hardening series; each comes with a regression test.ptr()rejectsbyteOffsetvalues that fall outside the source view (previously a negative or oversized offset produced a pointer outside the buffer)./etc/ssl/...) for the default CA store on Windows — those resolve to user-creatable locations likeC:\etc\ssl.SSL_CERT_FILE/SSL_CERT_DIRare still honored when explicitly set, and non-Windows platforms are unchanged.Note for reviewers: the Windows trust-path regression test is necessarily Windows-only and creates/removes
<drive-root>\etc\ssl\cert.pemduring the run (skipped if the file already exists); flag if that's undesirable on persistent runners and it can be reworked to a subprocess with a redirected drive.cargo checkpasses on all 10 CI targets; the ffi, archive, and TLS CA suites pass locally with no new failures.