Own the PEM parser: port node-saml's rewrite, and validate the encapsulated text #602

Summary

lib/utils.ts carries a PEM parser — PEM_FORMAT_REGEX, BASE64_REGEX, normalizePem, derToPem, pemToDer — that is a copy of the one in @node-saml/node-saml's src/crypto.ts, down to the doc comments. The two have drifted, and this copy is the weaker one: its encapsulated-text pattern is ([^-]*), so a PEM message's body is not validated at all.

node-saml has just rebuilt its copy over ~20 commits in node-saml/node-saml#409. The right end state is that the parser lives here and node-saml keeps a thin wrapper that only adds option-named errors. This issue is the port.

It also fills a gap this repo has already committed to. #551 deprecates PEM_FORMAT_REGEX, EXTRACT_X509_CERTS and BASE64_REGEX for removal in 7.0, each documented as "an internal parsing detail with no replacement." This work is the replacement: behavior instead of exported regexes.

This is not breaking, so it can go in 6.x

Two checks, both run against xml-crypto@6.2.0:

Everything a stricter parser would newly reject is already unusable. derToPem accepts it and hands back a PEM that OpenSSL then refuses:

No working code path loses anything. The throw moves from OpenSSL to the parser and gains a message that names the problem.

Nothing valid changes. Multiline PEM, singleline PEM, public key PEM, CRLF PEM, bare base64 and bare multiline base64 all produce byte-identical output from derToPem and from node-saml's keyInfoToPem. normalizePem is unchanged by this work, so that holds by construction.

The rest is purely additive: trailing blanks, BOM, arbitrary line wrapping and concatenated certificate bundles are all rejected today and would start being accepted.

What to implement

Port the parser from node-saml/src/crypto.ts as of #409. The shape that matters:

const PEM_FORMAT_REGEX =
  /^(?:-----BEGIN [A-Z\x20]{1,48}-----\n+(?:[A-Za-z0-9+/=]+\n)+-----END [A-Z\x20]{1,48}-----\n*)+$/;
const PEM_BODY_REGEX =
  /-----BEGIN [A-Z\x20]{1,48}-----\n+((?:[A-Za-z0-9+/=]+\n)+)-----END [A-Z\x20]{1,48}-----/g;
const BASE64_LINES_REGEX = /^(?:[A-Za-z0-9+/=]+\n)*[A-Za-z0-9+/=]+$/;
const BASE64_REGEX = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/;

Pipeline, in order: decode (latin1 for a Buffer) → strip BOM → normalize eol to \n → strip blanks at line ends → trim() → structural match → per-message data check.

New capabilities relative to 6.2.0: blanks at line ends, leading UTF-8 BOM, a blank line after -----BEGIN, arbitrary line width, concatenated messages, and a real base64 check on the body.

Export behavior, not regexes. #551 is removing the regex exports; do not add more. Predicates plus a normalizer (isPemFormat, isBase64, normalizePem) let node-saml compose its own option-named errors without depending on our error strings, which is what the wrapper needs.

Sharp edges

These cost node-saml a dozen review rounds. Each is a real trap, not a style preference.

1. Three regex shapes here are exponential or quadratic. This parser is reachable with attacker-influenced input.

• An eol alternation inside a repeated group — (?:\r\n|\r|\n) — lets CRLF parse two ways, so a value that fails to match backtracks exponentially. Normalize line endings before matching; never match them.
• {1,4} in the base64 group — the obvious way to allow a line break at an arbitrary offset — makes the group ambiguous. ~14x per added character. Keep {4} fixed-width.
• [ \t]+ against an anchor ([ \t]+$, [ \t]+\n) is quadratic in the length of the run: the engine retries from every position inside it. Measured 16s at 80k blanks, minutes at a megabyte. Strip end-of-line blanks with split("\n").map(l => l.trimEnd()).join("\n") — linear, 2ms at 1 MiB.

2. Check patterns with an analyzer, never with a timing test. npx recheck@4 check '<source>' '' — expect safe or linear. A timing assertion measures the CI runner: a slow box fails it, a fast one passes straight through a regression. node-saml added one, removed it, and an agent added it back. Don't.

3. De-lining the data before checking it is load-bearing, not a tidy-up. It is what lets an arbitrary line width be accepted without relaxing {4} to {1,4}. Structure (where lines may end) and data (what the bytes are) must be separate checks or one of them ends up ambiguous.

4. PEM-wrapped and bare base64 must accept exactly the same data, and it breaks in both directions. Before node-saml split structure from data: A=, AAAA=, AAA==, AAAAA and A were accepted with boundaries and rejected without; QUJD\nRE\nVG and QU\nJD were accepted without boundaries and rejected with. Pin this with a table-driven test over both forms — and assert the rejection message, not just that something threw, or two rejections for unrelated reasons will read as agreement.

5. Trailing whitespace is permitted by RFC 7468 Figure 1, not by laxtextualmsg. Figure 1 is plain textualmsg and already has preeb *WSP eol, base64line = 1*base64char *WSP eol, posteb *WSP. Figure 3 is stricttextualmsg. Getting these backwards is how node-saml justified rejecting the most widely-tolerated whitespace there is; §2 says blanks at the ends of lines are ignored by most extant parsers, while blanks at the starts of lines or inside the data are far less compatible. Keep that asymmetry: strip trailing, reject leading and interior.

6. A UTF-8 BOM does not arrive as U+FEFF. Because a Buffer is decoded latin1 to preserve bytes, the BOM becomes three characters () that trim() will not remove — and fs.readFileSync(path, "latin1") gives the same. Only a utf8-decoded string yields the U+FEFF that trim() handles. Strip /^(?:|)/ and test the Buffer and latin1 paths, not just the decoded string. node-saml's test covered only the one representation that happened to work.

7. normalizePem splits lines longer than 64 characters and never joins shorter ones. So its output is not stricttextualmsg, whose base64fullline is exactly 64. The current doc comment claims otherwise. Either fix the claim or make it reflow; don't leave the claim.

8. PEM_BODY_REGEX is a global regex, so lastIndex is shared mutable state. Reset it before use and run the loop to exhaustion, or a caller that returns early poisons the next call. Relevant if tests run in random order.

9. Padding. Confine = to the end of the data and require a whole final quantum (RFC 4648 §4: a multiple of four, or two then ==, or three then =). Checking the data de-lined also admits the split pad of base64finl (base64pad *WSP eol base64pad) for free, which is correct per Figure 1 even though nothing emits it.

Do not port the size cap

node-saml rejects values over 1 MiB. It is the only part of this work that would reject something genuinely valid, and it is option validation rather than parsing — it exists so the error can name idpCert. Leaving it out is what keeps this a 6.x change. It stays in node-saml's wrapper.

One decision to make first

derToPem currently strips spaces from bare base64 (.replace(/\r\n|\r| /g, "")). node-saml deliberately rejects them. This is the original report in node-saml#361: customer certificates whose line endings had been turned into spaces.

Keeping the strip is the non-breaking default here, and it would mean node-saml stops rejecting interior spaces once it wraps this — resolving #361's original case. That is a reversal of #409's stated position, so decide it deliberately rather than inheriting it. Settle this before writing the data check, because it determines whether stripTrailingBlanks is the only whitespace step or whether bare values get a separate de-spacing pass.

Tests

node-saml's test/crypto.spec.ts has 90 keyInfoToPem cases covering all of the above and should be the basis for the port. Worth carrying over specifically:

• the table asserting PEM and bare agree on the same body, including the rejection message
• partial quanta: A, A=, AA, AAA, AAAA=, AAA==, AAAAA, AAAA==
• BOM as a decoded string, as a Buffer, and via latin1
• CR, CRLF and LF in both forms
• leading blank rejected / trailing blank accepted
• concatenated bundles, with and without blank lines between messages
• a body that is one long run of blanks, asserting the rejection only — no timer

Verify each new rejection test fails against the current parser for the format-rejection reason before the fix lands.

Then

Once released, #409 becomes a caret bump plus deletion of src/crypto.ts's internals, keeping the wrapper for the type check, the size cap, the optionName threading, and the privateKey path — which never reaches xml-crypto, since node-saml signs redirect bindings with crypto.createSign directly.