[wesleyschlenker]

Maintainer review — 2026-09-11: held for a security regression (review). Resolved on 2026-09-25: the branch was reset, and it no longer collapses the value for verification.

---

Author update — 2026-09-25: branch reset to address security regression and limit scope.

Fixes #548. This is the diagnostic path requested after the security hold on unconditional anyURI collapse.

Algorithm is typed as xsd:anyURI (XML Schema 2 §3.2.17), which collapses whitespace. XML 1.0 §3.3.3 attribute-value normalization turns a pretty-printer line wrap into trailing spaces but does not collapse them, so a supported URI fails registry lookup with a generic is not supported error. Collapsing on read fixed that for legitimate wraps, but also let an unsigned, whitespace-padded SignatureMethod decoy verify while a consumer that bans RSA-SHA1 by comparing the raw attribute string still allowed the padded identifier:

Version	Consumer's raw-string check	Library verification
master	Allows the padded identifier	Throws is not supported
Collapse on read (held)	Allows the padded identifier	Collapses to RSA-SHA1; checkSignature returns true
This branch	Allows the padded identifier	Throws, naming the whitespace match

The broad descendant lookup that selects the decoy predates this PR. An exact, unpadded decoy is already honored on master.

Changes

• findSignatureAlgorithm, findCanonicalizationAlgorithm, and findHashAlgorithm keep looking up the exact attribute value. On miss, utils.formatUnsupportedAlgorithmMessage() collapses for diagnostics only: if the collapsed form is one of the registry's own keys, the error says so and that whitespace is not collapsed. Own keys only, because the registries are object literals and would otherwise match constructor or toString.
• collapseAnyUri stays module-private in utils.ts. formatUnsupportedAlgorithmMessage is not re-exported from the package barrel.
• Unsupported-algorithm errors quote the name with JSON.stringify, so trailing spaces are visible in the message.

This does not make line-wrapped algorithms verify. That, with the structure and policy changes it needs, is #628 on 7.0 (discussion).

Tests

• rejects line-wrapped {CanonicalizationMethod,Transform,SignatureMethod,DigestMethod} Algorithm with a whitespace diagnostic: still rejects; message names the supported URI after collapse.
• still rejects a genuinely unregistered Algorithm URI: trailing spaces, no "matches the supported algorithm" clause.
• does not report an inherited Object property as a supported algorithm: " constructor " collapses to an Object.prototype name, and the error has no whitespace clause.
• does not treat NBSP as Algorithm whitespace: \u00A0 is not collapsed as XML whitespace.
• rejects a whitespace-padded SignatureMethod decoy outside SignedInfo: Object before SignedInfo is selected (same as master); verification throws the diagnostic rather than returning true.
• documents that an exact SignatureMethod decoy outside SignedInfo is selected: RSA-SHA256 decoy is selected; verification fails with invalid signature: the signature value … is incorrect.

npm run build, npm test (517 passing) and npm run lint pass.

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Unsupported signature, canonicalization, and hash algorithm errors now provide clearer diagnostics when collapsing XML whitespace would match a supported algorithm. Algorithm identifiers containing whitespace remain unsupported and are not normalized.
  • Unsupported algorithm identifiers are shown in double quotes in error messages.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 8f482ebb-f5b5-40ac-8e4a-9cbfbabf18a1

📥 Commits

Reviewing files that changed from the base of the PR and between 391c91f and 0be0ab8.

📒 Files selected for processing (3)

• src/signed-xml.ts
• src/utils.ts
• test/algorithm-uri-whitespace.spec.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/signed-xml.ts

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Unsupported signature, canonicalization, and hash algorithm errors now use a shared formatter. When XML whitespace collapse produces a supported algorithm name, the error adds a diagnostic. Algorithm lookup and construction remain unchanged.

Changes

Algorithm whitespace diagnostics

Layer / File(s)	Summary
Format unsupported algorithm diagnostics src/utils.ts	A utility collapses XML whitespace for comparison and adds a diagnostic when the collapsed name matches an own registry property.
Apply diagnostics to algorithm lookups src/signed-xml.ts, test/algorithm-uri-whitespace.spec.ts, test/signed-references-tests.spec.ts	Signature, canonicalization, and hash lookup errors use the formatter. Tests cover whitespace-padded supported names, unsupported names, NBSP, signature selection, and updated error quoting.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: cjbarth

Merge Risk: ⚪ Minimal · up to 0be0a

The change improves unsupported-algorithm diagnostics without accepting whitespace-padded identifiers or changing signature verification. No actionable merge-blocking risk remains, subject to normal build and test checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 0be0a

The current revision improves diagnostics without accepting whitespace-padded algorithm identifiers. The previously described normalization bypass is not present in this change, and no introduced or worsened security risk was identified in the reviewed paths.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated attacker-controlled surface is algorithm text in supplied XML, including unsigned decoy content. The new behavior affects diagnostics on existing lookup failures, not accepted algorithms, verification authority, or authenticated-reference publication. Downstream consumer policies were not independently inspected.

Security Findings and Attack Paths

• inferred — The previously described padded-decoy normalization attack is blocked at this head because the padded identifier remains the lookup key and is rejected. The regression test asserts raw-value preservation and rejection for an unsigned RSA-SHA1 decoy; this is static source and assertion evidence, not a newly executed reproduction.

Trust Boundaries and Controls

• observed — Diagnostic recognition of a collapsed registered name does not authorize that algorithm. Exact lookup must succeed before an algorithm is instantiated, and signature verification must return true before current canonical references are published as signed.

Resilience and Maintainability Implications

• observed — For a valid checkSignature invocation, signed-reference publication is cleared before document processing. Unsupported-algorithm failure cannot publish the current references; successful verification publishes them before reporting callback success. The PR leaves this lifecycle unchanged. Dedicated changed tests for callback and repeated-instance transitions are absent.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	Issue #548 requires XML xsd:anyURI whitespace collapse for CanonicalizationMethod, SignatureMethod, DigestMethod, and Transform, with tests that signed-over wrapped identifiers verify. The P…	Do not mark #548 complete with this PR. Either implement an approved normalization design with signed-over coverage for all four elements, or formally update or unlink #548 so that its coding requirement is diagnostic-only.
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The formatter changes produce the diagnostic required for unsupported algorithm values. The whitespace, unsupported-URI, NBSP, inherited-property, and decoy tests exercise that diagnostic behavior. Th…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: diagnosing whitespace in Algorithm URI values.

Full details: Linked Issues check

Explanation

Issue #548 requires XML xsd:anyURI whitespace collapse for CanonicalizationMethod, SignatureMethod, DigestMethod, and Transform, with tests that signed-over wrapped identifiers verify. The PR keeps exact lookup and uses collapseAnyUri only to diagnose lookup failures. The tests verify rejection and diagnostic text, not verification after normalization. The reported maintainer intent to narrow #548 conflicts with the supplied issue requirements and does not establish that the issue was formally updated.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.89%. Comparing base (834655e) to head (0be0ab8).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #576      +/-   ##
==========================================
+ Coverage   83.63%   83.89%   +0.25%     
==========================================
  Files           9        9              
  Lines        1210     1217       +7     
  Branches      307      308       +1     
==========================================
+ Hits         1012     1021       +9     
+ Misses        119      118       -1     
+ Partials       79       78       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cjbarth]

Thanks, Wes. This implements the normalization requested in #548, but review uncovered a concrete algorithm-policy bypass. We should hold this PR in its current form.

Reproduced against master 2ac129bc319fb78c578dfed42f6babb78b09e00d and this PR at dad853008a748a144cd828fb9a6b7da7be93b6c9:

Start with a valid RSA-SHA1 signature and a consumer that rejects RSA-SHA1 by comparing the raw SignatureMethod/@Algorithm string with the registered URI. The consumer uses the same descendant lookup as loadSignature. Adding an unsigned Object before SignedInfo, containing a SignatureMethod with spaces around that URI, makes both lookups select the decoy. The original SignedInfo and signature value remain unchanged.

Version	Consumer's raw-string check	Library verification
Master	Allows the padded identifier	Throws is not supported
This PR	Allows the padded identifier	Collapses to RSA-SHA1; checkSignature returns true
Diagnostic-only alternative	Allows the padded identifier	Throws an error identifying the whitespace

This reduces security for that consumer: the upgrade turns an attacker-modified document that was rejected into one that passes the combined policy and verification checks. The attacker needs an existing valid RSA-SHA1 signature under the verifier's trusted key, but does not need the signing key to insert the decoy. The demonstrated impact is bypassing the consumer's SHA-1 prohibition; the cryptographic signature still verifies over the original signed content.

The broad lookup predates this PR. Both algorithm selectors search descendants of Signature by local name, so unsigned content can supply the selected method. An exact, unpadded decoy is already honored on master, although the demonstrated consumer rejects it. The existing unsupported-identifier path fails closed; the description's earlier “failed open” wording was incorrect.

There is a further constraint on fixing this separately: an isolated experiment that only scopes the library's selectors to namespace-qualified direct children of SignedInfo still lets the same consumer be fooled. The consumer follows the decoy while the library verifies using the authentic method. Even a decoy naming SHA-256 then passes that consumer's check. A correction must address malformed structure and ambiguity as well as selector scope. This reproduction places Object before SignedInfo, contrary to the specified Signature child order.

Controls confirmed that checking the actual method directly under SignedInfo rejects this example, and removing RSA-SHA1 from the verifier's algorithm registry also prevents verification.

For #548, the immediate change should preserve rejection and explain the whitespace in the error. The existing selector/structure problem needs a separate security fix with public-API regression tests. Any later normalization proposal needs review against consumer algorithm policies as well as XML schema semantics.

[markstos]

Further AI analysis:

A safe design needs three pieces:

1. Reject ambiguous signature structure. loadSignature() currently finds algorithm elements with descendant XPath queries, so unsigned content can shadow the real SignedInfo algorithms (signed-xml.ts). Parse the required direct children, validate cardinality/order, and reject extra or out-of-place CanonicalizationMethod/SignatureMethod elements. Merely ignoring the decoy is insufficient because a consumer may still select it.

2. Make whitespace collapsing opt-in. Preserve today’s fail-closed behavior by default, but offer an explicit option such as algorithmAttributeWhitespace: "collapse". Canonicalization must still operate on the original DOM—the normalized value is only for algorithm resolution—so signed whitespace remains covered by SignatureValue.

3. Require policy to use the normalized identifiers. When collapsing is enabled, callers should supply an in-library algorithm policy/allowlist covering signature, digest, canonicalization, and transforms. The verifier would apply that policy to the same normalized values it later uses for registry lookup. For example, a policy excluding RSA-SHA1 must see normalized RSA-SHA1 and reject it before verification can succeed.

The existing registries around signed-xml.ts are already close to algorithm allowlists, but a constructor-level verification policy would make the security decision explicit and cover all four algorithm types consistently.

That produces these behaviors:

• Existing callers, without the option: padded algorithms remain rejected, ideally with whitespace made visible in the error.
• Opted-in callers with a suitable policy: genuinely signed, line-wrapped Algorithm values verify.
• The PR’s unsigned decoy: rejected as ambiguous/malformed structure.
• A normalized RSA-SHA1 value: rejected when the caller’s normalized policy forbids it.
• Whitespace inserted after signing: still fails cryptographic verification.

I would therefore split this into:

• A patch now: improved diagnostic plus structural/ambiguity regression tests.
• A separate feature, possibly v7: explicit normalization paired with verifier-owned algorithm policy and migration documentation.

There is no general way for the library to make unconditional normalization safe for arbitrary consumers that inspect the raw DOM themselves. The policy decision has to move inside the verifier—or normalization must remain an explicit compatibility opt-in. This directly addresses the concern documented in the security review.

[wesleyschlenker]

@cjbarth, I've updated the branch to have a clean commit history with the patch you requested. Let me know if you have any follow-up change requests.

[cjbarth]

A note for the review, since it bears on the milestone. formatUnsupportedAlgorithmMessage() quotes with JSON.stringify, so this also changes the … is not supported message for a URI with no whitespace in it, from '…' to "…". That's a change to the error text for input that already fails. #583 is the same kind of change, so I've moved it to 6.3.3 as well.

[cjbarth]

#548 is now scoped to the diagnostic, so this PR closing it is intended. Letting a line-wrapped Algorithm verify is #628, on 7.0.