Skip to content

Commit e28abd0

Browse files
codeslakeclaude
andcommitted
fix(guard): require the END marker to end its own line
A review pass died mid-response, but its last line named the gap: the guard never validated what followed the END marker. Measured, and it was two more false accepts. `indexOf("\n-----END <label>-----")` matches a prefix, so it treated `-----END CERTIFICATE-----garbage` and `-----END CERTIFICATE-------` as terminators. Both make openssl reject the block: guard=accept while node loaded zero extra CAs, on a bundle whose remaining entries were healthy. Only whitespace may follow — 13/13 agreement with a real handshake on what a tail may contain (space, tab, nothing: loads; any other character, including a further dash run: does not). The END search now skips candidates whose line does not end there, rather than taking the first textual match. Three rows added, including the positive one: a trailing space must keep being ACCEPTED, or the fix trades two false accepts for a false reject. Mutation-verified — reverting to the bare indexOf fails exactly one test. Re-measured across all four sweeps at 164 shapes: 0 false accepts, no regression in either direction. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 2c0ea06 commit e28abd0

2 files changed

Lines changed: 20 additions & 1 deletion

File tree

bin/ca-trust.mjs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,21 @@ export function bundleCarriesOurCA(text, ourCaPem) {
7676
// Bounded by the NEXT marker, not by a search to end-of-file. An unbounded
7777
// indexOf lets a torn block borrow the END line of a later one, so the
7878
// unterminated check never fires and the slice spans two entries.
79+
// The END marker must also END ITS LINE, bar trailing whitespace. indexOf
80+
// alone ignored whatever followed it, so `-----END CERTIFICATE-----garbage`
81+
// and `-----END CERTIFICATE-------` both read as terminators here while
82+
// openssl rejected the block and node loaded zero CAs — measured, both as
83+
// false accepts on an otherwise healthy bundle. Whitespace is fine (13/13
84+
// agreement with a real handshake on what may follow), anything else is not.
7985
const endMarker = `\n-----END ${label}-----`;
8086
const nextBegin = text.indexOf("\n-----BEGIN ", m.index + 1);
81-
let end = text.indexOf(endMarker, m.index);
87+
let end = -1;
88+
for (let at = text.indexOf(endMarker, m.index); at !== -1;
89+
at = text.indexOf(endMarker, at + 1)) {
90+
const lineEnd = text.indexOf("\n", at + 1);
91+
const tail = text.slice(at + endMarker.length, lineEnd === -1 ? undefined : lineEnd);
92+
if (/^[ \t\r]*$/.test(tail)) { end = at; break; }
93+
}
8294
if (end !== -1 && nextBegin !== -1 && end > nextBegin) end = -1;
8395
// Unterminated, or closed by a different label. Fatal whatever the label
8496
// is, and deliberately not analyzed further: openssl's base64 decoder

test/proxy-forward-ca.test.mjs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,13 @@ test("ca-trust: the bundle guard never accepts a bundle NODE_EXTRA_CA_CERTS cann
403403
// to end-of-file let the torn block borrow it, so the unterminated check
404404
// never fired and the slice spanned two entries.
405405
["torn block borrowing a later END", `-----BEGIN CERTIFICATE-----\n${body}\n${other}${ours}`, false],
406+
// The END marker must end its own line. `indexOf` alone ignored whatever
407+
// followed it, so a block openssl rejects read as terminated here — both
408+
// of these were measured as false accepts on an otherwise healthy bundle.
409+
["END marker with trailing garbage", ours.replace("-----END CERTIFICATE-----", "-----END CERTIFICATE-----garbage"), false],
410+
["END marker with extra dashes", ours.replace("-----END CERTIFICATE-----", "-----END CERTIFICATE-------"), false],
411+
// ...but trailing whitespace is fine, and must stay fine: node loads it.
412+
["END marker with a trailing space", ours.replace("-----END CERTIFICATE-----", "-----END CERTIFICATE----- "), true],
406413
// Real cert, just not ours: the stale-builder case.
407414
["stale: a real cert that is not ours", other, false],
408415
["empty bundle", "", false],

0 commit comments

Comments
 (0)