From 0038121368055a90bf5efeb7f9802bee1980d304 Mon Sep 17 00:00:00 2001 From: minhn4 Date: Fri, 17 Jul 2026 14:58:32 +0700 Subject: [PATCH 1/2] fix: trust the OS certificate store so login works behind TLS proxies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Users behind a corporate proxy hit this at the Login step: Login failed: fetch failed (self-signed certificate in certificate chain) Node verifies TLS against its own bundled Mozilla CA snapshot and never consults the OS trust store (tls.rootCertificates: "fixed at release time... identical on all supported platforms"). A TLS-intercepting proxy (Zscaler/Netskope/Fortinet) or HTTPS-scanning AV re-signs every chain with a corporate root that MDM/GPO installed into the *OS* store — so the user's browser works and we fail. That per-machine variation is why only some users are affected. The error code pins it: SELF_SIGNED_CERT_IN_CHAIN is an untrusted self-signed ROOT in the presented chain (interception), not a self-signed leaf, which reports DEPTH_ZERO_SELF_SIGNED_CERT instead. Merge the OS store into Node's default CA set via tls.getCACertificates("system"), which reads that store even without the --use-system-ca flag — so affected users need no configuration. Load-bearing details: - Merge "default" + "system", never "bundled" + "system". "default" already folds in NODE_EXTRA_CA_CERTS; narrowing it would silently drop the certs of users who fixed this the documented way. - Call it from loggedFetch, not at startup: it costs ~25ms (a native OS-store read) and commands that never open a socket shouldn't pay. Every network path goes through loggedFetch (16 call sites, zero raw fetch), so first-request is the cheapest and completest hook. - Every failure mode is a no-op, including an empty system store. Trust config isn't ours to have opinions about, and a bad merge would break users whose certs already work. - The APIs need Node >=22.19/24.5 (our floor is 22.5), hence tlsApi.supported(). Accessed off the default import, never destructured — a named ESM import of a missing builtin export is a link-time error on older Node. Also add describeNetworkError: unwrap Node's bare `fetch failed` (the real reason hides on err.cause) and append a remedy for cert codes. Node only volunteers its own --use-system-ca hint for DEPTH_ZERO / UNABLE_TO_VERIFY_LEAF / UNABLE_TO_GET_ISSUER, which pointedly excludes SELF_SIGNED_CERT_IN_CHAIN — the corporate-proxy case it most exists for prints nothing. When a cert error survives the merge the root isn't in the OS store either, so the hint names NODE_EXTRA_CA_CERTS rather than --use-system-ca, which would be a dead end. Verified by driving the real loggedFetch against a proxy-shaped chain (leaf + untrusted self-signed root): with the root in the OS store login now succeeds (HTTP 200) where it previously failed with the exact reported error; without it, the error carries an actionable remedy. Note the blast radius: this covers our own process only. `npm i -g` during install and the agents themselves are separate processes behind the same proxy and need NODE_EXTRA_CA_CERTS / npm's cafile of their own. Co-Authored-By: Claude Opus 4.8 (1M context) --- AGENTS.md | 15 ++++ src/LoginApp.tsx | 3 +- src/components/AdminLogin.tsx | 3 +- src/components/Login.tsx | 16 +--- src/lib/log.ts | 19 +++++ src/lib/tls.ts | 148 ++++++++++++++++++++++++++++++++++ tests/lib/tls.test.ts | 140 ++++++++++++++++++++++++++++++++ 7 files changed, 330 insertions(+), 14 deletions(-) create mode 100644 src/lib/tls.ts create mode 100644 tests/lib/tls.test.ts diff --git a/AGENTS.md b/AGENTS.md index 9f19c84..8487ef1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -134,6 +134,21 @@ Supabase coordinates (`supabase_url`, `supabase_anon_key`) and the public gatewa - Tests that exercise real `login()` must mock `POST /codev-backend/config` if (and only if) the caller also calls `refreshCodevConfig`. 2. **`runUpload` retries once on a "refreshable" error.** `isRefreshableError` (in `src/lib/upload.ts`) is deliberately narrow: `Missing supabase_…` from the cache accessors, or HTTP `401`/`403` from any Supabase or backend fetch. `5xx`, `404`, network errors, and timeouts are NOT retried — refreshing won't help and we'd amplify the outage. Per-file upload errors stay in `summary.errors` and don't trigger the pipeline-level retry. If you change `runSupabaseUpload`'s shape, keep that boundary intact. +## TLS trust (corporate proxies) + +Node verifies TLS against its **own bundled Mozilla CA snapshot and never consults the OS trust store** ([tls.rootCertificates](https://nodejs.org/api/tls.html#tlsrootcertificates): "fixed at release time… identical on all supported platforms"). Users behind a TLS-intercepting proxy (Zscaler/Netskope/Fortinet) or HTTPS-scanning AV get every chain re-signed by a corporate root that MDM/GPO installed into the *OS* store — so their browser works and we fail with `fetch failed (self-signed certificate in certificate chain)`. + +`src/lib/tls.ts#ensureSystemCaCerts` fixes this with no user configuration: `tls.getCACertificates("system")` reads the OS store **even without the `--use-system-ca` flag**, so we merge it into the default set. Details that are load-bearing: + +- **Merge `default` + `system`, never `bundled` + `system`.** `"default"` already folds in `NODE_EXTRA_CA_CERTS`; narrowing it would silently drop the certs of users who fixed this the documented way. +- **Called from `loggedFetch`, not at startup.** It costs ~25ms (a native OS-store read), and commands that never open a socket shouldn't pay it. Every network path goes through `loggedFetch` — 16 call sites, zero raw `fetch` — so first-request is both the cheapest and the completest hook. Memoized per process; `tests/lib/tls.test.ts` pins that. +- **Every failure mode is a no-op**, including an empty system store (`setDefaultCACertificates` is then never called, keeping Node's behavior byte-identical). Trust config isn't ours to have opinions about, and a bad merge would break users whose certs already work. +- The APIs need Node ≥22.19/24.5 (our floor is 22.5), hence `tlsApi.supported()`. They're accessed off the default import, never destructured — a named ESM import of a builtin export that doesn't exist is a link-time error on older Node. + +`describeNetworkError` unwraps Node's bare `fetch failed` (the real reason hides on `err.cause`) and appends a remedy for cert codes. It exists because **Node's own `--use-system-ca` hint pointedly excludes `SELF_SIGNED_CERT_IN_CHAIN`** — `crypto_common.cc` gates it on `DEPTH_ZERO_SELF_SIGNED_CERT` / `UNABLE_TO_VERIFY_LEAF_SIGNATURE` / `UNABLE_TO_GET_ISSUER_CERT` only, so the corporate-proxy case, the one the hint most exists for, is the one that prints nothing. If a cert error survives the merge, the root isn't in the OS store either, so the hint names `NODE_EXTRA_CA_CERTS` rather than `--use-system-ca`, which would be a dead end. + +Note the blast radius: this only covers **our own** process. `npm i -g` during install and the agents themselves are separate processes behind the same proxy and need `NODE_EXTRA_CA_CERTS` / npm's `cafile` of their own. + ## Diagnostic logging `~/.codev-hub` has two log homes — don't mix them up: diff --git a/src/LoginApp.tsx b/src/LoginApp.tsx index c372464..1fd4382 100644 --- a/src/LoginApp.tsx +++ b/src/LoginApp.tsx @@ -19,6 +19,7 @@ import { saveSkillhubCookie, } from "@/lib/auth.js"; import { type SkillhubUser, skillhubSignIn } from "@/lib/skillhub.js"; +import { describeNetworkError } from "@/lib/tls.js"; type Phase = "preparing" | "login" | "refreshing-config" | "done"; @@ -98,7 +99,7 @@ function AdminLoginApp({ saveSkillhubCookie(cookie); handleDone(u); } catch (err) { - const msg = err instanceof Error ? err.message : String(err); + const msg = describeNetworkError(err); setError(msg); // Reject waitUntilExit so the dispatcher exits non-zero; hold the // error frame briefly so it's readable first. diff --git a/src/components/AdminLogin.tsx b/src/components/AdminLogin.tsx index 71016ec..4c3d921 100644 --- a/src/components/AdminLogin.tsx +++ b/src/components/AdminLogin.tsx @@ -3,6 +3,7 @@ import Spinner from "ink-spinner"; import { useCallback, useRef, useState } from "react"; import { saveSkillhubCookie } from "@/lib/auth.js"; import { type SkillhubUser, skillhubSignIn } from "@/lib/skillhub.js"; +import { describeNetworkError } from "@/lib/tls.js"; interface AdminLoginProps { onDone: (user: SkillhubUser) => void; @@ -56,7 +57,7 @@ export function AdminLogin({ saveSkillhubCookie(cookie); onDone(user); } catch (err) { - const msg = err instanceof Error ? err.message : String(err); + const msg = describeNetworkError(err); const used = attemptsRef.current + 1; attemptsRef.current = used; setAttempts(used); diff --git a/src/components/Login.tsx b/src/components/Login.tsx index 9b25d29..e34e4ec 100644 --- a/src/components/Login.tsx +++ b/src/components/Login.tsx @@ -4,6 +4,7 @@ import { useCallback, useEffect, useState } from "react"; import { PasteBackPrompt, usePasteBack } from "@/components/PasteBack.js"; import { type AuthData, login } from "@/lib/auth.js"; import { clipboard } from "@/lib/clipboard.js"; +import { describeNetworkError } from "@/lib/tls.js"; interface LoginProps { onDone: (auth: AuthData) => void; @@ -71,18 +72,9 @@ export function Login({ onDone, fallbackDelayMs = 3000 }: LoginProps) { onDone(auth); }) .catch((err: Error) => { - // Node's built-in fetch throws `TypeError: fetch failed` for any - // network-layer failure and stashes the real reason (DNS, TLS, - // proxy interception, etc.) on `err.cause`. Surface it so users - // can self-diagnose instead of staring at a bare "fetch failed". - const cause = err.cause; - const causeMsg = - cause instanceof Error - ? cause.message - : cause !== undefined - ? String(cause) - : ""; - setError(causeMsg ? `${err.message} (${causeMsg})` : err.message); + // Unwraps Node's bare `fetch failed` to the real reason (DNS, TLS, + // proxy interception) and appends a remedy for certificate failures. + setError(describeNetworkError(err)); }); }, [addLog, onDone, attempt]); diff --git a/src/lib/log.ts b/src/lib/log.ts index f027aaf..e614d39 100644 --- a/src/lib/log.ts +++ b/src/lib/log.ts @@ -9,6 +9,10 @@ import { import { join } from "node:path"; import { VERSION } from "@/lib/const.js"; import { cliLogsDir } from "@/lib/paths.js"; +import { ensureSystemCaCerts } from "@/lib/tls.js"; + +// The CA merge is logged once per process, on the first request that triggers it. +let caMergeLogged = false; // CoDev's local diagnostic log: one Elastic-Common-Schema NDJSON document per // line, written to ~/.codev-hub/logs/codev-YYYYMMDD.ndjson (UTC date). The files @@ -267,6 +271,21 @@ export async function loggedFetch( ): Promise { const url = String(input); const method = init?.method ?? "GET"; + // Trust the OS certificate store before the first socket opens, so users + // behind a TLS-intercepting proxy don't hit `self-signed certificate in + // certificate chain`. Memoized, so only the first request pays for it. + const ca = ensureSystemCaCerts(); + if (!caMergeLogged) { + caMergeLogged = true; + logDebug(`system CA store: ${ca.status}`, { + action: "http.request", + extra: { + ca_status: ca.status, + ca_system_count: ca.systemCount, + ca_error: ca.error ?? null, + }, + }); + } logDebug(`http ${method} ${endpoint}`, { action: "http.request", eventType: "start", diff --git a/src/lib/tls.ts b/src/lib/tls.ts new file mode 100644 index 0000000..a815815 --- /dev/null +++ b/src/lib/tls.ts @@ -0,0 +1,148 @@ +import tls from "node:tls"; + +// Node verifies TLS against its own bundled Mozilla CA snapshot and never +// consults the OS trust store (tls.rootCertificates: "fixed at release time… +// identical on all supported platforms"). On a machine behind a TLS-intercepting +// proxy (Zscaler/Netskope/Fortinet) or HTTPS-scanning AV, every chain is +// re-signed by a corporate root that MDM/GPO installed into the *OS* store — so +// browsers work and we fail with `fetch failed (self-signed certificate in +// certificate chain)`. +// +// tls.getCACertificates("system") reads that OS store even without the +// --use-system-ca flag, so merging it into the default set fixes those users +// with no configuration on their side. +// +// Indirection so tests can simulate a Node that predates these APIs (mirrors +// `spawner` in run.ts / `browserOpener` in auth.ts). Not destructured at import: +// on Node < 22.19 these are `undefined`, and a named ESM import of a missing +// builtin export is a link-time error. +export const tlsApi = { + getCACertificates: (type: string): string[] => + ( + tls as unknown as { + getCACertificates?: (t: string) => string[]; + } + ).getCACertificates?.(type) ?? [], + setDefaultCACertificates: (certs: string[]): void => { + ( + tls as unknown as { + setDefaultCACertificates?: (c: string[]) => void; + } + ).setDefaultCACertificates?.(certs); + }, + supported: (): boolean => + typeof (tls as unknown as Record).getCACertificates === + "function" && + typeof (tls as unknown as Record) + .setDefaultCACertificates === "function", +}; + +export type CaMergeStatus = "merged" | "unsupported" | "empty" | "failed"; + +export interface CaMergeResult { + status: CaMergeStatus; + /** How many certs the OS store contributed. */ + systemCount: number; + error?: string; +} + +let cached: CaMergeResult | null = null; + +// Merges the OS trust store into Node's default CA set, once per process. +// +// Called from `loggedFetch` rather than at startup: it costs ~25ms (a native +// read of the OS store), and commands that never open a socket — help, version, +// logs, restore — shouldn't pay for it. Every network path in the CLI goes +// through `loggedFetch`, so first-request is both the cheapest and the +// completest hook. +// +// Best-effort by construction: TLS trust is not this CLI's job to have opinions +// about, and any failure here must leave Node's default behavior exactly as it +// was rather than break a user whose certs already work. +export function ensureSystemCaCerts(): CaMergeResult { + if (cached) return cached; + cached = mergeSystemCaCerts(); + return cached; +} + +function mergeSystemCaCerts(): CaMergeResult { + // Node < 22.19 / < 24.5. Those users need --use-system-ca (22.15+) or + // NODE_EXTRA_CA_CERTS; describeNetworkError tells them so. + if (!tlsApi.supported()) return { status: "unsupported", systemCount: 0 }; + try { + const system = tlsApi.getCACertificates("system"); + if (system.length === 0) return { status: "empty", systemCount: 0 }; + // "default" already folds in NODE_EXTRA_CA_CERTS, so a user who fixed this + // the documented way keeps working — don't narrow this to the bundled set. + const defaults = tlsApi.getCACertificates("default"); + tlsApi.setDefaultCACertificates([...defaults, ...system]); + return { status: "merged", systemCount: system.length }; + } catch (err) { + return { + status: "failed", + systemCount: 0, + error: err instanceof Error ? err.message : String(err), + }; + } +} + +// Test-only: drop the memoized result so each case re-runs the merge. +export function resetSystemCaCertsCache(): void { + cached = null; +} + +// OpenSSL verify failures that mean "I don't trust this chain", as opposed to +// DNS/connection/timeout errors. Node surfaces them on `err.cause.code`. +const CERT_ERROR_CODES = new Set([ + "SELF_SIGNED_CERT_IN_CHAIN", + "DEPTH_ZERO_SELF_SIGNED_CERT", + "UNABLE_TO_GET_ISSUER_CERT", + "UNABLE_TO_GET_ISSUER_CERT_LOCALLY", + "UNABLE_TO_VERIFY_LEAF_SIGNATURE", + "CERT_UNTRUSTED", + "SELF_SIGNED_CERT_IN_CHAIN_ERR", +]); + +function certHint(): string { + const ca = ensureSystemCaCerts(); + if (ca.status === "unsupported") { + return ( + `This looks like a TLS-intercepting proxy or antivirus. Node ${process.version} ` + + "can't read your system certificate store — upgrade to Node 22.15+ and re-run, " + + "or point NODE_EXTRA_CA_CERTS at your organization's root CA (PEM file)." + ); + } + // The merge ran and the chain still isn't trusted, so the intercepting root + // isn't in the OS store either — pointing at --use-system-ca would be a dead + // end. Only the explicit PEM can help. + return ( + "This looks like a TLS-intercepting proxy or antivirus, and its root CA isn't " + + "in your system certificate store. Ask IT for the root CA and point " + + "NODE_EXTRA_CA_CERTS at it (a PEM file), then re-run." + ); +} + +// Renders a network error for humans. +// +// Node's fetch throws a bare `TypeError: fetch failed` and hides the real reason +// (DNS, TLS, proxy) on `err.cause` — so unwrap it. For certificate failures, +// append the remedy: Node only volunteers its own --use-system-ca hint for +// DEPTH_ZERO_SELF_SIGNED_CERT / UNABLE_TO_VERIFY_LEAF_SIGNATURE / +// UNABLE_TO_GET_ISSUER_CERT, which pointedly excludes SELF_SIGNED_CERT_IN_CHAIN +// — the corporate-proxy case this exists for. Those users get a dead-end string +// unless we say something. +export function describeNetworkError(err: unknown): string { + if (!(err instanceof Error)) return String(err); + const cause: unknown = err.cause; + const causeMsg = + cause instanceof Error + ? cause.message + : cause !== undefined + ? String(cause) + : ""; + const base = causeMsg ? `${err.message} (${causeMsg})` : err.message; + const code = + cause instanceof Error ? (cause as NodeJS.ErrnoException).code : undefined; + if (code && CERT_ERROR_CODES.has(code)) return `${base}\n${certHint()}`; + return base; +} diff --git a/tests/lib/tls.test.ts b/tests/lib/tls.test.ts new file mode 100644 index 0000000..e741b07 --- /dev/null +++ b/tests/lib/tls.test.ts @@ -0,0 +1,140 @@ +import { afterEach, describe, expect, test, vi } from "vitest"; +import { + describeNetworkError, + ensureSystemCaCerts, + resetSystemCaCertsCache, + tlsApi, +} from "@/lib/tls.js"; + +afterEach(() => { + resetSystemCaCertsCache(); + vi.restoreAllMocks(); +}); + +// Builds the error Node's fetch actually throws: a bare `fetch failed` with the +// real reason hidden on `cause`. +function fetchError(code: string, message: string): Error { + const err = new TypeError("fetch failed"); + const cause: NodeJS.ErrnoException = new Error(message); + cause.code = code; + err.cause = cause; + return err; +} + +describe("ensureSystemCaCerts", () => { + test("merges the OS store into the default set, keeping the defaults", () => { + const set = vi + .spyOn(tlsApi, "setDefaultCACertificates") + .mockImplementation(() => {}); + vi.spyOn(tlsApi, "getCACertificates").mockImplementation((type) => + type === "system" ? ["sys-a", "sys-b"] : ["def-a"], + ); + + const result = ensureSystemCaCerts(); + + expect(result.status).toBe("merged"); + expect(result.systemCount).toBe(2); + // "default" already folds in NODE_EXTRA_CA_CERTS — dropping it would break + // users who fixed their proxy the documented way. + expect(set).toHaveBeenCalledWith(["def-a", "sys-a", "sys-b"]); + }); + + test("runs once per process and caches the result", () => { + const get = vi + .spyOn(tlsApi, "getCACertificates") + .mockImplementation((type) => (type === "system" ? ["sys"] : ["def"])); + vi.spyOn(tlsApi, "setDefaultCACertificates").mockImplementation(() => {}); + + const first = ensureSystemCaCerts(); + const second = ensureSystemCaCerts(); + + expect(second).toBe(first); + // 2 = one "system" + one "default" from the single real run. + expect(get).toHaveBeenCalledTimes(2); + }); + + test("reports unsupported on a Node without the CA APIs, without touching TLS", () => { + vi.spyOn(tlsApi, "supported").mockReturnValue(false); + const set = vi.spyOn(tlsApi, "setDefaultCACertificates"); + + expect(ensureSystemCaCerts().status).toBe("unsupported"); + expect(set).not.toHaveBeenCalled(); + }); + + test("leaves the default set alone when the OS store is empty", () => { + vi.spyOn(tlsApi, "getCACertificates").mockReturnValue([]); + const set = vi.spyOn(tlsApi, "setDefaultCACertificates"); + + expect(ensureSystemCaCerts().status).toBe("empty"); + // Merging an empty system store would replace the defaults with themselves + // for no reason; skipping keeps Node's behavior byte-identical. + expect(set).not.toHaveBeenCalled(); + }); + + test("swallows a throwing OS store read rather than breaking the command", () => { + vi.spyOn(tlsApi, "getCACertificates").mockImplementation(() => { + throw new Error("store unreadable"); + }); + + const result = ensureSystemCaCerts(); + + expect(result.status).toBe("failed"); + expect(result.error).toBe("store unreadable"); + }); +}); + +describe("describeNetworkError", () => { + test("unwraps Node's bare `fetch failed` to the real reason", () => { + const msg = describeNetworkError( + fetchError("ENOTFOUND", "getaddrinfo ENOTFOUND netmind.viettel.vn"), + ); + expect(msg).toContain("fetch failed"); + expect(msg).toContain("getaddrinfo ENOTFOUND netmind.viettel.vn"); + }); + + test("appends a remedy to the corporate-proxy cert error", () => { + // The exact error users report. Node itself does NOT hint for this code — + // its --use-system-ca suggestion covers only DEPTH_ZERO/UNABLE_TO_VERIFY_ + // LEAF/UNABLE_TO_GET_ISSUER — so without us it's a dead end. + vi.spyOn(tlsApi, "getCACertificates").mockImplementation((type) => + type === "system" ? ["sys"] : ["def"], + ); + vi.spyOn(tlsApi, "setDefaultCACertificates").mockImplementation(() => {}); + + const msg = describeNetworkError( + fetchError( + "SELF_SIGNED_CERT_IN_CHAIN", + "self-signed certificate in certificate chain", + ), + ); + + expect(msg).toContain("self-signed certificate in certificate chain"); + expect(msg).toContain("NODE_EXTRA_CA_CERTS"); + }); + + test("tells old-Node users to upgrade instead of pointing at the OS store", () => { + vi.spyOn(tlsApi, "supported").mockReturnValue(false); + + const msg = describeNetworkError( + fetchError( + "SELF_SIGNED_CERT_IN_CHAIN", + "self-signed certificate in certificate chain", + ), + ); + + expect(msg).toContain("Node 22.15+"); + expect(msg).toContain("NODE_EXTRA_CA_CERTS"); + }); + + test("does not blame certificates for a plain connection failure", () => { + const msg = describeNetworkError( + fetchError("ECONNREFUSED", "connect ECONNREFUSED 10.0.0.1:443"), + ); + expect(msg).not.toContain("NODE_EXTRA_CA_CERTS"); + }); + + test("handles an error with no cause, and a non-Error throw", () => { + expect(describeNetworkError(new Error("boom"))).toBe("boom"); + expect(describeNetworkError("just a string")).toBe("just a string"); + }); +}); From ca29d2ef3ed8de02f02524af4589931ef80457f0 Mon Sep 17 00:00:00 2001 From: minhn4 Date: Fri, 17 Jul 2026 15:11:28 +0700 Subject: [PATCH 2/2] fix: merge the OS CA store only after a cert failure, not up front MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The eager merge regressed Windows CI. tls.getCACertificates("system") is synchronous and costs ~20ms on macOS but ~300ms+ on Windows, where it blocks the event loop — and loggedFetch ran it before the first request of every command, including in tests. Windows per-file durations vs the same runner on #204: backend.test.ts 172ms -> 511ms (+197%) TaskList.test.tsx 7,644ms -> 19,541ms (+156%) ConfigApp.test.tsx 10,091ms -> 17,240ms (+71%) InstallApp.test.tsx 57,903ms -> 87,657ms (+51%) Everything slowed, including TaskList, which never fetches — the stalls starved Ink's render timers, and the three tests with the tightest budgets failed. Not a test-only problem: every networked command on Windows would have eaten the same stall. Invert it. Run the request first; only when it fails with a cert error do we merge and retry once (fetchTrustingSystemCa). A cert error is a precise signal that the user is one of the affected minority, so the happy path now costs exactly zero and only affected users pay. applySystemCaCertsOnce returns null once it has run, bounding this at one retry per process — a chain that stays untrusted surfaces its error instead of looping. Replay is safe: a TLS handshake fails before any body is sent, and every call site passes a replayable body (string/URLSearchParams/FormData/Buffer). certHint is now pure for the same reason: reading the store to word a sentence would put the stall back on the error path, and the retry already read it. Tests pin both halves — a successful request never touches the OS store, and a cert failure merges, retries once, then gives up. Re-verified end-to-end: with the root in the OS store login still succeeds (HTTP 200); without it the error still carries the remedy; the store is read only on the failure path. Co-Authored-By: Claude Opus 4.8 (1M context) --- AGENTS.md | 7 ++-- src/lib/log.ts | 53 +++++++++++++++++---------- src/lib/tls.ts | 52 +++++++++++++++++---------- tests/lib/log.test.ts | 83 +++++++++++++++++++++++++++++++++++++++++++ tests/lib/tls.test.ts | 61 +++++++++++++++++++++---------- 5 files changed, 196 insertions(+), 60 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8487ef1..4ca1949 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -138,14 +138,15 @@ Supabase coordinates (`supabase_url`, `supabase_anon_key`) and the public gatewa Node verifies TLS against its **own bundled Mozilla CA snapshot and never consults the OS trust store** ([tls.rootCertificates](https://nodejs.org/api/tls.html#tlsrootcertificates): "fixed at release time… identical on all supported platforms"). Users behind a TLS-intercepting proxy (Zscaler/Netskope/Fortinet) or HTTPS-scanning AV get every chain re-signed by a corporate root that MDM/GPO installed into the *OS* store — so their browser works and we fail with `fetch failed (self-signed certificate in certificate chain)`. -`src/lib/tls.ts#ensureSystemCaCerts` fixes this with no user configuration: `tls.getCACertificates("system")` reads the OS store **even without the `--use-system-ca` flag**, so we merge it into the default set. Details that are load-bearing: +`src/lib/tls.ts#applySystemCaCertsOnce` fixes this with no user configuration: `tls.getCACertificates("system")` reads the OS store **even without the `--use-system-ca` flag**, so we merge it into the default set. Details that are load-bearing: +- **Merge on failure, never speculatively.** `loggedFetch` runs the request; only if it fails with a cert error does it merge and retry once (`fetchTrustingSystemCa`). The OS-store read is **synchronous** and costs ~20ms on macOS but ~300ms+ on Windows, where it blocks the event loop — an earlier revision merged before the first request and slowed the whole Windows suite by 50–200%, stalling Ink's render timers badly enough to fail three timing-sensitive tests (including `TaskList`, which never fetches). A cert error is a precise signal that the user is one of the affected minority, so paying only then keeps the happy path at exactly zero cost. `tests/lib/log.test.ts` pins that a successful request never touches the store. +- **At most one retry per process.** `applySystemCaCertsOnce` returns null once it has run, so a chain that stays untrusted surfaces its error instead of looping. Replay is safe: a TLS handshake fails before any body is sent, and every call site passes a replayable body (string/URLSearchParams/FormData/Buffer), never a stream — keep it that way. - **Merge `default` + `system`, never `bundled` + `system`.** `"default"` already folds in `NODE_EXTRA_CA_CERTS`; narrowing it would silently drop the certs of users who fixed this the documented way. -- **Called from `loggedFetch`, not at startup.** It costs ~25ms (a native OS-store read), and commands that never open a socket shouldn't pay it. Every network path goes through `loggedFetch` — 16 call sites, zero raw `fetch` — so first-request is both the cheapest and the completest hook. Memoized per process; `tests/lib/tls.test.ts` pins that. - **Every failure mode is a no-op**, including an empty system store (`setDefaultCACertificates` is then never called, keeping Node's behavior byte-identical). Trust config isn't ours to have opinions about, and a bad merge would break users whose certs already work. - The APIs need Node ≥22.19/24.5 (our floor is 22.5), hence `tlsApi.supported()`. They're accessed off the default import, never destructured — a named ESM import of a builtin export that doesn't exist is a link-time error on older Node. -`describeNetworkError` unwraps Node's bare `fetch failed` (the real reason hides on `err.cause`) and appends a remedy for cert codes. It exists because **Node's own `--use-system-ca` hint pointedly excludes `SELF_SIGNED_CERT_IN_CHAIN`** — `crypto_common.cc` gates it on `DEPTH_ZERO_SELF_SIGNED_CERT` / `UNABLE_TO_VERIFY_LEAF_SIGNATURE` / `UNABLE_TO_GET_ISSUER_CERT` only, so the corporate-proxy case, the one the hint most exists for, is the one that prints nothing. If a cert error survives the merge, the root isn't in the OS store either, so the hint names `NODE_EXTRA_CA_CERTS` rather than `--use-system-ca`, which would be a dead end. +`describeNetworkError` unwraps Node's bare `fetch failed` (the real reason hides on `err.cause`) and appends a remedy for cert codes. It exists because **Node's own `--use-system-ca` hint pointedly excludes `SELF_SIGNED_CERT_IN_CHAIN`** — `crypto_common.cc` gates it on `DEPTH_ZERO_SELF_SIGNED_CERT` / `UNABLE_TO_VERIFY_LEAF_SIGNATURE` / `UNABLE_TO_GET_ISSUER_CERT` only, so the corporate-proxy case, the one the hint most exists for, is the one that prints nothing. If a cert error survives the merge, the root isn't in the OS store either, so the hint names `NODE_EXTRA_CA_CERTS` rather than `--use-system-ca`, which would be a dead end. Keep `certHint` **pure** — reading the OS store to word a sentence would put that same Windows stall on the error path, and the retry has already read it. Note the blast radius: this only covers **our own** process. `npm i -g` during install and the agents themselves are separate processes behind the same proxy and need `NODE_EXTRA_CA_CERTS` / npm's `cafile` of their own. diff --git a/src/lib/log.ts b/src/lib/log.ts index e614d39..0f10b8c 100644 --- a/src/lib/log.ts +++ b/src/lib/log.ts @@ -9,10 +9,40 @@ import { import { join } from "node:path"; import { VERSION } from "@/lib/const.js"; import { cliLogsDir } from "@/lib/paths.js"; -import { ensureSystemCaCerts } from "@/lib/tls.js"; +import { applySystemCaCertsOnce, isCertError } from "@/lib/tls.js"; -// The CA merge is logged once per process, on the first request that triggers it. -let caMergeLogged = false; +// Runs a request, and if it fails because the certificate chain isn't trusted, +// merges the OS trust store into Node's defaults and tries once more. +// +// Node ignores the OS store, so a user behind a TLS-intercepting proxy fails +// every request while their browser works (see lib/tls.ts). Recovering *here*, +// on the failure, rather than merging up-front, is what keeps the cost off the +// happy path: the OS-store read is synchronous and blocks the event loop for +// ~300ms on Windows, which is enough to stall Ink's render timers. +// +// At most one retry per process: applySystemCaCertsOnce returns null once it has +// run, so a chain that stays untrusted surfaces its error instead of looping. +// Safe to replay — a TLS handshake fails before any body is sent, and every +// call site passes a replayable body (string/URLSearchParams/FormData/Buffer), +// never a stream. +async function fetchTrustingSystemCa( + input: string | URL, + init: RequestInit | undefined, + endpoint: string, +): Promise { + try { + return await fetch(input, init); + } catch (err) { + if (!isCertError(err)) throw err; + const ca = applySystemCaCertsOnce(); + if (ca?.status !== "merged") throw err; + logInfo("certificate chain untrusted; retrying with the OS CA store", { + action: "http.request", + extra: { endpoint, ca_system_count: ca.systemCount }, + }); + return await fetch(input, init); + } +} // CoDev's local diagnostic log: one Elastic-Common-Schema NDJSON document per // line, written to ~/.codev-hub/logs/codev-YYYYMMDD.ndjson (UTC date). The files @@ -271,21 +301,6 @@ export async function loggedFetch( ): Promise { const url = String(input); const method = init?.method ?? "GET"; - // Trust the OS certificate store before the first socket opens, so users - // behind a TLS-intercepting proxy don't hit `self-signed certificate in - // certificate chain`. Memoized, so only the first request pays for it. - const ca = ensureSystemCaCerts(); - if (!caMergeLogged) { - caMergeLogged = true; - logDebug(`system CA store: ${ca.status}`, { - action: "http.request", - extra: { - ca_status: ca.status, - ca_system_count: ca.systemCount, - ca_error: ca.error ?? null, - }, - }); - } logDebug(`http ${method} ${endpoint}`, { action: "http.request", eventType: "start", @@ -295,7 +310,7 @@ export async function loggedFetch( }); const startedAt = Date.now(); try { - const res = await fetch(input, init); + const res = await fetchTrustingSystemCa(input, init, endpoint); const durationMs = Date.now() - startedAt; if (res.ok) { logDebug(`http ${method} ${endpoint} → ${res.status}`, { diff --git a/src/lib/tls.ts b/src/lib/tls.ts index a815815..9a05387 100644 --- a/src/lib/tls.ts +++ b/src/lib/tls.ts @@ -46,23 +46,28 @@ export interface CaMergeResult { error?: string; } -let cached: CaMergeResult | null = null; +let attempted = false; -// Merges the OS trust store into Node's default CA set, once per process. +// Merges the OS trust store into Node's default CA set. Runs at most once per +// process; every later call returns null so a caller can't retry forever. // -// Called from `loggedFetch` rather than at startup: it costs ~25ms (a native -// read of the OS store), and commands that never open a socket — help, version, -// logs, restore — shouldn't pay for it. Every network path in the CLI goes -// through `loggedFetch`, so first-request is both the cheapest and the -// completest hook. +// Deliberately called only *after* a certificate failure, never speculatively. +// The OS-store read is synchronous and costs ~20ms on macOS but ~300ms+ on +// Windows, where it blocks the event loop — enough to stall Ink's render timers. +// Since the users who need this are the minority behind an intercepting proxy, +// and a cert failure is a precise signal that they're one of them, paying on +// failure keeps the happy path at exactly zero cost. +// +// Returns null when a previous call already attempted the merge — the caller +// must not retry the request again, or a permanently untrusted chain would loop. // // Best-effort by construction: TLS trust is not this CLI's job to have opinions // about, and any failure here must leave Node's default behavior exactly as it // was rather than break a user whose certs already work. -export function ensureSystemCaCerts(): CaMergeResult { - if (cached) return cached; - cached = mergeSystemCaCerts(); - return cached; +export function applySystemCaCertsOnce(): CaMergeResult | null { + if (attempted) return null; + attempted = true; + return mergeSystemCaCerts(); } function mergeSystemCaCerts(): CaMergeResult { @@ -86,9 +91,9 @@ function mergeSystemCaCerts(): CaMergeResult { } } -// Test-only: drop the memoized result so each case re-runs the merge. +// Test-only: forget that the merge ran so each case starts clean. export function resetSystemCaCertsCache(): void { - cached = null; + attempted = false; } // OpenSSL verify failures that mean "I don't trust this chain", as opposed to @@ -103,9 +108,21 @@ const CERT_ERROR_CODES = new Set([ "SELF_SIGNED_CERT_IN_CHAIN_ERR", ]); +// True when a thrown fetch error bottoms out in "I don't trust this chain". +// Node's fetch hides the reason on `err.cause`, so unwrap before matching. +export function isCertError(err: unknown): boolean { + if (!(err instanceof Error)) return false; + const cause = err.cause; + if (!(cause instanceof Error)) return false; + const code = (cause as NodeJS.ErrnoException).code; + return code !== undefined && CERT_ERROR_CODES.has(code); +} + +// Pure: reading the OS store here would put a ~300ms Windows stall on the error +// path just to word a sentence. By the time this runs, loggedFetch has already +// tried the merge and retried, so "not in your store either" is accurate. function certHint(): string { - const ca = ensureSystemCaCerts(); - if (ca.status === "unsupported") { + if (!tlsApi.supported()) { return ( `This looks like a TLS-intercepting proxy or antivirus. Node ${process.version} ` + "can't read your system certificate store — upgrade to Node 22.15+ and re-run, " + @@ -141,8 +158,5 @@ export function describeNetworkError(err: unknown): string { ? String(cause) : ""; const base = causeMsg ? `${err.message} (${causeMsg})` : err.message; - const code = - cause instanceof Error ? (cause as NodeJS.ErrnoException).code : undefined; - if (code && CERT_ERROR_CODES.has(code)) return `${base}\n${certHint()}`; - return base; + return isCertError(err) ? `${base}\n${certHint()}` : base; } diff --git a/tests/lib/log.test.ts b/tests/lib/log.test.ts index 86cd736..ce11733 100644 --- a/tests/lib/log.test.ts +++ b/tests/lib/log.test.ts @@ -27,6 +27,7 @@ import { } from "@/lib/log.js"; import { execAsync } from "@/lib/npm.js"; import { runAgent, spawner as runSpawner } from "@/lib/run.js"; +import { resetSystemCaCertsCache, tlsApi } from "@/lib/tls.js"; let tempDir: string; let logDir: string; @@ -336,6 +337,88 @@ describe("pruneLogs", () => { }); }); +describe("loggedFetch system-CA recovery", () => { + function certFailure(): Error { + const err = new TypeError("fetch failed"); + const cause: NodeJS.ErrnoException = new Error( + "self-signed certificate in certificate chain", + ); + cause.code = "SELF_SIGNED_CERT_IN_CHAIN"; + err.cause = cause; + return err; + } + + // This file's top-level afterEach doesn't restore mocks, so the fetch/tls + // spies below would otherwise leak into the next case's call counts. + afterEach(() => { + resetSystemCaCertsCache(); + vi.restoreAllMocks(); + }); + + // The regression that broke Windows CI: reading the OS store is a synchronous + // ~300ms stall there, so a request that succeeds must never touch it. + test("does not read the OS store on the happy path", async () => { + const get = vi.spyOn(tlsApi, "getCACertificates"); + vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("ok")); + + await loggedFetch("backend.config", "https://x.test/"); + + expect(get).not.toHaveBeenCalled(); + }); + + test("merges the OS store and retries once after a cert failure", async () => { + vi.spyOn(tlsApi, "getCACertificates").mockImplementation((type) => + type === "system" ? ["sys"] : ["def"], + ); + vi.spyOn(tlsApi, "setDefaultCACertificates").mockImplementation(() => {}); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockRejectedValueOnce(certFailure()) + .mockResolvedValueOnce(new Response("ok", { status: 200 })); + + const res = await loggedFetch("sso.token", "https://x.test/"); + + expect(res.status).toBe(200); + expect(fetchSpy).toHaveBeenCalledTimes(2); + }); + + test("gives up after one retry when the chain stays untrusted", async () => { + vi.spyOn(tlsApi, "getCACertificates").mockImplementation((type) => + type === "system" ? ["sys"] : ["def"], + ); + vi.spyOn(tlsApi, "setDefaultCACertificates").mockImplementation(() => {}); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockRejectedValue(certFailure()); + + await expect(loggedFetch("sso.token", "https://x.test/")).rejects.toThrow( + "fetch failed", + ); + expect(fetchSpy).toHaveBeenCalledTimes(2); + + // A second request must not re-read the store or retry again — the merge + // already happened and didn't help. + fetchSpy.mockClear(); + await expect(loggedFetch("sso.token", "https://x.test/")).rejects.toThrow( + "fetch failed", + ); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + test("does not retry a non-certificate failure", async () => { + const err = new TypeError("fetch failed"); + const cause: NodeJS.ErrnoException = new Error("getaddrinfo ENOTFOUND"); + cause.code = "ENOTFOUND"; + err.cause = cause; + const fetchSpy = vi.spyOn(globalThis, "fetch").mockRejectedValue(err); + + await expect(loggedFetch("sso.token", "https://x.test/")).rejects.toThrow( + "fetch failed", + ); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); +}); + describe("loggedFetch", () => { test("passes through and writes start + completion documents", async () => { initLogging("upload", [], { installProcessHooks: false }); diff --git a/tests/lib/tls.test.ts b/tests/lib/tls.test.ts index e741b07..51778ec 100644 --- a/tests/lib/tls.test.ts +++ b/tests/lib/tls.test.ts @@ -1,7 +1,8 @@ import { afterEach, describe, expect, test, vi } from "vitest"; import { + applySystemCaCertsOnce, describeNetworkError, - ensureSystemCaCerts, + isCertError, resetSystemCaCertsCache, tlsApi, } from "@/lib/tls.js"; @@ -21,7 +22,7 @@ function fetchError(code: string, message: string): Error { return err; } -describe("ensureSystemCaCerts", () => { +describe("applySystemCaCertsOnce", () => { test("merges the OS store into the default set, keeping the defaults", () => { const set = vi .spyOn(tlsApi, "setDefaultCACertificates") @@ -30,25 +31,26 @@ describe("ensureSystemCaCerts", () => { type === "system" ? ["sys-a", "sys-b"] : ["def-a"], ); - const result = ensureSystemCaCerts(); + const result = applySystemCaCertsOnce(); - expect(result.status).toBe("merged"); - expect(result.systemCount).toBe(2); + expect(result?.status).toBe("merged"); + expect(result?.systemCount).toBe(2); // "default" already folds in NODE_EXTRA_CA_CERTS — dropping it would break // users who fixed their proxy the documented way. expect(set).toHaveBeenCalledWith(["def-a", "sys-a", "sys-b"]); }); - test("runs once per process and caches the result", () => { + // The null is what bounds loggedFetch's retry: a chain that stays untrusted + // after the merge must surface its error, not re-request forever. + test("runs at most once per process, returning null afterwards", () => { const get = vi .spyOn(tlsApi, "getCACertificates") .mockImplementation((type) => (type === "system" ? ["sys"] : ["def"])); vi.spyOn(tlsApi, "setDefaultCACertificates").mockImplementation(() => {}); - const first = ensureSystemCaCerts(); - const second = ensureSystemCaCerts(); - - expect(second).toBe(first); + expect(applySystemCaCertsOnce()?.status).toBe("merged"); + expect(applySystemCaCertsOnce()).toBeNull(); + expect(applySystemCaCertsOnce()).toBeNull(); // 2 = one "system" + one "default" from the single real run. expect(get).toHaveBeenCalledTimes(2); }); @@ -57,7 +59,7 @@ describe("ensureSystemCaCerts", () => { vi.spyOn(tlsApi, "supported").mockReturnValue(false); const set = vi.spyOn(tlsApi, "setDefaultCACertificates"); - expect(ensureSystemCaCerts().status).toBe("unsupported"); + expect(applySystemCaCertsOnce()?.status).toBe("unsupported"); expect(set).not.toHaveBeenCalled(); }); @@ -65,7 +67,7 @@ describe("ensureSystemCaCerts", () => { vi.spyOn(tlsApi, "getCACertificates").mockReturnValue([]); const set = vi.spyOn(tlsApi, "setDefaultCACertificates"); - expect(ensureSystemCaCerts().status).toBe("empty"); + expect(applySystemCaCertsOnce()?.status).toBe("empty"); // Merging an empty system store would replace the defaults with themselves // for no reason; skipping keeps Node's behavior byte-identical. expect(set).not.toHaveBeenCalled(); @@ -76,10 +78,31 @@ describe("ensureSystemCaCerts", () => { throw new Error("store unreadable"); }); - const result = ensureSystemCaCerts(); + const result = applySystemCaCertsOnce(); - expect(result.status).toBe("failed"); - expect(result.error).toBe("store unreadable"); + expect(result?.status).toBe("failed"); + expect(result?.error).toBe("store unreadable"); + }); +}); + +describe("isCertError", () => { + test("recognizes the corporate-proxy chain error", () => { + expect( + isCertError( + fetchError( + "SELF_SIGNED_CERT_IN_CHAIN", + "self-signed certificate in certificate chain", + ), + ), + ).toBe(true); + }); + + test("does not fire for DNS/connection failures or bare errors", () => { + expect(isCertError(fetchError("ENOTFOUND", "getaddrinfo ENOTFOUND"))).toBe( + false, + ); + expect(isCertError(new Error("fetch failed"))).toBe(false); + expect(isCertError("nope")).toBe(false); }); }); @@ -96,10 +119,7 @@ describe("describeNetworkError", () => { // The exact error users report. Node itself does NOT hint for this code — // its --use-system-ca suggestion covers only DEPTH_ZERO/UNABLE_TO_VERIFY_ // LEAF/UNABLE_TO_GET_ISSUER — so without us it's a dead end. - vi.spyOn(tlsApi, "getCACertificates").mockImplementation((type) => - type === "system" ? ["sys"] : ["def"], - ); - vi.spyOn(tlsApi, "setDefaultCACertificates").mockImplementation(() => {}); + const get = vi.spyOn(tlsApi, "getCACertificates"); const msg = describeNetworkError( fetchError( @@ -110,6 +130,9 @@ describe("describeNetworkError", () => { expect(msg).toContain("self-signed certificate in certificate chain"); expect(msg).toContain("NODE_EXTRA_CA_CERTS"); + // Rendering an error must not touch the OS store: that read is a ~300ms + // event-loop stall on Windows, and it already ran on the retry path. + expect(get).not.toHaveBeenCalled(); }); test("tells old-Node users to upgrade instead of pointing at the OS store", () => {