Skip to content

Commit 8ed796a

Browse files
codeslakeclaude
andcommitted
fix(ca-trust): see a malformed BEGIN line instead of skipping it
Codex review against the real merge base, P1 and the only finding. The marker pattern described a WELL-FORMED opener, so an over-dashed one (`-----BEGIN CERTIFICATE-------`) matched nothing at all and the block became invisible to the guard: nothing was checked, and our CA later in the file carried the verdict. openssl does not skip it — it consumes the line as an opener and then fails the ENTIRE extras load on the END it cannot match. Measured, node v24.11.1: guard=accept, loader=0 CAs, `bad end line`. A trailing `.*` makes the line match, which is all the fix needs: the block is then seen and the existing per-block check rejects it as an undecodable CERTIFICATE. Being SEEN is what a guard needs; skipping is what lets a bad block through. This is the same defect already fixed on the END side, in its mirror position. The lesson: a shape fixed at one marker is a shape to go and check at the other. Two rows: the malformed opener rejects, and a BEGIN wearing one trailing space still ACCEPTS, so the fix cannot drift into the over-strict guard this PR set out to remove. Mutation-checked: reverting to the strict pattern fails the new row. An earlier attempt added a separate pre-scan loop and an `undefined` label branch. The branch was dead — `(?!-----)` still captures `CERTIFICATE` from an over-dashed line — and the mutation SURVIVED, which is what exposed it. Removed rather than kept as defence for a case that cannot happen. Suite 1505/1507, the 2 being the inotify EMFILE that fails identically at the merge base. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent ee31cb3 commit 8ed796a

2 files changed

Lines changed: 26 additions & 1 deletion

File tree

bin/ca-trust.mjs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,20 @@ export function bundleCarriesOurCA(text, ourCaPem) {
7171
// `-` is legal INSIDE a label, so the stop condition is the `-----` run, not
7272
// the first hyphen: `[^-]*` failed to match `X-FOO` at all, which is the same
7373
// blind spot in a new place.
74-
const marker = /^-----BEGIN ((?:(?!-----).)*)-----[ \t]*$/gm;
74+
// The trailing `.*` is load-bearing: it makes a malformed opener VISIBLE so
75+
// the existing per-block checks can reject it. Without it the pattern
76+
// described only a well-formed marker, so `-----BEGIN CERTIFICATE-------`
77+
// matched nothing at all, the block was skipped, and our CA later in the file
78+
// carried the verdict. openssl does not skip it — it consumes the line as an
79+
// opener and fails the entire extras load on the END it cannot match.
80+
// Measured, node v24.11.1: guard=accept, loader=0 CAs, `bad end line`; now the
81+
// block is seen and rejected as an undecodable CERTIFICATE.
82+
//
83+
// Exactly the defect already fixed on the END side, in its mirror position.
84+
// The lesson worth keeping: a shape fixed at one marker is a shape to go and
85+
// check at the other. Being SEEN is what a guard needs; skipping is what lets
86+
// a bad block through.
87+
const marker = /^-----BEGIN ((?:(?!-----).)*)-----[ \t]*.*$/gm;
7588
let carriesUs = false;
7689
for (let m; (m = marker.exec(text)); ) {
7790
const label = m[1];

test/proxy-forward-ca.test.mjs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,18 @@ test("ca-trust: the bundle guard never accepts a bundle NODE_EXTRA_CA_CERTS cann
434434
// of these were measured as false accepts on an otherwise healthy bundle.
435435
["END marker with trailing garbage", ours.replace("-----END CERTIFICATE-----", "-----END CERTIFICATE-----garbage"), false],
436436
["END marker with extra dashes", ours.replace("-----END CERTIFICATE-----", "-----END CERTIFICATE-------"), false],
437+
// The SAME defect on the BEGIN side, which the END fix above did not cover.
438+
// `$`-anchoring the marker made an over-dashed opener invisible to us — the
439+
// block was skipped entirely, so nothing was ever checked and our CA later
440+
// in the file carried the verdict. openssl does NOT skip it: it consumes
441+
// the line as an opener and then fails the whole extras load on the END it
442+
// cannot match. Measured, node v24.11.1: guard=accept, loader=0 CAs,
443+
// `bad end line`. A block we cannot parse must never be one we ignore.
444+
["BEGIN marker with extra dashes", `-----BEGIN CERTIFICATE-------\nZm9vYmFy\n-----END CERTIFICATE-----\n${ours}`, false],
445+
// ...and the trailing-whitespace tolerance on BEGIN must survive the fix,
446+
// for the same reason the END side keeps it: openssl accepts it and so
447+
// must we, or we drop a healthy bundle.
448+
["BEGIN marker with a trailing space", ours.replace("-----BEGIN CERTIFICATE-----", "-----BEGIN CERTIFICATE----- "), true],
437449
// ...but trailing whitespace is fine, and must stay fine: node loads it.
438450
["END marker with a trailing space", ours.replace("-----END CERTIFICATE-----", "-----END CERTIFICATE----- "), true],
439451
// Real cert, just not ours: the stale-builder case.

0 commit comments

Comments
 (0)