Repository navigation
Conversation
The native CA loader read the variable with getenv(), which only sees the C environment. Bun loads .env files into its own env map, so a value set there showed in process.env but never reached the trust store. tls.getCACertificates() read process.env, so the JS view and the native view could disagree in both directions. bun_uws::root_certs now holds one process-wide snapshot of the path, taken from the env map when the runtime VM boots, and falls back to the process environment when nothing takes it. The C++ loader and tls.ts read that snapshot. As in Node, a later write to process.env changes nothing. bun install keeps reading the process environment only.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 4 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 4 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 38 seconds for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (7)
Comment |
|
The automated review hit its plan limit on this run and posted no findings. The diff is unchanged and ready for review. CI is in progress. |
|
Intentional security thing. |
|
Understood. A project's .env must not add trust anchors, so the native store keeps reading the process environment only. Leaving this closed. |
![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.
Beyond the inline findings, I also checked two things and ruled them out: the runtime boot ordering (load_env runs in load_extra_env_and_source_code_printer before any script executes, so no SSL_CTX exists yet on the bun run/bun test paths), and Worker env overrides (web_worker.rs:761 also calls load_extra_env_and_source_code_printer, but the OnceLock keeps the main thread's snapshot, which matches Node's read-once semantics).
Extended reasoning...
Findings were reported inline (bundler-CLI and bun install paths never call load_env, plus a test-concurrency nit), so this body only records what else was examined. I verified from the diff that the OnceLock<Option<ZBox>> returns a process-lifetime NUL-terminated pointer to C++, that path_of treats empty as unset (replacing the dropped C++ empty-string check), and that all VM entry points including workers route through load_extra_env_and_source_code_printer, so first-write-wins yields the main-thread value. Nothing on the runtime path constructs a TLS context before that call. The remaining gaps are the ones already posted inline.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
| // Node reads NODE_EXTRA_CA_CERTS once at startup. Bun loads .env files into | ||
| // process.env, so a value set there must reach the native trust store the | ||
| // same way a value from the real environment does. | ||
| describe("NODE_EXTRA_CA_CERTS from a .env file", () => { |
There was a problem hiding this comment.
🟡 nit (optional): the three new subprocess tests run serially, so the file takes three debug-build startups back to back instead of overlapping them. test/CLAUDE.md asks that independent process-spawning tests be made concurrent unless that is hard; these share no state (each has its own tempDir and env). Fix: use describe.concurrent("NODE_EXTRA_CA_CERTS from a .env file", ...) (or test.concurrent) for the block at test/js/node/tls/test-node-extra-ca-certs.test.ts:177.
Extended reasoning...
Each of the three tests spawns a fresh bun process that starts a TLS server, fetches, and exits. Under a debug+ASAN build every startup is slow, and the serial describe at line 177 adds their wall time together. Nothing in the tests shares state: separate tempDir per test, env built per test, port 0. Switching to describe.concurrent overlaps them with no behavior change. The rest of the file predates this PR and is also serial, which is why this is only a nit and not a blocker.
Verification: nit. Triggering condition: every run of this file (CI debug/ASAN lanes in particular). The new block at test/js/node/tls/test-node-extra-ca-certs.test.ts:177 is describe("NODE_EXTRA_CA_CERTS from a .env file", () => { with three plain test(...) calls (lines 197, 221, 245), each doing await using dir = tempDir(...) and await using proc = spawn({ cmd: [bunExe(), "main.js"], ... }).…
| /// Only the first call counts. | ||
| pub fn load_env(extra_ca_certs: Option<&[u8]>) { | ||
| let _ = EXTRA_CA_CERTS.set(path_of(extra_ca_certs)); | ||
| } |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Developers running bun build with a macro or plugin that fetches over HTTPS still cannot trust a CA given via .env, even though the bundler loads that .env and shows the path in process.env. Nothing on the bundler-CLI path calls load_env, so src/uws/root_certs.rs:28 falls back to the process environment and the macro's fetch fails with DEPTH_ZERO_SELF_SIGNED_CERT. The PR fixes one entry point of a multi-entry class. Fix: seed the snapshot from the bundler's env map wherever a VM is created for macros/plugins (jsc_hooks macro context, build_command), or state the exclusion in the PR alongside bun install.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Extended reasoning...
User runs bun build ./entry.ts in a project whose .env sets NODE_EXTRA_CA_CERTS; a macro (import { data } from './m.ts' with { type: 'macro' }) calls fetch() against an internal HTTPS host signed by that CA. build_command.rs:451 sets the bundler env behavior from the CLI so run_env_loader (transpiler.rs:773) loads process env plus .env into the bundler map; process.env inside the macro VM shows the path. No caller on this path invokes bun_uws::root_certs::load_env (callers: run_command.rs:1087/1215, test_command.rs:1963, repl_command.rs:128, bake/production.rs:195, web_worker.rs:761 only). The macro's fetch creates the HTTPS context lazily; root_certs.cpp:182 calls Bun__Node__extraCACertsPath, root_certs.rs:28 initialises from env_var::NODE_EXTRA_CA_CERTS::get() (process env only) and returns null; verification fails. The finder classed this pre_existing because the base used getenv there too; that is true, but REVIEW.md asks that every sibling entry point receiving the same fix be covered or explicitly excluded, and the PR notes exclude only bun install. Population: bundler users…
Verification: pre-existing (the base's getenv() already misses .env on this path; the PR fixes bun run/bun test/workers but not the bun build CLI macro VM). Trigger: bun build ./entry.ts in a project whose .env sets NODE_EXTRA_CA_CERTS, with a macro that fetch()es an HTTPS host issued by that CA. Mechanism verified: - The only callers of load_extra_env_and_source_code_printer (the sole caller of…
| /// The path as a NUL-terminated string that lives for the process, or null when the variable is unset or empty. | ||
| #[unsafe(no_mangle)] | ||
| extern "C" fn Bun__Node__extraCACertsPath() -> *const c_char { | ||
| match EXTRA_CA_CERTS.get_or_init(|| path_of(env_var::NODE_EXTRA_CA_CERTS::get())) { |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: bun install still ignores a NODE_EXTRA_CA_CERTS set in the project's .env, so installs from a private registry keep failing while bun run now works. Nothing on the install path calls load_env, so Bun__Node__extraCACertsPath at src/uws/root_certs.rs:28 falls back to the C environment. The PR note says #42597 keeps .env out of the install's proxy and TLS settings, but PackageManager.rs:1914 loads the default .env files and PackageManager.rs:897/:901 read proxy and NODE_TLS_REJECT_UNAUTHORIZED from that map. Fix: snapshot the install env map too, e.g. call bun_uws::root_certs::load_env(env.get(b"NODE_EXTRA_CA_CERTS")) right after env.load(...) in PackageManager init, so every command that honors .env for TLS honors it for the CA file.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Extended reasoning...
The PR's stated reason for leaving bun install on the process environment is that the install's HTTP client already excludes .env for proxy and TLS settings. The code says otherwise. PackageManager.rs:1895-1919 allocates the install's env loader, calls env.load_process() and then env.load(&env_probe_keys, &[], DotEnvFileSuffix::Production, false); the last argument is skip_default_env = false, so env_loader.rs:617-618 loads .env.production.local, .env.local, .env.production and .env into the map. PackageManager.rs:897 http_proxy and PackageManager.rs:901 tls_reject_unauthorized read from that map (env_loader.rs:320 reads NODE_TLS_REJECT_UNAUTHORIZED from self.get). NetworkTask.rs:685 and :936 apply pm.tls_reject_unauthorized() to every registry request. So for the install command, .env already drives proxy and certificate verification, which is exactly the class of setting the note claims it excludes. After this PR, Bun__Node__extraCACertsPath (root_certs.rs:28) runs get_or_init with env_var::NODE_EXTRA_CA_CERTS::get() because no install code path calls…
Verification: pre-existing; acknowledged in diff: the PR description's Notes say "bun install does not take the .env snapshot on purpose. #42597 keeps the project's .env out of the install's HTTP client for proxy and TLS settings, and the trust store follows that rule" — the first half holds, the stated justification does not. Trigger: bun install from a registry whose CA is only reachable via…
2 participants
Problem
NODE_EXTRA_CA_CERTSset in a.envfile shows inprocess.env, buttls.getCACertificates("extra")is empty andfetch()to a server that the file's CA issued fails withDEPTH_ZERO_SELF_SIGNED_CERT(orUNABLE_TO_VERIFY_LEAF_SIGNATURE). The same value in the real environment works.us_get_root_extra_cert_instances()inpackages/bun-usockets/src/crypto/root_certs.cpp:178readsgetenv("NODE_EXTRA_CA_CERTS"). Bun loads.envfiles into its own env map, not into the C environment.tls.ts:1727readprocess.envinstead, so the JS list and the native store could also disagree after a runtime write.Fix
src/uws/root_certs.rskeeps one process-wide snapshot of the path in aOnceLock.VirtualMachine::load_extra_env_and_source_code_printerfills it from the env map at VM boot, after the.envfiles are loaded and before any TLS context exists. Without that call (bun install), the first read falls back to the process environment.Bun__Node__extraCACertsPath()instead ofgetenv.tls.tsalways appends the native extra list, which is empty when the variable is unset.NODE_EXTRA_CA_CERTSonce at startup and ignores laterprocess.envwrites. The snapshot gives the C++ store andtls.getCACertificates()the same single source. A worker with a customenvcannot change it, because only the first call counts.test/js/node/tls/test-node-extra-ca-certs.test.ts(three new tests, two fail on 1.4.3). Alsotest-use-system-ca,node-tls-cert,node-tls-context,node-tls-root-certs-concurrent-init,fetch.tls, issue 24339, and the Nodetest-tls-env-*andtest-tls-get-ca-certificates-*files.Background
src/dotenv/env_loader.rs) holds the process environment plus the.envfiles in one map.process.envis built from that map. A.envvalue never overrides a real environment variable.us_get_default_ca_store()from the bundled roots, the OpenSSL default file, theNODE_EXTRA_CA_CERTSfile, and (with--use-system-ca) the OS store.fetch(),tls.connect(), andBun.serveshare it.NODE_TLS_REJECT_UNAUTHORIZEDand the proxy variables already take effect from.envthrough the same map. This change givesNODE_EXTRA_CA_CERTSthe same treatment.Notes
Scope: the issue also describes
NODE_USE_SYSTEM_CAfrom.env. That variable is left as is. #34424 changes its plumbing to a per-thread model, and this PR does not touchus_should_use_system_ca(),Bun__Node__UseSystemCA, or theprocess.env.NODE_USE_SYSTEM_CAcheck intls.ts.bun installdoes not take the.envsnapshot on purpose. #42597 keeps the project's.envout of the install's HTTP client for proxy and TLS settings, and the trust store follows that rule. The install reads the variable from the process environment, as before.maybeWarnAboutExtraCACertsintls.tsstill readsprocess.envfor the missing-file warning. #42991 replaces that function with the native load, which reads the snapshot.Manual probes with the debug build: a worker started with
env: { NODE_EXTRA_CA_CERTS: ... }sees the main thread's snapshot.bun installin a directory withNODE_EXTRA_CA_CERTSin.envruns unchanged.Self-review: the review asked to split the
NODE_USE_SYSTEM_CAhalf out (done), to wirebun install(not done, see above), and to move the warning to the native path (left to #42991).