Skip to content

Commit 2a037f5

Browse files
wchy1128claude
authored andcommitted
fix(tls): don't drop Mozilla roots when NODE_EXTRA_CA_CERTS is set on win32
Passing an explicit `connect.ca` to undici replaces Node's default trust store entirely (Mozilla roots + NODE_EXTRA_CA_CERTS), it does not merge. The previous change made win32 (and any platform where no system bundle matched) return only the extra CA, dropping every public root CA — so any public HTTPS endpoint failed with UNABLE_TO_GET_ISSUER_CERT_LOCALLY. Narrow the extra-CA merge to the case where a system bundle was already found (i.e. an explicit `ca` override is already happening). When no system bundle is found, return `null` so Node's default trust store — which already honors NODE_EXTRA_CA_CERTS — handles TLS. Addresses maintainer review on #152. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent d33f7e9 commit 2a037f5

2 files changed

Lines changed: 44 additions & 21 deletions

File tree

__tests__/unit/tls-config.test.ts

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,14 @@ async function runTests() {
2929
assert.ok(certs === null || typeof certs === 'string');
3030
}, results);
3131

32-
await testFunction('getSystemCACerts returns null on Windows unless NODE_EXTRA_CA_CERTS is set', () => {
32+
await testFunction('getSystemCACerts returns null on Windows even when NODE_EXTRA_CA_CERTS is set', () => {
33+
// On Windows, no system bundle is detected and we deliberately return
34+
// null so Node's default trust store (Mozilla roots + NODE_EXTRA_CA_CERTS)
35+
// handles TLS — passing the extra CA as an explicit `ca` would replace
36+
// that store and drop the public roots.
3337
if (process.platform === 'win32') {
3438
const certs = getSystemCACerts();
35-
if (process.env.NODE_EXTRA_CA_CERTS) {
36-
assert.ok(typeof certs === 'string' && certs.length > 0, 'extra CA should be returned on Windows when NODE_EXTRA_CA_CERTS is set');
37-
} else {
38-
assert.equal(certs, null);
39-
}
39+
assert.equal(certs, null, 'win32 always returns null; extra CA flows through Node default path');
4040
} else {
4141
const certs = getSystemCACerts();
4242
assert.ok(certs === null || (typeof certs === 'string' && certs.length > 0));
@@ -84,17 +84,32 @@ async function runTests() {
8484
assert.equal(touched, false, 'win32 skips system bundle discovery and only reads the extra CA path');
8585
}, results);
8686

87-
await testFunction('getSystemCACerts honors NODE_EXTRA_CA_CERTS on win32 (skips system bundle loop)', () => {
88-
const reads: string[] = [];
87+
await testFunction('getSystemCACerts returns null on win32 even with NODE_EXTRA_CA_CERTS (no system bundle → defer to Node default)', () => {
88+
let touched = false;
8989
const certs = getSystemCACerts({
9090
platformName: 'win32',
91-
fileExists: () => { throw new Error('system bundle path should not be probed on win32'); },
92-
readFile: (p) => { reads.push(p); return PEM; },
91+
fileExists: () => { touched = true; return true; },
92+
readFile: () => { touched = true; return PEM; },
9393
caPaths: ['/should/not/be/read'],
9494
extraCaPath: '/opt/extra.pem',
9595
});
96-
assert.equal(certs, PEM, 'win32 returns the extra CA bundle');
97-
assert.deepEqual(reads, ['/opt/extra.pem'], 'only the extra CA path is read on win32');
96+
assert.equal(certs, null, 'win32 defers to Node default trust store; extra CA is honored via NODE_EXTRA_CA_CERTS at the Node level, not by overriding ca');
97+
assert.equal(touched, false, 'no bundle paths are read on win32; NODE_EXTRA_CA_CERTS is handled by Node itself');
98+
}, results);
99+
100+
await testFunction('getSystemCACerts returns null when no system bundle is found even if NODE_EXTRA_CA_CERTS is set', () => {
101+
// Mirrors the win32 case on a minimal Linux container: no CA_BUNDLE_PATHS
102+
// match, but NODE_EXTRA_CA_CERTS is set. Returning null lets Node's
103+
// default trust store (Mozilla + extra) handle TLS, instead of replacing
104+
// it with the extra CA alone and dropping public roots.
105+
const certs = getSystemCACerts({
106+
platformName: 'linux',
107+
fileExists: () => false,
108+
readFile: () => { throw new Error('should not be called'); },
109+
caPaths: ['/nope/a.crt', '/nope/b.crt'],
110+
extraCaPath: '/opt/extra.pem',
111+
});
112+
assert.equal(certs, null, 'no system bundle → defer to Node default; do not override ca with extra alone');
98113
}, results);
99114

100115
await testFunction('getSystemCACerts returns the first readable bundle', () => {

src/tls-config.ts

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,17 @@ export interface CACertDeps {
3535
* Reads system CA certificates from well-known bundle paths, plus an optional
3636
* user-provided extra bundle pointed to by `NODE_EXTRA_CA_CERTS`.
3737
*
38-
* On Windows there is no universal CA bundle path, so only the extra bundle
39-
* (if any) is returned. On other platforms the first readable system bundle
40-
* wins and the extra bundle is appended.
38+
* On Windows (and on any platform where no system bundle is found) this
39+
* returns `null`, so callers pass no explicit `ca` to undici and Node's
40+
* default trust store — Mozilla roots plus `NODE_EXTRA_CA_CERTS` — is used.
41+
* This is intentional: passing an explicit `connect.ca` *replaces* the
42+
* default trust store entirely, which would drop both the Mozilla roots and
43+
* the extra CA unless we re-merged them ourselves.
4144
*
42-
* The extra bundle is folded in here because undici's `connect.ca` option,
43-
* when set, overrides Node's default CA handling and would otherwise ignore
44-
* `NODE_EXTRA_CA_CERTS` — which the built-in `https` module does honor.
45+
* On Linux/macOS when a system bundle *is* found, the system bundle is
46+
* returned with the extra bundle appended. Here an explicit `ca` is already
47+
* being set (overriding the default path), so folding in the extra bundle is
48+
* required to keep `NODE_EXTRA_CA_CERTS` honored in that case.
4549
*/
4650
export function getSystemCACerts(deps: CACertDeps = {}): string | null {
4751
const {
@@ -55,8 +59,8 @@ export function getSystemCACerts(deps: CACertDeps = {}): string | null {
5559

5660
const bundles: string[] = [];
5761

58-
// Windows has no universal CA bundle path; skip auto-detection but still
59-
// honor NODE_EXTRA_CA_CERTS below (undici does not read it natively).
62+
// Windows has no universal CA bundle path; skip auto-detection. Node's
63+
// default trust store (Mozilla + NODE_EXTRA_CA_CERTS) handles Windows.
6064
if (platformName !== "win32") {
6165
for (const caPath of caPaths) {
6266
if (fileExists(caPath)) {
@@ -71,7 +75,11 @@ export function getSystemCACerts(deps: CACertDeps = {}): string | null {
7175
}
7276
}
7377

74-
if (extraCaPath) {
78+
// Only fold in the extra CA when a system bundle already forces an explicit
79+
// `ca` override. If no system bundle was found, returning `null` here lets
80+
// Node's default trust store — which already includes NODE_EXTRA_CA_CERTS —
81+
// handle validation, instead of replacing it with the extra CA alone.
82+
if (extraCaPath && bundles.length > 0) {
7583
try {
7684
bundles.push(readFile(extraCaPath));
7785
} catch {

0 commit comments

Comments
 (0)