Add XmlDSigVerifier wrapper for SignedXml#519
Add XmlDSigVerifier wrapper for SignedXml#519shunkica wants to merge 12 commits intonode-saml:masterfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces a comprehensive XML digital signature verification framework through a new Changes
Sequence DiagramsequenceDiagram
participant User
participant XmlDSigVerifier
participant SignedXml
participant KeySelector as Key/Certificate<br/>Selector
participant Crypto as Cryptographic<br/>Validation
User->>XmlDSigVerifier: verifySignature(xml, options, signatureNode?)
XmlDSigVerifier->>XmlDSigVerifier: resolveOptions(options)<br/>merge with defaults
XmlDSigVerifier->>XmlDSigVerifier: discriminate keySelector type<br/>(KeyInfo/Cert/Secret)
XmlDSigVerifier->>SignedXml: createSignedXml(resolved)<br/>configure with algorithms
alt KeyInfo Mode
XmlDSigVerifier->>KeySelector: getCertFromKeyInfo(keyInfo)
KeySelector-->>XmlDSigVerifier: certificate/key
else Certificate Mode
XmlDSigVerifier->>KeySelector: use publicCert
KeySelector-->>XmlDSigVerifier: certificate
else Shared Secret Mode
XmlDSigVerifier->>KeySelector: use sharedSecret
KeySelector-->>XmlDSigVerifier: secret key
end
XmlDSigVerifier->>SignedXml: validate signature
SignedXml->>Crypto: perform cryptographic<br/>verification
Crypto-->>SignedXml: verification result
SignedXml-->>XmlDSigVerifier: signed references
alt Verification Success
XmlDSigVerifier-->>User: SuccessfulXmlDsigVerificationResult
else Verification Failure
alt throwOnError enabled
XmlDSigVerifier-->>User: throw Error
else throwOnError disabled
XmlDSigVerifier-->>User: FailedXmlDsigVerificationResult
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #519 +/- ##
==========================================
+ Coverage 73.05% 77.61% +4.55%
==========================================
Files 9 11 +2
Lines 902 1157 +255
Branches 239 318 +79
==========================================
+ Hits 659 898 +239
- Misses 143 152 +9
- Partials 100 107 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (15)
src/types.ts (1)
13-19: Preserve IntelliSense by deriving algorithm types from constants (while still allowing custom strings).Current
= stringerodes DX and safety. Recommend unions fromalgorithms.tsvalues plus a widened fallback.@@ -import * as crypto from "crypto"; +import * as crypto from "crypto"; +import { + CANONICALIZATION_ALGORITHMS, + TRANSFORM_ALGORITHMS, + HASH_ALGORITHMS, + SIGNATURE_ALGORITHMS, +} from "./algorithms"; @@ -export type CanonicalizationAlgorithmType = string; +type ValuesOf<T> = T[keyof T]; +export type CanonicalizationAlgorithmType = + | ValuesOf<typeof CANONICALIZATION_ALGORITHMS> + | (string & {}); @@ -export type CanonicalizationOrTransformAlgorithmType = string; +export type CanonicalizationOrTransformAlgorithmType = + | ValuesOf<typeof TRANSFORM_ALGORITHMS> + | (string & {}); @@ -export type HashAlgorithmType = string; +export type HashAlgorithmType = + | ValuesOf<typeof HASH_ALGORITHMS> + | (string & {}); @@ -export type SignatureAlgorithmType = string; +export type SignatureAlgorithmType = + | ValuesOf<typeof SIGNATURE_ALGORITHMS> + | (string & {});Based on learnings.
src/algorithms.ts (1)
1-37: Freeze constants at runtime and annotate legacy algorithms.
as constis compile-time only. Freeze to guard against accidental mutation, and mark SHA‑1 variants as legacy in JSDoc.-export const CANONICALIZATION_ALGORITHMS = { +export const CANONICALIZATION_ALGORITHMS = Object.freeze({ C14N: "http://www.w3.org/TR/2001/REC-xml-c14n-20010315", C14N_WITH_COMMENTS: "http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments", EXCLUSIVE_C14N: "http://www.w3.org/2001/10/xml-exc-c14n#", EXCLUSIVE_C14N_WITH_COMMENTS: "http://www.w3.org/2001/10/xml-exc-c14n#WithComments", -} as const; +} as const); @@ -export const TRANSFORM_ALGORITHMS = { +export const TRANSFORM_ALGORITHMS = Object.freeze({ ...CANONICALIZATION_ALGORITHMS, ENVELOPED_SIGNATURE: "http://www.w3.org/2000/09/xmldsig#enveloped-signature", -} as const; +} as const); @@ -export const HASH_ALGORITHMS = { +export const HASH_ALGORITHMS = Object.freeze({ + /** @deprecated SHA-1 is considered weak; prefer SHA256/512. */ SHA1: "http://www.w3.org/2000/09/xmldsig#sha1", SHA256: "http://www.w3.org/2001/04/xmlenc#sha256", SHA512: "http://www.w3.org/2001/04/xmlenc#sha512", -} as const; +} as const); @@ -export const SIGNATURE_ALGORITHMS = { +export const SIGNATURE_ALGORITHMS = Object.freeze({ + /** @deprecated RSA-SHA1 is considered weak; prefer RSA-SHA256/512. */ RSA_SHA1: "http://www.w3.org/2000/09/xmldsig#rsa-sha1", RSA_SHA256: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", RSA_SHA256_MGF1: "http://www.w3.org/2007/05/xmldsig-more#sha256-rsa-MGF1", RSA_SHA512: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha512", HMAC_SHA1: "http://www.w3.org/2000/09/xmldsig#hmac-sha1", -} as const; +} as const);test/xml-validator.spec.ts (7)
17-27: Use exported algorithm constants instead of hard-coded URIs.Reduces string typos and couples tests to public API.
-import { XmlValidator, XmlValidatorFactory } from "../src/xml-validator"; +import { XmlValidator, XmlValidatorFactory } from "../src/xml-validator"; +import { + CANONICALIZATION_ALGORITHMS, + HASH_ALGORITHMS, + SIGNATURE_ALGORITHMS, +} from "../src/algorithms"; @@ - const sig = new SignedXml({ + const sig = new SignedXml({ privateKey, - canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", - signatureAlgorithm: "http://www.w3.org/2000/09/xmldsig#rsa-sha1", + canonicalizationAlgorithm: CANONICALIZATION_ALGORITHMS.EXCLUSIVE_C14N, + signatureAlgorithm: SIGNATURE_ALGORITHMS.RSA_SHA1, }); @@ - digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", - transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], + digestAlgorithm: HASH_ALGORITHMS.SHA1, + transforms: [CANONICALIZATION_ALGORITHMS.EXCLUSIVE_C14N], });Apply similarly in other helper/test blocks.
Also applies to: 41-47, 56-63
331-348: Confirm non-standard error-first callback behavior is intentional.Async tests expect
err === nulleven on validation errors. If this is the desired API, document it; otherwise, return errors viaerr.Would you like a PR to switch to Node-style
(err)semantics and update tests accordingly?
550-561: Strengthen maxTransforms coverage with a failing case.Current test only asserts success when within limit; it doesn’t prove enforcement.
Add a case where a signed reference uses 2+ transforms and validator is created with
maxTransforms: 1; expect invalid result or thrown error (per API contract).
114-123: Type-level constraint isn’t verifiable at runtime; consider tsd tests.This test’s comment references a TS compile-time error. Add a tsd test or a build-time check instead.
I can add a small
test-d.tsusingtsdto assertenableWSSecurityModeforbidsidAttributes.
273-301: Add at least one modern-algorithm path (SHA-256 / RSA-SHA256).Most tests use SHA‑1 for simplicity. Include one RSA‑SHA256 + SHA256 case to cover contemporary defaults.
- const sig = new SignedXml({ + const sig = new SignedXml({ privateKey, - canonicalizationAlgorithm: CANONICALIZATION_ALGORITHMS.EXCLUSIVE_C14N, - signatureAlgorithm: SIGNATURE_ALGORITHMS.RSA_SHA1, + canonicalizationAlgorithm: CANONICALIZATION_ALGORITHMS.EXCLUSIVE_C14N, + signatureAlgorithm: SIGNATURE_ALGORITHMS.RSA_SHA256, }); @@ - digestAlgorithm: HASH_ALGORITHMS.SHA1, + digestAlgorithm: HASH_ALGORITHMS.SHA256,Also applies to: 591-626
456-467: Factory override test is solid; consider one negative path.Add a case where factory returns wrong cert and no override is provided to assert failure path.
1-64: Offer: add helper to DRY signature creation across tests.
createSignedXmlandcreateMultiSignedXmlduplicate algorithm strings and options. Extract a parametric helper that accepts algorithms and xpath to reduce repetition.Happy to push a refactor commit.
src/index.ts (1)
7-14: All exports verified; consider adding JSDoc annotations for clarity.All named exports (XmlSigner, XmlSignerFactory, XmlValidator, XmlValidatorFactory, and algorithm constants) exist and are correctly re-exported. To improve developer experience, add JSDoc comments with
@experimentalor migration path notes on these exports in src/index.ts lines 7-14.test/xml-signer.spec.ts (2)
783-787: Use exported algorithm constants instead of raw URIs to avoid typos and ease future changes.Example:
- signatureAlgorithm: "http://www.w3.org/2000/09/xmldsig#rsa-sha1", + import { SIGNATURE_ALGORITHMS, CANONICALIZATION_ALGORITHMS, HASH_ALGORITHMS } from "../src/algorithms"; + signatureAlgorithm: SIGNATURE_ALGORITHMS.RSA_SHA1,Apply similarly for canonicalization and digest algorithms across tests.
Also applies to: 815-820, 847-852
374-394: Add an edge-case test where the target ID contains quotes to ensure URI→XPath conversion handles XPath literal quoting correctly.Example:
it("should sign when URI ID contains quotes", function () { const factory = new XmlSignerFactory({ signatureAlgorithm: "http://www.w3.org/2000/09/xmldsig#rsa-sha1", canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", }); const signer = factory.createSigner(privateKey); signer.addReference({ uri: "#it\"s'id", transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", }); const xml = '<root><test id="it"s'id">content</test></root>'; expect(() => signer.sign(xml)).to.not.throw(); });src/xml-signer.ts (2)
296-323: Block re-entrancy and mid-flight mutation during async signing.Set a signingInProgress flag (or set hasBeenSigned upfront) to prevent a second sign() or addReference() while computeSignature is running:
- sign(xml: string, callback?: ErrorFirstCallback<string>): string | void { - if (this.hasBeenSigned) { + private signingInProgress = false; + sign(xml: string, callback?: ErrorFirstCallback<string>): string | void { + if (this.hasBeenSigned || this.signingInProgress) { throw new Error( "This XmlSigner instance has already been used to sign a document. Create a new instance to sign another document.", ); } - - if (callback) { + if (callback) { + this.signingInProgress = true; try { this.signedXml.computeSignature(xml, this.computeOptions, (err, signedXmlInstance) => { if (err) { - callback(err); + this.signingInProgress = false; + callback(err); } else if (signedXmlInstance) { this.hasBeenSigned = true; + this.signingInProgress = false; callback(null, signedXmlInstance.getSignedXml()); } else { + this.signingInProgress = false; callback(new Error("Signing failed: no signed XML instance returned")); } }); } catch (error) { + this.signingInProgress = false; callback(error instanceof Error ? error : new Error("Unknown signing error")); } } else { + this.signingInProgress = true; this.signedXml.computeSignature(xml, this.computeOptions); this.hasBeenSigned = true; + this.signingInProgress = false; return this.signedXml.getSignedXml(); } }
222-226: Optional: for enveloped signatures, consider explicitly selecting the document element regardless of prefix to be resilient to processing instructions or comments before the root.E.g., use the existing "/*" but document the intent in a comment, or use a SignedXml-native empty-URI path exclusively without an XPath if supported.
src/xml-validator.ts (1)
154-159: Nit: trim the trailing space in the single-use error message.- "This XmlValidator instance has already been used. Create a new instance to validate another document. ", + "This XmlValidator instance has already been used. Create a new instance to validate another document.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks).nvmrc(1 hunks)example/new-api-example.js(1 hunks)src/algorithms.ts(1 hunks)src/index.ts(1 hunks)src/types.ts(1 hunks)src/xml-signer.ts(1 hunks)src/xml-validator.ts(1 hunks)test/xml-signer.spec.ts(1 hunks)test/xml-validator.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.309Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.734Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
📚 Learning: 2025-10-22T21:50:05.441Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.441Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
src/xml-signer.ts
🧬 Code graph analysis (6)
src/algorithms.ts (1)
src/index.ts (4)
CANONICALIZATION_ALGORITHMS(10-10)TRANSFORM_ALGORITHMS(11-11)HASH_ALGORITHMS(12-12)SIGNATURE_ALGORITHMS(13-13)
test/xml-validator.spec.ts (1)
src/xml-validator.ts (2)
XmlValidator(75-245)XmlValidatorFactory(251-293)
test/xml-signer.spec.ts (1)
src/xml-signer.ts (5)
XmlSignerFactory(343-436)XmlSignerFactoryOptions(147-190)XmlSigner(255-337)SigningReference(142-142)ReferenceAttributes(39-46)
example/new-api-example.js (3)
src/xml-signer.ts (1)
XmlSignerFactory(343-436)src/algorithms.ts (4)
SIGNATURE_ALGORITHMS(31-37)CANONICALIZATION_ALGORITHMS(4-9)TRANSFORM_ALGORITHMS(14-17)HASH_ALGORITHMS(22-26)src/xml-validator.ts (1)
XmlValidatorFactory(251-293)
src/xml-signer.ts (2)
src/types.ts (6)
ObjectAttributes(34-43)ComputeSignatureOptionsLocation(81-84)SignatureAlgorithmType(19-19)CanonicalizationAlgorithmType(13-13)ComputeSignatureOptions(98-103)ErrorFirstCallback(11-11)src/signed-xml.ts (1)
getKeyInfoContent(197-226)
src/xml-validator.ts (1)
src/types.ts (1)
ErrorFirstCallback(11-11)
🪛 Biome (2.1.2)
test/xml-signer.spec.ts
[error] 888-888: await is only allowed within async functions and at the top levels of modules.
(parse)
🔇 Additional comments (4)
.gitignore (1)
15-15: LGTM — safe tooling ignore.test/xml-validator.spec.ts (2)
564-570: Validator single-use: avoid consuming the instance for multiple edge sub-tests.Calling
validate("")marks the instance used; keep that behavior but ensure subsequent unrelated tests don’t accidentally reuse the same instance.If isolated per test (as here), ignore.
1-4: All verification checks passed—test/API alignment confirmed.The imports in lines 1-4 are appropriate and correctly aligned:
maxTransformsis actively used in the implementation (lines 12, 77, 116 ofsrc/xml-validator.ts)- SHA-1 algorithms are comprehensively covered in tests across multiple test files
- Tests properly import from the public API (
../src/index).nvmrc (1)
1-1: Align .nvmrc with CI test matrix or add v22 to CI.Your .nvmrc pins to v22.16.0, but CI tests only [16, 18, 20] + latest + lts/*. Local development uses v22 while CI doesn't test it, creating a mismatch risk. Either:
- Change .nvmrc to a tested version (e.g., v20 or use lts/*)
- Add v22 to the CI matrix if v22 is intentional
Verify this alignment is deliberate and update accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
example/new-api-example.js (1)
18-77: Certificate extraction could use library utility.The manual extraction of certificate content (lines 33-36) works but could use the library's
pemToDerutility for robustness. However, the current approach is acceptable for an example demonstrating the API.Consider this alternative:
const { pemToDer } = require("xml-crypto"); // ... const certContent = pemToDer(cert).toString("base64");This would make the example more robust against PEM format variations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
example/new-api-example.js(1 hunks)src/utils.ts(1 hunks)src/xml-signer.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.309Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.734Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
📚 Learning: 2025-10-22T21:50:05.441Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.441Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
src/xml-signer.tsexample/new-api-example.js
📚 Learning: 2025-10-25T14:41:13.608Z
Learnt from: shunkica
PR: node-saml/xml-crypto#519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.608Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Applied to files:
example/new-api-example.js
📚 Learning: 2025-10-22T20:36:00.734Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.734Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Applied to files:
example/new-api-example.js
📚 Learning: 2025-10-22T21:03:38.309Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.309Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Applied to files:
example/new-api-example.js
🧬 Code graph analysis (2)
src/xml-signer.ts (3)
src/types.ts (6)
ObjectAttributes(34-43)ComputeSignatureOptionsLocation(81-84)SignatureAlgorithmType(19-19)CanonicalizationAlgorithmType(13-13)ComputeSignatureOptions(98-103)ErrorFirstCallback(11-11)src/utils.ts (1)
buildIdXPath(368-376)src/signed-xml.ts (1)
getKeyInfoContent(197-226)
example/new-api-example.js (3)
src/xml-signer.ts (1)
XmlSignerFactory(376-476)src/algorithms.ts (4)
SIGNATURE_ALGORITHMS(31-37)CANONICALIZATION_ALGORITHMS(4-9)TRANSFORM_ALGORITHMS(14-17)HASH_ALGORITHMS(22-26)src/xml-validator.ts (1)
XmlValidatorFactory(251-293)
🔇 Additional comments (15)
src/utils.ts (2)
335-366: LGTM! XPath literal escaping is correctly implemented.The function correctly handles XPath 1.0's lack of escape mechanism for quotes by switching quote delimiters or using
concat()when both types are present.
368-376: LGTM! ID-based XPath construction is correct and safe.The function properly validates inputs, uses safe literal construction, and generates XPath expressions that handle namespaced ID attributes via
local-name()comparisons.example/new-api-example.js (7)
1-9: LGTM! Correct package import.The example correctly uses
require("xml-crypto")to demonstrate end-user consumption of the library.Based on learnings.
79-109: LGTM! Default key pattern correctly demonstrated.The example clearly shows how to use a factory-configured default private key.
111-138: LGTM! Enveloped signature pattern is correct.The example properly demonstrates enveloped signatures with the correct transform order.
140-171: LGTM! Basic validation pattern is clear.The example demonstrates proper validation with error handling.
173-201: LGTM! Certificate override pattern is correctly demonstrated.The example shows how to override the factory's certificate configuration, with appropriate comments acknowledging the simplified KeyInfo extraction.
203-226: LGTM! Async validation pattern is correct.The example properly demonstrates callback-based validation with error-first callback conventions.
228-294: LGTM! Multiple signatures example is instructive.The example effectively demonstrates both the failure case (validating without loading a specific signature) and the success case (using
loadSignatureto select a specific signature).src/xml-signer.ts (6)
1-12: LGTM! Imports and constants are appropriate.The imports are well-structured and the default ID attributes cover common XML ID attribute casings.
14-225: LGTM! Type definitions are well-designed and type-safe.The use of discriminated unions and the
nevertype to enforce mutual exclusivity betweenenableWSSecurityModeandidAttributesis an excellent type-safety pattern.
232-282: LGTM! Reference handling is correct and safe.The function properly converts signing references to SignedXml format, with special handling for:
- Enveloped signatures (empty URI →
"/*")- Fragment IDs (using
buildIdXPathfor safe escaping)- External URIs (correctly rejected for signing)
The use of
buildIdXPathensures safe XPath literal construction for IDs with quotes, resolving the escaping concerns from past reviews.
288-370: LGTM! XmlSigner class enforces safe single-use pattern.The class correctly prevents common misuse scenarios:
- Adding references after signing
- Reusing a signer instance
- Accessing signature XML before signing
Error handling for async operations is properly implemented with try-catch wrapping.
376-394: LGTM! Factory constructor properly validates required options.The constructor enforces required configuration and safely copies options to prevent external mutations.
396-476: LGTM! Factory signer creation is well-structured.The method correctly:
- Resolves private keys with proper fallback logic
- Filters undefined KeyInfo attributes to maintain clean output
- Configures ID handling for both standard and WS-Security modes
- Applies factory-level references to each signer
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
example/new-api-example.js (1)
1-3: Use the package name in examplesExamples should demonstrate end‑user usage via the published package.
-const { XmlSignerFactory, XmlDSigValidator, Algorithms, pemToDer } = require("../lib/index"); +const { XmlSignerFactory, XmlDSigValidator, Algorithms, pemToDer } = require("xml-crypto");Based on learnings.
🧹 Nitpick comments (12)
src/algorithms.ts (1)
1-44: Centralized allow-lists: great; tighten types and immutabilityConsider exporting derived types from these const maps and (optionally) freezing at runtime to prevent accidental mutation.
+// Optionally export strongly-typed unions for downstream usage +export type CanonicalizationAlgorithm = + typeof CANONICALIZATION_ALGORITHMS[keyof typeof CANONICALIZATION_ALGORITHMS]; +export type TransformAlgorithm = + typeof TRANSFORM_ALGORITHMS[keyof typeof TRANSFORM_ALGORITHMS]; +export type HashAlgorithm = + typeof HASH_ALGORITHMS[keyof typeof HASH_ALGORITHMS]; +export type SignatureAlgorithm = + typeof SIGNATURE_ALGORITHMS[keyof typeof SIGNATURE_ALGORITHMS]; + -export const Algorithms = { +export const Algorithms = { canonicalization: CANONICALIZATION_ALGORITHMS, transform: TRANSFORM_ALGORITHMS, hash: HASH_ALGORITHMS, signature: SIGNATURE_ALGORITHMS, -}; +} as const; // If you want runtime immutability too: // export const Algorithms = Object.freeze({ // canonicalization: Object.freeze(CANONICALIZATION_ALGORITHMS), // transform: Object.freeze(TRANSFORM_ALGORITHMS), // hash: Object.freeze(HASH_ALGORITHMS), // signature: Object.freeze(SIGNATURE_ALGORITHMS), // });test/xml-validator.spec.ts (1)
169-182: Duplicate reusability checksBoth blocks assert validator reusability. Keep one to reduce test time/noise.
Also applies to: 264-277
src/signed-xml.ts (2)
556-601: Duplicate ID-search logic; unify and fix messageThe ID lookup is duplicated for QName and non-QName paths and repeats an error string with “ID / Id / Id”. Consider extracting a helper to build the XPath and to check duplicates; also fix the message casing.
- throw new Error( - "Cannot validate a document which contains multiple elements with the " + - "same value for the ID / Id / Id attributes, in order to prevent " + - "signature wrapping attack.", - ); + throw new Error( + "Cannot validate a document which contains multiple elements with the " + + "same value for the ID / Id / id attributes, in order to prevent " + + "signature wrapping attack.", + );Optional extraction sketch:
function xpathForIdAttr(local: string, ns?: string, val?: string) { const pred = ns ? `@*[local-name(.)='${local}' and namespace-uri(.)='${ns}']='${val}'` : `@*[local-name(.)='${local}']='${val}'`; return `//*[@*[${ pred }]]`; // or keep current structure for exact parity }
1404-1421: Avoid redundant xmlns prefix injection when setting QName IdBefore adding
xmlns:${prefix}, check if it already exists on the element to reduce noise.- node.setAttributeNS("http://www.w3.org/2000/xmlns/", `xmlns:${prefix}`, namespace); + if (!node.hasAttributeNS("http://www.w3.org/2000/xmlns/", prefix)) { + node.setAttributeNS("http://www.w3.org/2000/xmlns/", `xmlns:${prefix}`, namespace); + } node.setAttributeNS(namespace, qName, id);src/types.ts (2)
13-20: Widening tostringreduces safety; derive unions from Algorithms while allowing extensionPreserve DX with flexible strings but recover strong types via branded unions derived from
Algorithms.-export type CanonicalizationAlgorithmType = string; -export type CanonicalizationOrTransformAlgorithmType = string; -export type HashAlgorithmType = string; -export type SignatureAlgorithmType = string; +// Derive exact URI unions from the central registry and still allow custom strings. +type _Algos = typeof import("./algorithms").Algorithms; +export type CanonicalizationAlgorithmType = + | _Algos["canonicalization"][keyof _Algos["canonicalization"]] + | (string & {}); +export type CanonicalizationOrTransformAlgorithmType = + | _Algos["transform"][keyof _Algos["transform"]] + | (string & {}); +export type HashAlgorithmType = + | _Algos["hash"][keyof _Algos["hash"]] + | (string & {}); +export type SignatureAlgorithmType = + | _Algos["signature"][keyof _Algos["signature"]] + | (string & {});
51-53: Align options with validator usage: addmaxTransforms?
XmlDSigValidatorpassesmaxTransformsintoSignedXmloptions; define it here to avoid excess-property issues and to support future enforcement at the core.export interface SignedXmlOptions { idMode?: "wssecurity"; idAttribute?: string; idAttributeQNames?: string[]; namespaceMap?: Record<string, string>; privateKey?: crypto.KeyLike; publicCert?: crypto.KeyLike; signatureAlgorithm?: SignatureAlgorithmType; canonicalizationAlgorithm?: CanonicalizationAlgorithmType; inclusiveNamespacesPrefixList?: string | string[]; implicitTransforms?: ReadonlyArray<CanonicalizationOrTransformAlgorithmType>; + /** Maximum allowed number of transforms per Reference (defense-in-depth). */ + maxTransforms?: number; keyInfoAttributes?: Record<string, string>; getKeyInfoContent?(args?: GetKeyInfoContentArgs): string | null; getCertFromKeyInfo?(keyInfo?: Node | null): string | null; objects?: Array<{ content: string; attributes?: ObjectAttributes }>; }src/xmldsig-validator.ts (3)
97-106: Defaulting idAttributeQNames may be misleadingYou default idAttributeQNames to DEFAULT_ID_ATTRIBUTES, then also pass it into SignedXml and later assign idAttributes. Prefer leaving idAttributeQNames undefined unless the caller opts in (so SignedXml’s own defaults apply), or ensure you don’t assign idAttributes when QNames contain prefixes.
- idAttributeQNames: - Array.isArray(options.idAttributeQNames) && options.idAttributeQNames.length > 0 - ? options.idAttributeQNames - : DEFAULT_ID_ATTRIBUTES, + idAttributeQNames: + Array.isArray(options.idAttributeQNames) && options.idAttributeQNames.length > 0 + ? options.idAttributeQNames + : undefined,This reduces double‑configuration and avoids accidental misuse when QNames (e.g., "wsu:Id") are provided. Based on learnings.
46-47: Tighten algorithm typing for implicitTransformsIf available in your types, prefer CanonicalizationOrTransformAlgorithmType[] to catch invalid values at compile time.
- implicitTransforms?: ReadonlyArray<string>; + implicitTransforms?: ReadonlyArray<CanonicalizationOrTransformAlgorithmType>;You may need:
import type { CanonicalizationOrTransformAlgorithmType } from "./types";
146-151: Consider bounding returned signedReferences for large inputsReturning full canonicalized references can be large. Expose an option to cap size or return counts only, keeping a debug path when needed.
example/new-api-example.js (3)
176-199: Reference Type value looks incorrectType is meant to describe the data object being referenced, not a transform. Consider using an appropriate Type URI or omit it.
22-27: Guard pem parsing errors in example helperWrap pemToDer in try/catch to print a concise message for learners.
function extractCertificateContent(certPath) { - const cert = fs.readFileSync(certPath, "utf8"); - // Use the library's pemToDer utility for robust PEM parsing - return pemToDer(cert).toString("base64"); + try { + const cert = fs.readFileSync(certPath, "utf8"); + return pemToDer(cert).toString("base64"); + } catch (e) { + console.error("Failed to read/parse certificate:", e && e.message ? e.message : e); + throw e; + } }
560-606: Examples rely on repo test assetsThese paths won’t exist for consumers. Consider adding a short comment at the top instructing users to replace file reads with their own keys/certs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
example/new-api-example.js(1 hunks)src/algorithms.ts(1 hunks)src/index.ts(1 hunks)src/signed-xml.ts(8 hunks)src/types.ts(2 hunks)src/utils.ts(2 hunks)src/xmldsig-validator.ts(1 hunks)test/xml-validator.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.309Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.734Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
📚 Learning: 2025-10-22T21:50:05.441Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.441Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
src/signed-xml.tsexample/new-api-example.js
📚 Learning: 2025-10-25T14:41:13.608Z
Learnt from: shunkica
PR: node-saml/xml-crypto#519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.608Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Applied to files:
example/new-api-example.js
📚 Learning: 2025-10-22T20:36:00.734Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.734Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Applied to files:
example/new-api-example.js
📚 Learning: 2025-10-22T21:03:38.309Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.309Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Applied to files:
example/new-api-example.js
🧬 Code graph analysis (4)
src/xmldsig-validator.ts (1)
src/signed-xml.ts (1)
SignedXml(30-1523)
test/xml-validator.spec.ts (3)
src/algorithms.ts (1)
Algorithms(39-44)src/xmldsig-validator.ts (2)
createSignedXml(157-189)XmlDSigValidator(89-206)src/signed-xml.ts (1)
SignedXml(30-1523)
example/new-api-example.js (4)
src/utils.ts (1)
pemToDer(163-175)src/xml-signer.ts (1)
XmlSignerFactory(376-476)src/algorithms.ts (1)
Algorithms(39-44)src/xmldsig-validator.ts (1)
XmlDSigValidator(89-206)
src/algorithms.ts (1)
src/index.ts (1)
Algorithms(9-9)
🔇 Additional comments (2)
src/index.ts (1)
7-9: Public API surface looks goodRe-exports read clean and preserve backward compatibility with
SignedXml. No issues from me here.src/signed-xml.ts (1)
1187-1219: Keeps ref.uri mutation during signing (intended compatibility)
ref.uri = idis preserved while computing references. This aligns with prior guidance to defer behavioral changes to a separate PR. Looks good.Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
test/xmldsig-validator.spec.ts (2)
279-290: “maxTransforms” test doesn’t exercise the limitThis signs with one transform and validates with
maxTransforms: 1, so it can’t fail. Add ≥2 transforms and expect invalid once validator-side enforcement lands. See suggested enforcement in the validator file comment.- const signedXml = createSignedXml(xml); + // Create a signature with two transforms to exceed the limit + const sig = new SignedXml({ privateKey, canonicalizationAlgorithm: canonicalization.EXCLUSIVE_C14N, signatureAlgorithm: signature.RSA_SHA1 }); + sig.addReference({ + xpath: "//*[local-name(.)='test']", + digestAlgorithm: hash.SHA1, + transforms: [transform.ENVELOPED_SIGNATURE, transform.EXCLUSIVE_C14N], + }); + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); @@ - expect(result.valid).to.be.true; + expect(result.valid).to.be.false; + expect(result.error).to.match(/transform/i);
360-387: Test title claims XmlSigner but uses SignedXmlRename for accuracy or switch to
XmlDSigSignerto exercise the new API.-it("should validate signatures created by XmlSigner", function () { +it("should validate signatures created by SignedXml (XmlSigner compatibility)", function () {src/xmldsig-validator.ts (3)
12-47: Optional: add WS-Security mode toggle to align with examplesExpose an
enableWSSecurityMode?: booleanoption and passidMode: "wssecurity"to SignedXml when true; skip settingidAttributesin that case. This matches the WS-Security profile used in examples.Also applies to: 165-172
159-163: Fix class name in error messageUse the correct name to avoid confusion in logs/tests.
- "XmlValidator requires either a publicCert or getCertFromKeyInfo function in options.", + "XmlDSigValidator requires either a publicCert or getCertFromKeyInfo function in options.",
183-186: Avoid setting legacy idAttributes when using QName-based IDsSetting
idAttributeswith namespaced QNames is ineffective and can conflict. Only set when all entries are unprefixed.- // Set ID attributes on the SignedXml instance - if (this.options.idAttributeQNames) { - signedXml.idAttributes = this.options.idAttributeQNames; - } + // Set legacy idAttributes only for unprefixed names + if (this.options.idAttributeQNames?.every((n) => !n.includes(":"))) { + signedXml.idAttributes = this.options.idAttributeQNames; + }
🧹 Nitpick comments (5)
test/xmldsig-signer.spec.ts (1)
36-38: Prefer Algorithms constants over raw strings for readability and consistencyReplace hardcoded URIs with
Algorithms.signature/*,Algorithms.canonicalization/*, andAlgorithms.hash/*to avoid typos and ease refactoring later. Example:- signatureAlgorithm: "http://www.w3.org/2000/09/xmldsig#rsa-sha1", - canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", + signatureAlgorithm: Algorithms.signature.RSA_SHA1, + canonicalizationAlgorithm: Algorithms.canonicalization.EXCLUSIVE_C14N,Also applies to: 62-68, 86-89, 110-115, 138-144, 161-166, 187-191, 210-221, 269-277, 354-359, 573-579
test/xmldsig-validator.spec.ts (1)
24-29: Use transform constants in transforms for claritySwap
canonicalization.EXCLUSIVE_C14Nfortransform.EXCLUSIVE_C14Nwhen populatingReference.transforms. Values are identical today, but using the correct namespace avoids confusion. Example:- transforms: [canonicalization.EXCLUSIVE_C14N], + transforms: [transform.EXCLUSIVE_C14N],Also applies to: 42-46, 57-61, 205-209, 327-333
example/xmldsig-signer-example.js (2)
3-9: Guard fallback import for pre-release compatibilityOlder published versions may not export
XmlDSigSigner/pemToDer. Add a runtime check and message to avoid confusing runtime errors.try { ({ XmlDSigSigner, Algorithms, pemToDer } = require("../lib/index")); } catch (error) { - ({ XmlDSigSigner, Algorithms, pemToDer } = require("xml-crypto")); + ({ XmlDSigSigner, Algorithms, pemToDer } = require("xml-crypto")); + if (!XmlDSigSigner || !pemToDer) { + console.warn("[examples] Installed xml-crypto version does not expose XmlDSigSigner/pemToDer yet. Build locally (npm run build) or use a version that includes the wrappers."); + } }
29-34: Avoid double-base64 if input is already DER
pemToDer(...).toString("base64")is correct for PEM, but ifcertPathever points to DER, this re-encodes arbitary bytes. Consider a small type/format check or keep as-is with a comment clarifying PEM input only.src/xmldsig-signer.ts (1)
342-373: Normalize inclusiveNamespacesPrefixList to an array and widen key type in factoryEnsure consistent shape for downstream code and match privateKey type.
-private createSignedXml(privateKey: crypto.KeyLike): SignedXml { +private createSignedXml(privateKey: crypto.KeyLike | string | Buffer): SignedXml { @@ - return new SignedXml({ + const inclusive = + this.options.inclusiveNamespacesPrefixList == null + ? undefined + : Array.isArray(this.options.inclusiveNamespacesPrefixList) + ? this.options.inclusiveNamespacesPrefixList + : [this.options.inclusiveNamespacesPrefixList]; + + return new SignedXml({ privateKey, signatureAlgorithm: this.options.signatureAlgorithm, canonicalizationAlgorithm: this.options.canonicalizationAlgorithm, - inclusiveNamespacesPrefixList: this.options.inclusiveNamespacesPrefixList, + inclusiveNamespacesPrefixList: inclusive, keyInfoAttributes: keyInfoAttributes, getKeyInfoContent: getKeyInfoContent, objects: this.options.objects, idAttributeQNames: this.options.idAttributeQNames, namespaceMap: this.options.namespaceMap, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
example/xmldsig-signer-example.js(1 hunks)example/xmldsig-validator-example.js(1 hunks)src/index.ts(1 hunks)src/utils.ts(2 hunks)src/xmldsig-signer.ts(1 hunks)src/xmldsig-validator.ts(1 hunks)test/xmldsig-signer.spec.ts(1 hunks)test/xmldsig-validator.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- src/utils.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.309Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.734Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
📚 Learning: 2025-10-22T21:50:05.441Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.441Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
test/xmldsig-validator.spec.tstest/xmldsig-signer.spec.tssrc/xmldsig-signer.ts
📚 Learning: 2025-10-25T14:41:13.608Z
Learnt from: shunkica
PR: node-saml/xml-crypto#519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.608Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Applied to files:
example/xmldsig-validator-example.js
🧬 Code graph analysis (6)
example/xmldsig-signer-example.js (4)
example/xmldsig-validator-example.js (7)
require(12-19)require(209-209)result(33-33)result(65-65)result(94-94)result(126-126)result(150-150)src/index.ts (2)
XmlDSigSigner(7-7)Algorithms(9-9)src/xmldsig-signer.ts (1)
XmlDSigSigner(213-374)src/utils.ts (1)
pemToDer(163-175)
test/xmldsig-validator.spec.ts (2)
src/xmldsig-signer.ts (1)
createSignedXml(342-373)src/xmldsig-validator.ts (2)
createSignedXml(157-189)XmlDSigValidator(89-212)
example/xmldsig-validator-example.js (3)
example/xmldsig-signer-example.js (8)
result(76-76)result(119-119)result(156-156)result(221-221)result(307-307)result(337-337)result1(367-367)result2(379-379)src/index.ts (2)
XmlDSigValidator(8-8)Algorithms(9-9)src/xmldsig-validator.ts (1)
XmlDSigValidator(89-212)
test/xmldsig-signer.spec.ts (1)
src/xmldsig-signer.ts (3)
XmlDSigSigner(213-374)SigningReference(146-146)ReferenceAttributes(41-48)
src/xmldsig-validator.ts (2)
src/index.ts (2)
XmlDSigValidator(8-8)SignedXml(6-6)src/signed-xml.ts (1)
SignedXml(30-1523)
src/xmldsig-signer.ts (3)
src/types.ts (4)
ComputeSignatureOptionsLocation(83-86)SignatureAlgorithmType(19-19)CanonicalizationAlgorithmType(13-13)ComputeSignatureOptions(100-105)src/utils.ts (1)
buildIdXPathWithNamespaces(407-427)src/signed-xml.ts (1)
getKeyInfoContent(203-232)
🔇 Additional comments (2)
src/xmldsig-signer.ts (1)
284-289: Good guard: reject URI inside attributesThe early reject prevents ambiguous configuration and keeps URI source single-sourced.
example/xmldsig-validator-example.js (1)
3-9: Import pattern is acceptable for development examples.The try-catch fallback from local build to published package is a reasonable approach for development and testing, while maintaining end-user perspective through the fallback.
shunkica
changed the title
shunkica
force-pushed
the
signer-validator-separation
branch
from
ca57699 to
414ec7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/signed-xml.ts (1)
507-517: Harden against XPath injection in validateElementAgainstReferencesUnlike
validateReference(Lines 551–554), this path does not reject URIs containing quotes before embedding into XPath. Add the same guard here.Apply:
- const uri = ref.uri?.[0] === "#" ? ref.uri.substring(1) : ref.uri; + const uri = ref.uri?.[0] === "#" ? ref.uri.substring(1) : ref.uri; + if (uri?.includes("'")) { + throw new Error("Cannot validate a uri with quotes inside it"); + }
♻️ Duplicate comments (8)
src/signed-xml.ts (1)
511-523: Bug: namespace-uri predicate used when attribute has no namespaceWhen
attr.namespaceURIis falsy, XPath becomesnamespace-uri(.)='undefined'. Mirror the conditional logic used invalidateReference.Apply:
- ref.xpath = `//*[@*[local-name(.)='${attr.localName}' and namespace-uri(.)='${attr.namespaceURI}']='${uri}']`; + if (attr.namespaceURI) { + ref.xpath = `//*[@*[local-name(.)='${attr.localName}' and namespace-uri(.)='${attr.namespaceURI}']='${uri}']`; + } else { + ref.xpath = `//*[@*[local-name(.)='${attr.localName}']='${uri}']`; + }test/xmldsig-validator.spec.ts (2)
360-387: Test title claims XmlSigner but uses SignedXmlRename the title or use the wrapper to avoid confusion.
Suggestion:
-it("should validate signatures created by XmlSigner", function () { +it("should validate signatures created by SignedXml (XmlDSigSigner compatibility)", function () {
279-290: “maxTransforms” test doesn’t exercise or assert the limitIt signs with a single transform and no enforcement exists; the test will always pass.
Update the test to exceed the cap and expect failure, and add enforcement in the validator (see comment in src/xmldsig-validator.ts).
Example change here:
- const signedXml = createSignedXml(xml); + // Create a signature with two transforms + const sig = new SignedXml({ + privateKey, + canonicalizationAlgorithm: canonicalization.EXCLUSIVE_C14N, + signatureAlgorithm: signature.RSA_SHA1, + }); + sig.addReference({ + xpath: "//*[local-name(.)='test']", + digestAlgorithm: hash.SHA1, + transforms: [ + Algorithms.transform.ENVELOPED_SIGNATURE, + Algorithms.canonicalization.EXCLUSIVE_C14N, + ], + }); + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); @@ - expect(result.valid).to.be.true; + expect(result.valid).to.be.false; + expect(result.error).to.match(/transform/i);src/xmldsig-validator.ts (3)
53-55: Allow PEM string/Buffer for publicCertTests and examples pass PEM strings; typing only
KeyLikeis too narrow.-export type PublicCertXmlValidatorOptions = CommonValidatorOptions & { - publicCert: KeyLike; -}; +export type PublicCertXmlValidatorOptions = CommonValidatorOptions & { + publicCert: KeyLike | string | Buffer; +};
138-146: Enforce maxTransforms before cryptographic verificationDefensive check: reject if any Reference has more transforms than allowed.
- // Perform cryptographic validation + // Enforce maxTransforms limit (defense-in-depth) + if (this.options.maxTransforms != null) { + const over = signedXml + .getReferences?.() + ?.find((r: any) => Array.isArray(r.transforms) && r.transforms.length > this.options.maxTransforms!); + if (over) { + return this.handleError( + `Reference contains ${over.transforms.length} transforms (limit=${this.options.maxTransforms}).`, + ); + } + } + // Perform cryptographic validation const isValid = signedXml.checkSignature(xml);
159-163: Fix class name in error messageUse the correct class name to avoid confusion.
- throw new Error( - "XmlValidator requires either a publicCert or getCertFromKeyInfo function in options.", - ); + throw new Error( + "XmlDSigValidator requires either a publicCert or getCertFromKeyInfo function in options.", + );example/xmldsig-validator-example.js (1)
209-213: DOMParser.parseFromString missing mimeTypePass the content type to avoid runtime issues.
- const doc = new DOMParser().parseFromString(signedTwice); + const doc = new DOMParser().parseFromString(signedTwice, "application/xml");src/xmldsig-signer.ts (1)
256-276: Widen privateKey type to accept common formats.The
privateKeyparameter currently only acceptscrypto.KeyLike, but should also acceptstring | Bufferto match common usage patterns with PEM-encoded keys.
🧹 Nitpick comments (7)
src/types.ts (2)
13-19: Keep algorithm type safety while allowing extensibilitySwitching to plain
stringremoves useful checking. Prefer an open union: known literals OR branded string.Example pattern:
-export type CanonicalizationAlgorithmType = string; +export type KnownCanonicalizationAlgorithm = + | "http://www.w3.org/TR/2001/REC-xml-c14n-20010315" + | "http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments" + | "http://www.w3.org/2001/10/xml-exc-c14n#" + | "http://www.w3.org/2001/10/xml-exc-c14n#WithComments"; +export type CanonicalizationAlgorithmType = KnownCanonicalizationAlgorithm | (string & {});Apply similarly to CanonicalizationOrTransformAlgorithmType, HashAlgorithmType, SignatureAlgorithmType. This keeps autocomplete and catches typos while still permitting custom algorithms.
51-53: Doc the new QName options in JSDoc for discoverabilityAdd brief JSDoc to
idAttributeQNamesandnamespaceMapdescribing precedence overidAttributes/idModeand expected QName format (e.g.,["wsu:Id", "Id"]).src/algorithms.ts (1)
1-44: Algorithm registry looks good; consider exporting group types for DXThe constants/URIs are correct. Optionally export per-group types to help consumers type options precisely.
Example:
export const Algorithms = { canonicalization: CANONICALIZATION_ALGORITHMS, transform: TRANSFORM_ALGORITHMS, hash: HASH_ALGORITHMS, signature: SIGNATURE_ALGORITHMS, }; + +export type CanonicalizationAlgorithm = + typeof CANONICALIZATION_ALGORITHMS[keyof typeof CANONICALIZATION_ALGORITHMS]; +export type TransformAlgorithm = + typeof TRANSFORM_ALGORITHMS[keyof typeof TRANSFORM_ALGORITHMS]; +export type HashAlgorithm = + typeof HASH_ALGORITHMS[keyof typeof HASH_ALGORITHMS]; +export type SignatureAlgorithm = + typeof SIGNATURE_ALGORITHMS[keyof typeof SIGNATURE_ALGORITHMS];src/signed-xml.ts (1)
1375-1392: Avoid redundant xmlns re-declaration when setting QName IDsBefore calling
setAttributeNSforxmlns:${prefix}, check if the prefix is already bound on the element or ancestors to prevent duplicate namespace attributes.Example:
- if (namespace) { - node.setAttributeNS("http://www.w3.org/2000/xmlns/", `xmlns:${prefix}`, namespace); - node.setAttributeNS(namespace, qName, id); + if (namespace) { + const current = node.lookupNamespaceURI(prefix); + if (current !== namespace) { + node.setAttributeNS("http://www.w3.org/2000/xmlns/", `xmlns:${prefix}`, namespace); + } + node.setAttributeNS(namespace, qName, id);test/xmldsig-signer.spec.ts (1)
540-571: Prefer Algorithms constants over raw URIs in testsUsing
Algorithmsavoids string drift and improves readability.Example:
-const algorithms = [ - "http://www.w3.org/2000/09/xmldsig#rsa-sha1", - "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", - "http://www.w3.org/2001/04/xmldsig-more#rsa-sha512", -]; +import { Algorithms } from "../src"; +const algorithms = [ + Algorithms.signature.RSA_SHA1, + Algorithms.signature.RSA_SHA256, + Algorithms.signature.RSA_SHA512, +];src/xmldsig-validator.ts (2)
183-186: Don’t assign QName values into idAttributesWhen
idAttributeQNamesis set, SignedXml already uses it; assigning colonized QNames intoidAttributescan be misleading. Only setidAttributesif all names are non‑prefixed.- if (this.options.idAttributeQNames) { - signedXml.idAttributes = this.options.idAttributeQNames; - } + if (this.options.idAttributeQNames?.every((n) => !n.includes(":"))) { + signedXml.idAttributes = this.options.idAttributeQNames; + }
12-47: Optional: add WS‑Security convenience flagMany users expect a simple switch. Offer
enableWSSecurityMode?: booleanto passidMode: "wssecurity"through to SignedXml automatically (or document usingidAttributeQNames: ["wsu:Id"]withnamespaceMap).Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks).nvmrc(1 hunks)example/xmldsig-signer-example.js(1 hunks)example/xmldsig-validator-example.js(1 hunks)src/algorithms.ts(1 hunks)src/index.ts(1 hunks)src/signed-xml.ts(7 hunks)src/types.ts(2 hunks)src/utils.ts(2 hunks)src/xmldsig-signer.ts(1 hunks)src/xmldsig-validator.ts(1 hunks)test/xmldsig-signer.spec.ts(1 hunks)test/xmldsig-validator.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/index.ts
- .gitignore
- src/utils.ts
- example/xmldsig-signer-example.js
- .nvmrc
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
src/xmldsig-signer.tssrc/signed-xml.tssrc/xmldsig-validator.tstest/xmldsig-validator.spec.ts
🧬 Code graph analysis (6)
src/algorithms.ts (1)
src/index.ts (1)
Algorithms(9-9)
src/xmldsig-signer.ts (3)
src/types.ts (4)
ComputeSignatureOptionsLocation(83-86)SignatureAlgorithmType(19-19)CanonicalizationAlgorithmType(13-13)ComputeSignatureOptions(100-105)src/utils.ts (1)
buildIdXPathWithNamespaces(407-427)src/signed-xml.ts (1)
getKeyInfoContent(203-232)
src/xmldsig-validator.ts (2)
src/index.ts (2)
XmlDSigValidator(8-8)SignedXml(6-6)src/signed-xml.ts (1)
SignedXml(30-1494)
example/xmldsig-validator-example.js (3)
example/xmldsig-signer-example.js (8)
result(76-76)result(119-119)result(156-156)result(221-221)result(307-307)result(337-337)result1(367-367)result2(379-379)src/index.ts (2)
XmlDSigValidator(8-8)Algorithms(9-9)src/xmldsig-validator.ts (1)
XmlDSigValidator(89-212)
test/xmldsig-signer.spec.ts (1)
src/xmldsig-signer.ts (3)
XmlDSigSigner(213-374)SigningReference(146-146)ReferenceAttributes(41-48)
test/xmldsig-validator.spec.ts (2)
src/xmldsig-signer.ts (1)
createSignedXml(342-373)src/xmldsig-validator.ts (2)
createSignedXml(157-189)XmlDSigValidator(89-212)
🔇 Additional comments (7)
src/xmldsig-signer.ts (7)
1-11: LGTM: Clean imports and sensible defaults.The imports are appropriate and the default ID attribute QNames cover the most common XML ID conventions.
13-207: LGTM: Well-documented type definitions.The type definitions are comprehensive and well-documented with XMLDSig spec references. The discriminated union for
SigningReference(xpath vs uri) provides good type safety.
222-246: LGTM: Solid constructor validation and normalization.The required field validation and default ID attribute handling are well-implemented. Preparing
computeOptionsonce for reuse is a good performance optimization.
284-288: LGTM: Good validation to prevent misuse.Validating that URI is not in attributes prevents confusion about where URI should be specified in the reference configuration.
291-302: LGTM: XPath reference handling is correct.The conversion from
SigningReferenceto SignedXml format correctly handles XPath-based references with appropriate defaults.
303-340: LGTM: Comprehensive URI reference handling with good security.The URI-based reference handling correctly supports:
- Enveloped signatures (empty URI)
- Fragment identifiers with namespace-aware ID lookup
- Security: External URIs are appropriately rejected since signing can only operate on document content
342-373: LGTM: Proper SignedXml instantiation with filtered attributes.The
createSignedXmlmethod correctly:
- Handles KeyInfo configuration with content generation
- Filters undefined values from attributes (preventing empty attributes)
- Passes through all necessary options including namespace-aware ID handling
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
test/xmldsig-validator.spec.ts (1)
679-708: Rename test title or switch to XmlDSigSignerThe test body uses SignedXml, but the title says XmlSigner. Rename for accuracy or update to exercise the new XmlDSigSigner.
- it("should validate signatures created by XmlSigner", function () { + it("should validate signatures created by SignedXml (XmlDSigSigner compatibility)", function () {src/xmldsig-validator.ts (4)
199-204: Defense-in-depth: enforce maxTransforms after loading the signatureSignedXml enforces during load, but validating here guards against future regressions and alternate code paths.
- // Perform cryptographic validation + // Enforce maxTransforms before cryptographic validation (defense-in-depth) + if (this.options.security.maxTransforms != null) { + const over = (this.signedXml as any) + .getReferences?.() + ?.find((r: any) => Array.isArray(r.transforms) && r.transforms.length > this.options.security.maxTransforms!); + if (over) { + return this.handleError( + `Reference contains ${over.transforms.length} transforms (limit=${this.options.security.maxTransforms}).`, + ); + } + } + // Perform cryptographic validation const isValid = this.signedXml.checkSignature(xml);
214-233: Optional: add WS‑Security mode flag to simplify configurationAdd enableWSSecurityMode?: boolean and pass idMode: "wssecurity" to SignedXml when true, so users don’t have to wire idAttributeQNames/namespaceMap manually.
@@ -interface XmlDSigValidatorOptions { +interface XmlDSigValidatorOptions { + /** Treat only wsu:Id as ID per WS‑Security profile */ + enableWSSecurityMode?: boolean; @@ - const signedXmlOptions = { + const signedXmlOptions = { publicCert: undefined as KeyLike | undefined, getCertFromKeyInfo: undefined as KeySelectorFunction | undefined, idAttributeQNames: this.options.idAttributeQNames, namespaceMap: this.options.namespaceMap, maxTransforms: this.options.security.maxTransforms, implicitTransforms: this.options.implicitTransforms, + idMode: this.options.enableWSSecurityMode ? "wssecurity" : undefined, };
123-125: Error message should use the correct class nameUse XmlDSigValidator for clarity.
- throw new Error("XmlValidator requires a keySelector in options."); + throw new Error("XmlDSigValidator requires a keySelector in options.");
220-222: Error message should use the correct class nameSame naming fix here.
- throw new Error( - "XmlValidator requires either a publicCert or getCertFromKeyInfo function in options.", - ); + throw new Error( + "XmlDSigValidator requires either a publicCert or getCertFromKeyInfo function in options.", + );src/signed-xml.ts (1)
514-525: Fix QName XPath when attribute has no namespaceAvoid generating namespace-uri(.)='undefined' for non-namespaced attrs. Mirror the conditional used in validateReference.
- if (uri === elemId) { - ref.xpath = `//*[@*[local-name(.)='${attr.localName}' and namespace-uri(.)='${attr.namespaceURI}']='${uri}']`; - break; // found the correct element, no need to check further - } + if (uri === elemId) { + if (attr.namespaceURI) { + ref.xpath = `//*[@*[local-name(.)='${attr.localName}' and namespace-uri(.)='${attr.namespaceURI}']='${uri}']`; + } else { + ref.xpath = `//*[@*[local-name(.)='${attr.localName}']='${uri}']`; + } + break; // found the correct element, no need to check further + }example/xmldsig-validator-example.js (1)
231-234: Missing required mimeType in DOMParser.parseFromStringPass "application/xml" (or "text/xml") as the second argument to avoid runtime errors.
- const doc = new DOMParser().parseFromString(signedTwice); + const doc = new DOMParser().parseFromString(signedTwice, "application/xml");
🧹 Nitpick comments (7)
test/xmldsig-validator.spec.ts (1)
433-447: Add a failure-path test for maxTransforms to actually exercise the limitCurrently only the success case with 1 transform is covered. Add a test that signs with ≥2 transforms and asserts validation fails when maxTransforms: 1.
@@ it("should validate with maxTransforms limit", function () { @@ expect(result.valid).to.be.true; }); + + it("should reject when Reference has more transforms than maxTransforms", function () { + const xml = "<root><test>content</test></root>"; + const sig = new SignedXml({ + privateKey, + canonicalizationAlgorithm: canonicalization.EXCLUSIVE_C14N, + signatureAlgorithm: signature.RSA_SHA1, + }); + sig.addReference({ + xpath: "//*[local-name(.)='test']", + digestAlgorithm: hash.SHA1, + transforms: [ + Algorithms.transform.ENVELOPED_SIGNATURE, + canonicalization.EXCLUSIVE_C14N, + ], + }); + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); + + const validator = new XmlDSigValidator({ + keySelector: { publicCert }, + security: { maxTransforms: 1 }, + throwOnError: false, + }); + const result = validator.validate(signedXml); + expect(result.valid).to.be.false; + expect(result.error).to.match(/exceeds the maximum/i); + });src/types.ts (3)
126-129: Type accuracy: digestValue should be a string (base64)Unknown weakens safety and downstream messaging. Prefer string.
- // Optional. The digest value of the referenced data. - digestValue?: unknown; + // Optional. The digest value of the referenced data (base64). + digestValue?: string;
13-20: Optional: restore stricter algorithm typings via literal unionsUsing plain strings degrades DX. Consider exporting algorithm literal unions (or deriving from Algorithms registry) to catch typos at compile time.
113-149: Future: split Reference into SigningReference and ValidationReferenceDifferent fields are meaningful per phase (e.g., xpath/id/type vs uri/digestValue/validationError). Splitting clarifies contracts and avoids accidental misuse. Based on learnings.
src/xmldsig-validator.ts (2)
23-44: Naming consistency: XmlDSig vs XmlDsigConsider renaming XmlDsigValidatorSecurityOptions to XmlDSigValidatorSecurityOptions for consistency with the class name.
138-149: Minor: simplify truststore guardA direct nullish coalescing/array check improves readability.
- if ("truststore" in this.options.security && this.options.security.truststore !== undefined) { + if (Array.isArray(this.options.security.truststore)) {example/xmldsig-validator-example.js (1)
1-332: Add mimeType parameter to all parseFromString() callsYour scan confirms 50+ instances of
parseFromString()calls missing themimeTypeparameter across test files, source files, and examples. The call at line 233 inexample/xmldsig-validator-example.jsis one of many that should specify"text/xml"as the second argument for consistency and proper document parsing:const doc = new DOMParser().parseFromString(signedTwice, "text/xml");This should be applied uniformly across:
test/**/*.spec.ts(~30 instances)src/signed-xml.ts(~4 instances)example/xmldsig-validator-example.js(line 233)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
test/static/chain_client.crt.pemis excluded by!**/*.pemtest/static/chain_client.key.pemis excluded by!**/*.pemtest/static/chain_root.crt.pemis excluded by!**/*.pemtest/static/chain_root.key.pemis excluded by!**/*.pemtest/static/expired_certificate.crt.pemis excluded by!**/*.pemtest/static/expired_certificate.key.pemis excluded by!**/*.pem
📒 Files selected for processing (6)
example/xmldsig-validator-example.js(1 hunks)src/signed-xml.ts(10 hunks)src/types.ts(2 hunks)src/xmldsig-validator.ts(1 hunks)test/static/chain_root.crt.srl(1 hunks)test/xmldsig-validator.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/static/chain_root.crt.srl
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
src/signed-xml.tssrc/xmldsig-validator.tstest/xmldsig-validator.spec.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/signed-xml.ts
🧬 Code graph analysis (4)
example/xmldsig-validator-example.js (3)
example/xmldsig-signer-example.js (9)
fs(1-1)result(76-76)result(119-119)result(156-156)result(221-221)result(307-307)result(337-337)result1(367-367)result2(379-379)src/xmldsig-validator.ts (1)
XmlDSigValidator(112-305)src/index.ts (2)
XmlDSigValidator(8-8)Algorithms(9-9)
src/signed-xml.ts (2)
example/xmldsig-validator-example.js (2)
xpath(232-232)doc(233-233)example/example.js (1)
doc(17-17)
src/xmldsig-validator.ts (3)
src/index.ts (2)
XmlDSigValidator(8-8)SignedXml(6-6)src/signed-xml.ts (2)
SignedXml(30-1505)getCertFromKeyInfo(244-253)src/types.ts (1)
KeySelectorFunction(45-45)
test/xmldsig-validator.spec.ts (2)
src/xmldsig-validator.ts (2)
createSignedXml(214-282)XmlDSigValidator(112-305)src/signed-xml.ts (1)
SignedXml(30-1505)
🔇 Additional comments (1)
src/signed-xml.ts (1)
1198-1201: Retaining ref.uri mutation during signing is OK for nowSetting ref.uri = id aligns with existing behavior and the maintainer’s request to defer changes.
shunkica
force-pushed
the
signer-validator-separation
branch
2 times, most recently
from
7c419b5 to
8fd99f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/xmldsig-validator.ts (1)
207-216: Incorrect class name in error message.The error message references "XmlValidator" but the class is named "XmlDSigValidator". Update for consistency.
Apply this diff:
throw new Error( - "XmlValidator requires either a publicCert or getCertFromKeyInfo function in options.", + "XmlDSigValidator requires either a publicCert or getCertFromKeyInfo function in options.", );src/signed-xml.ts (1)
807-813: maxTransforms check needs null-safe comparison.The truthy check
if (this.maxTransforms)will skip enforcement whenmaxTransformsis set to0(a valid limit). A past review comment indicated this was addressed, but the code still uses the truthy check.Apply this diff to fix:
- if (this.maxTransforms) { - if (transforms.length > this.maxTransforms) { + if (this.maxTransforms != null) { + if (transforms.length > this.maxTransforms) { throw new Error( `Number of transforms (${transforms.length}) exceeds the maximum allowed (${this.maxTransforms})`, ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
test/static/chain_client.crt.pemis excluded by!**/*.pemtest/static/chain_client.key.pemis excluded by!**/*.pemtest/static/chain_root.crt.pemis excluded by!**/*.pemtest/static/chain_root.key.pemis excluded by!**/*.pemtest/static/expired_certificate.crt.pemis excluded by!**/*.pemtest/static/expired_certificate.key.pemis excluded by!**/*.pem
📒 Files selected for processing (12)
.gitignore(1 hunks).nvmrc(1 hunks)src/algorithms.ts(1 hunks)src/index.ts(1 hunks)src/signed-xml.ts(11 hunks)src/types.ts(2 hunks)src/utils.ts(3 hunks)src/xmldsig-signer.ts(1 hunks)src/xmldsig-validator.ts(1 hunks)test/static/chain_root.crt.srl(1 hunks)test/xmldsig-signer.spec.ts(1 hunks)test/xmldsig-validator.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/algorithms.ts
- .nvmrc
- test/static/chain_root.crt.srl
- src/index.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
src/signed-xml.tssrc/xmldsig-signer.tssrc/xmldsig-validator.tstest/xmldsig-validator.spec.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/signed-xml.ts
🧬 Code graph analysis (6)
src/utils.ts (1)
src/types.ts (1)
IdAttributeType(13-13)
src/signed-xml.ts (1)
src/types.ts (1)
IdAttributeType(13-13)
src/xmldsig-signer.ts (3)
src/types.ts (6)
ObjectAttributes(36-45)IdAttributeType(13-13)ComputeSignatureOptionsLocation(87-90)SignatureAlgorithmType(21-21)CanonicalizationAlgorithmType(15-15)ComputeSignatureOptions(104-109)src/signed-xml.ts (2)
SignedXml(31-1476)getKeyInfoContent(202-231)src/utils.ts (1)
buildIdXPathWithNamespaces(407-432)
test/xmldsig-signer.spec.ts (2)
src/xmldsig-signer.ts (3)
XmlDSigSigner(207-363)SigningReference(147-147)ReferenceAttributes(42-49)src/signed-xml.ts (1)
SignedXml(31-1476)
src/xmldsig-validator.ts (2)
src/types.ts (2)
IdAttributeType(13-13)KeySelectorFunction(47-47)src/signed-xml.ts (2)
SignedXml(31-1476)getCertFromKeyInfo(240-249)
test/xmldsig-validator.spec.ts (3)
src/algorithms.ts (1)
Algorithms(39-44)src/xmldsig-validator.ts (2)
createSignedXml(207-274)XmlDSigValidator(105-297)src/signed-xml.ts (1)
SignedXml(31-1476)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/signed-xml.ts (3)
171-176: Regression: default key extraction disabled.Constructor overwrites getCertFromKeyInfo with noop when no override is supplied, breaking KeyInfo-based validation. Restore the static default.
- this.getKeyInfoContent = getKeyInfoContent ?? this.getKeyInfoContent; - this.getCertFromKeyInfo = getCertFromKeyInfo ?? SignedXml.noop; + this.getKeyInfoContent = getKeyInfoContent ?? this.getKeyInfoContent; + // Preserve the class default (SignedXml.getCertFromKeyInfo) unless explicitly overridden. + this.getCertFromKeyInfo = getCertFromKeyInfo ?? this.getCertFromKeyInfo;
547-576: False positive “multiple elements with same ID” when the same element matches multiple idAttributes.The code sums matches across different idAttributes and throws when total > 1, even if all matches point to the same element (e.g., both Id and ID present on the same node). Track distinct element identity instead.
- let num_elements_for_id = 0; + let matchedElem: Element | null = null; for (const idAttr of this.idAttributes) { let tmp_elemXpath: string; if (typeof idAttr === "string") { tmp_elemXpath = `//*[@*[local-name(.)='${idAttr}']='${uri}']`; } else { if (idAttr.namespaceUri) { tmp_elemXpath = `//*[@*[local-name(.)='${idAttr.localName}' and namespace-uri(.)='${idAttr.namespaceUri}']='${uri}']`; } else { tmp_elemXpath = `//*[@*[local-name(.)='${idAttr.localName}']='${uri}']`; } } - const tmp_elem = xpath.select(tmp_elemXpath, doc); - if (utils.isArrayHasLength(tmp_elem)) { - num_elements_for_id += tmp_elem.length; - if (num_elements_for_id > 1) { - throw new Error( - "Cannot validate a document which contains multiple elements with the " + - "same value for the ID / Id / Id attributes, in order to prevent " + - "signature wrapping attack.", - ); - } - elem = tmp_elem[0]; - ref.xpath = tmp_elemXpath; - } + const tmp_elem = xpath.select(tmp_elemXpath, doc); + if (utils.isArrayHasLength(tmp_elem)) { + const candidate = tmp_elem[0] as Element; + if (matchedElem && candidate !== matchedElem) { + throw new Error( + "Cannot validate a document which contains multiple distinct elements with the same ID value (prevents signature wrapping).", + ); + } + matchedElem = candidate; + elem = candidate; + ref.xpath = tmp_elemXpath; + } }
566-571: Typo/clarity in error message.Use consistent casing for attribute names.
- "same value for the ID / Id / Id attributes, in order to prevent " + + "same value for the ID / Id / id attributes, in order to prevent " +
♻️ Duplicate comments (1)
test/xmldsig-validator.spec.ts (1)
683-711: Exercise XmlDSigSigner once it’s ready.Title references XmlDSigSigner but the body simulates with SignedXml. Fine for now; please switch to XmlDSigSigner in this test when available to prevent drift.
🧹 Nitpick comments (2)
test/xmldsig-validator.spec.ts (1)
23-41: Add SHA‑256 coverage alongside SHA‑1.Current helpers/signatures use SHA‑1. Please add at least one test path that signs with SHA‑256 (hash.SHA256 + signature.RSA_SHA256) to avoid normalizing legacy algorithms and to exercise stronger defaults. Keep existing SHA‑1 tests for backward compatibility.
Also applies to: 44-74
src/xmldsig-validator.ts (1)
218-274: Optional: WS‑Security convenience flag.You already accept idAttributes; consider adding an enableWSSecurityMode boolean that passes idMode: "wssecurity" to SignedXml to ease WS‑Security usage without requiring callers to craft QName arrays. Low-risk additive API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/signed-xml.ts(11 hunks)src/xmldsig-validator.ts(1 hunks)test/xmldsig-validator.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
test/xmldsig-validator.spec.tssrc/xmldsig-validator.tssrc/signed-xml.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/signed-xml.ts
🧬 Code graph analysis (3)
test/xmldsig-validator.spec.ts (3)
src/algorithms.ts (1)
Algorithms(39-44)src/xmldsig-validator.ts (2)
createSignedXml(207-274)XmlDSigValidator(105-297)src/signed-xml.ts (1)
SignedXml(31-1476)
src/xmldsig-validator.ts (2)
src/types.ts (2)
IdAttributeType(13-13)KeySelectorFunction(47-47)src/signed-xml.ts (2)
SignedXml(31-1476)getCertFromKeyInfo(240-249)
src/signed-xml.ts (1)
src/types.ts (1)
IdAttributeType(13-13)
🔇 Additional comments (1)
src/xmldsig-validator.ts (1)
106-145: Remove this review comment; the proposed change contradicts the documented API contract and claims are unsubstantiated.The constructor docstring explicitly states "The instance can be reused for multiple validations" (line 111). The underlying
SignedXml.loadSignature()clears state viathis.references = [](line 696 in signed-xml.ts), preventing cross-talk. JavaScript is single-threaded—no concurrency model exists to "break." The review's claimed "design goal of new internal SignedXml per call" is not documented in the codebase and contradicts the existing API contract.While there is a minor architectural inconsistency (XmlDSigValidator uses shared state; XmlDSigSigner uses per-call), this is a separate optional refactoring concern, not a bug or violation. Current tests pass; no state bleed occurs.
Likely an incorrect or invalid review comment.
shunkica
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/c14n-canonicalization.ts (1)
87-94: Blocker: incorrect type for defaultNsForPrefix (causes TS error at usages)renderNs declares defaultNsForPrefix as string but uses it as a map (e.g., defaultNsForPrefix[node.prefix]). Fix the signature to Record<string, string>.
Apply:
- renderNs( - node: Element, - prefixesInScope: string[], - defaultNs: string, - defaultNsForPrefix: string, - ancestorNamespaces: NamespacePrefix[], - ): RenderedNamespace { + renderNs( + node: Element, + prefixesInScope: string[], + defaultNs: string, + defaultNsForPrefix: Record<string, string>, + ancestorNamespaces: NamespacePrefix[], + ): RenderedNamespace {
🧹 Nitpick comments (11)
src/constants.ts (2)
14-17: Clarify/decouple transform vs canonicalization constantsTRANSFORM_ALGORITHMS spreads canonicalization URIs and adds ENVELOPED_SIGNATURE. This is pragmatic, but it blurs categories. Consider documenting this explicitly here, or (preferable next) separating transform-only names and mapping canonicalization via a dedicated lookup to reduce confusion in consumers.
39-44: Harden runtime immutability for security‑critical constantsUse Object.freeze on both the top-level and nested objects to prevent accidental mutation at runtime (as const is compile-time only).
Apply pattern like:
-export const Algorithms = { - canonicalization: CANONICALIZATION_ALGORITHMS, - transform: TRANSFORM_ALGORITHMS, - hash: HASH_ALGORITHMS, - signature: SIGNATURE_ALGORITHMS, -}; +export const Algorithms = Object.freeze({ + canonicalization: Object.freeze(CANONICALIZATION_ALGORITHMS), + transform: Object.freeze(TRANSFORM_ALGORITHMS), + hash: Object.freeze(HASH_ALGORITHMS), + signature: Object.freeze(SIGNATURE_ALGORITHMS), +} as const); -export const Namespaces = NAMESPACES; +export const Namespaces = Object.freeze(NAMESPACES);Also applies to: 46-56
src/types.ts (4)
10-10: Avoid runtime import for type queries to reduce couplingUsing a value import of Algorithms in a types module introduces an unnecessary runtime dependency. Switch to typeof import to keep this in the type space only.
Apply:
-import { Algorithms } from "./constants"; +type AlgorithmsConst = typeof import("./constants").Algorithms;Then replace occurrences of
typeof AlgorithmswithAlgorithmsConst, e.g.:-export type SignatureAlgorithmName = - | (typeof Algorithms.signature)[keyof typeof Algorithms.signature] +export type SignatureAlgorithmName = + | AlgorithmsConst["signature"][keyof AlgorithmsConst["signature"]] | string;Repeat similarly for hash/transform/canonicalization name types.
23-26: Narrow GetKeyInfoContentArgs.publicCert typegetKeyInfoContent only handles string/Buffer; allowing crypto.KeyLike suggests KeyObject support which is not implemented. Narrow to match behavior.
-export interface GetKeyInfoContentArgs { - publicCert?: crypto.KeyLike; +export interface GetKeyInfoContentArgs { + publicCert?: string | Buffer;
129-147: Define precedence and deprecate duplicate optionsBoth idAttribute and idAttributes exist; unclear precedence if both are set. Mark idAttribute as deprecated and specify that idAttributes takes precedence to avoid ambiguity.
Add JSDoc:
export interface SignedXmlOptions { idMode?: "wssecurity"; - idAttribute?: IdAttributeType; + /** + * @deprecated Use `idAttributes` instead. If both are provided, `idAttributes` takes precedence. + */ + idAttribute?: IdAttributeType; idAttributes?: IdAttributeType[];
194-196: Tighten digestValue typingDigest values are base64 strings in practice. Using unknown weakens consumer type safety for validation flows.
- digestValue?: unknown; + digestValue?: string;test/xmldsig-validator.spec.ts (3)
377-385: Exercise expiration check path explicitlyWith keySelector.publicCert, checkCertExpiration logic in getCertFromKeyInfo wrapper won’t run. Use getCertFromKeyInfo for the non‑expired case too to cover that branch.
-const validator = new XmlDSigValidator({ - keySelector: { publicCert }, - security: { checkCertExpiration: true }, -}); +const validator = new XmlDSigValidator({ + keySelector: { getCertFromKeyInfo: () => publicCert }, + security: { checkCertExpiration: true }, +});
291-292: Avoid brittle “fail” substring assertionsMatch a specific, stable error phrase to reduce test flakiness (e.g., “Id attribute namespace mismatch”).
-expectInvalidResult(validator.validate(signedXml), "fail"); +expectInvalidResult(validator.validate(signedXml), "Id attribute namespace mismatch");Apply to both assertions.
Also applies to: 314-315
25-43: Optional: include KeyInfo when using getCertFromKeyInfoTo better mimic real‑world flows, consider embedding X509Data via getKeyInfoContent in createSignedXml for tests that rely on keyInfo parsing; then have getCertFromKeyInfo read from the provided node.
Also applies to: 175-185
src/enveloped-signature.ts (1)
58-60: Return the namespaced constant, not a string literalUse Algorithms.transform.ENVELOPED_SIGNATURE for consistency with other algorithms.
- getAlgorithmName(): TransformAlgorithmName { - return "http://www.w3.org/2000/09/xmldsig#enveloped-signature"; - } + getAlgorithmName(): TransformAlgorithmName { + return Algorithms.transform.ENVELOPED_SIGNATURE; + }Add import at top:
import { Algorithms } from "./constants";src/signed-xml.ts (1)
1417-1436: Consider defensive check for empty prefix.The code correctly handles
IdAttributeTypevariants. However, iffirstIdAttrhas type{ prefix: string; localName: string; namespaceUri: string }with an empty string prefix (e.g.,prefix: ""), the checkfirstIdAttr.prefixwould be falsy and fall through to theelsebranch, usingsetAttributeinstead ofsetAttributeNS. This would not properly handle the namespace.While the type system doesn't prevent empty strings and this is likely an edge case that won't occur in practice, a defensive check could prevent subtle bugs.
Consider strengthening the check:
- if ("prefix" in firstIdAttr && firstIdAttr.prefix) { + if ("prefix" in firstIdAttr && firstIdAttr.prefix && firstIdAttr.prefix.length > 0) {Or use a more explicit length check:
- if ("prefix" in firstIdAttr && firstIdAttr.prefix) { + if ("prefix" in firstIdAttr && typeof firstIdAttr.prefix === "string" && firstIdAttr.prefix.length > 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/c14n-canonicalization.ts(4 hunks)src/constants.ts(1 hunks)src/enveloped-signature.ts(2 hunks)src/exclusive-canonicalization.ts(5 hunks)src/index.ts(1 hunks)src/signed-xml.ts(26 hunks)src/types.ts(3 hunks)src/xmldsig-validator.ts(1 hunks)test/xmldsig-validator.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- src/xmldsig-validator.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
test/xmldsig-validator.spec.tssrc/signed-xml.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/signed-xml.tssrc/types.ts
🧬 Code graph analysis (7)
src/constants.ts (1)
src/index.ts (1)
Algorithms(8-8)
src/exclusive-canonicalization.ts (2)
src/types.ts (3)
CanonicalizationAlgorithm(114-120)TransformAlgorithmOptions(50-56)CanonicalizationAlgorithmName(110-112)src/constants.ts (1)
Algorithms(39-44)
src/enveloped-signature.ts (1)
src/types.ts (3)
TransformAlgorithm(103-107)TransformAlgorithmOptions(50-56)TransformAlgorithmName(99-101)
src/c14n-canonicalization.ts (2)
src/types.ts (3)
CanonicalizationAlgorithm(114-120)TransformAlgorithmOptions(50-56)CanonicalizationAlgorithmName(110-112)src/constants.ts (1)
Algorithms(39-44)
test/xmldsig-validator.spec.ts (7)
src/constants.ts (1)
Algorithms(39-44)src/xmldsig-validator.ts (3)
createSignedXml(265-331)XmlDSigValidationResult(122-137)XmlDSigValidator(142-353)src/signed-xml.ts (1)
SignedXml(33-1526)src/signature-algorithms.ts (1)
RsaSha1(4-28)src/hash-algorithms.ts (1)
Sha1(4-15)src/enveloped-signature.ts (1)
EnvelopedSignature(10-61)src/exclusive-canonicalization.ts (1)
ExclusiveCanonicalization(22-327)
src/signed-xml.ts (2)
src/types.ts (10)
IdAttributeType(14-17)SignatureAlgorithmName(58-60)CanonicalizationAlgorithmName(110-112)TransformAlgorithmName(99-101)CanonicalizationAlgorithmMap(121-124)HashAlgorithmMap(97-97)SignatureAlgorithmMap(88-88)TransformAlgorithmMap(108-108)HashAlgorithmName(90-90)TransformAlgorithmOptions(50-56)src/constants.ts (2)
Namespaces(56-56)Algorithms(39-44)
src/types.ts (1)
src/constants.ts (1)
Algorithms(39-44)
🔇 Additional comments (14)
src/types.ts (1)
181-217: Good: kept unified Reference shape (split later)Retains the existing Reference fields while adopting algorithm name types. This aligns with the maintainer’s request to split Signing vs Validation refs in a follow-up, not here.
Based on learnings
src/exclusive-canonicalization.ts (1)
324-337: LGTM: returns via centralized Algorithms constantsConsistent with the new API surface; no behavior change.
src/signed-xml.ts (12)
1-31: LGTM! Type system imports align with algorithm map refactor.The new type imports (algorithm names, maps, and IdAttributeType) support the broader refactoring to typed algorithm maps, consistent with changes in src/types.ts and src/constants.ts.
34-46: LGTM! IdAttributeType[] enables flexible ID attribute specifications.The field type change from
string[]toIdAttributeType[]allows both simple string-based IDs and QName-based IDs with namespace support, with clear documentation and examples.
56-73: LGTM! Algorithm fields properly typed.Algorithm fields now use the typed name types (SignatureAlgorithmName, CanonicalizationAlgorithmName, TransformAlgorithmName), and
maxTransformscorrectly usesnumber | nullto distinguish between "no limit" (null) and "zero allowed" (0).
105-120: LGTM! Algorithm maps use new typed map types.The algorithm map fields now use the typed Map types, with
TransformAlgorithmsremaining optional for backward compatibility.
128-158: LGTM! Static default getters provide consistent algorithm defaults.The static getter methods provide sensible defaults for algorithm maps and ID attributes, using the Algorithms constants for type-safe keys. The TODO at line 134 correctly notes the future separation of transform and canonicalization algorithms.
164-214: LGTM! Constructor properly initializes new fields with sensible defaults.The constructor correctly initializes
idAttributes,maxTransforms, and the algorithm maps with sensible defaults from the static getters, providing a clean initialization path.
497-540: LGTM! Algorithm finder methods properly typed and backward-compatible.The finder methods now use typed algorithm names and maps. The new
findTransformAlgorithmincludes a backward-compatible fallback to canonicalization algorithms whenTransformAlgorithmsis undefined, with a TODO noting this will be removed in a future breaking change.
594-605: LGTM! XPath construction correctly handles undefined namespaceUri.The XPath construction here correctly checks if
namespaceUriis defined (line 600) before including it in the XPath predicate, properly handling all variants ofIdAttributeType.
857-863: LGTM! maxTransforms validation correctly handles 0.The null check (
this.maxTransforms !== null) correctly distinguishes betweennull(no limit) and0(no transforms allowed), ensuring the validation runs when maxTransforms is explicitly set to 0.
1245-1245: LGTM! Using findTransformAlgorithm for transform lookup.Correctly uses the new
findTransformAlgorithmmethod to get transform algorithm instances.
1446-1472: LGTM! createSignedInfo properly uses algorithm instances.The method correctly retrieves canonicalization and signature algorithm instances and uses
getAlgorithmName()to get their URIs. Line 1462 correctly uses the canonicalization algorithm URI as the xmlns for the InclusiveNamespaces element (defined in the exclusive canonicalization namespace).
666-666: LGTM! Proper use of Namespaces constant.Correctly uses
Namespaces.dsfor the XML Digital Signature namespace in XPath queries and attribute values.
|
@cjbarth I have removed the XmlDSigSigner wrapper because the scope just got too large for a single PR. |
|
I am also having second thoughts about the name and think that the proposal by @ahacker1-securesaml has some merit.
Consequentially |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/types.ts (1)
24-26: Align namespaceUri sentinel with our agreed contractDuring the earlier API split (see #506) the maintainer asked us to keep
{ namespaceUri: string | null }as the explicit “no namespace” sentinel. Switching this back toundefinedreintroduces the same ambiguity and forces downstream code to check bothundefinedandnull. Please revert to the nullable type:- | { localName: string; namespaceUri: string | undefined }; + | { localName: string; namespaceUri: string | null };Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/types.ts(4 hunks)src/xmldsig-verifier.ts(1 hunks)test/xmldsig-verifier.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
test/xmldsig-verifier.spec.tssrc/xmldsig-verifier.tssrc/types.ts
📚 Learning: 2025-10-22T21:03:38.354Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Applied to files:
test/xmldsig-verifier.spec.tssrc/xmldsig-verifier.tssrc/types.ts
📚 Learning: 2025-10-25T14:41:13.728Z
Learnt from: shunkica
PR: node-saml/xml-crypto#519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.728Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Applied to files:
test/xmldsig-verifier.spec.tssrc/types.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/xmldsig-verifier.tssrc/types.ts
📚 Learning: 2025-08-17T10:50:18.024Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1447-1451
Timestamp: 2025-08-17T10:50:18.024Z
Learning: In the xml-crypto codebase, ref.digestValue is only used during signature validation when loading references from existing signatures, not during signature creation in methods like createReferences or processSignatureReferences.
Applied to files:
src/types.ts
🧬 Code graph analysis (3)
test/xmldsig-verifier.spec.ts (5)
src/xmldsig-verifier.ts (2)
createSignedXml(149-213)XmlDSigVerifier(30-235)src/types.ts (1)
XmlDsigVerificationResult(410-412)src/signature-algorithms.ts (1)
RsaSha1(4-28)src/hash-algorithms.ts (1)
Sha1(4-15)src/enveloped-signature.ts (1)
EnvelopedSignature(10-61)
src/xmldsig-verifier.ts (3)
src/types.ts (7)
KeySelector(299-299)VerificationIdAttributeType(24-26)XmlDSigVerifierSecurityOptions(301-351)XmlDSigVerifierOptions(356-389)XmlDsigVerificationResult(410-412)SignedXmlOptions(138-157)KeySelectorFunction(53-53)src/signed-xml.ts (2)
SignedXml(41-1527)getCertFromKeyInfo(283-292)src/utils.ts (1)
isArrayHasLength(5-7)
src/types.ts (1)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(52-58)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/signed-xml.ts(25 hunks)src/types.ts(4 hunks)src/xmldsig-verifier.ts(1 hunks)test/xmldsig-verifier.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
test/xmldsig-verifier.spec.tssrc/xmldsig-verifier.tssrc/signed-xml.tssrc/types.ts
📚 Learning: 2025-10-22T21:03:38.354Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Applied to files:
test/xmldsig-verifier.spec.tssrc/xmldsig-verifier.tssrc/signed-xml.tssrc/types.ts
📚 Learning: 2025-10-25T14:41:13.728Z
Learnt from: shunkica
PR: node-saml/xml-crypto#519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.728Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Applied to files:
test/xmldsig-verifier.spec.tssrc/signed-xml.tssrc/types.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/xmldsig-verifier.tssrc/signed-xml.tssrc/types.ts
📚 Learning: 2025-10-22T20:36:00.758Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Applied to files:
src/signed-xml.ts
📚 Learning: 2025-08-17T10:50:18.024Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1447-1451
Timestamp: 2025-08-17T10:50:18.024Z
Learning: In the xml-crypto codebase, ref.digestValue is only used during signature validation when loading references from existing signatures, not during signature creation in methods like createReferences or processSignatureReferences.
Applied to files:
src/types.ts
🧬 Code graph analysis (4)
test/xmldsig-verifier.spec.ts (5)
src/xmldsig-verifier.ts (2)
createSignedXml(149-213)XmlDSigVerifier(30-235)src/types.ts (1)
XmlDsigVerificationResult(410-412)src/signature-algorithms.ts (1)
RsaSha1(4-28)src/hash-algorithms.ts (1)
Sha1(4-15)src/enveloped-signature.ts (1)
EnvelopedSignature(10-61)
src/xmldsig-verifier.ts (3)
src/types.ts (7)
KeySelector(299-299)VerificationIdAttributeType(24-26)XmlDSigVerifierSecurityOptions(301-351)XmlDSigVerifierOptions(356-389)XmlDsigVerificationResult(410-412)SignedXmlOptions(138-157)KeySelectorFunction(53-53)src/signed-xml.ts (2)
SignedXml(41-1534)getCertFromKeyInfo(283-292)src/utils.ts (1)
isArrayHasLength(5-7)
src/signed-xml.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(52-58)src/types.ts (11)
IdAttributeType(27-27)SignatureAlgorithmName(68-70)CanonicalizationAlgorithmName(120-122)TransformAlgorithmName(109-111)CanonicalizationAlgorithmMap(131-134)DigestAlgorithmMap(107-107)SignatureAlgorithmMap(98-98)TransformAlgorithmMap(118-118)VerificationIdAttributeType(24-26)HashAlgorithmName(100-100)TransformAlgorithmOptions(60-66)
src/types.ts (2)
src/index.ts (1)
XMLDSIG_URIS(8-8)src/xmldsig-uris.ts (1)
XMLDSIG_URIS(52-58)
🔇 Additional comments (8)
test/xmldsig-verifier.spec.ts (1)
1-527: Test coverage is comprehensive and well-structured.The test suite thoroughly exercises the XmlDSigVerifier API across all major paths: constructor validation, both key selector types, custom ID attributes, error handling modes, and all security options (maxTransforms, certificate expiration, truststore, algorithm allowlists). The helper functions and assertion utilities promote maintainability.
Note: The PR description mentions tests are "AI slop" and incomplete, but they appear production-ready. Consider updating the PR description if this assessment is outdated.
src/xmldsig-verifier.ts (1)
173-205: Certificate validation logic correctly checks expiration and trust.The wrapped
getCertFromKeyInfoproperly validates certificate dates usingX509Certificate.validToandvalidFrom, and verifies trust by comparing public keys or verifying signatures against the truststore. The error messages are clear and actionable.src/signed-xml.ts (3)
565-585: XPath construction correctly handles namespaced and non-namespaced ID attributes.The logic properly distinguishes three cases:
- String idAttributes: match any namespace (line 569)
- Object with namespaceUri: match specific namespace (line 575-577)
- Object with undefined namespaceUri: match no namespace (line 579-580)
This matches the validation logic in
validateReferenceand correctly resolves the namespace handling concerns from previous reviews.
873-879: maxTransforms check correctly handles 0 as a valid limit.Using
this.maxTransforms !== null(line 873) properly distinguishes null (unlimited) from 0 (no transforms allowed), fixing the falsy-check bug flagged in prior reviews.
1404-1448: IdAttributeType handling is correct but relies on property presence check.The method correctly iterates through
idAttributesto find existing IDs (lines 1405-1411) and sets new IDs using the first attribute's shape (lines 1426-1444). Line 1430's"prefix" in firstIdAttrcheck distinguishesSignatureIdAttributeTypefromVerificationIdAttributeTypeby property presence, which is a pragmatic approach for the union type. The resulting behavior is correct: prefixed IDs are set with namespace declarations, while unprefixed IDs usesetAttribute.src/types.ts (3)
21-27: IdAttributeType union properly separates signing and validation concerns.The three-type structure cleanly distinguishes:
SignatureIdAttributeType: includesprefixfor namespace-qualified ID generation during signingVerificationIdAttributeType: omitsprefixsince validation only cares about namespace URI matchingIdAttributeType: union allowing both in contexts where either may appearThis design aligns with the learnings that signing and validation have different ID attribute requirements.
68-134: Algorithm type system provides extensibility and type safety.The new design replaces rigid type aliases with name-based generics:
- Algorithm interfaces (
SignatureAlgorithm,HashAlgorithm,TransformAlgorithm,CanonicalizationAlgorithm) define consistent contracts withgetAlgorithmName()methods- Corresponding
*Maptypes enable runtime algorithm registration viaRecord<Name, new () => Algorithm>- Algorithm name types use
typeof XMLDSIG_URIS.*[keyof typeof ...]for compile-time URI validation while allowing custom stringsThis supports the PR's goal of separating concerns and enabling gradual migration.
287-412: XmlDSigVerifier types provide comprehensive verification configuration.The new types cover all security-critical verification aspects:
KeySelectorunion distinguishes direct certificate vs. KeyInfo extraction pathsXmlDSigVerifierSecurityOptionsexposes all security controls (max transforms, cert expiration, truststore, algorithm allowlists) with clear defaultsXmlDsigVerificationResultdiscriminated union enables type-safe success/failure handlingDocumentation is thorough with default value references and usage examples.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/types.ts (1)
287-299: Consider adding a discriminant field to KeySelector union.The
KeySelectorunion type lacks a discriminant property, so TypeScript cannot narrow the type automatically. Users must manually check for the presence ofpublicCertorgetCertFromKeyInfo.If you want type narrowing, consider adding a discriminant:
export type CertificateKeySelector = { + type: "certificate"; /** Public certificate or key to use for verification */ publicCert: KeyLike; }; export type KeyInfoKeySelector = { + type: "keyInfo"; /** Function to extract the public key from KeyInfo element */ getCertFromKeyInfo: (keyInfo?: Node | null) => string | null; };This allows users to write
if (keySelector.type === "certificate")for type-safe narrowing. However, if you prefer structural typing for flexibility, the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/types.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/types.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
src/types.ts
📚 Learning: 2025-10-22T21:03:38.354Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Applied to files:
src/types.ts
📚 Learning: 2025-10-22T20:36:00.758Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Applied to files:
src/types.ts
📚 Learning: 2025-08-17T10:50:18.024Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1447-1451
Timestamp: 2025-08-17T10:50:18.024Z
Learning: In the xml-crypto codebase, ref.digestValue is only used during signature validation when loading references from existing signatures, not during signature creation in methods like createReferences or processSignatureReferences.
Applied to files:
src/types.ts
📚 Learning: 2025-10-25T14:41:13.728Z
Learnt from: shunkica
PR: node-saml/xml-crypto#519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.728Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Applied to files:
src/types.ts
🧬 Code graph analysis (1)
src/types.ts (1)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(52-58)
🔇 Additional comments (7)
src/types.ts (7)
9-17: LGTM! Clean import structure.The imports and constant destructuring properly set up the type system for algorithm name literals derived from XMLDSIG_URIS.
53-66: LGTM! Well-defined utility types.The KeySelectorFunction, NamespacePrefix, and TransformAlgorithmOptions types are properly structured and provide clear contracts for the new algorithm abstraction layer.
68-134: LGTM! Strong type-safe algorithm abstraction.The algorithm name types leverage TypeScript's
typeof keyofpattern to extract URI literals, while the algorithm interfaces provide clean extension points. The decision to makeCanonicalizationAlgorithmextendTransformAlgorithmcorrectly models the XML-DSig specification hierarchy.
138-157: Well-structured options with new algorithm types.The SignedXmlOptions interface correctly adopts the new type-safe algorithm names and maps. The separation of
idAttribute(SignatureIdAttributeType) andidAttributes(VerificationIdAttributeType[]) properly distinguishes signing from validation concerns.
196-199: LGTM! Reference interface aligned with new algorithm types.The Reference interface correctly adopts
TransformAlgorithmNameandHashAlgorithmName, maintaining consistency with the new type-safe algorithm abstraction.
301-351: LGTM! Comprehensive security options.The XmlDSigVerifierSecurityOptions interface provides thorough security controls including transform limits, certificate expiration checks, truststore validation, and algorithm allowlists. The algorithm map types correctly use the specific Map types (SignatureAlgorithmMap, DigestAlgorithmMap, etc.).
410-412: LGTM! Clean discriminated union for verification results.The
XmlDsigVerificationResultuses a proper discriminated union pattern with thesuccessfield, allowing TypeScript to narrow the type and provide correct inference forerrorandsignedReferences.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils.ts (1)
49-60: UsesplitQNamehelper for consistency.Lines 57-58 inline a custom QName split that differs from the
splitQNameutility (lines 396-402). When there's no colon, this code usesprefix=""whilesplitQNamereturnsprefix=undefined. This inconsistency could lead to confusion.Apply this diff to use the existing helper:
export function findAttrQName( node: Element, qName: string, nsMap: Record<string, string> | undefined, ) { if (!nsMap) { return findAttr(node, qName); } - const [prefix, localName] = qName.includes(":") ? qName.split(":") : ["", qName]; - const namespace = nsMap[prefix] || undefined; + const [prefix, localName] = splitQName(qName); + const namespace = prefix ? nsMap[prefix] : undefined; return findAttr(node, localName, namespace); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/types.ts(4 hunks)src/utils.ts(3 hunks)test/utils-tests.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/types.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
src/types.ts
📚 Learning: 2025-10-22T21:03:38.354Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Applied to files:
src/types.ts
📚 Learning: 2025-10-22T20:36:00.758Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Applied to files:
src/types.ts
📚 Learning: 2025-10-25T14:41:13.728Z
Learnt from: shunkica
PR: node-saml/xml-crypto#519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.728Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Applied to files:
src/types.ts
📚 Learning: 2025-08-17T10:50:18.024Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1447-1451
Timestamp: 2025-08-17T10:50:18.024Z
Learning: In the xml-crypto codebase, ref.digestValue is only used during signature validation when loading references from existing signatures, not during signature creation in methods like createReferences or processSignatureReferences.
Applied to files:
src/types.ts
🧬 Code graph analysis (2)
src/utils.ts (1)
src/types.ts (1)
IdAttributeType(27-27)
src/types.ts (1)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(52-58)
🔇 Additional comments (14)
test/utils-tests.spec.ts (1)
84-138: LGTM! Comprehensive test coverage for namespace-aware attribute resolution.The test suite properly validates all key scenarios for
findAttr:
- Explicit null namespace matching non-namespaced attributes only
- Namespaced attributes excluded when null is passed
- Matching namespaced attributes by namespace URI
- Distinguishing between identically-named attributes in different namespaces
src/utils.ts (6)
9-29: LGTM! Proper three-state namespace matching.The functions correctly distinguish between:
null(explicit non-namespaced)undefined(namespace-agnostic)string(specific namespace)This aligns with the type system using
nullto explicitly match non-namespaced attributes.
31-47: LGTM! Updated signature supports explicit namespace control.The function signature correctly accepts
nullto explicitly match non-namespaced attributes, aligning with the type system changes.
363-394: LGTM! Correct XPath 1.0 literal handling.The function properly handles XPath 1.0's lack of quote escaping by:
- Using single quotes when no single quotes in value
- Using double quotes when no double quotes in value
- Using
concat()to interleave parts when both quote types presentThe edge cases (empty string, single quote only) are handled correctly.
396-402: LGTM! Proper QName splitting with defensive limit.The
split(":", 2)limit is good defensive programming, even though valid QNames shouldn't have multiple colons. Returnsundefinedfor prefix when unprefixed, which is semantically clear.
404-420: LGTM! Clear error handling for namespace resolution.The function properly validates the namespace map and prefix, throwing descriptive errors when resolution fails. The three-element tuple return type correctly represents all resolution states.
422-447: LGTM! Correct XPath construction for ID attribute matching.The function properly builds an XPath expression that:
- Uses safe string literals via
buildXPathLiteral- Filters attributes by name and optionally namespace using
local-name()andnamespace-uri()- Checks attribute values against the ID
- Handles both string and object
IdAttributeTypeformats correctlyThe resulting XPath
//*[@*[conditions]=idValue]correctly finds elements with matching ID attributes.src/types.ts (7)
10-17: LGTM! Proper algorithm constant imports.Importing algorithm names from
XMLDSIG_URISestablishes a single source of truth for algorithm identifiers, enabling type-safe references throughout the codebase.
21-27: LGTM! Well-designed ID attribute type separation.The distinction between
SignatureIdAttributeType(with prefix for XML generation) andVerificationIdAttributeType(prefix-agnostic, withnullfor explicit non-namespaced matching) properly models the different requirements of signing vs validation workflows.
53-66: LGTM! Comprehensive algorithm processing types.
TransformAlgorithmOptionsproperly captures all context needed for transform operations (namespaces, signature node, inclusive prefixes).KeySelectorFunctionprovides a clean abstraction for key extraction.NamespacePrefixis well-defined.
68-134: LGTM! Consistent algorithm interface design.The algorithm interfaces follow a uniform pattern with:
- Name-based typing via XMLDSIG_URIS constants
getAlgorithmName()method for introspection- Processing methods with appropriate signatures
- Map types for algorithm registries
The TODO comment (lines 127-129) about tightening
CanonicalizationAlgorithm.process()to return onlystringis noted for future cleanup.
138-157: LGTM! Enhanced SignedXmlOptions with security features.The updated options properly integrate:
- ID attribute configuration for both signing (
idAttribute) and validation (idAttributes)- Transform complexity limits (
maxTransforms)- Algorithm allowlists for security hardening
- Name-based algorithm types for type safety
The interface is backward-compatible (all new fields are optional) while enabling gradual migration to the new XmlDSigVerifier wrapper.
196-199: LGTM! Name-based algorithm types in Reference.Updating
transformsanddigestAlgorithmto use name-based types (TransformAlgorithmName,HashAlgorithmName) provides type safety and aligns with the new algorithm abstraction system. TheReadonlyArrayfor transforms is good immutability practice.
287-412: LGTM! Well-architected XmlDSigVerifier type system.The verifier types demonstrate solid design:
KeySelector types (289-299): Discriminated union enables "provide certificate OR extraction function" pattern.
Security options (301-351): Comprehensive security controls including:
- Transform complexity limits
- Certificate expiration checking
- Truststore validation
- Algorithm allowlists
Verifier options (356-389): Clean interface with good defaults and thorough JSDoc.
Result types (394-412): Discriminated union with
successfield enables type-safe result handling.The corrected type on line 350 (
CanonicalizationAlgorithmMap) properly constrains canonicalization algorithms.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils.ts (1)
31-35: Optional: Simplify signature (nitpick).The signature can be simplified from
namespace?: string | null | undefinedtonamespace?: string | null, since the?modifier already impliesundefined.Apply this diff:
export function findAttr( element: Element, localName: string, - namespace?: string | null | undefined, + namespace?: string | null, ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
test/static/future_certificate.crt.pemis excluded by!**/*.pemtest/static/future_certificate.csr.pemis excluded by!**/*.pemtest/static/future_certificate.key.pemis excluded by!**/*.pem
📒 Files selected for processing (4)
src/utils.ts(1 hunks)src/xmldsig-uris.ts(1 hunks)src/xmldsig-verifier.ts(1 hunks)test/xmldsig-verifier.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/xmldsig-uris.ts
- test/xmldsig-verifier.spec.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
src/xmldsig-verifier.ts
📚 Learning: 2025-10-22T21:03:38.354Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Applied to files:
src/xmldsig-verifier.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/xmldsig-verifier.ts
📚 Learning: 2025-10-22T20:36:00.758Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Applied to files:
src/xmldsig-verifier.ts
📚 Learning: 2025-08-17T10:50:18.024Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1447-1451
Timestamp: 2025-08-17T10:50:18.024Z
Learning: In the xml-crypto codebase, ref.digestValue is only used during signature validation when loading references from existing signatures, not during signature creation in methods like createReferences or processSignatureReferences.
Applied to files:
src/xmldsig-verifier.ts
🧬 Code graph analysis (1)
src/xmldsig-verifier.ts (3)
src/types.ts (8)
KeySelector(299-299)VerificationIdAttributeType(24-26)TransformAlgorithmName(109-111)XmlDSigVerifierSecurityOptions(301-351)XmlDSigVerifierOptions(356-389)XmlDsigVerificationResult(410-412)SignedXmlOptions(138-157)KeySelectorFunction(53-53)src/signed-xml.ts (2)
SignedXml(41-1534)getCertFromKeyInfo(283-292)src/utils.ts (1)
isArrayHasLength(5-7)
🔇 Additional comments (6)
src/utils.ts (2)
9-14: LGTM! Clean null-namespace support.The three-way namespace distinction is correctly implemented:
null(explicit): matches attributes with no namespaceundefined: matches attributes with any namespacestring: matches that specific namespaceThe loose equality check on line 13 correctly catches only
undefinedafter the strictnullcheck.
16-29: LGTM! Implicit namespace handling is consistent.The null-namespace branch (lines 22-24) correctly mirrors the explicit version, and the implicit inheritance logic (lines 25-28) appropriately handles attributes that don't have an explicit namespace but inherit from their parent element.
src/xmldsig-verifier.ts (4)
1-18: LGTM: Clean imports and sensible defaults.The imports are well-organized and the default constants are appropriate for a security-focused verification API.
31-64: Well-structured initialization with comprehensive defaults.The constructor properly merges user options with secure defaults and creates a reusable
SignedXmlinstance. The instance-reuse pattern (line 63) is safe becauseloadSignature()resets relevant state, making this design efficient for server environments processing multiple documents with the same configuration.
73-133: Solid verification flow with appropriate error handling.Both the static convenience method and instance method are well-implemented:
- Static method handles constructor errors
- Instance method handles verification errors
- Auto-detection logic correctly handles edge cases (no signature, multiple signatures)
- Layered error handling is appropriate for the two different failure modes
135-201: Comprehensive certificate validation with reasonable trust logic.The
createSignedXmlfactory correctly implements:
- Certificate expiration validation
- Truststore verification (both direct public key match and chain-of-trust via signature verification)
The truststore validation logic (lines 182-190) is sound: it checks if the signing certificate's public key matches a trusted public key OR if the certificate was signed by a trusted key.
Given the complexity of the trust validation logic, ensure thorough test coverage includes:
- Direct public key matches
- CA-signed certificate chains
- Expiration edge cases
- Truststore rejection scenarios
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/signed-xml.ts (1)
551-583: Inconsistent namespace handling; null producesnamespace-uri(.)='null'
validateElementAgainstReferencestreatsnamespaceUri !== undefinedas “namespaced”, which misclassifiesnulland diverges fromvalidateReference(which treats falsy as “no namespace constraint”). Align withvalidateReference.- if (attr && uri === attr.value) { - if (idAttr.namespaceUri !== undefined) { - // When namespaceUri is set, we look for attributes in that specific namespace - ref.xpath = `//*[@*[local-name(.)='${idAttr.localName}' and namespace-uri(.)='${idAttr.namespaceUri}']='${uri}']`; - } else { - // When namespaceUri is explicitly set to undefined, we look only for attributes without a namespace - ref.xpath = `//*[@*[local-name(.)='${idAttr.localName}' and namespace-uri(.)='']='${uri}']`; - } + if (attr && uri === attr.value) { + if (idAttr.namespaceUri) { + // Specific namespace + ref.xpath = `//*[@*[local-name(.)='${idAttr.localName}' and namespace-uri(.)='${idAttr.namespaceUri}']='${uri}']`; + } else { + // No namespace constraint (matches any or none), like validateReference + ref.xpath = `//*[@*[local-name(.)='${idAttr.localName}']='${uri}']`; + } break; // found the correct element, no need to check further }
🧹 Nitpick comments (6)
src/enveloped-signature.ts (1)
13-21: Prefer direct node removal when signatureNode is providedIf
options.signatureNodeis set and is a descendant ofnode, remove it by identity rather than scanning bySignatureValue. This avoids edge cases with duplicated values and is faster. Keep the current value-based fallback if the node isn’t undernode.- if (null == options.signatureNode) { + if (options.signatureNode == null) { ... } else { - const signatureNode = options.signatureNode; + const signatureNode = options.signatureNode; + // Fast path: remove the exact node if it belongs to this subtree + if (isDomNode.isNodeLike(signatureNode) && signatureNode.parentNode && xpath.select1(".", node) === node && xpath.select1("ancestor-or-self::*", signatureNode)) { + if (signatureNode.parentNode) signatureNode.parentNode.removeChild(signatureNode); + return node; + } const expectedSignatureValue = xpath.select1( ".//*[local-name(.)='SignatureValue']/text()", signatureNode, ); ... }Also applies to: 32-49
test/xmldsig-verifier.spec.ts (2)
561-569: Add a RSA‑SHA256 happy‑path to discourage SHA‑1 in examplesKeep current RSA‑SHA1 tests for back-compat, but add one parallel case using
SIGNATURE_ALGORITHMS.RSA_SHA256to set a stronger norm in docs/tests.
7-9: Unify import specifiersUse either
"../src"or"../src/"consistently to avoid duplicate module instances in some tooling.test/signature-unit-tests.spec.ts (1)
711-735: Include at least one RSA‑SHA256 path in “creates correct signature values”Mirror this RSA‑SHA1 case with
SIGNATURE_ALGORITHMS.RSA_SHA256to exercise non‑SHA1 defaults too.src/xmldsig-verifier.ts (1)
181-189: Tighten truststore.somecallback to explicit boolean returnMinor readability; avoids relying on implicit
undefinedfalsy.- const isTrusted = truststore.some((trustedCert) => { - if (trustedCert.equals(x509.publicKey) || x509.verify(trustedCert)) { - return true; - } - }); + const isTrusted = truststore.some( + (trustedCert) => trustedCert.equals(x509.publicKey) || x509.verify(trustedCert), + );src/signed-xml.ts (1)
1398-1441: Namespaced Id creation silently drops namespace when no prefix is providedWhen
firstIdAttrhas anamespaceUribut noprefix, the code falls back tosetAttribute(localName, id), creating a non‑namespaced attribute. That diverges from intent and later matching.Either:
- Require a
prefixfor namespaced attributes (documented and enforced at type level), or- Auto-assign a stable prefix (e.g.,
idns) and declare it before callingsetAttributeNS.Below applies the “require prefix” route by honoring namespace only when prefix is provided, else create non‑namespaced (and document this behavior).
- } else { - if ("prefix" in firstIdAttr && firstIdAttr.prefix) { + } else { + if (firstIdAttr.prefix) { node.setAttributeNS( NAMESPACES.xmlns, `xmlns:${firstIdAttr.prefix}`, firstIdAttr.namespaceUri, ); node.setAttributeNS( firstIdAttr.namespaceUri, `${firstIdAttr.prefix}:${firstIdAttr.localName}`, id, ); } else { - node.setAttribute(firstIdAttr.localName, id); + // No prefix provided: create a non-namespaced attribute + node.setAttribute(firstIdAttr.localName, id); } }And update the JSDoc above
idAttributesto clarify: “If a namespaceUri is used, a prefix must be provided.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/c14n-canonicalization.ts(4 hunks)src/enveloped-signature.ts(3 hunks)src/exclusive-canonicalization.ts(6 hunks)src/hash-algorithms.ts(4 hunks)src/signature-algorithms.ts(6 hunks)src/signed-xml.ts(26 hunks)src/types.ts(4 hunks)src/xmldsig-uris.ts(1 hunks)src/xmldsig-verifier.ts(1 hunks)test/c14n-non-exclusive-unit-tests.spec.ts(1 hunks)test/c14nWithComments-unit-tests.spec.ts(5 hunks)test/canonicalization-unit-tests.spec.ts(5 hunks)test/document-tests.spec.ts(3 hunks)test/hmac-tests.spec.ts(4 hunks)test/key-info-tests.spec.ts(3 hunks)test/saml-response-tests.spec.ts(12 hunks)test/signature-integration-tests.spec.ts(9 hunks)test/signature-object-tests.spec.ts(24 hunks)test/signature-unit-tests.spec.ts(31 hunks)test/validators/XmlCryptoUtilities/XmlCryptoUtilities/program-repro-misc-validation-and-canon.cs(7 hunks)test/wsfed-metadata-tests.spec.ts(2 hunks)test/xmldsig-verifier.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/validators/XmlCryptoUtilities/XmlCryptoUtilities/program-repro-misc-validation-and-canon.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/xmldsig-uris.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
test/key-info-tests.spec.tstest/document-tests.spec.tstest/wsfed-metadata-tests.spec.tstest/xmldsig-verifier.spec.tstest/saml-response-tests.spec.tstest/hmac-tests.spec.tstest/signature-integration-tests.spec.tssrc/xmldsig-verifier.tssrc/c14n-canonicalization.tssrc/signed-xml.tstest/canonicalization-unit-tests.spec.tstest/signature-unit-tests.spec.tssrc/signature-algorithms.tstest/c14n-non-exclusive-unit-tests.spec.tssrc/exclusive-canonicalization.tstest/signature-object-tests.spec.tssrc/enveloped-signature.tssrc/types.tssrc/hash-algorithms.tstest/c14nWithComments-unit-tests.spec.ts
📚 Learning: 2025-10-25T14:41:13.728Z
Learnt from: shunkica
PR: node-saml/xml-crypto#519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.728Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Applied to files:
test/key-info-tests.spec.tstest/document-tests.spec.tstest/wsfed-metadata-tests.spec.tstest/xmldsig-verifier.spec.tstest/saml-response-tests.spec.tstest/hmac-tests.spec.tstest/signature-integration-tests.spec.tssrc/signed-xml.tstest/canonicalization-unit-tests.spec.tstest/signature-unit-tests.spec.tssrc/signature-algorithms.tstest/c14n-non-exclusive-unit-tests.spec.tstest/signature-object-tests.spec.tssrc/types.tssrc/hash-algorithms.tstest/c14nWithComments-unit-tests.spec.ts
📚 Learning: 2025-10-22T20:36:00.758Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Applied to files:
test/key-info-tests.spec.tstest/wsfed-metadata-tests.spec.tstest/saml-response-tests.spec.tstest/hmac-tests.spec.tstest/signature-integration-tests.spec.tssrc/xmldsig-verifier.tssrc/signed-xml.tstest/canonicalization-unit-tests.spec.tstest/signature-unit-tests.spec.tssrc/signature-algorithms.tstest/signature-object-tests.spec.tssrc/types.tssrc/hash-algorithms.tstest/c14nWithComments-unit-tests.spec.ts
📚 Learning: 2025-10-22T21:03:38.354Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Applied to files:
test/key-info-tests.spec.tstest/document-tests.spec.tstest/wsfed-metadata-tests.spec.tstest/xmldsig-verifier.spec.tstest/saml-response-tests.spec.tstest/hmac-tests.spec.tstest/signature-integration-tests.spec.tssrc/xmldsig-verifier.tssrc/c14n-canonicalization.tssrc/signed-xml.tstest/canonicalization-unit-tests.spec.tstest/signature-unit-tests.spec.tssrc/signature-algorithms.tssrc/exclusive-canonicalization.tstest/signature-object-tests.spec.tssrc/enveloped-signature.tssrc/types.tstest/c14nWithComments-unit-tests.spec.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
test/key-info-tests.spec.tstest/document-tests.spec.tstest/wsfed-metadata-tests.spec.tstest/saml-response-tests.spec.tstest/hmac-tests.spec.tstest/signature-integration-tests.spec.tssrc/xmldsig-verifier.tssrc/signed-xml.tstest/canonicalization-unit-tests.spec.tstest/signature-unit-tests.spec.tssrc/exclusive-canonicalization.tstest/signature-object-tests.spec.tssrc/enveloped-signature.tssrc/types.tstest/c14nWithComments-unit-tests.spec.ts
📚 Learning: 2025-08-17T10:50:18.024Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1447-1451
Timestamp: 2025-08-17T10:50:18.024Z
Learning: In the xml-crypto codebase, ref.digestValue is only used during signature validation when loading references from existing signatures, not during signature creation in methods like createReferences or processSignatureReferences.
Applied to files:
test/key-info-tests.spec.tstest/hmac-tests.spec.tstest/signature-integration-tests.spec.tssrc/xmldsig-verifier.tssrc/signed-xml.tstest/signature-unit-tests.spec.tstest/signature-object-tests.spec.ts
📚 Learning: 2025-08-16T14:07:57.644Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1088-1119
Timestamp: 2025-08-16T14:07:57.644Z
Learning: The xml-crypto library does not validate user input and places the responsibility for providing properly encoded values on the users. This is a consistent design philosophy throughout the library.
Applied to files:
test/signature-unit-tests.spec.tstest/signature-object-tests.spec.ts
🧬 Code graph analysis (19)
test/key-info-tests.spec.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (1)
XMLDSIG_URIS(8-8)
test/document-tests.spec.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (1)
XMLDSIG_URIS(8-8)
test/wsfed-metadata-tests.spec.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (1)
XMLDSIG_URIS(8-8)
test/xmldsig-verifier.spec.ts (8)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/xmldsig-verifier.ts (2)
createSignedXml(135-201)XmlDSigVerifier(31-216)src/signed-xml.ts (1)
SignedXml(41-1530)src/types.ts (1)
XmlDsigVerificationResult(406-408)src/signature-algorithms.ts (1)
RsaSha1(5-29)src/hash-algorithms.ts (1)
Sha1(5-16)src/enveloped-signature.ts (1)
EnvelopedSignature(6-57)src/exclusive-canonicalization.ts (1)
ExclusiveCanonicalization(22-327)
test/saml-response-tests.spec.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (1)
XMLDSIG_URIS(8-8)
test/hmac-tests.spec.ts (3)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (1)
XMLDSIG_URIS(8-8)example/example.js (5)
sig(9-9)sig(22-22)xml(32-32)doc(17-17)signature(18-21)
test/signature-integration-tests.spec.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (1)
XMLDSIG_URIS(8-8)
src/xmldsig-verifier.ts (3)
src/types.ts (8)
KeySelector(295-295)VerificationIdAttributeType(20-22)TransformAlgorithmURI(105-107)XmlDSigVerifierSecurityOptions(297-347)XmlDSigVerifierOptions(352-385)XmlDsigVerificationResult(406-408)SignedXmlOptions(134-153)KeySelectorFunction(49-49)src/signed-xml.ts (2)
SignedXml(41-1530)getCertFromKeyInfo(283-292)src/utils.ts (1)
isArrayHasLength(5-7)
src/c14n-canonicalization.ts (2)
src/types.ts (3)
CanonicalizationAlgorithm(120-126)TransformAlgorithmOptions(56-62)CanonicalizationAlgorithmURI(116-118)src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)
src/signed-xml.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/types.ts (10)
IdAttributeType(23-23)SignatureAlgorithmURI(64-66)CanonicalizationAlgorithmURI(116-118)TransformAlgorithmURI(105-107)CanonicalizationAlgorithmMap(127-130)HashAlgorithmMap(103-103)TransformAlgorithmMap(114-114)VerificationIdAttributeType(20-22)HashAlgorithmURI(96-96)TransformAlgorithmOptions(56-62)
test/canonicalization-unit-tests.spec.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (1)
XMLDSIG_URIS(8-8)
test/signature-unit-tests.spec.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (2)
XMLDSIG_URIS(8-8)SignedXml(6-6)
src/signature-algorithms.ts (1)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)
src/exclusive-canonicalization.ts (2)
src/types.ts (3)
CanonicalizationAlgorithm(120-126)TransformAlgorithmOptions(56-62)CanonicalizationAlgorithmURI(116-118)src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)
test/signature-object-tests.spec.ts (1)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)
src/enveloped-signature.ts (3)
src/types.ts (3)
TransformAlgorithm(109-113)TransformAlgorithmOptions(56-62)TransformAlgorithmURI(105-107)example/example.js (1)
signature(18-21)src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)
src/types.ts (1)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)
src/hash-algorithms.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (1)
XMLDSIG_URIS(8-8)
test/c14nWithComments-unit-tests.spec.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/index.ts (1)
XMLDSIG_URIS(8-8)
🔇 Additional comments (30)
src/types.ts (13)
10-13: LGTM: Centralized URI imports.The import and destructuring of XMLDSIG_URIS provides a clean foundation for the algorithm abstraction system. This centralizes URI constants and reduces the risk of typos.
17-23: LGTM: ID attribute type definitions.The separation of
SignatureIdAttributeTypeandVerificationIdAttributeTypeproperly distinguishes signing (requires prefix) from validation (prefix ignored, explicit null for non-namespaced) use cases. Past review comments regardingnullvsundefinedhave been addressed.
49-62: LGTM: Supporting type definitions for algorithms.The
KeySelectorFunction,NamespacePrefix, andTransformAlgorithmOptionstypes provide a solid foundation for the algorithm abstraction layer.
64-94: LGTM: SignatureAlgorithm interface and map.The generic
SignatureAlgorithminterface with both sync/async overloads and theSignatureAlgorithmMaptype provide extensibility while maintaining type safety.
96-103: LGTM: HashAlgorithm interface and map.Clean abstraction for hash algorithms with proper URI typing and extensibility via the map type.
105-114: LGTM: TransformAlgorithm interface and map.The transform algorithm abstraction is well-designed with proper options passing via
TransformAlgorithmOptions.
116-130: LGTM: CanonicalizationAlgorithm interface and map.Properly extends
TransformAlgorithmto express the is-a relationship. The TODO comment at lines 123-124 appropriately flags future cleanup when canonicalization algorithms are fully separated from transforms.
134-153: LGTM: Expanded SignedXmlOptions interface.The additions properly integrate the new algorithm abstraction system:
idAttributeandidAttributesuse the new ID typesmaxTransformssecurity option (nullable to explicitly disable limit)- Algorithm URIs replace legacy string-only options
- Allowed algorithm maps provide runtime security controls
getCertFromKeyInfonow usesKeySelectorFunctiontypeAll past review comments regarding type consistency have been addressed.
187-223: LGTM: Reference interface updates.The
transformsanddigestAlgorithmfields now use the appropriate URI types (TransformAlgorithmURIandHashAlgorithmURI), aligning with the new algorithm abstraction system while maintaining backward compatibility.
283-295: LGTM: KeySelector type variants.The discriminated union of
CertificateKeySelectorandKeyInfoKeySelectorprovides a clean API for specifying verification keys. Using a union instead of optional properties ensures callers provide exactly one selector method.
297-347: LGTM: XmlDSigVerifierSecurityOptions.Comprehensive security controls for verification:
maxTransformsprevents DoS via excessive transform complexitycheckCertExpirationvalidates certificate validitytruststoreenables certificate chain validation- Algorithm maps restrict allowed algorithms
JSDoc is clear about defaults and purpose. Past review comment about
canonicalizationAlgorithmstype has been addressed (now usesCanonicalizationAlgorithmMap).
349-385: LGTM: XmlDSigVerifierOptions interface.Well-structured options for the verifier:
keySelectoris required (discriminated union ensures proper key configuration)idAttributesfor custom ID resolutionimplicitTransformsfor profile-specific transformsthrowOnErrorfor error handling preferencesecurityfor optional hardeningJSDoc properly documents each field with examples and defaults. Past review comment about
implicitTransformstype has been addressed (now usesTransformAlgorithmURI).
387-408: LGTM: Verification result types.The discriminated union of
SuccessfulXmlDsigVerificationResultandFailedXmlDsigVerificationResultprovides type-safe result handling. Thesuccessdiscriminator enables proper narrowing. Past review typo ("sucessfuly" → "successfully") has been fixed.test/c14n-non-exclusive-unit-tests.spec.ts (1)
3-3: LGTM: Use public API import.The import path change from the specific module to the barrel export (
"../src") is consistent with best practices for consuming the public API.src/hash-algorithms.ts (4)
3-3: LGTM: Import centralized URIs.Clean addition of the XMLDSIG_URIS import to support centralized URI constants.
14-15: LGTM: Use XMLDSIG_URIS.HASH_ALGORITHMS.SHA1.Replacing the hardcoded URI with the centralized constant improves consistency and reduces the risk of typos.
27-28: LGTM: Use XMLDSIG_URIS.HASH_ALGORITHMS.SHA256.Consistent with the SHA1 change, properly centralizes the SHA256 URI.
40-41: LGTM: Use XMLDSIG_URIS.HASH_ALGORITHMS.SHA512.Completes the hash algorithm URI centralization for all three supported algorithms.
test/document-tests.spec.ts (2)
1-1: LGTM: Import XMLDSIG_URIS from public API.Clean addition of
XMLDSIG_URISto the import list enables centralized URI usage in tests.
13-13: LGTM: Use XMLDSIG_URIS.NAMESPACES.ds in XPath.Replacing the hardcoded namespace URI with the centralized constant in both XPath expressions improves consistency and maintainability across the test suite.
Also applies to: 31-31
test/wsfed-metadata-tests.spec.ts (2)
1-1: LGTM: Import XMLDSIG_URIS from public API.Consistent with other test files, enables centralized URI usage.
13-13: LGTM: Use XMLDSIG_URIS.NAMESPACES.ds in XPath.Consistent with the document-tests.spec.ts changes, improves maintainability.
src/signature-algorithms.ts (2)
3-3: LGTM: Import centralized URIs.Clean addition of the XMLDSIG_URIS import for signature algorithm constants.
27-27: LGTM: Use XMLDSIG_URIS.SIGNATURE_ALGORITHMS constants.All five signature algorithms (RSA_SHA1, RSA_SHA256, RSA_SHA256_MGF1, RSA_SHA512, HMAC_SHA1) now use centralized URI constants, ensuring consistency across the codebase.
Also applies to: 53-53, 100-100, 126-126, 152-152
test/saml-response-tests.spec.ts (2)
1-1: LGTM: Import XMLDSIG_URIS from public API.Enables centralized URI usage across all SAML response tests.
13-13: LGTM: Use XMLDSIG_URIS.NAMESPACES.ds throughout.All 11 test cases now use the centralized namespace constant in their XPath expressions. This comprehensive update ensures consistency across the SAML response test suite.
Also applies to: 30-30, 46-46, 64-64, 86-86, 104-104, 120-120, 136-136, 154-154, 175-175, 194-194
test/key-info-tests.spec.ts (3)
4-4: LGTM: Import XMLDSIG_URIS from public API.Enables comprehensive URI constant usage for both namespace and algorithm URIs.
14-16: LGTM: Use XMLDSIG_URIS algorithm constants.The signature configuration now uses centralized constants for canonicalization (
EXCLUSIVE_C14N) and signature algorithm (RSA_SHA1), demonstrating proper integration of the new URI system beyond just namespaces.
30-38: LGTM: Comprehensive XMLDSIG_URIS usage in HMAC test.The HMAC test properly uses centralized constants for:
- Signature algorithm (
HMAC_SHA1)- Hash algorithm (
SHA1)- Transforms and canonicalization (
EXCLUSIVE_C14N)This demonstrates the full integration of the URI constant system across signature, hash, and transform/canonicalization algorithms.
src/signed-xml.ts (1)
41-56: Fix TypeScript type mismatch in SignedXmlOptions.idAttributesThe constructor option
idAttributesis incorrectly typed asVerificationIdAttributeType[], which excludes theprefixproperty. However, the JSDoc example explicitly shows{ prefix: "wsu", localName: "Id", namespaceUri: "..." }, and the runtime code inensureHasId(lines 1426–1436) checks for and uses theprefixproperty when adding namespaced ID attributes during signing.Change line 137 in
src/types.ts:- idAttributes?: VerificationIdAttributeType[]; + idAttributes?: IdAttributeType[];This aligns the constructor option type with the class property (line 56) and enables TypeScript users to pass prefix-qualified ID attributes as documented.
⛔ Skipped due to learnings
Learnt from: shunkica PR: node-saml/xml-crypto#0 File: :0-0 Timestamp: 2025-10-22T21:50:05.454Z Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.Learnt from: shunkica PR: node-saml/xml-crypto#506 File: src/signed-xml.ts:1159-1159 Timestamp: 2025-10-22T21:03:38.354Z Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.Learnt from: shunkica PR: node-saml/xml-crypto#0 File: :0-0 Timestamp: 2025-10-22T21:50:05.454Z Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.Learnt from: shunkica PR: node-saml/xml-crypto#506 File: src/signed-xml.ts:1159-1159 Timestamp: 2025-10-22T20:36:00.758Z Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/signed-xml.ts(26 hunks)src/types.ts(4 hunks)src/xmldsig-verifier.ts(1 hunks)test/xmldsig-verifier.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
test/xmldsig-verifier.spec.tssrc/signed-xml.tssrc/types.tssrc/xmldsig-verifier.ts
📚 Learning: 2025-10-22T21:03:38.354Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Applied to files:
test/xmldsig-verifier.spec.tssrc/signed-xml.tssrc/types.tssrc/xmldsig-verifier.ts
📚 Learning: 2025-10-25T14:41:13.728Z
Learnt from: shunkica
PR: node-saml/xml-crypto#519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.728Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Applied to files:
test/xmldsig-verifier.spec.tssrc/signed-xml.tssrc/types.ts
📚 Learning: 2025-10-22T21:50:05.454Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Applied to files:
src/signed-xml.tssrc/types.tssrc/xmldsig-verifier.ts
📚 Learning: 2025-10-22T20:36:00.758Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Applied to files:
src/signed-xml.tssrc/types.tssrc/xmldsig-verifier.ts
📚 Learning: 2025-08-17T10:50:18.024Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1447-1451
Timestamp: 2025-08-17T10:50:18.024Z
Learning: In the xml-crypto codebase, ref.digestValue is only used during signature validation when loading references from existing signatures, not during signature creation in methods like createReferences or processSignatureReferences.
Applied to files:
src/signed-xml.tssrc/xmldsig-verifier.ts
🧬 Code graph analysis (4)
test/xmldsig-verifier.spec.ts (6)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/xmldsig-verifier.ts (2)
createSignedXml(229-297)XmlDSigVerifier(78-312)src/types.ts (1)
XmlDsigVerificationResult(449-451)src/signature-algorithms.ts (1)
RsaSha1(5-29)src/hash-algorithms.ts (1)
Sha1(5-16)src/enveloped-signature.ts (1)
EnvelopedSignature(6-57)
src/signed-xml.ts (2)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)src/types.ts (11)
IdAttributeType(23-23)SignatureAlgorithmURI(64-66)CanonicalizationAlgorithmURI(116-118)TransformAlgorithmURI(105-107)CanonicalizationAlgorithmMap(127-130)HashAlgorithmMap(103-103)SignatureAlgorithmMap(94-94)TransformAlgorithmMap(114-114)VerificationIdAttributeType(20-22)HashAlgorithmURI(96-96)TransformAlgorithmOptions(56-62)
src/types.ts (1)
src/xmldsig-uris.ts (1)
XMLDSIG_URIS(54-60)
src/xmldsig-verifier.ts (3)
src/types.ts (14)
VerificationIdAttributeType(20-22)TransformAlgorithmURI(105-107)KeyInfoKeySelector(290-293)KeyInfoXmlDSigSecurityOptions(361-376)CertificateKeySelector(285-288)XmlDSigVerifierSecurityOptions(324-359)SharedSecretKeySelector(295-298)XmlDSigVerifierOptions(425-428)KeyInfoXmlDSigVerifierOptions(382-392)SharedSecretXmlDSigVerifierOptions(413-423)PublicCertXmlDSigVerifierOptions(398-408)XmlDsigVerificationResult(449-451)SignedXmlOptions(134-153)KeySelectorFunction(49-49)src/signed-xml.ts (2)
SignedXml(41-1530)getCertFromKeyInfo(283-292)src/utils.ts (1)
isArrayHasLength(5-7)
Remove backward-compatibility type aliases: - CanonicalizationOrTransformationAlgorithm - CanonicalizationOrTransformAlgorithmType - CanonicalizationAlgorithmType - SignatureAlgorithmType - HashAlgorithmType - CanonicalizationOrTransformationAlgorithmProcessOptions Replace all internal usages with canonical names and remove associated eslint-disable comments and TODO markers. Replace map-based algorithm options (Record<URI, Constructor>) with simple constructor arrays in XmlDSigVerifierSecurityOptions. Each class provides its own URI via getAlgorithmName(), eliminating redundant keys and risk of URI mismatches. - Add static default algorithm arrays on XmlDSigVerifier - Add private toAlgorithmMap helper for internal conversion - Export algorithm classes (Sha*, Rsa*, HmacSha1, EnvelopedSignature) - Update tests and XMLDSIG_VERIFIER.md documentation
|
@cjbarth I have been using this branch in production for a few months and have not encountered any issues. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/signed-xml.ts (1)
735-795:⚠️ Potential issue | 🔴 CriticalReset per-signature state in
loadSignatureto prevent cross-verification leakage.
signatureAlgorithm,signatureValue, andkeyInfoare only overwritten when corresponding nodes exist. If a later signature omits one of these fields, stale values from a previous document can be reused. This is a critical correctness/security risk, especially with reusable verifier instances.🔒 Proposed fix
loadSignature(signatureNode: Node | string): void { + // Reset fields that must come from the current signature payload + this.signatureAlgorithm = undefined; + this.signatureValue = ""; + this.keyInfo = null; + if (typeof signatureNode === "string") { this.signatureNode = signatureNode = new xmldom.DOMParser().parseFromString(signatureNode); } else { this.signatureNode = signatureNode; } @@ const signatureAlgorithm = xpath.select1( ".//*[local-name(.)='SignatureMethod']/@Algorithm", signatureNode, ); - - if (isDomNode.isAttributeNode(signatureAlgorithm)) { - this.signatureAlgorithm = signatureAlgorithm.value as SignatureAlgorithmURI; - } + if (!isDomNode.isAttributeNode(signatureAlgorithm)) { + throw new Error("could not find SignatureMethod/@Algorithm element"); + } + this.signatureAlgorithm = signatureAlgorithm.value as SignatureAlgorithmURI; @@ const signatureValue = xpath.select1( ".//*[local-name(.)='SignatureValue']/text()", signatureNode, ); - - if (isDomNode.isTextNode(signatureValue)) { - this.signatureValue = signatureValue.data.replace(/\r?\n/g, ""); - } + if (!isDomNode.isTextNode(signatureValue)) { + throw new Error("could not find SignatureValue element"); + } + this.signatureValue = signatureValue.data.replace(/\r?\n/g, ""); @@ const keyInfo = xpath.select1(".//*[local-name(.)='KeyInfo']", signatureNode); - - if (isDomNode.isNodeLike(keyInfo)) { - this.keyInfo = keyInfo; - } + this.keyInfo = isDomNode.isNodeLike(keyInfo) ? keyInfo : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/signed-xml.ts` around lines 735 - 795, In loadSignature, reset per-signature state before parsing to avoid leaking values from previous signatures: explicitly set this.signatureAlgorithm = undefined (or null), this.signatureValue = undefined (or null), and this.keyInfo = undefined (or null) at the start of the loadSignature method (the function that processes signatureNode), so later conditional assignments (e.g., when isAttributeNode(signatureAlgorithm), isTextNode(signatureValue), or isNodeLike(keyInfo)) cannot reuse stale values from a prior document.
🧹 Nitpick comments (2)
test/xmldsig-verifier.spec.ts (1)
123-998: Add a reuse-regression case for sequential verifications.Given reusable verifier instances are a core design goal, add a test where one verification has KeyInfo/signature metadata and the next omits it, then assert the second call fails (no state carry-over).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/xmldsig-verifier.spec.ts` around lines 123 - 998, Add a new test to ensure a reusable XmlDSigVerifier instance does not carry state between sequential calls: create one XmlDSigVerifier instance (using keySelector: { publicCert } or getCertFromKeyInfo) and call verifier.verifySignature twice—first with a signed XML that includes KeyInfo/signature metadata (should validate), then with a modified/tampered XML or one missing KeyInfo/signature metadata (should fail). Use the existing helper functions (createSignedXml or createChainSignedXml) and the expectValidResult/expectInvalidResult assertions to assert the first call succeeds and the second call fails (no state carried over), and set throwOnError appropriately so failures are returned for assertions.src/xmldsig-verifier.ts (1)
110-117: Consider hardened default allow-lists for the verifier.Given this API is positioned as security-focused, defaulting to SHA-1 / RSA-SHA1 / HMAC-SHA1 keeps legacy risk enabled by default. Consider making SHA-256+ defaults and requiring explicit opt-in for SHA-1 families.
Also applies to: 229-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig-verifier.ts` around lines 110 - 117, Update the default allow-lists to remove SHA-1 families and weak HMAC by changing the static fields: defaultHashAlgorithms should omit Sha1 and only include Sha256 and Sha512; defaultAsymmetricSignatureAlgorithms should remove RsaSha1 and only include RsaSha256, RsaSha256Mgf1, and RsaSha512; defaultSymmetricSignatureAlgorithms should remove HmacSha1 and include a stronger HMAC (e.g., HmacSha256 or HmacSha512) instead. Keep the Sha1 / RsaSha1 / HmacSha1 symbols available only behind an explicit opt-in, so callers must explicitly enable legacy algorithms if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/signed-xml.ts`:
- Around line 1430-1448: The ensureHasId code uses setAttribute(...) when
firstIdAttr has no prefix, which drops the namespace and breaks namespace-aware
ID resolution; update the branch handling firstIdAttr (in the ensureHasId method
that uses this.idAttributes and firstIdAttr) so that if firstIdAttr.namespaceUri
exists but firstIdAttr.prefix is falsy you call
setAttributeNS(firstIdAttr.namespaceUri, firstIdAttr.localName, id) instead of
setAttribute(...), preserving the namespace for namespaced idAttributes while
keeping the existing prefixed and non-namespaced logic intact.
In `@src/types.ts`:
- Around line 291-294: Update the KeyInfoKeySelector JSDoc to clearly state that
getCertFromKeyInfo must return an X.509 certificate (not arbitrary public-key
material) that the verifier will parse for expiration/truststore checks; mention
accepted formats (e.g., PEM string or base64-encoded DER) and that returning
non-certificate key data may cause runtime failures. Change the comment on
getCertFromKeyInfo in the KeyInfoKeySelector type to reflect this expectation so
integrators provide a certificate string or null.
In `@src/xmldsig-verifier.ts`:
- Around line 360-367: The handleError function currently throws raw values when
throwOnError is true; update XmlDsigVerifier.handleError to ensure any thrown
value is an actual Error instance: if error is already an Error, rethrow it;
otherwise create a new Error using the computed errorMessage and set the
original value as the cause or attach it (so original details are preserved),
then throw that Error; when not throwing, continue returning the
XmlDsigVerificationResult using the normalized errorMessage. Use the existing
handleError symbol to locate and implement this normalization so downstream code
can reliably use instanceof Error and stack/cause information.
In `@XMLDSIG_VERIFIER.md`:
- Around line 157-175: Update the docs to clarify that when using the security
options checkCertExpiration or truststore (under the security section) the
getCertFromKeyInfo selector must return certificate-parsable material (e.g.,
PEM/DER bytes or an X509Certificate) rather than just a generic key identifier;
explicitly state this requirement and the expected formats/types returned by
getCertFromKeyInfo so consumers of the getCertFromKeyInfo callback know to
supply cert-parsable material usable by the truststore and expiration checks.
---
Outside diff comments:
In `@src/signed-xml.ts`:
- Around line 735-795: In loadSignature, reset per-signature state before
parsing to avoid leaking values from previous signatures: explicitly set
this.signatureAlgorithm = undefined (or null), this.signatureValue = undefined
(or null), and this.keyInfo = undefined (or null) at the start of the
loadSignature method (the function that processes signatureNode), so later
conditional assignments (e.g., when isAttributeNode(signatureAlgorithm),
isTextNode(signatureValue), or isNodeLike(keyInfo)) cannot reuse stale values
from a prior document.
---
Nitpick comments:
In `@src/xmldsig-verifier.ts`:
- Around line 110-117: Update the default allow-lists to remove SHA-1 families
and weak HMAC by changing the static fields: defaultHashAlgorithms should omit
Sha1 and only include Sha256 and Sha512; defaultAsymmetricSignatureAlgorithms
should remove RsaSha1 and only include RsaSha256, RsaSha256Mgf1, and RsaSha512;
defaultSymmetricSignatureAlgorithms should remove HmacSha1 and include a
stronger HMAC (e.g., HmacSha256 or HmacSha512) instead. Keep the Sha1 / RsaSha1
/ HmacSha1 symbols available only behind an explicit opt-in, so callers must
explicitly enable legacy algorithms if needed.
In `@test/xmldsig-verifier.spec.ts`:
- Around line 123-998: Add a new test to ensure a reusable XmlDSigVerifier
instance does not carry state between sequential calls: create one
XmlDSigVerifier instance (using keySelector: { publicCert } or
getCertFromKeyInfo) and call verifier.verifySignature twice—first with a signed
XML that includes KeyInfo/signature metadata (should validate), then with a
modified/tampered XML or one missing KeyInfo/signature metadata (should fail).
Use the existing helper functions (createSignedXml or createChainSignedXml) and
the expectValidResult/expectInvalidResult assertions to assert the first call
succeeds and the second call fails (no state carried over), and set throwOnError
appropriately so failures are returned for assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2baf088e-b6f1-49bf-abf2-ffa0a2a578f8
📒 Files selected for processing (7)
XMLDSIG_VERIFIER.mdsrc/enveloped-signature.tssrc/index.tssrc/signed-xml.tssrc/types.tssrc/xmldsig-verifier.tstest/xmldsig-verifier.spec.ts
- Clarify KeyInfoKeySelector JSDoc: returned string is used as KeyLike, must be certificate material when expiration/truststore checks are on - Normalize thrown values to Error instances in handleError - Update XMLDSIG_VERIFIER.md comment for getCertFromKeyInfo
|
@shunkica , that's great. Do you have any additional comments on the coderabbitai comments? Are there any other things you'd like to see tightened up before I start my code review (and encourage others to do the same)? Since we're doing a major release, we can do breaking stuff here, and I'm a big fan of reducing code debt by removing old stuff in major releases. |
|
@cjbarth Something I have considered is creating a We could then do API modernization in a 3-phase plan:
|
|
If it was not clear, I would add the |
|
@shunkica I've pushed a new commit to fix the build. BTW, did you want to stop exporting the old types? There really isn't a need for them since we're doing a 7.x release. |
Which types are you referring to? I think I removed all the types that were superseded. |
This exposes a runtime gap that the type system was preventing.
@shunkica , these types. I didn't actually check which you might be referring to, but I wanted to make sure we addressed this. We might as well just make a clean break in 7.x Also, I was reviewing your tests and found that there were some gaps (which I filled) and there were some tests that were not testing what they said they were (I adjusted the names), and there were some tests that were not testing for what they said they were in the correct way (I fixed that, which now causes the tests to fail). Did you want to have a look at these newly failing tests and comment on them? |
|
@shunkica , I've gone over everything made a few tweaks. I welcome your feedback and your comments on the now-failing tests. I have some other work planned for this release, but, given how hard you've worked on this, I want to get this landed before I do anything else so that we have fewer merge conflicts for you. Thank you very much for sticking with this and in adding these features. |
|
The Same for I will defer this decision to you if you consider that we should throw if user provides these options. Fixing should be as easy as |
|
@shunkica , I completely agree that throwing when providing the wrong options is a reasonable course of action. The tests originally were stating that a throw would happen, but weren't written in such a way as to actually test that a throw would happen. So, I adjusted the test to catch that case, but then saw that they weren't throwing, so I wanted to discuss with you. I can put in that code. |
Ok then, if that is the decision I will keep in mind for future changes that unsupported option combinations should throw errors instead of silently continue by ignoring one of them. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resume |
|
@shunkica , can you please have another look at this code and "sign off" on it, given the minor changes I've made? @markstos , @srd90, @ahacker1-securesaml, would you like to comment on this before I land it? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/signed-xml.ts (1)
203-206: Avoid mutating the caller’sidAttributesarray.
this.idAttributes = idAttributesfollowed byunshift(idAttribute)mutates the options object passed by the caller. Reusing the same config across instances will accumulate duplicate entries.♻️ Suggested fix
- this.idAttributes = idAttributes ?? SignedXml.getDefaultIdAttributes(); + this.idAttributes = [...(idAttributes ?? SignedXml.getDefaultIdAttributes())]; if (idAttribute) { this.idAttributes.unshift(idAttribute); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/signed-xml.ts` around lines 203 - 206, The constructor currently assigns the caller's idAttributes array directly to this.idAttributes and then mutates it with unshift, which alters the caller's options; instead clone the chosen array before modifying it (use a shallow copy of idAttributes or SignedXml.getDefaultIdAttributes()) so subsequent unshift on this.idAttributes does not mutate the original; ensure the code paths around idAttributes and the idAttribute handling use the cloned array so multiple SignedXml instances don't accumulate entries.src/xmldsig-verifier.ts (1)
109-129: Freeze the exported default algorithm lists.These are public mutable arrays, so a consumer can
push/splicethem and silently change verifier behavior process-wide. In a security-sensitive API, prefer frozen/readonly defaults or factory methods.♻️ Possible hardening
- static readonly defaultHashAlgorithms = [Sha1, Sha256, Sha512]; + static readonly defaultHashAlgorithms = Object.freeze([Sha1, Sha256, Sha512] as const); static readonly defaultAsymmetricSignatureAlgorithms = [ // TODO(v7): remove RSA-SHA1 from default signature algorithms. RsaSha1, RsaSha256, RsaSha256Mgf1, RsaSha512, - ]; + ] as const; // TODO: add HMAC-SHA256 support and make it the default instead of HMAC-SHA1. - static readonly defaultSymmetricSignatureAlgorithms = [HmacSha1]; - static readonly defaultCanonicalizationAlgorithms = [ + static readonly defaultSymmetricSignatureAlgorithms = Object.freeze([HmacSha1] as const); + static readonly defaultCanonicalizationAlgorithms = Object.freeze([ C14nCanonicalization, C14nCanonicalizationWithComments, ExclusiveCanonicalization, ExclusiveCanonicalizationWithComments, - ]; - static readonly defaultTransformAlgorithms = [ + ] as const); + static readonly defaultTransformAlgorithms = Object.freeze([ ...XmlDSigVerifier.defaultCanonicalizationAlgorithms, EnvelopedSignature, - ]; + ] as const);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig-verifier.ts` around lines 109 - 129, The exported static arrays (XmlDSigVerifier.defaultHashAlgorithms, defaultAsymmetricSignatureAlgorithms, defaultSymmetricSignatureAlgorithms, defaultCanonicalizationAlgorithms, defaultTransformAlgorithms) are public mutable and can be changed by consumers; make them immutable by freezing their contents (e.g., Object.freeze on each array and on any nested objects) or expose them via getters that return shallow copies so callers cannot push/splice the originals; update the static declarations to use frozen arrays (or return copies) and ensure any code that composes defaultTransformAlgorithms uses the frozen canonicalization array rather than sharing a mutable reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/signed-xml.ts`:
- Around line 1423-1445: The code assumes this.idAttributes[0] exists when
generating an auto-ID; if idAttributes is empty firstIdAttr will be undefined
and cause an unexpected error. Add a guard before using firstIdAttr (e.g., check
this.idAttributes.length === 0 or firstIdAttr === undefined) and throw a clear
Error indicating that idAttributes must contain at least one entry (or require a
string/IdAttribute object) so the calling code receives a meaningful message;
update the logic around the firstIdAttr variable and subsequent branches (the
block using node.setAttribute, node.setAttributeNS, and the prefix/namespace
checks) so they only run after the presence check.
- Around line 539-549: findTransformAlgorithm currently only falls back to
findCanonicalizationAlgorithm when TransformAlgorithms is null, which causes
canonicalization URIs to be ignored once an allowlist is configured; update
findTransformAlgorithm so that after checking this.TransformAlgorithms[name] it
falls back to calling this.findCanonicalizationAlgorithm(name) before throwing.
In short: in findTransformAlgorithm, if a transform entry exists return new
algo(); otherwise call this.findCanonicalizationAlgorithm(name) to allow
CanonicalizationAlgorithms to resolve C14N URIs, and only throw the error if
neither source provides the algorithm.
In `@src/xmldsig-verifier.ts`:
- Around line 323-358: The truststore check in
signedXmlOptions.getCertFromKeyInfo currently only compares the leaf cert
against truststore entries or tries a direct signature check, which fails for
normal leaf→intermediate→root chains; update getCertFromKeyInfo to extract any
intermediate certs present in KeyInfo (in addition to the leaf returned by
getCertFromKeyInfo), construct the certificate chain (leaf + intermediates),
validate the chain by walking signatures up to a trust anchor from the
truststore (truststore array entries should be treated as root/intermediate
anchors, not only exact leaf matches), and perform expiration checks (using
X509Certificate.validFrom/validTo) for each cert in the chain; use the same
identifiers shown (truststore, signedXmlOptions.getCertFromKeyInfo,
X509Certificate, isArrayHasLength) so the change locates the mapping and replace
the simple trust check with full chain validation that throws the same errors
when the chain is untrusted or any cert is expired/not yet valid.
---
Nitpick comments:
In `@src/signed-xml.ts`:
- Around line 203-206: The constructor currently assigns the caller's
idAttributes array directly to this.idAttributes and then mutates it with
unshift, which alters the caller's options; instead clone the chosen array
before modifying it (use a shallow copy of idAttributes or
SignedXml.getDefaultIdAttributes()) so subsequent unshift on this.idAttributes
does not mutate the original; ensure the code paths around idAttributes and the
idAttribute handling use the cloned array so multiple SignedXml instances don't
accumulate entries.
In `@src/xmldsig-verifier.ts`:
- Around line 109-129: The exported static arrays
(XmlDSigVerifier.defaultHashAlgorithms, defaultAsymmetricSignatureAlgorithms,
defaultSymmetricSignatureAlgorithms, defaultCanonicalizationAlgorithms,
defaultTransformAlgorithms) are public mutable and can be changed by consumers;
make them immutable by freezing their contents (e.g., Object.freeze on each
array and on any nested objects) or expose them via getters that return shallow
copies so callers cannot push/splice the originals; update the static
declarations to use frozen arrays (or return copies) and ensure any code that
composes defaultTransformAlgorithms uses the frozen canonicalization array
rather than sharing a mutable reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17c5ef2f-e6b3-438b-b6fe-44d8ee8ac025
📒 Files selected for processing (19)
XMLDSIG_VERIFIER.mdsrc/exclusive-canonicalization.tssrc/signed-xml.tssrc/types.tssrc/utils.tssrc/xmldsig-verifier.tstest/c14n-non-exclusive-unit-tests.spec.tstest/c14nWithComments-unit-tests.spec.tstest/canonicalization-unit-tests.spec.tstest/document-tests.spec.tstest/hmac-tests.spec.tstest/key-info-tests.spec.tstest/saml-response-tests.spec.tstest/signature-integration-tests.spec.tstest/signature-object-tests.spec.tstest/signature-unit-tests.spec.tstest/utils-tests.spec.tstest/wsfed-metadata-tests.spec.tstest/xmldsig-verifier.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/document-tests.spec.ts
- test/utils-tests.spec.ts
- test/canonicalization-unit-tests.spec.ts
- test/wsfed-metadata-tests.spec.ts
- XMLDSIG_VERIFIER.md
| private findTransformAlgorithm(name: TransformAlgorithmURI) { | ||
| // TODO: remove this fallback (breaking change) | ||
| if (this.TransformAlgorithms == null) { | ||
| return this.findCanonicalizationAlgorithm(name); | ||
| } | ||
| const algo = this.TransformAlgorithms[name]; | ||
| if (algo) { | ||
| return new algo(); | ||
| } else { | ||
| throw new Error(`transform algorithm '${name}' is not supported`); | ||
| } |
There was a problem hiding this comment.
Keep canonicalization lookup reachable when a transform allowlist is configured.
As soon as allowedTransformAlgorithms is provided, this method stops consulting CanonicalizationAlgorithms. Any reference that includes a normal C14N URI then fails unless the caller duplicates those entries into both maps.
🩹 Suggested fix
private findTransformAlgorithm(name: TransformAlgorithmURI) {
- // TODO: remove this fallback (breaking change)
- if (this.TransformAlgorithms == null) {
- return this.findCanonicalizationAlgorithm(name);
- }
- const algo = this.TransformAlgorithms[name];
+ const algo = this.TransformAlgorithms?.[name] ?? this.CanonicalizationAlgorithms[name];
if (algo) {
return new algo();
} else {
throw new Error(`transform algorithm '${name}' is not supported`);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/signed-xml.ts` around lines 539 - 549, findTransformAlgorithm currently
only falls back to findCanonicalizationAlgorithm when TransformAlgorithms is
null, which causes canonicalization URIs to be ignored once an allowlist is
configured; update findTransformAlgorithm so that after checking
this.TransformAlgorithms[name] it falls back to calling
this.findCanonicalizationAlgorithm(name) before throwing. In short: in
findTransformAlgorithm, if a transform entry exists return new algo(); otherwise
call this.findCanonicalizationAlgorithm(name) to allow
CanonicalizationAlgorithms to resolve C14N URIs, and only throw the error if
neither source provides the algorithm.
| // Use the first idAttribute to set the new ID | ||
| const firstIdAttr = this.idAttributes[0]; | ||
| if (typeof firstIdAttr === "string") { | ||
| node.setAttribute(firstIdAttr, id); | ||
| } else { | ||
| if ("prefix" in firstIdAttr && firstIdAttr.prefix) { | ||
| node.setAttributeNS( | ||
| NAMESPACES.xmlns, | ||
| `xmlns:${firstIdAttr.prefix}`, | ||
| firstIdAttr.namespaceUri, | ||
| ); | ||
| node.setAttributeNS( | ||
| firstIdAttr.namespaceUri, | ||
| `${firstIdAttr.prefix}:${firstIdAttr.localName}`, | ||
| id, | ||
| ); | ||
| } else if (typeof firstIdAttr.namespaceUri === "string") { | ||
| throw new Error( | ||
| `Invalid idAttributes[0]: prefix is required when namespaceUri is provided (${firstIdAttr.localName}).`, | ||
| ); | ||
| } else { | ||
| node.setAttribute(firstIdAttr.localName, id); | ||
| } |
There was a problem hiding this comment.
Guard the auto-ID path when idAttributes is empty.
If a caller passes idAttributes: [], firstIdAttr becomes undefined here and Line 1428 throws before you can surface a meaningful error.
🩹 Suggested fix
// Use the first idAttribute to set the new ID
const firstIdAttr = this.idAttributes[0];
+ if (firstIdAttr == null) {
+ throw new Error(
+ "Cannot auto-generate an Id when `idAttributes` is empty. Provide at least one id attribute or use `isEmptyUri`.",
+ );
+ }
if (typeof firstIdAttr === "string") {
node.setAttribute(firstIdAttr, id);
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/signed-xml.ts` around lines 1423 - 1445, The code assumes
this.idAttributes[0] exists when generating an auto-ID; if idAttributes is empty
firstIdAttr will be undefined and cause an unexpected error. Add a guard before
using firstIdAttr (e.g., check this.idAttributes.length === 0 or firstIdAttr ===
undefined) and throw a clear Error indicating that idAttributes must contain at
least one entry (or require a string/IdAttribute object) so the calling code
receives a meaningful message; update the logic around the firstIdAttr variable
and subsequent branches (the block using node.setAttribute, node.setAttributeNS,
and the prefix/namespace checks) so they only run after the presence check.
| const truststore = options.security.truststore.map((cert) => { | ||
| if (typeof cert === "string" || Buffer.isBuffer(cert)) { | ||
| const x509 = new X509Certificate(cert); | ||
| return x509.publicKey; | ||
| } | ||
| return cert.publicKey; | ||
| }); | ||
| const checkCertExpiration = options.security.checkCertExpiration; | ||
|
|
||
| signedXmlOptions.getCertFromKeyInfo = (keyInfo?: Node | null): string | null => { | ||
| const certPem = getCertFromKeyInfo(keyInfo); | ||
| if (!certPem) { | ||
| return null; | ||
| } | ||
|
|
||
| if (checkCertExpiration || isArrayHasLength(truststore)) { | ||
| const x509 = new X509Certificate(certPem); | ||
| if (checkCertExpiration) { | ||
| const now = new Date(); | ||
| if (x509.validTo && new Date(x509.validTo) < now) { | ||
| throw new Error("The certificate used to sign the XML has expired."); | ||
| } | ||
| if (x509.validFrom && new Date(x509.validFrom) > now) { | ||
| throw new Error("The certificate used to sign the XML is not yet valid."); | ||
| } | ||
| } | ||
| if (isArrayHasLength(truststore)) { | ||
| const isTrusted = truststore.some((trustedCert) => { | ||
| if (trustedCert.equals?.(x509.publicKey) || x509.verify(trustedCert)) { | ||
| return true; | ||
| } | ||
| return false; | ||
| }); | ||
| if (!isTrusted) { | ||
| throw new Error("The certificate used to sign the XML is not trusted."); | ||
| } |
There was a problem hiding this comment.
truststore currently does not validate real certificate chains.
This code only trusts the signing cert when it exactly matches a truststore cert or when the leaf cert is directly signed by a truststore key. A normal leaf → intermediate → root chain from <X509Data> still fails because intermediates are never loaded or walked here. That does not match the truststore contract exposed by the new verifier API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/xmldsig-verifier.ts` around lines 323 - 358, The truststore check in
signedXmlOptions.getCertFromKeyInfo currently only compares the leaf cert
against truststore entries or tries a direct signature check, which fails for
normal leaf→intermediate→root chains; update getCertFromKeyInfo to extract any
intermediate certs present in KeyInfo (in addition to the leaf returned by
getCertFromKeyInfo), construct the certificate chain (leaf + intermediates),
validate the chain by walking signatures up to a trust anchor from the
truststore (truststore array entries should be treated as root/intermediate
anchors, not only exact leaf matches), and perform expiration checks (using
X509Certificate.validFrom/validTo) for each cert in the chain; use the same
identifiers shown (truststore, signedXmlOptions.getCertFromKeyInfo,
X509Certificate, isArrayHasLength) so the change locates the mapping and replace
the simple trust check with full chain validation that throws the same errors
when the chain is untrusted or any cert is expired/not yet valid.
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/signed-xml.ts (1)
716-722: Redundant error throw after validation call.The
findCanonicalizationAlgorithmmethod already throwsError('canonicalization algorithm ... is not supported')when the algorithm is not found (lines 519-528). The additionalif (!this.findCanonicalizationAlgorithm(...))check and throw at lines 718-722 will never execute becausefindCanonicalizationAlgorithmthrows before returning a falsy value.♻️ Suggested simplification
if (isDomNode.isAttributeNode(node)) { this.canonicalizationAlgorithm = node.value as CanonicalizationAlgorithmURI; - - if (!this.findCanonicalizationAlgorithm(this.canonicalizationAlgorithm)) { - throw new Error( - `unsupported canonicalization algorithm: ${this.canonicalizationAlgorithm}`, - ); - } + // Validate the algorithm is supported (throws if not) + this.findCanonicalizationAlgorithm(this.canonicalizationAlgorithm); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/signed-xml.ts` around lines 716 - 722, Remove the redundant boolean check and throw after assigning this.canonicalizationAlgorithm: the call to this.findCanonicalizationAlgorithm(this.canonicalizationAlgorithm) already throws if the algorithm is unsupported, so you should call it for validation and not follow it with an additional if/throw. Update the code around the assignment to simply set this.canonicalizationAlgorithm and invoke this.findCanonicalizationAlgorithm(this.canonicalizationAlgorithm) (or remove the extra conditional block that checks its return), referencing the canonicalizationAlgorithm property and the findCanonicalizationAlgorithm method to locate and simplify the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig-verifier.ts`:
- Around line 302-370: Update the documentation in src/types.ts to clarify that
the truststore used by createSignedXml/truststore only enforces direct trust
relationships (matching public key or directly signed by a truststore
certificate) and does not perform full certificate chain path validation;
explicitly note that this limitation is due to Node.js X509Certificate.verify()
only checking direct signatures and not validating intermediate-to-root chains,
and replace wording like "must chain to one of these trusted certificates" with
language such as "must be directly trusted (public key match or directly signed
by) one of these trusted certificates — full chain validation is not performed."
---
Nitpick comments:
In `@src/signed-xml.ts`:
- Around line 716-722: Remove the redundant boolean check and throw after
assigning this.canonicalizationAlgorithm: the call to
this.findCanonicalizationAlgorithm(this.canonicalizationAlgorithm) already
throws if the algorithm is unsupported, so you should call it for validation and
not follow it with an additional if/throw. Update the code around the assignment
to simply set this.canonicalizationAlgorithm and invoke
this.findCanonicalizationAlgorithm(this.canonicalizationAlgorithm) (or remove
the extra conditional block that checks its return), referencing the
canonicalizationAlgorithm property and the findCanonicalizationAlgorithm method
to locate and simplify the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49a3b267-0614-4522-81e2-70f3f5943620
📒 Files selected for processing (19)
XMLDSIG_VERIFIER.mdsrc/exclusive-canonicalization.tssrc/signed-xml.tssrc/types.tssrc/utils.tssrc/xmldsig-verifier.tstest/c14n-non-exclusive-unit-tests.spec.tstest/c14nWithComments-unit-tests.spec.tstest/canonicalization-unit-tests.spec.tstest/document-tests.spec.tstest/hmac-tests.spec.tstest/key-info-tests.spec.tstest/saml-response-tests.spec.tstest/signature-integration-tests.spec.tstest/signature-object-tests.spec.tstest/signature-unit-tests.spec.tstest/utils-tests.spec.tstest/wsfed-metadata-tests.spec.tstest/xmldsig-verifier.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/c14n-non-exclusive-unit-tests.spec.ts
- test/wsfed-metadata-tests.spec.ts
- XMLDSIG_VERIFIER.md
- test/signature-object-tests.spec.ts
| private static createSignedXml(options: ResolvedXmlDsigVerifierOptions): SignedXml { | ||
| const signedXmlOptions: SignedXmlOptions = { | ||
| publicCert: undefined as KeyLike | undefined, | ||
| getCertFromKeyInfo: undefined as KeySelectorFunction | undefined, | ||
| idAttributes: options.idAttributes, | ||
| maxTransforms: options.security.maxTransforms, | ||
| implicitTransforms: options.implicitTransforms, | ||
| allowedSignatureAlgorithms: options.security.signatureAlgorithms, | ||
| allowedHashAlgorithms: options.security.hashAlgorithms, | ||
| allowedTransformAlgorithms: options.security.transformAlgorithms, | ||
| allowedCanonicalizationAlgorithms: options.security.canonicalizationAlgorithms, | ||
| }; | ||
|
|
||
| if (isResolvedKeyInfoOptions(options)) { | ||
| const keySelector = options.keySelector; | ||
|
|
||
| if (typeof keySelector.getCertFromKeyInfo !== "function") { | ||
| throw new Error("XmlDSigVerifier requires a valid getCertFromKeyInfo function."); | ||
| } | ||
|
|
||
| const getCertFromKeyInfo = keySelector.getCertFromKeyInfo; | ||
| const truststore = options.security.truststore.map((cert) => { | ||
| if (typeof cert === "string" || Buffer.isBuffer(cert)) { | ||
| const x509 = new X509Certificate(cert); | ||
| return x509.publicKey; | ||
| } | ||
| return cert.publicKey; | ||
| }); | ||
| const checkCertExpiration = options.security.checkCertExpiration; | ||
|
|
||
| signedXmlOptions.getCertFromKeyInfo = (keyInfo?: Node | null): string | null => { | ||
| const certPem = getCertFromKeyInfo(keyInfo); | ||
| if (!certPem) { | ||
| return null; | ||
| } | ||
|
|
||
| if (checkCertExpiration || isArrayHasLength(truststore)) { | ||
| const x509 = new X509Certificate(certPem); | ||
| if (checkCertExpiration) { | ||
| const now = new Date(); | ||
| if (x509.validTo && new Date(x509.validTo) < now) { | ||
| throw new Error("The certificate used to sign the XML has expired."); | ||
| } | ||
| if (x509.validFrom && new Date(x509.validFrom) > now) { | ||
| throw new Error("The certificate used to sign the XML is not yet valid."); | ||
| } | ||
| } | ||
| if (isArrayHasLength(truststore)) { | ||
| const isTrusted = truststore.some((trustedCert) => { | ||
| if (trustedCert.equals?.(x509.publicKey) || x509.verify(trustedCert)) { | ||
| return true; | ||
| } | ||
| return false; | ||
| }); | ||
| if (!isTrusted) { | ||
| throw new Error("The certificate used to sign the XML is not trusted."); | ||
| } | ||
| } | ||
| } | ||
| return certPem; | ||
| }; | ||
| } else if (isResolvedPublicCertOptions(options)) { | ||
| signedXmlOptions.publicCert = options.keySelector.publicCert; | ||
| } else if (isResolvedSharedSecretOptions(options)) { | ||
| signedXmlOptions.privateKey = options.keySelector.sharedSecretKey; | ||
| } | ||
|
|
||
| return new SignedXml(signedXmlOptions); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js X509Certificate verify method full chain validation documentation
💡 Result:
- In Node.js,
X509Certificate.prototype.verify(publicKey)only verifies the certificate’s signature against the provided public key (i.e., “was this cert signed by the private key corresponding to this public key?”) and “does not perform any other validation checks”—so it does not do full chain/path validation (trust anchors, EKU/KU, hostname, revocation, etc.). [1] - Node’s full certificate chain validation is performed by the TLS stack during a TLS handshake (e.g.,
tls.connect(),https.request()), which is where you’ll see standard chain-validation failures likeUNABLE_TO_VERIFY_LEAF_SIGNATURE,SELF_SIGNED_CERT_IN_CHAIN, etc. [2] - For trust configuration used by TLS validation (system roots, extra CAs), see Node’s CA configuration options like
--use-system-ca,NODE_EXTRA_CA_CERTS, and programmatic CA store management. [3] - If you need to do offline / manual chain validation (without making a TLS connection), Node’s
X509CertificateAPI doesn’t provide a built-in “validate chain to trusted roots” function; you’d typically use OpenSSL’s path validation (e.g.,X509_verify_cert) via native bindings or an external tool/library. [4]
Sources: Node.js crypto docs for x509.verify [1]; Node.js tls docs (certificate chain errors / chain exposure) [2]; Node.js Enterprise Network Configuration (CA trust for TLS validation) [3]; OpenSSL X509_verify_cert (chain discovery/validation) [4].
🏁 Script executed:
# First, let's examine the actual file to see if there's any documentation
head -320 src/xmldsig-verifier.ts | tail -30Repository: node-saml/xml-crypto
Length of output: 1290
🏁 Script executed:
# Check for any JSDoc or comments around the truststore validation logic
sed -n '340,365p' src/xmldsig-verifier.tsRepository: node-saml/xml-crypto
Length of output: 1133
🏁 Script executed:
# Look for any type definitions or documentation files related to truststore
fd -type f -name "*.ts" -o -name "*.md" | xargs grep -l "truststore" 2>/dev/null | head -10Repository: node-saml/xml-crypto
Length of output: 234
🏁 Script executed:
# Correct search for files mentioning truststore
fd -type f \( -name "*.ts" -o -name "*.md" \) | xargs grep -l "truststore" 2>/dev/null | head -10Repository: node-saml/xml-crypto
Length of output: 234
🏁 Script executed:
# Check if there's any documentation comment or JSDoc in the createSignedXml method
sed -n '295,375p' src/xmldsig-verifier.ts | head -80Repository: node-saml/xml-crypto
Length of output: 3290
🏁 Script executed:
# Look for any README or documentation about security/truststore
fd -name "README*" -o -name "SECURITY*" -o -name "*security*" | head -5Repository: node-saml/xml-crypto
Length of output: 294
🏁 Script executed:
# Find documentation files
find . -maxdepth 2 -type f \( -name "README*" -o -name "*security*" \)Repository: node-saml/xml-crypto
Length of output: 75
🏁 Script executed:
# Look for any comments or documentation about truststore in the codebase
rg -i "truststore" -A 3 -B 3 --type tsRepository: node-saml/xml-crypto
Length of output: 11889
🏁 Script executed:
# Check if there's any JSDoc or inline documentation about the security/truststore feature
rg "truststore" -B 10 src/xmldsig-verifier.ts | head -40Repository: node-saml/xml-crypto
Length of output: 1517
🏁 Script executed:
# Check the actual documentation in types.ts for the truststore description
rg -A 3 "truststore" src/types.tsRepository: node-saml/xml-crypto
Length of output: 665
Clarify truststore documentation: only validates direct trust, not full certificate chains.
The truststore validation at lines 349-359 only validates direct trust (checking if the signing certificate's public key matches a truststore entry or was directly signed by one). It does not support full certificate chain validation (leaf → intermediate → root), as Node.js X509Certificate.verify() only verifies direct signatures, not chain paths.
The current documentation in src/types.ts stating "the certificate used to sign the XML must chain to one of these trusted certificates" is misleading and should be updated to clarify that only direct trust relationships are validated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/xmldsig-verifier.ts` around lines 302 - 370, Update the documentation in
src/types.ts to clarify that the truststore used by createSignedXml/truststore
only enforces direct trust relationships (matching public key or directly signed
by a truststore certificate) and does not perform full certificate chain path
validation; explicitly note that this limitation is due to Node.js
X509Certificate.verify() only checking direct signatures and not validating
intermediate-to-root chains, and replace wording like "must chain to one of
these trusted certificates" with language such as "must be directly trusted
(public key match or directly signed by) one of these trusted certificates —
full chain validation is not performed."
There was a problem hiding this comment.
Leaf cert in truststore = trusted.
The user controls what goes in the truststore. If they put a leaf cert there, they're explicitly trusting it.
There was a problem hiding this comment.
@shunkica, understood — the truststore is a user-controlled allow-list, so placing a leaf cert in it is an explicit direct-trust assertion. That's a valid and simple model.
The only residual concern is that the documentation in src/types.ts currently says "must chain to one of these trusted certificates", which carries PKI chain-validation connotations for many readers. A small wording tweak like "must be directly trusted by (public key match or directly signed by) one of these trusted certificates" would prevent confusion without changing any behavior. That said, I'll leave it to your discretion.
(ᓀ ᓀ)
✏️ Learnings added
Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 519
File: src/xmldsig-verifier.ts:302-370
Timestamp: 2026-03-10T07:22:43.317Z
Learning: In node-saml/xml-crypto PR `#519` (src/xmldsig-verifier.ts), the `truststore` option in `XmlDSigVerifier` intentionally implements a direct-trust model only: a certificate in the truststore is explicitly trusted by the user (leaf cert in truststore = trusted). Full PKI certificate chain validation is deliberately out of scope. Node.js `X509Certificate.verify()` only checks direct signatures, not intermediate-to-root chains, and this is acceptable per the maintainer (shunkica).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 519
File: src/signed-xml.ts:1430-1448
Timestamp: 2026-03-05T17:13:35.483Z
Learning: In node-saml/xml-crypto (src/types.ts), `SignatureIdAttributeType` is a discriminated union: `{ prefix?: undefined; localName: string; namespaceUri?: null }` enforces that omitting `prefix` also requires `namespaceUri` to be null/omitted, so no namespace-string can exist without a prefix. The `ensureHasId` fallback `node.setAttribute(localName, id)` in `src/signed-xml.ts` is therefore always namespace-free and correct.
Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.354Z
Learning: In node-saml/xml-crypto PR `#506`, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 519
File: example/new-api-example.js:1-9
Timestamp: 2025-10-25T14:41:13.728Z
Learning: Example files in the node-saml/xml-crypto repository should use `require("xml-crypto")` (the package name) rather than relative paths to build artifacts, since they demonstrate usage from an end-user's perspective.
Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 0
File: :0-0
Timestamp: 2025-10-22T21:50:05.454Z
Learning: The current Reference fields are defined in src/types.ts Lines 109–168: xpath?, transforms, digestAlgorithm, uri, digestValue?, inclusiveNamespacesPrefixList, isEmptyUri, ancestorNamespaces?, validationError?, getValidatedNode(), signedReference?.
Learnt from: shunkica
Repo: node-saml/xml-crypto PR: 506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.758Z
Learning: In node-saml/xml-crypto PR `#506`, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
| // TODO(v7): remove SHA-1 from default hash algorithms. | ||
| static readonly defaultHashAlgorithms = [Sha1, Sha256, Sha512]; | ||
| static readonly defaultAsymmetricSignatureAlgorithms = [ | ||
| // TODO(v7): remove RSA-SHA1 from default signature algorithms. |
There was a problem hiding this comment.
Next release is going to be v7(?) ... should this todo and one at the line 109 be marked for v8?
There was a problem hiding this comment.
I think it will be done in a different PR.
| } | ||
| } | ||
| } | ||
| return certPem; |
There was a problem hiding this comment.
If user of this library does not provide any CAs (i.e. if truststore is empty array when keyinfo "mode" is used for verification) does this code return certPem from input controllable by attacker for signature verification purposes without any validation of trustfullness of certPem? Based on very quick glance this looks a lot like another case / new version of GHSA-2xp3-57p7-qf4v (which was caused when cert from input was used as-is without any additional checks for signature verification).
Disclaimers:
- I did not have time to read whole PR and current terminal is mobile phone which almost worst possible tool to read PRs so I am not going to try to review this any further. Spotting usage of getCertFromKeyInfo raised eye browses due to history attached to it
- I did not spot any guards against situation that truststore is empty when keyinfo is used
There was a problem hiding this comment.
Empty truststore = trust everything.
The truststore is opt-in additional security added by this PR. If a user doesn't provide one, the behavior is basically the same as prior to this PR.
IMO this is not GHSA-2xp3-57p7-qf4v because that was about the default behavior being insecure - here the user explicitly has to choose getCertFromKeyInfo.
We could add some kind of trustAllCerts option but IMO that would unnecessarily pollute the interface.
There was a problem hiding this comment.
Firstly, I think the function getCertFromKeyInfo should have a better name or description. The point of the function is to return a public key that has been proven to be trusted. So a function like getAuthenticatedCert or such a description would make it clearer. Currently, a user could mis-interpret it and return any untrusted certificate, allowing the signature to be signed with the untrusted certificate.
With regards to the current implementation of the function, i don't think empty truststore should imply trust everything. When user didn't provide a trust store, it means that they don't trust anything (i.e. no certificates are trusted). This follows the approach of secure-by-default.
The implementation should clearly show that the certificate inside keyInfo is trusted, (I'll have to review the final implementation, but currently it's not very clear, since it uses the unverified certificate).
The truststore is opt-in additional security added by this PR. If a user doesn't provide one, the behavior is basically the same as prior to this PR.
Here's the behavior:
const trustedRootCert = fs.readFileSync("root_ca.pem", "utf-8");
const result = XmlDSigVerifier.verifySignature(xml, {
keySelector: {
// Extract the certificate from KeyInfo
getCertFromKeyInfo: (keyInfo) => SignedXml.getCertFromKeyInfo(keyInfo),
},
security: {
// Ensure the certificate is trusted by your root CA
truststore: [trustedRootCert],
// Automatically check if the certificate is expired
checkCertExpiration: true,
},
});Then
if (isResolvedKeyInfoOptions(options)) {
// ... our interesting codeBased on my understanding, is that user can configure to use trust store to authenticate the certificate inside keyInfo.
Here, the optionType is 'keyinfo' for it to go through the path. I think a better name would be 'truststore', the client should never use the keyinfo to authenticate, rather it's using the X509 trust store to authenticate the keyinfo.
I am also concerned about:
keySelector: {
// Extract the certificate from KeyInfo
getCertFromKeyInfo: (keyInfo) => SignedXml.getCertFromKeyInfo(keyInfo),
},It seems like the getCertFromKeyInfo function in keySelector is used to extract the unverified certificate (but not to perform any authentication on it). I think you should rename it appropiately, e.g. extractUnverifiedCertificateFromKeyInfo ..., and I think we also need to re-name SignedXml.getCertFromKeyInfo to reflect that it is only extracting it. Otherwise, a user could get confused as there are two functions with same name.
There was a problem hiding this comment.
We could add some kind of trustAllCerts option but IMO that would unnecessarily pollute the interface.
What would be use case for that (possibility to disable xml-crypto's core functionality i.e. signature verification)? Some test scenarios of client SW? If someone is testing consuming incoming signed documents then surely that someone is able to add certificate (of key used to sign messages in tests) to truststore or to use publicCert alternative. If that someone really wants to disable signature verification than situation is IMHO same as not using xml-crypto at all in processing flow and just consume incoming XML as-is but such verification bypass should be behind client SW's internal feature flag. Difference is that xml-crypto would not provide/enable such "footgun" possibility.
If xml-crypto lib includes possibility to disable signature verification (regardless of how such possibility is implemented) then there should be constant stream of very annoying warn / error level log events because xml-crypto is not able to do what it was designed to do and why it was introduced to be part of SW stack.
4 participants
As per the discussion #517 I set out to do some work on this problematic.
My conclusion was that it would be best to not just separate the reference types, but separate the signer from the validator entirely, while providing a more streamlined interface.
I am submitting this initial draft for your review and opinions.
The goal was to not introduce any breaking change, instead produce these wrappers so users can slowly start migrating toward them. The next step would be to deprecate
SignedXml, migrate the specific signature/validation functions in their respective class, with the final breaking change being to removeSignedXmlentirely.The tests are currently just AI slop. I welcome anyone that wants to contribute.
Once the XmlDSigSigner and XmlDSigValidator are configured, they can be reused to sign/validate multiple xml files, since each time sign/compute is called a new SignedXml instance is created internally.
Summary by CodeRabbit
Release Notes
New Features
Documentation