Skip to content

Commit ee31cb3

Browse files
codeslakeclaude
andcommitted
fix(ca-trust): reject the whitespace node rejects, and never publish an unparseable CA
Two false-accept paths found by Codex review, both reproduced here before being agreed with. STRIP ASCII WHITESPACE ONLY. isBase64Body stripped with /\s+/, which is the Unicode whitespace set. Node's PEM reader accepts space, tab, CR and LF and nothing else. Measured one character at a time against a real NODE_EXTRA_CA_CERTS load: those four load 1, while U+00A0 U+2003 U+2028 U+2029 U+FEFF U+1680 U+205F U+3000 and ASCII VTAB and FORMFEED each load 0 with `bad base64 decode`. All ten are stripped by \s, so a body damaged by any of them read as clean and the guard accepted a bundle that costs the session every extra root. A NBSP is what a paste through a rich-text field leaves behind. PARSE BEFORE PUBLISHING. The copy into ca-trust.d/ccf.pem happened before the X509 parse, so a corrupt ca.pem was handed to every OTHER component. Our own session degrades fine (it falls back to node's built-in store), but the builder concatenates sort(*.pem) and "ccf" sorts first — the same fatal leading position the torn-write guard already protects, reached by a different cause. Atomicity guarantees whole bytes, never loadable ones. Now the parse throws into the existing catch, which warns and leaves any previous good ccf.pem for siblings to keep trusting. Both TDD: each test fails on the pre-fix code and passes after. Both mutation-checked: reverting [ \t\r\n] to \s fails proxy-forward-ca, removing the pre-publish parse fails proxy-wrapper. Suite 1505/1507. The 2 failures are the inotify EMFILE (max_user_instances=128 on this host) and fail identically at the merge base. Four of the six review findings were against upstream code outside this PR's diff — session-budget-breaker and tier-advisor — and are not touched here. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 21ba6c2 commit ee31cb3

4 files changed

Lines changed: 78 additions & 1 deletion

File tree

bin/ca-trust.mjs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,14 @@ import { X509Certificate } from "node:crypto";
1515
// A body containing `-` reads as damaged here even though node accepts it
1616
// (openssl stops at the dash), which is the conservative direction and the only
1717
// measured disagreement.
18+
//
19+
// Whitespace is stripped as `[ \t\r\n]`, NOT `\s`: node's reader accepts those
20+
// four, and rejects every other character `\s` covers. Measured one at a time —
21+
// U+00A0 U+2003 U+2028 U+2029 U+FEFF U+1680 U+205F U+3000 and ASCII VTAB and
22+
// FORMFEED each load 0 with `bad base64 decode`. Stripping them read a damaged
23+
// body as clean, and a NBSP is what a paste through a rich-text field leaves.
1824
function isBase64Body(body) {
19-
const b = body.replace(/\s+/g, "");
25+
const b = body.replace(/[ \t\r\n]+/g, "");
2026
return b.length > 0 && b.length % 4 === 0 && /^[A-Za-z0-9+/]+={0,2}$/.test(b);
2127
}
2228

bin/claude-via-proxy.mjs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,15 @@ if (remoteControl) {
242242
try {
243243
mkdirSync(caTrustDir, { recursive: true });
244244
const ours = readFileSync(caPem);
245+
// Parse BEFORE publishing. Validating later (we do, at the own-CA step
246+
// below) still handed a corrupt ca.pem to every OTHER component: "ccf" sorts
247+
// first in the builder's sort(*.pem), so a bad entry lands in the same fatal
248+
// leading position the torn-write guard above protects — measured, such a
249+
// bundle loads 0 extra CAs and warns `bad base64 decode`. Atomicity
250+
// guarantees whole bytes, never loadable ones. Throwing lands in the catch
251+
// below, leaving any previous good ccf.pem in place for siblings to keep
252+
// trusting.
253+
new X509Certificate(ours);
245254
const dst = join(caTrustDir, "ccf.pem");
246255
// Byte-compare skip so a bundle builder keying on mtime is not woken by a
247256
// launch that changed nothing.

test/proxy-forward-ca.test.mjs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,25 @@ test("ca-trust: the bundle guard never accepts a bundle NODE_EXTRA_CA_CERTS cann
399399
// Padding is positional, not merely present: `AAA=` loads, `A===` does not.
400400
["PUBLIC KEY body with misplaced padding", `-----BEGIN PUBLIC KEY-----\nA===\n-----END PUBLIC KEY-----\n${ours}`, false],
401401
["corrupt X509 CRL ahead", `-----BEGIN X509 CRL-----\n!!!not base64!!!\n-----END X509 CRL-----\n${ours}`, false],
402+
// Whitespace inside a body is stripped before the alphabet test, and WHICH
403+
// whitespace decides the verdict. JavaScript's `\s` covers the Unicode set;
404+
// node's PEM reader accepts only ASCII space, tab, CR and LF. Measured, one
405+
// character at a time, against a real NODE_EXTRA_CA_CERTS load:
406+
//
407+
// space, tab, CR, LF -> loads 1
408+
// U+00A0 U+2003 U+2028 U+2029 U+FEFF U+1680 U+205F -> loads 0
409+
// U+3000, and ASCII VTAB (\x0b) and FORMFEED (\x0c) -> loads 0
410+
//
411+
// Every one of those ten is stripped by `\s`, so stripping with `\s` makes
412+
// the guard read a damaged body as clean. A NBSP is what a paste through a
413+
// rich-text field leaves behind, which is exactly how a bundle acquires one.
414+
["PUBLIC KEY body with U+00A0", `-----BEGIN PUBLIC KEY-----\nMFkw EwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE\n-----END PUBLIC KEY-----\n${ours}`, false],
415+
["PUBLIC KEY body with a vertical tab", `-----BEGIN PUBLIC KEY-----\nMFkw EwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE\n-----END PUBLIC KEY-----\n${ours}`, false],
416+
["our CA with U+2028 in its body", ours.replace(/\n/, "\n
"), false],
417+
// ...and the other direction, or the fix above becomes an over-strict guard
418+
// that drops the sibling CAs this PR exists to keep. A tab and a bare CR
419+
// inside a body are both loader-legal.
420+
["PUBLIC KEY body with a tab", `-----BEGIN PUBLIC KEY-----\nMFkw\tEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE\n-----END PUBLIC KEY-----\n${ours}`, true],
402421
// ...but a WELL-FORMED one must still be accepted, or the fix above turns
403422
// into the over-strict guard this PR set out to remove.
404423
["valid PUBLIC KEY ahead", `${pubKeyPem(ourCa)}${ours}`, true],

test/proxy-wrapper.test.mjs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,49 @@ describe("launch wrapper (claude-via-proxy)", { concurrency: 1 }, () => {
598598
`a healthy bundle must not be blamed for our own CA failing to parse; stderr: ${err}`);
599599
});
600600

601+
it("--remote-control does not publish a ca.pem that failed to parse", async () => {
602+
// Blaming the right file and not consuming it (the two tests above) covered
603+
// what THIS session does with a corrupt ca.pem. It still published those
604+
// bytes: the copy into ca-trust.d/ccf.pem happens before the X509 parse, so
605+
// an unparseable CA was handed to every OTHER component on the machine.
606+
//
607+
// That is the worse half. Our own session degrades to node's built-in store
608+
// and keeps working; the bundle builder concatenates sort(*.pem) and "ccf"
609+
// sorts first, so a corrupt entry in the leading position aborts node's
610+
// whole extras load for every sibling — measured on this box, a fused
611+
// bundle loads 0 extra CAs and warns `bad base64 decode`. One broken file
612+
// here costs every other component its CA, and its corporate roots with it.
613+
//
614+
// Publishing nothing is the honest state, and it is strictly better than
615+
// publishing garbage: a builder that finds no ccf.pem simply builds a bundle
616+
// without us, which our own guard then rejects (it does not carry our CA)
617+
// and we fall back to our own — exactly the no-builder path that already
618+
// works. Any PREVIOUS good ccf.pem must survive, because it is what siblings
619+
// are currently trusting and a stale-but-valid CA beats none.
620+
const configDir = tempDir("cfftrust-");
621+
const caDir = tempDir("cffca-");
622+
// Same reuse-guard reasoning as the blame test above: the key must be
623+
// present or the proxy regenerates a healthy ca.pem and this exercises
624+
// nothing.
625+
writeFileSync(join(caDir, "ca.key"), "-----BEGIN PRIVATE KEY-----\nplaceholder\n-----END PRIVATE KEY-----\n");
626+
writeFileSync(join(caDir, "ca.pem"), "-----BEGIN CERTIFICATE-----\ntruncated\n");
627+
// A previously-published, well-formed entry. Whatever we do with the corrupt
628+
// one, this must still be here afterwards.
629+
const trustDir = join(configDir, "ca-trust.d");
630+
mkdirSync(trustDir, { recursive: true });
631+
const priorGood = "-----BEGIN CERTIFICATE-----\ncHJldmlvdXNseS1wdWJsaXNoZWQtb3Vycw==\n-----END CERTIFICATE-----\n";
632+
writeFileSync(join(trustDir, "ccf.pem"), priorGood);
633+
634+
await runWrapper('process.stdout.write("OK\\n")',
635+
{ CLAUDE_CONFIG_DIR: configDir, CACHE_FIX_CA_DIR: caDir });
636+
637+
const published = readFileSync(join(trustDir, "ccf.pem"), "utf8");
638+
assert.doesNotMatch(published, /truncated/,
639+
"the unparseable ca.pem must never reach ca-trust.d — it voids every sibling's CA");
640+
assert.equal(published, priorGood,
641+
"the last known-good published CA must survive a corrupt ca.pem");
642+
});
643+
601644
it("--remote-control does not hand claude a ca.pem that failed to parse", async () => {
602645
// Naming the broken file in the warning was only half the fix. caForClaude
603646
// still defaulted to it, so the session was wired to a PEM we had just

0 commit comments

Comments
 (0)