Add support for inserting and signing Object elements inside the Signature #506
Conversation
shunkica
force-pushed
the
signature-object
branch
from
77f860c to
b10a651
Compare
|
@cjbarth I am happy with this PR. Let me know if you need any changes for this to be merged. |
shunkica
force-pushed
the
signature-object
branch
from
50d8350 to
014b0fb
Compare
|
I have added the option to specify custom This is required to allow XAdES implementations because the Reference must contain |
|
@cjbarth I have addressed your review comments and I am keen to get this merged. Additionally, in an effort to simplify using this functionality, I have removed the need for the |
WalkthroughAdds ds:Object embedding via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant SX as SignedXml
participant DOM as DOM
participant C14N as Canonicalizer
participant DG as Digest
Caller->>SX: new SignedXml({ objects?, ... })
Caller->>SX: addReference({ xpath, transforms, digestAlgorithm, id?, type? })
Caller->>SX: computeSignature(xml, options)
SX->>DOM: Insert Signature placeholder (signatureElem)
SX->>DOM: getObjects() -> append ds:Object elements to signatureElem
SX->>SX: addAllReferences() %% DOM-based reference assembly
loop for each Reference
SX->>DOM: locate target node (xpath or URI)
alt found
SX->>C14N: apply transforms & canonicalize
C14N-->>SX: canonicalized data
SX->>DG: compute digest
DG-->>SX: DigestValue
SX->>DOM: append Reference with URI, Id, Type, Transforms, DigestMethod, DigestValue
else not found
SX-->>Caller: throw missing xpath/URI error
end
end
SX->>C14N: canonicalize SignedInfo
SX->>DG: compute SignatureValue
SX-->>DOM: set SignatureValue and finalize Signature
SX-->>Caller: return signed XML
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential areas to focus review on:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/signed-xml.ts (1)
1134-1184: CreateReferences: validate Id/Type to avoid malformed XML in attributes
ref.idandref.typeare appended as raw attribute values. If they contain quotes or angle brackets, the signature becomes invalid XML.Apply this diff to validate before concatenation:
- if (ref.id) { + if (ref.id) { + if (/[<>&"]/.test(ref.id)) { + throw new Error('Invalid character in Reference Id attribute'); + } referenceAttrs += ` Id="${ref.id}"`; } - if (ref.type) { + if (ref.type) { + if (/[<>&"]/.test(ref.type)) { + throw new Error('Invalid character in Reference Type attribute'); + } referenceAttrs += ` Type="${ref.type}"`; }Also, consider switching this method to DOM construction (like
processSignatureReferences) for consistency and to avoid manual string concatenation issues.
🧹 Nitpick comments (8)
src/types.ts (1)
46-60: New ObjectAttributes type looks good; consider clarifying allowed attribute namesThe interface aligns with xmldsig spec (Id, MimeType, Encoding) and allows custom attributes. No functional issues spotted.
To reduce ambiguity, add a brief note in the JSDoc that attribute names are emitted verbatim and consumers must ensure XML-safe values (no quotes, angle brackets, or ampersands), or that the library will validate/escape them (see suggested changes in src/signed-xml.ts).
test/signature-unit-tests.spec.ts (1)
1282-1319: Good coverage for Reference Id/Type attributesThis test ensures Id and Type are present when provided. Consider adding a negative test asserting they’re omitted when not set, and one that guards against invalid characters (quotes/angle brackets) if you add validation as suggested.
README.md (2)
264-265: Docs: objects option clearly documentedThe new
objectsoption is well explained. To set expectations, consider adding a note that attribute values must be valid XML attribute values (no quotes, <, >, &), or that the library will throw/escape.
540-575: Great new section on adding custom Objects; add a short safety noteExample is clear and mirrors API. Add one sentence reminding users that
contentis inserted as-is; provide guidance to wrap non-XML content appropriately (e.g., base64 with Encoding/MimeType) or ensure it’s valid XML.test/signature-object-tests.spec.ts (2)
94-101: Rename test to reflect current API (objects), not getObjectContentThe description mentions getObjectContent but the code uses the
objectsoption. Rename for clarity.Apply this diff:
- it("should handle empty or null getObjectContent", function () { + it("should handle missing or empty objects option", function () {
223-313: SHA256 path is sound; consider direct use of registered algorithm instead of importing classYou validate both Reference DigestMethod and SignatureMethod URIs and end-to-end signature. As a minor cleanup, since the algorithms are already registered within SignedXml, you could avoid importing
Sha256entirely by hashing using Node crypto directly for the XAdES case (or import from src, per above fix).src/signed-xml.ts (2)
994-996: Second-pass reference processing is a nice simplificationDeferring unresolved references and appending them later removes the need for a special flag and supports signature-internal references. Be sure to update
wasProcessedin this pass (see below).
1324-1436: Finalizing unprocessed references: mark processed and keep model consistentAfter appending Reference nodes, mark each ref as processed and (optionally) set
ref.digestValueto the computed value to keep in-memory state aligned with the XML.Apply this diff at the end of the inner loop and after processing each ref:
- // Append the reference element to SignedInfo + // Append the reference element to SignedInfo signedInfoNode.appendChild(referenceElem); + // Update in-memory model + ref.digestValue = digestValueElem.textContent; + ref.wasProcessed = true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
README.md(2 hunks)src/signed-xml.ts(14 hunks)src/types.ts(3 hunks)test/signature-object-tests.spec.ts(1 hunks)test/signature-unit-tests.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/signature-object-tests.spec.ts (2)
src/signed-xml.ts (1)
SignedXml(30-1463)src/hash-algorithms.ts (1)
Sha256(17-28)
src/signed-xml.ts (1)
src/types.ts (2)
ObjectAttributes(50-59)Reference(124-160)
🔇 Additional comments (9)
src/types.ts (2)
76-76: Option for objects on SignedXmlOptions is a solid API surfaceIntroducing
objects?: Array<{ content: string; attributes?: ObjectAttributes }>is clear and future-proof. It also keeps the signing data close to the signer config, which is nice.
146-151: Expose Reference Id/Type (Id/Type attributes) — OK; ensure downstream escapes/validationExtending Reference with
id?: stringandtype?: stringis correct and matches xmldsig-core1. Please ensure these values are validated/escaped where rendered (see src/signed-xml.ts comments) to avoid producing malformed XML when values contain quotes or angle brackets.README.md (1)
272-278: Docs: addReference now includes id/type — goodThis clarifies the API expansion and XAdES use case for Type. Looks consistent with the implementation.
test/signature-object-tests.spec.ts (2)
155-167: Base64 content under Encoding is correctly exercisedGood scenario covering Object Encoding/MimeType/Id and end-to-end validity.
669-745: Excellent XAdES flow coverageThe test builds QualifyingProperties in an Object, references SignedProperties with Type, and uses SHA-256 RSA-SHA256; also verifies with public cert. This should help prevent regressions.
src/signed-xml.ts (4)
60-61: Public objects property: OKExposing
objects?: Array<{ content: string; attributes?: ObjectAttributes }>on the class is consistent with options and tests. No issues here.
75-75: Track wasProcessed on Reference — OK, but ensure second pass updates itThe new flag is useful to avoid double-processing. In the second-pass
processSignatureReferences, remember to mark processed references as well (see suggestion later in that method).
979-981: Including Object elements in the Signature: good placement
this.getObjects(prefix)is inserted after KeyInfo and before closing Signature. This matches spec flexibility for ds:Object placement.
1189-1216: Transform pipeline: OK; note on enveloped-signature in second passBecause
getCanonReferenceXmlsetsoptions.signatureNode = this.signatureNode, andthis.signatureNodeis only initialized after this method (in computeSignature), references that include the enveloped-signature transform in the second pass would not remove the signature element. Current tests don’t hit that path, but it’s a corner case.If you plan to support enveloped-signature in second pass, set
this.signatureNode = signatureDoc.documentElementjust before callingprocessSignatureReferences, then restore it after — or refactorgetCanonReferenceXmlto accept an explicit signature node via options.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
test/signature-object-tests.spec.ts (8)
55-61: Select ds:Object by namespace and scope it under ds:SignatureCurrent XPath matches any Object in any namespace. Tighten it to ensure we only validate ds:Object elements within ds:Signature.
Apply this change (and consider the same pattern in other tests that select Object elements):
- const objectNodes = xpath.select("//*[local-name(.)='Object']", doc); + const objectNodes = xpath.select( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']/*[local-name(.)='Object' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + doc, + );
69-75: Avoid brittle exact text match for Object contentDirect equality can fail if the serializer inserts whitespace. Trimming makes the check resilient.
- expect(secondObject.textContent).to.equal("Plain text content"); + expect(secondObject.textContent?.trim()).to.equal("Plain text content");
94-94: Rename test to reflect what it validates (objects option, not getObjectContent)This test doesn’t exercise getObjectContent; it verifies undefined/empty objects option. Renaming improves clarity. If you want, I can add a separate test that exercises getObjectContent().
- it("should handle empty or null getObjectContent", function () { + it("should handle undefined or empty objects option", function () {
121-123: Prefer isDomNode assertions and namespace-scoped XPath for consistencyKeep type guards consistent and ensure the selection targets ds:Object under ds:Signature.
- const objectNodesWithNull = xpath.select("//*[local-name(.)='Object']", docWithNull); - expect(objectNodesWithNull).to.be.an("array").that.is.empty; + const objectNodesWithNull = xpath.select( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']/*[local-name(.)='Object' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + docWithNull, + ); + isDomNode.assertIsArrayOfNodes(objectNodesWithNull); + expect(objectNodesWithNull.length).to.equal(0);
147-149: Repeat consistent type guards and namespace-scoped XPathMirror the previous change for the “empty objects” branch.
- const objectNodesWithEmpty = xpath.select("//*[local-name(.)='Object']", docWithEmpty); - expect(objectNodesWithEmpty).to.be.an("array").that.is.empty; + const objectNodesWithEmpty = xpath.select( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']/*[local-name(.)='Object' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + docWithEmpty, + ); + isDomNode.assertIsArrayOfNodes(objectNodesWithEmpty); + expect(objectNodesWithEmpty.length).to.equal(0);
589-667: This test largely duplicates the previous “add a reference to an Object element” caseBoth tests set up one Object, two references (doc + object), and verify presence and validity. Consider consolidating to reduce suite run time and maintenance.
731-745: Add assertions for XAdES: SignedProperties Reference Type/URI and QualifyingProperties TargetStrengthen the XAdES test by asserting the SignedProperties Reference has the correct Type and URI and that QualifyingProperties.Target points to the Signature Id. Also assert there are exactly two SignedInfo/Reference elements.
const signedXml = sig.getSignedXml(); const signedDoc = new xmldom.DOMParser().parseFromString(signedXml); const signatureNode = xpath.select1( "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", signedDoc, ); isDomNode.assertIsNodeLike(signatureNode); + // Validate references and XAdES wiring + const referenceNodes = xpath.select( + "//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference']", + signedDoc, + ); + isDomNode.assertIsArrayOfNodes(referenceNodes); + expect(referenceNodes.length).to.equal(2); + + const signedPropsRef = xpath.select( + `//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference' and @Type='http://uri.etsi.org/01903#SignedProperties' and @URI='#${signedPropertiesId}']`, + signedDoc, + ); + isDomNode.assertIsArrayOfNodes(signedPropsRef); + expect(signedPropsRef.length).to.equal(1); + + const qualifyingProps = xpath.select( + \`//*[local-name(.)='Object']/*[local-name(.)='QualifyingProperties' and @Target='#\${signatureId}']\`, + signedDoc, + ); + isDomNode.assertIsArrayOfNodes(qualifyingProps); + expect(qualifyingProps.length).to.equal(1); + const verifier = new SignedXml({ publicCert: publicCert, });
9-667: Minor: prefer modern algorithms where practicalYou already have SHA-256 and SHA-512 coverage—great. Consider defaulting to SHA-256 for non-legacy tests to discourage SHA-1 usage in new code paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/signature-object-tests.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/signature-object-tests.spec.ts (2)
src/signed-xml.ts (1)
SignedXml(30-1463)src/hash-algorithms.ts (1)
Sha256(17-28)
🔇 Additional comments (2)
test/signature-object-tests.spec.ts (2)
1-8: Sha256 import path corrected (src) — good catchUsing ../src/hash-algorithms aligns with the rest of the suite and avoids CI path issues. Looks good.
223-297: Good coverage for SHA-256 flow (two references, digests, and signature method)This test exercises both document and Object references with SHA-256 and validates digest and signature algorithms. Solid.
shunkica
force-pushed
the
signature-object
branch
from
e151d31 to
9fd58a5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #506 +/- ##
==========================================
+ Coverage 73.05% 75.73% +2.67%
==========================================
Files 9 9
Lines 902 1026 +124
Branches 239 272 +33
==========================================
+ Hits 659 777 +118
+ Misses 143 136 -7
- Partials 100 113 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/signature-object-tests.spec.ts (1)
7-7: Sha256 import path corrected to src — resolvedUsing "../src/hash-algorithms" matches the repo’s pattern and avoids CI/ts-node path issues previously flagged.
🧹 Nitpick comments (6)
test/signature-object-tests.spec.ts (6)
13-25: Surface validation errors in helper for faster debuggingWhen a signature check fails, the tests will only report “expected true” without context. Emitting
validationErrorshere greatly speeds up failure triage while keeping the boolean contract.verifier.loadSignature(signatureNode); - return verifier.checkSignature(signedXml); + const ok = verifier.checkSignature(signedXml); + if (!ok && Array.isArray((verifier as any).validationErrors) && (verifier as any).validationErrors.length) { + // Keep returning a boolean so existing assertions remain unchanged, + // but surface details to aid debugging if a test fails. + // eslint-disable-next-line no-console + console.error("Signature validation errors:", (verifier as any).validationErrors); + } + return ok;
86-133: Prefer DOM-aware assertions for consistency and type-safetyElsewhere you assert the XPath result type with
assertIsArrayOfNodes. Mirror that here for consistency and clearer failures.- const objectNodesWithNull = xpath.select("//*[local-name(.)='Object']", docWithNull); - expect(objectNodesWithNull).to.be.an("array").that.is.empty; + const objectNodesWithNull = xpath.select("//*[local-name(.)='Object']", docWithNull); + isDomNode.assertIsArrayOfNodes(objectNodesWithNull); + expect(objectNodesWithNull.length).to.equal(0); @@ - const objectNodesWithEmpty = xpath.select("//*[local-name(.)='Object']", docWithEmpty); - expect(objectNodesWithEmpty).to.be.an("array").that.is.empty; + const objectNodesWithEmpty = xpath.select("//*[local-name(.)='Object']", docWithEmpty); + isDomNode.assertIsArrayOfNodes(objectNodesWithEmpty); + expect(objectNodesWithEmpty.length).to.equal(0);
180-246: Reduce duplication via a parameterized test for SHA256/SHA512 variantsBoth tests are identical aside from algorithms. Consider a small matrix to DRY and make it easier to extend with more variants later.
Example:
[ { name: "SHA256", signatureAlg: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", digestAlg: "http://www.w3.org/2001/04/xmlenc#sha256", }, { name: "SHA512", signatureAlg: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha512", digestAlg: "http://www.w3.org/2001/04/xmlenc#sha512", }, ].forEach(({ name, signatureAlg, digestAlg }) => { it(`should sign Object with ${name} digest algorithm and RSA-${name} signature`, () => { // construct SignedXml with signatureAlg, add references with digestAlg, then assert as you do now }); });Also applies to: 247-312
443-494: Test appears redundant with the previous “add a reference to an Object element”This test exercises essentially the same flow; consider merging to keep the suite tight and reduce runtime.
496-531: Strengthen InclusiveNamespaces assertion using XPath (avoid brittle string match)Prefer asserting the actual element and attribute via XPath and DOM. Also assert the object reference via XPath.
sig.computeSignature(xml); const signedXml = sig.getSignedXml(); - - // Verify that the Object element is present - expect(signedXml).to.include('<InclusiveNamespaces PrefixList="ns1 ns2"'); - // Verify that the Reference URI is correct - expect(signedXml).to.include('URI="#object1"'); - - // Verify that the signature is valid - expect(checkSignature(signedXml)).to.be.true; + const doc = new xmldom.DOMParser().parseFromString(signedXml); + + // Verify InclusiveNamespaces element and PrefixList value + const incNs = xpath.select1( + "//*[local-name(.)='InclusiveNamespaces' and namespace-uri(.)='http://www.w3.org/2001/10/xml-exc-c14n#']", + doc, + ); + isDomNode.assertIsElementNode(incNs); + expect(incNs.getAttribute("PrefixList")).to.equal("ns1 ns2"); + + // Verify that the Reference URI points to the Object + const objectRef = xpath.select1("//*[local-name(.)='Reference' and @URI='#object1']", doc); + isDomNode.assertIsElementNode(objectRef); + + // Verify that the signature is valid + expect(checkSignature(signedXml, doc)).to.be.true;
534-598: Add structural XAdES assertions (Type on Reference, Target on QualifyingProperties, SignedProperties Id)The signature validity check is great, but it would be stronger to assert that:
- The SignedProperties Reference carries the XAdES Type.
- QualifyingProperties targets the Signature’s Id.
- SignedProperties has the expected Id.
- const signedXml = sig.getSignedXml(); - - // Verify that the signature is valid - expect(checkSignature(signedXml)).to.be.true; + const signedXml = sig.getSignedXml(); + const doc = new xmldom.DOMParser().parseFromString(signedXml); + + // QualifyingProperties should target the signature Id + const qp = xpath.select1( + "//*[local-name(.)='QualifyingProperties' and namespace-uri(.)='http://uri.etsi.org/01903/v1.3.2#']", + doc, + ); + isDomNode.assertIsElementNode(qp); + expect(qp.getAttribute("Target")).to.equal(`#${signatureId}`); + + // SignedProperties should have expected Id + const sp = xpath.select1("//*[local-name(.)='SignedProperties']", doc); + isDomNode.assertIsElementNode(sp); + expect(sp.getAttribute("Id")).to.equal(signedPropertiesId); + + // Reference to SignedProperties should carry the XAdES Type and correct URI + const spRef = xpath.select1( + "//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference' and @Type='http://uri.etsi.org/01903#SignedProperties' and @URI='#" + + signedPropertiesId + + "']", + doc, + ); + isDomNode.assertIsElementNode(spRef); + + // Verify that the signature is valid + expect(checkSignature(signedXml, doc)).to.be.true;Also consider adding a small test asserting that a custom Reference
idset viaaddReference({ id: 'ref-1', ... })ends up on ds:Reference asId="ref-1", since this is part of the new API surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
test/signature-object-tests.spec.ts(1 hunks)test/signature-unit-tests.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/signature-unit-tests.spec.ts
🔇 Additional comments (3)
test/signature-object-tests.spec.ts (3)
27-85: LGTM: solid coverage for basic ds:Object insertion and verificationGood end-to-end check for attributes, content, and successful verification.
134-179: LGTM: base64 Encoding attribute path is exercised wellGood check for Id, MimeType, Encoding, and base64 content; verification passes.
382-441: LGTM: verifies reference to Object is included in SignedInfoClear assertion that the Reference with
@URI='#object1'lives under SignedInfo.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/signature-object-tests.spec.ts (1)
7-7: Fixed import path for Sha256 — thanks for addressing earlier feedbackImport now correctly targets ../src/hash-algorithms, aligning with the rest of the test suite and avoiding CI path issues.
🧹 Nitpick comments (5)
test/signature-object-tests.spec.ts (5)
13-25: Improve signature verification diagnostics in testsWhen verification fails, the current helper only returns false and hides the underlying validation errors. Emitting the library’s validation errors makes triage much faster.
Apply this diff to surface errors on failure:
- verifier.loadSignature(signatureNode); - return verifier.checkSignature(signedXml); + verifier.loadSignature(signatureNode); + const ok = verifier.checkSignature(signedXml); + if (!ok) { + const errors = + typeof (verifier as any).getValidationErrors === "function" + ? (verifier as any).getValidationErrors() + : (verifier as any).validationErrors || []; + throw new Error(`Signature verification failed: ${JSON.stringify(errors)}`); + } + return true;
59-84: Assert that ds:Object entries are referenced by SignedInfo (exercise auto object reference creation)Per PR goals, Objects should be created and referenced automatically. This test validates Object presence and overall signature validity, but not that SignedInfo contains references to the ds:Object Ids. Adding assertions here tightens coverage on the new behavior.
Apply this diff to assert the object references exist in SignedInfo:
const doc = new xmldom.DOMParser().parseFromString(signedXml); // Should have two Object elements const objectNodes = xpath.select("//*[local-name(.)='Object']", doc); isDomNode.assertIsArrayOfNodes(objectNodes); expect(objectNodes.length).to.equal(2); + // Objects should be referenced from SignedInfo + const refObj1 = xpath.select( + "//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference' and @URI='#object1']", + doc, + ); + const refObj2 = xpath.select( + "//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference' and @URI='#object2']", + doc, + ); + isDomNode.assertIsArrayOfNodes(refObj1); + isDomNode.assertIsArrayOfNodes(refObj2); + expect(refObj1.length).to.equal(1); + expect(refObj2.length).to.equal(1); + // Verify the first Object element
327-386: Reduce duplication with the next testThis test and the one below both add a reference to the Object by Id and assert similar invariants. Consider merging them or parameterizing differences to cut duplication and speed up the suite.
388-439: Duplicate of previous scenarioThis is functionally identical to “should add a reference to an Object element”. Recommend consolidating to a single parametrized test to avoid drift.
488-552: Strengthen XAdES assertions: check Reference@Type, SignedProperties@Id, and QualifyingProperties@TargetThe test verifies overall signature validity but misses key XAdES wiring checks. Adding these assertions better validates the new addReference type/id support and the XAdES structure.
Apply this diff to add targeted assertions:
const signedXml = sig.getSignedXml(); - // Verify that the signature is valid - expect(checkSignature(signedXml)).to.be.true; + const signedDoc = new xmldom.DOMParser().parseFromString(signedXml); + + // ds:Signature has the expected Id + const elSig = xpath.select1("//*[local-name(.)='Signature']", signedDoc); + isDomNode.assertIsElementNode(elSig); + expect(elSig.getAttribute("Id")).to.equal(signatureId); + + // xades:QualifyingProperties targets the signature Id + const elQP = xpath.select1("//*[local-name(.)='QualifyingProperties']", signedDoc); + isDomNode.assertIsElementNode(elQP); + expect(elQP.getAttribute("Target")).to.equal(`#${signatureId}`); + + // xades:SignedProperties has the expected Id + const elSP = xpath.select1("//*[local-name(.)='SignedProperties']", signedDoc); + isDomNode.assertIsElementNode(elSP); + expect(elSP.getAttribute("Id")).to.equal(signedPropertiesId); + + // Reference for SignedProperties exists with correct @Type and @URI + const elSPRef = xpath.select1( + "//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference' and @Type='http://uri.etsi.org/01903#SignedProperties']", + signedDoc, + ); + isDomNode.assertIsElementNode(elSPRef); + expect(elSPRef.getAttribute("URI")).to.equal(`#${signedPropertiesId}`); + + // DigestMethod for SignedProperties is SHA-256 + const elSPDigestMethod = xpath.select1( + "//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference' and @URI=$uri]/*[local-name(.)='DigestMethod']", + signedDoc, + ) as Element | null; + // Fallback if variables are not supported by the XPath engine: + const elSPDigestMethodFallback = elSPDigestMethod + ? elSPDigestMethod + : (xpath.select1( + `//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference' and @URI='#${signedPropertiesId}']/*[local-name(.)='DigestMethod']`, + signedDoc, + ) as Element); + isDomNode.assertIsElementNode(elSPDigestMethodFallback); + expect(elSPDigestMethodFallback.getAttribute("Algorithm")).to.equal( + "http://www.w3.org/2001/04/xmlenc#sha256", + ); + + // Verify signature validity last for clearer failure messages + expect(checkSignature(signedXml, signedDoc)).to.be.true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/signature-object-tests.spec.ts(1 hunks)
🔇 Additional comments (5)
test/signature-object-tests.spec.ts (5)
86-134: LGTM: Correct handling of undefined and empty objectsGood coverage to ensure no ds:Object nodes are emitted when objects is undefined or empty. Signature validity checks are appropriate.
136-180: LGTM: Encoded Object attributes and content verifiedSolid assertions on Id, MimeType, Encoding, and content, plus end-to-end verification.
193-256: LGTM: Algorithm matrix strengthens confidenceIterating over SHA256/SHA512 for both digest and signature is thorough. Verifying DigestMethod and SignatureMethod URIs is spot-on.
258-325: LGTM: Canonicalization coverageAsserting both transforms and CanonicalizationMethod for C14N ensures correct wiring of canonicalization options.
441-485: LGTM: InclusiveNamespaces coverage is preciseGood assertion on InclusiveNamespaces PrefixList and that the detected URI uses the Object’s Id.
|
I have identified a problem in a specific situation where there is a Reference to an Object which has InclusiveNamespaces PrefixList which contains namespaces not present in the signature, but the xml document itself, and am working on a fix. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/signed-xml.ts (1)
1158-1184: Restrict InclusiveNamespaces to Exclusive C14n Transforms OnlyOnly Exclusive Canonicalization supports the
<InclusiveNamespaces>element. Emitting it for other transforms (e.g. enveloped-signature) produces invalid XML.– File:
src/signed-xml.ts
– Around lines 1160–1170, update thefor (const trans of ref.transforms…)loop:const transform = this.findCanonicalizationAlgorithm(trans); res += `<${prefix}Transform Algorithm="${transform.getAlgorithmName()}"`; - if (utils.isArrayHasLength(ref.inclusiveNamespacesPrefixList)) { + const algoName = transform.getAlgorithmName(); + const isExclusiveC14n = + algoName === "http://www.w3.org/2001/10/xml-exc-c14n#" || + algoName === "http://www.w3.org/2001/10/xml-exc-c14n#WithComments"; + if (isExclusiveC14n && utils.isArrayHasLength(ref.inclusiveNamespacesPrefixList)) { res += ">"; res += `<InclusiveNamespaces PrefixList="${ref.inclusiveNamespacesPrefixList.join( " ", )}" xmlns="${transform.getAlgorithmName()}"/>`; res += `</${prefix}Transform>`; } else { res += " />"; }If you merge this change, review and update any tests that assume
<InclusiveNamespaces>appears on the first transform regardless of its algorithm.
♻️ Duplicate comments (1)
src/signed-xml.ts (1)
168-171: Restore default getCertFromKeyInfo to preserve embedded KeyInfo behaviorDefaulting to noop breaks verification when callers rely on embedded KeyInfo without supplying publicCert/privateKey. Restore the previous default.
- this.getCertFromKeyInfo = getCertFromKeyInfo ?? SignedXml.noop; + this.getCertFromKeyInfo = getCertFromKeyInfo ?? this.getCertFromKeyInfo;
🧹 Nitpick comments (5)
test/signature-object-tests.spec.ts (3)
122-154: Strengthen namespace assertion for ds:ObjectThe test currently only asserts existence. Add an assertion on namespaceURI to ensure the ds:Object is actually emitted in the ds namespace when no default namespace is present.
- const objectNode = select1Ns("/root/ds:Signature/ds:Object[@Id='object1']", doc); + const objectNode = select1Ns("/root/ds:Signature/ds:Object[@Id='object1']", doc); isDomNode.assertIsElementNode(objectNode); + expect(objectNode.namespaceURI).to.equal("http://www.w3.org/2000/09/xmldsig#");
206-206: Fix typo in test name (“Rerefence” → “Reference”)Minor typo in test description.
- it("should handle Rerefence to Object", function () { + it("should handle Reference to Object", function () {
209-212: Avoid redundant property shorthand in object literalNit: Use property shorthand for privateKey.
- const sig = new SignedXml({ - privateKey: privateKey, + const sig = new SignedXml({ + privateKey,src/signed-xml.ts (2)
1328-1435: Mark references as processed after deferred generationYou set wasProcessed in createReferences but not after the deferred path. This can mislead consumers that inspect internal state.
// Append the reference element to SignedInfo signedInfoNode.appendChild(referenceElem); } + // Mark as processed once all nodes are handled + ref.wasProcessed = true; }
60-61: API ergonomics: allow Node content for Object to avoid brittle string concatenationRight now content is stringly-typed and concatenated. Accepting a Node (or a union Node | string with a contentIsXml flag) would prevent well-formedness issues and reduce escaping concerns. It also allows callers to build Object content with DOM safely.
If you want, I can sketch a backward-compatible change for the types and serializer so callers can pass either a string (escaped as text) or a Node (inserted as-is).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/signed-xml.ts(14 hunks)test/signature-object-tests.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/signature-object-tests.spec.ts (2)
src/signed-xml.ts (2)
checkSignature(261-385)SignedXml(30-1463)src/hash-algorithms.ts (1)
Sha256(17-28)
src/signed-xml.ts (1)
src/types.ts (2)
ObjectAttributes(50-59)Reference(124-160)
🔇 Additional comments (1)
test/signature-object-tests.spec.ts (1)
7-7: Import path fixed to src — LGTMThanks for aligning the Sha256 import to ../src/hash-algorithms. This resolves the prior CI/ts-node path issue.
|
Out of curiosity: if/when this PR ( #506 ) is merged what should be done to another PR: I see that this PR ( #506 ) seems to enables similar functionality ( b5aa833#diff-bf627b29a0628573db9f32289a7b987716b7c1ad162446b74be13cee6af9bbfbR704 ) |
I think this PR would make that redundant. It provides much more versatility. Furthermore, with some minor additions it can allow the library to generate Enveloping and Detached signatures as well. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/signed-xml.ts (1)
169-171: Bug: default getCertFromKeyInfo changed to noop (breaks verification of embedded certs)This reintroduces the previously flagged behavior change. It diverges from the prior default and will break scenarios that rely on extracting certs from KeyInfo when
publicCertisn’t provided.Apply this diff to preserve the intended default:
- this.getCertFromKeyInfo = getCertFromKeyInfo ?? SignedXml.noop; + this.getCertFromKeyInfo = getCertFromKeyInfo ?? this.getCertFromKeyInfo;
🧹 Nitpick comments (3)
src/signed-xml.ts (3)
29-29: Avoid duplicate imports from the same moduleYou already import the module as
utils; preferutils.isDescendantOf(...)instead of adding a named import.Apply this diff to remove the extra import:
-import { isDescendantOf } from "./utils"; +// use utils.isDescendantOf to avoid duplicate imports
1363-1367: Useutils.isDescendantOfto match the module-level importThis keeps imports consistent and avoids an extra named import from the same file.
- node === signedInfoNode || - isDescendantOf(node, signedInfoNode) + node === signedInfoNode || + utils.isDescendantOf(node, signedInfoNode)
1326-1441: Create all new DOM nodes from a single ownerDocument and mark references as processedYou’re mixing
signatureElem.ownerDocument.createElementNS(Reference) withdoc.createElementNS(Transforms, DigestMethod, DigestValue). IfownerDocument !== docin some environments, appending may throw WRONG_DOCUMENT_ERR. After insertion, they’re typically the same in xmldom, but relying on that coupling is brittle. Also,ref.wasProcessedisn’t set here, which leaves stale state.Proposed refactor:
- Use a single
ownerDoc(prefersignatureElem.ownerDocument || doc) to create all elements.- Set
ref.wasProcessed = trueafter successfully appending Reference(s) for the given ref.private processSignatureReferences(doc: Document, signatureElem: Element, prefix?: string) { @@ - const signatureNamespace = "http://www.w3.org/2000/09/xmldsig#"; + const signatureNamespace = "http://www.w3.org/2000/09/xmldsig#"; + const ownerDoc = signatureElem.ownerDocument || doc; @@ - // Process the reference + // Process the reference for (const node of nodes) { @@ - const referenceElem = signatureElem.ownerDocument.createElementNS( - signatureNamespace, - `${prefix}Reference`, - ); + const referenceElem = ownerDoc.createElementNS(signatureNamespace, `${prefix}Reference`); @@ - const transformsElem = doc.createElementNS(signatureNamespace, `${prefix}Transforms`); + const transformsElem = ownerDoc.createElementNS(signatureNamespace, `${prefix}Transforms`); @@ - const transformElem = doc.createElementNS(signatureNamespace, `${prefix}Transform`); + const transformElem = ownerDoc.createElementNS(signatureNamespace, `${prefix}Transform`); transformElem.setAttribute("Algorithm", transform.getAlgorithmName()); @@ - const inclusiveNamespacesElem = doc.createElementNS( + const inclusiveNamespacesElem = ownerDoc.createElementNS( transform.getAlgorithmName(), "InclusiveNamespaces", ); @@ - const digestMethodElem = doc.createElementNS(signatureNamespace, `${prefix}DigestMethod`); + const digestMethodElem = ownerDoc.createElementNS(signatureNamespace, `${prefix}DigestMethod`); digestMethodElem.setAttribute("Algorithm", digestAlgorithm.getAlgorithmName()); - const digestValueElem = doc.createElementNS(signatureNamespace, `${prefix}DigestValue`); + const digestValueElem = ownerDoc.createElementNS(signatureNamespace, `${prefix}DigestValue`); digestValueElem.textContent = digestAlgorithm.getHash(canonXml); @@ signedInfoNode.appendChild(referenceElem); } + // Mark this reference as processed + ref.wasProcessed = true; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/signed-xml.ts(17 hunks)src/utils.ts(1 hunks)test/signature-object-tests.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/signature-object-tests.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-16T14:07:57.604Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1088-1119
Timestamp: 2025-08-16T14:07:57.604Z
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:
src/signed-xml.ts
🧬 Code Graph Analysis (1)
src/signed-xml.ts (2)
src/types.ts (2)
ObjectAttributes(50-59)Reference(124-160)src/utils.ts (1)
isDescendantOf(318-333)
🔇 Additional comments (7)
src/utils.ts (1)
317-333: Utility is correct and fits the intended “descendant-only” checkImplementation is straightforward, null-safe, and intentionally excludes the “self” case, which aligns with its usage guarding against signing SignedInfo or its descendants.
src/signed-xml.ts (6)
61-61: Nice addition:objectsproperty for ds:Object embeddingAdding
objects?: Array<{ content: string; attributes?: ObjectAttributes }>is a clean way to support ds:Object without complicating the existing API with more callbacks.
76-76: TrackingwasProcessedper reference is a good simplificationThis replaces an extra flag with a per-reference state and enables the two-pass flow without mutation games.
976-982: Correct placement of ds:Object before SignatureValue insertionAppending Objects as part of the initial Signature string and inserting SignatureValue afterward ensures the final order matches the schema: SignedInfo, SignatureValue, KeyInfo?, Object*. Good.
1136-1139: Gracefully deferring unresolved references is the right callContinuing when a reference target isn’t found in the first pass avoids premature failure and lets post-processing finalize Ds:Object and similar cases.
1152-1159: Support for Reference Id and Type attributes looks goodThis aligns with xmldsig-core1 and enables XAdES scenarios (e.g., SignedProperties). The attributes are correctly optional and serialized when present.
1090-1120: Object rendering path is clear and minimalConcise implementation that keeps with library philosophy (no input validation) while enabling flexible ds:Object creation with arbitrary attributes. Tests should ensure typical XAdES Object contents serialize as expected.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/signed-xml.ts (2)
168-170: DefaultinggetCertFromKeyInfoto noop likely breaks embedded KeyInfo resolutionThis was previously flagged and acknowledged as out of scope for this PR. It remains a potential breaking change for users relying on embedded certs without passing
publicCert.If you’d like, I can open a follow-up issue to restore the previous default or add a deprecation path.
1089-1119: Object rendering: raw attribute/content passthrough matches library philosophyDirect concatenation of attributes and content is consistent with xml-crypto’s stance of not validating/escaping user input. Given that, this implementation is fine.
If you ever choose to add an opt-in escape path later, I can sketch a minimal
escapeXmlAttrhelper and acontentIsXmltoggle without changing the default behavior.
🧹 Nitpick comments (2)
src/signed-xml.ts (2)
60-60: New publicobjectsproperty: API addition looks goodClear, typed entry-point for ds:Object embedding. Consider adding a short JSDoc describing expected content semantics (XML vs plain text) in a follow-up.
1355-1361: Namespace resolver: add ds fallback to improve ergonomics for Object XPathsSecond-pass selection fails if callers reference
ds:Objectbut didn’t supplyexistingPrefixes.ds. Since ds is a well-known default for this library, fall back toSignedXml.defaultNsForPrefixhere to make Object XPaths “just work” without extra config.Apply this diff:
- const nodes = xpath.selectWithResolver(ref.xpath ?? "", doc, this.namespaceResolver); + // Prefer caller-provided prefixes but fall back to built-in ds mapping + const resolver: XPathNSResolver = { + lookupNamespaceURI: (p) => + p + ? this.namespaceResolver.lookupNamespaceURI(p) || + (SignedXml.defaultNsForPrefix as Record<string, string>)[p] || + null + : null, + }; + const nodes = xpath.selectWithResolver(ref.xpath ?? "", doc, resolver);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/signed-xml.ts(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-16T14:07:57.604Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1088-1119
Timestamp: 2025-08-16T14:07:57.604Z
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:
src/signed-xml.ts
🧬 Code Graph Analysis (1)
src/signed-xml.ts (1)
src/types.ts (2)
ObjectAttributes(50-59)Reference(124-160)
🔇 Additional comments (6)
src/signed-xml.ts (6)
11-11: Import of ObjectAttributes is appropriateType import matches new Object attributes usage. No issues.
75-75: Tracking reference processing state viawasProcessedis a good simplificationThis removes the need for the previous
isSignatureReferenceflag and enables a clean two-pass flow.
148-149: Wiring theobjectsoption through the constructorCorrectly threads the new option. LGTM.
979-980: Placement of ds:Object elements is soundObjects are rendered before the SignatureValue is inserted and ultimately end up after SignatureValue (and after KeyInfo when present), matching schema ordering expectations.
1056-1067: SignatureValue insertion after SignedInfo is correctInserting SignatureValue immediately after SignedInfo preserves the required element order: SignedInfo, SignatureValue, KeyInfo?, Object*.
1132-1138: Two-pass Reference handling: good approach; minor state consistency note
- Skipping unresolved references in the first pass (Line 1136) and handling them later is the right trade-off.
- Including
IdandTypeattributes is correctly implemented.- Marking
ref.wasProcessed = truehappens in this pass (Line 1184).No changes requested here.
Also applies to: 1151-1159, 1184-1184
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/signed-xml.ts (3)
11-11: Object elements wiring and serializer: solid integration; consider a brief API doc noteNice addition and clean plumbing of ds:Object through options ➜ instance ➜ serializer ➜ placement within Signature. Emitting Objects before closing Signature and inserting SignatureValue later yields the correct final order: SignedInfo, SignatureValue, KeyInfo, Object*.
Given this library’s philosophy of trusting inputs, concatenating attribute values and content verbatim is consistent. A small DX improvement would be to document that expectation right where users see the property.
You can add a terse TSDoc to the
objectsproperty to clarify content/attribute handling:- objects?: Array<{ content: string; attributes?: ObjectAttributes }>; + /** + * Optional ds:Object nodes to embed under Signature. + * Note: `content` must be a well-formed XML string. Attributes are inserted verbatim. + * This library does not escape or validate user-provided values. + */ + objects?: Array<{ content: string; attributes?: ObjectAttributes }>;Also applies to: 60-60, 148-149, 170-170, 979-980, 1089-1119
1355-1361: Make XPath against ds:Object work out-of-the-box by mapping the ds prefixIf callers write XPaths like
//ds:Objectfor intra-signature references, they currently need to pass a namespace map inexistingPrefixes. Since this PR introduces signature-internal selection, it’s ergonomic to map “ds” (and the configuredoptions.prefix) by default.Apply this diff in
computeSignatureto augment the resolver:- this.namespaceResolver = { - lookupNamespaceURI: function (prefix) { - return prefix ? existingPrefixes[prefix] : null; - }, - }; + this.namespaceResolver = { + lookupNamespaceURI: (p) => { + if (!p) return null; + // User-provided mappings win + if (existingPrefixes[p]) return existingPrefixes[p]; + // Convenience: map 'ds' and the configured signature prefix to the ds namespace + if (p === "ds" || (prefix && p === prefix)) { + return "http://www.w3.org/2000/09/xmldsig#"; + } + return null; + }, + };This keeps existing behavior intact while making common ds-prefixed XPaths work without extra setup.
1056-1057: Micro‑DRY: factor SignatureValue insertion to a helperThe two branches duplicate the same insertion and serialization steps.
You can factor into a small helper to reduce duplication:
- this.signatureValue = signature || ""; - signatureElem.insertBefore(this.createSignature(prefix), signedInfoNode.nextSibling); - this.signatureXml = signatureElem.toString(); - this.signedXml = doc.toString(); + this.signatureValue = signature || ""; + this.insertSignatureValue(signatureElem, signedInfoNode, prefix, doc);and
- this.calculateSignatureValue(doc); - signatureElem.insertBefore(this.createSignature(prefix), signedInfoNode.nextSibling); - this.signatureXml = signatureElem.toString(); - this.signedXml = doc.toString(); + this.calculateSignatureValue(doc); + this.insertSignatureValue(signatureElem, signedInfoNode, prefix, doc);with:
// place near createSignature() private insertSignatureValue(signatureElem: Element, signedInfoNode: Node, prefix: string | undefined, doc: Document) { signatureElem.insertBefore(this.createSignature(prefix), signedInfoNode.nextSibling); this.signatureXml = signatureElem.toString(); this.signedXml = doc.toString(); }Also applies to: 1065-1066
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/signed-xml.ts(16 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-16T14:07:57.604Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1088-1119
Timestamp: 2025-08-16T14:07:57.604Z
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:
src/signed-xml.ts
📚 Learning: 2025-08-17T10:50:18.014Z
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1447-1451
Timestamp: 2025-08-17T10:50:18.014Z
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.ts
🧬 Code Graph Analysis (1)
src/signed-xml.ts (1)
src/types.ts (2)
ObjectAttributes(50-59)Reference(124-160)
🔇 Additional comments (3)
src/signed-xml.ts (3)
1136-1138: Two‑pass reference handling and Id/Type support look correct
- Deferring unresolved refs in
createReferencesand finalizing them inprocessSignatureReferencesis clean and keeps the first pass simple.- Adding
IdandTypeto Reference elements aligns with xmldsig/XAdES needs.- Marking
ref.wasProcessed = trueprevents duplication on subsequent passes.Also applies to: 1141-1149, 1151-1158, 1159-1185
1365-1374: Good guard: prevent self-signing of Signature/SignedInfoBlocking references to the Signature element, SignedInfo, or descendants of SignedInfo avoids foot-guns and aligns with signature processing model expectations.
1402-1423: InclusiveNamespaces emission matches initial path (consistent behavior)Emitting InclusiveNamespaces whenever a prefix list is provided mirrors
createReferences. This consistency avoids subtle discrepancies between first- and second-pass generation. The known corner case around PrefixList containing namespaces absent from the signature scope is acknowledged in PR comments; fine to address separately.If you have a failing repro for the “PrefixList contains namespaces present in the doc but not in the signature” case, consider adding it as a pending test to track the follow-up.
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 (3)
README.md(2 hunks)src/signed-xml.ts(16 hunks)src/types.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
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/signed-xml.ts
🧬 Code graph analysis (3)
src/types.ts (1)
test/signature-unit-tests.spec.ts (4)
xml(123-164)xml(1209-1243)signedXml(744-778)xml(1114-1140)
README.md (2)
test/signature-integration-tests.spec.ts (1)
xml(180-225)test/signature-unit-tests.spec.ts (4)
xml(253-283)xml(166-188)xml(123-164)xml(1114-1140)
src/signed-xml.ts (1)
src/types.ts (2)
ObjectAttributes(51-60)Reference(125-161)
🔇 Additional comments (12)
src/signed-xml.ts (9)
815-816: LGTM! Clean extension of the addReference API.The addition of optional
idandtypeparameters maintains backward compatibility while enabling XAdES and other advanced signature scenarios. InitializingwasProcessed: falsefor the two-pass reference resolution is correct.Also applies to: 834-836
1090-1120: LGTM! Object rendering follows library conventions.The
getObjectsmethod correctly renders<Object>elements with attributes. Per established learnings, the library places responsibility for properly encoded attribute values on users, consistent with the design philosophy throughout the codebase.
1034-1037: Correct sequencing for two-pass reference resolution.The post-processing flow is well-ordered: the signature element is inserted into the DOM first (lines 1014-1032), then
processSignatureReferencesresolves any deferred references (e.g., references to<Object>elements within the signature), and finallysignatureNodeis assigned. This enables signing embedded objects and supports enveloping signatures.
1137-1139: Deferred reference resolution enables signing signature elements.Changing from throwing an error to
continuewhen nodes aren't found (lines 1137-1139) enables the two-pass approach: unresolved references are markedwasProcessed: falseand handled inprocessSignatureReferencesafter the signature is in the DOM. Line 1185 correctly marks processed references.Also applies to: 1185-1185
1142-1159: LGTM! Reference attributes are correctly rendered.The construction of
referenceAttrsstring now includes optionalIdandTypeattributes per the XMLDSig spec. The conditional logic correctly handles empty URI, Id, and Type attributes.
1326-1362: Well-structured post-processing with appropriate error handling.The method correctly:
- Returns early if no unprocessed references (lines 1331-1335)
- Locates
SignedInfoin the signature element (lines 1342-1348)- Throws when XPath resolution fails at this stage (lines 1358-1361), which is correct since references must resolve after the signature is in the DOM
1365-1375: Critical validation prevents signing the signature itself.The check preventing references to
Signature,SignedInfo, or descendants ofSignedInfo(lines 1367-1371) correctly guards against circular references that would break signature computation. The error message is clear.
1377-1453: DOM-based Reference construction is robust and correct.Using DOM methods (
createElementNS,setAttribute,appendChild) instead of string concatenation for post-processing ensures proper namespace handling and avoids potential issues with namespace prefixes in complex documents. The digest computation, attribute assignment (Id/Type), andwasProcessedmarking (line 1451) are all correct.
996-998: Consistent use ofsignatureElemthroughout signature insertion.The code correctly extracts
signatureElemfrom the dummy wrapper (lines 996-998) and uses it consistently for:
- DOM insertion operations (lines 1015-1032)
- Post-processing (line 1035)
- Final signature value insertion (lines 1057, 1066)
- Signature XML extraction (lines 1058, 1067)
The comment clarifying the non-null assertion (lines 996-997) is helpful.
Also applies to: 1015-1032, 1057-1068
src/types.ts (3)
47-60: Well-defined interface following W3C XMLDSig specification.The
ObjectAttributesinterface correctly models the standard attributes (Id, MimeType, Encoding) defined in the W3C XMLDSig spec. The index signature[key: string]: string | undefinedenables custom attributes while maintaining type safety.
77-77: LGTM! Optionalobjectsproperty maintains backward compatibility.The type
Array<{ content: string; attributes?: ObjectAttributes }>correctly models the structure consumed bygetObjects()insigned-xml.ts. Making it optional ensures existing code continues to work without modification.
147-151: Appropriate extension of Reference interface for advanced scenarios.Adding optional
idandtypeproperties enables XAdES signatures (e.g.,Type="http://uri.etsi.org/01903#SignedProperties") and other XMLDSig use cases that require Reference attributes. The JSDoc comments clearly describe their purpose.
I am open to any contributions you can make. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@cjbarth Maybe the best idea I have is to first add the Id attributes without actually adding the references: Then we can unify This removes the need for any tracking which reference was processed, removes the need for two separate reference processing functions, and preservers originalXmlWithIds. I can push this if it is acceptable to you. |
|
Technically I suppose you could have multiple |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/signed-xml.ts (2)
1089-1242: **Refactor: ReusesignedInfoNodeinstead of finding SignedInfo twice.**TheaddAllReferencesmethod correctly processes references and creates DOM elements. However, line 1132 findsSignedInfoinside the node iteration loop, whilesignedInfoNodewas already located at line 1102. This is inefficient—both variables refer to the same element.The validation logic at lines 1133-1141 correctly prevents signing the Signature or SignedInfo itself while allowing signing of KeyInfo or Object elements (as intended per the PR objectives).
Apply this diff to eliminate the redundant lookup:
for (const node of nodes) { isDomNode.assertIsElementNode(node); // Must not be a reference to Signature, SignedInfo, or a child of SignedInfo - const signedInfo = utils.findChildren(signatureElem, "SignedInfo")[0]; if ( node === signatureElem || - node === signedInfo || - utils.isDescendantOf(node, signedInfo) + node === signedInfoNode || + utils.isDescendantOf(node, signedInfoNode) ) { throw new Error( `Cannot sign a reference to the Signature or SignedInfo element itself: ${ref.xpath}`, ); }
1102-1108: Optional: Improve type safety with assertion.The cast to
Elementon line 1103 happens before the null check on line 1106. While functionally correct (the check catches falsy values), usingisDomNode.assertIsElementNode()after retrieval would be more type-safe and consistent with patterns elsewhere in the codebase (e.g., line 1130).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/signed-xml.ts(13 hunks)src/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
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/signed-xml.ts
🧬 Code graph analysis (1)
src/signed-xml.ts (1)
src/types.ts (1)
ObjectAttributes(51-60)
🔇 Additional comments (6)
src/utils.ts (1)
317-333: LGTM! Clean DOM traversal utility.The implementation correctly checks ancestry by walking the
parentNodechain. The null guards and loop logic are sound.src/signed-xml.ts (5)
149-149: LGTM! Objects option integrated into constructor.The
objectsproperty is properly destructured from options and assigned, following the existing pattern for other optional properties.Also applies to: 171-171
804-806: LGTM! Reference API extended to support XAdES attributes.The
idandtypeparameters enable custom Reference attributes as described in the PR objectives. The optional parameters maintain backward compatibility.Also applies to: 815-816, 834-835
958-977: LGTM! Upfront ID assignment enables unified reference processing.This change adds IDs to all referenced nodes before signature creation, eliminating the need for the
isSignatureReferenceflag. The approach allows all references (including those targeting the signature itself) to be processed uniformly after insertion.
1268-1292: LGTM! Object element rendering follows established patterns.The method correctly renders
<Object>elements with optional attributes. As per the library's design philosophy, users are responsible for providing properly encoded attribute values and content.Based on learnings
958-1086: LGTM! Unified reference processing successfully removes theisSignatureReferenceflag.The refactored flow is well-designed:
- IDs are added upfront to all referenced nodes
- Original XML with IDs is captured before signature insertion
- Signature is built and inserted into the DOM
- All references (including those targeting signature elements) are processed uniformly via
addAllReferences- Signature value is computed and inserted
This approach elegantly handles the complex ordering requirements for signing Object and KeyInfo elements within the signature itself, enabling XAdES support as described in the PR objectives.
|
@shunkica , I think it is a very simple flow from what it was; good work! I've made some minor adjustments and a few comments. Please have another look. Also, correct me if I'm wrong, but I don't think there are any breaking changes here. I looked but didn't see any. |
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 (1)
src/signed-xml.ts(15 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
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/signed-xml.ts
🧬 Code graph analysis (1)
src/signed-xml.ts (1)
src/types.ts (1)
ObjectAttributes(51-60)
🔇 Additional comments (8)
src/signed-xml.ts (8)
11-11: LGTM: ObjectAttributes import and objects property initializationThe new
objectsproperty is correctly imported, declared, and initialized following the existing patterns in the constructor.Also applies to: 60-60, 149-149, 171-171
804-841: LGTM: addReference extended to support Id and Type attributesThe new optional
idandtypeparameters are properly documented, defaulted, and stored on the reference object. This enables XAdES-compliant signatures as mentioned in the PR objectives.
958-976: LGTM: Upfront ID assignment enables unified reference processingThis refactor adds IDs to all referenced nodes before creating the signature, which simplifies the reference processing flow and eliminates the need for tracking processed references. The
originalXmlWithIdsis correctly captured after ID assignment but before signature insertion.
999-999: LGTM: getObjects follows library conventionsThe
getObjectsmethod correctly generates XML for embedded Object elements. Attribute values are concatenated directly without escaping, consistent with the library's design philosophy of placing encoding responsibility on users. Based on learnings.Also applies to: 1258-1282
1092-1132: LGTM: addAllReferences validation logic is soundThe method correctly validates that references don't target the Signature element, SignedInfo element, or any descendants of SignedInfo. This prevents invalid signature structures and potential circular references.
1163-1230: LGTM: DOM-based reference creation is well-structuredThe method correctly creates Reference elements and their children (Transforms, DigestMethod, DigestValue) using DOM APIs. The new
idandtypeattributes are properly set on the Reference element when present. Digest computation and element assembly follow the correct flow. Based on learnings.
1234-1250: LGTM: getKeyInfo extraction improves code organizationThe
getKeyInfomethod is correctly extracted from inline code, maintaining the same logic while improving readability and consistency with the newgetObjectsmethod.
1051-1086: LGTM: Two-pass signature assembly flow is correctly implementedThe refactored flow correctly:
- Inserts the signature element (with empty SignedInfo) into the document
- Calls
addAllReferencesto populate references (including any to the signature itself for XAdES)- Computes and inserts the SignatureValue
- Captures the final XML
The comment at line 1385 clarifies that references are intentionally omitted from
createSignedInfoand added later byaddAllReferences.Also applies to: 1370-1387
I agree with your assessment. These changes only build on the public API and add new functionality, but do not change any existing functionality. The references are now created using DOM methods instead of string building, but I do not see how that could affect anyone. |
shunkica
force-pushed
the
signature-object
branch
from
7546fae to
f53e7fe
Compare
…ature Co-authored-by: Chris Barth <[email protected]>
shunkica
force-pushed
the
signature-object
branch
from
f53e7fe to
683834c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/signed-xml.ts (1)
1092-1232: Solid implementation: unified DOM-based reference assembly.The new
addAllReferencesmethod:
- Processes all references in a single pass after signature insertion
- Uses DOM methods to avoid namespace string-concatenation issues
- Correctly handles Reference Id and Type attributes (lines 1170-1176)
- Computes digests and appends Reference elements to SignedInfo
- Integrates the security self-reference checks
The method is somewhat long, but the logic is clear and the approach is sound. As an optional future refactor, you could extract subroutines for creating Transform/DigestMethod/DigestValue elements, but this is low priority.
Optional: Extract DOM element creation helpers
If you want to reduce the length of
addAllReferencesin a future PR, consider extracting helpers likecreateTransformsElement(ref, signatureDoc, currentPrefix)andcreateDigestElements(ref, signatureDoc, currentPrefix, digestValue). This would improve readability but isn't necessary for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.vscode/settings.json(1 hunks)README.md(2 hunks)src/signed-xml.ts(15 hunks)src/types.ts(3 hunks)src/utils.ts(1 hunks)test/signature-object-tests.spec.ts(1 hunks)test/signature-unit-tests.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils.ts
- test/signature-unit-tests.spec.ts
- README.md
🧰 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.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.tssrc/types.ts
📚 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:
src/signed-xml.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:
src/signed-xml.ts
📚 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:
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/signed-xml.ts
📚 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: 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 (2)
test/signature-object-tests.spec.ts (2)
src/signed-xml.ts (1)
SignedXml(30-1442)src/hash-algorithms.ts (1)
Sha256(17-28)
src/signed-xml.ts (1)
src/types.ts (1)
ObjectAttributes(51-60)
🔇 Additional comments (14)
.vscode/settings.json (1)
9-15: All added spell-check words are legitimate and used in the codebase.The verification confirms that "posteb", "preeb", and "stricttextualmsg" are technical terms referenced in
src/utils.tscomments and documentation related to RFC7468 PEM format validation and normalization. These align with the PR's goal of adding XAdES support, which requires proper certificate and PEM handling. The "xades" addition is also appropriate. All spell-check word additions are justified.src/types.ts (3)
47-60: Well-designed ObjectAttributes interface.The interface cleanly maps to the W3C XMLDSig Object element spec, with flexible optional properties and an index signature for extensibility. The JSDoc reference to the spec is helpful.
77-77: LGTM: objects option enables Object element embedding.Clean addition to the public API. The type correctly references the new ObjectAttributes interface.
147-151: LGTM: Reference extensions support XAdES and custom reference attributes.Adding
idandtypeproperties enables XAdES SignedProperties references (withType="http://uri.etsi.org/01903#SignedProperties") and other advanced use cases. The optional nature preserves backward compatibility.test/signature-object-tests.spec.ts (4)
1-39: Excellent test infrastructure and helpers.The namespace-aware XPath helpers and
checkSignatureutility provide clean, reusable test infrastructure. The import from../src/hash-algorithmsis correct (avoiding the../libpitfall from earlier reviews).
41-271: Comprehensive Object element test coverage.The test suite thoroughly validates:
- Object rendering with all attribute types (Id, MimeType, Encoding)
- Base64 content handling
- Namespace correctness with and without prefixes
- Empty/undefined objects handling
- References to Object elements with InclusiveNamespaces
This coverage ensures the core Object feature works correctly across scenarios.
371-452: XAdES integration test validates real-world use case.This test demonstrates end-to-end XAdES support:
- Embeds QualifyingProperties/SignedProperties in an Object
- Uses SHA-256 for certificate digest (modern crypto)
- Validates Reference[@type='http://uri.etsi.org/01903#SignedProperties']
- Verifies the complete signature including XAdES structure
This is exactly the use case described in the PR objectives and confirms the implementation works for production XAdES scenarios.
454-520: Critical security tests for self-reference prevention.The three tests correctly validate that references to Signature, SignedInfo, and SignedInfo descendants are rejected. This prevents circular reference attacks and ensures signature integrity. The error messages are clear and actionable.
src/signed-xml.ts (6)
11-11: LGTM: Clean integration of objects option.The ObjectAttributes import, property declaration, and constructor handling are well-integrated. The constructor correctly accepts and stores the
objectsarray for later rendering.Also applies to: 60-60, 149-171
958-976: Excellent: Upfront ID assignment enables Object references.This approach assigns IDs to all referenced nodes before inserting the signature, ensuring that references to Object elements (and other signature-internal elements) can resolve correctly. Capturing
originalXmlWithIdsat this point preserves the pre-signature state for validation. This is a key enabler for the XAdES and Object use cases.
1123-1131: Critical security check: prevent self-references.The guard against references to the Signature, SignedInfo, or SignedInfo descendants prevents circular reference attacks. The use of
utils.isDescendantOfcorrectly handles nested scenarios (e.g., signing a Transform or DigestMethod child). This is essential for signature integrity.
1258-1282: LGTM: getObjects renders Object elements correctly.The method iterates over
this.objects, concatenates attributes (including undefined checks), and wraps content in Object elements with the correct prefix. Consistent with the library's design philosophy, attribute values are not escaped—users are responsible for providing properly encoded values. The implementation correctly returns an empty string when no objects are defined.Based on learnings
804-841: LGTM: addReference signature extended to support id and type.The method now accepts optional
idandtypeparameters and stores them on the Reference object. The parameters are correctly passed through to the internal references array (lines 834-835). This enables XAdES SignedProperties references and other advanced use cases. Backward compatibility is preserved since both are optional.
1031-1087: LGTM: Signature insertion and finalization flow.The revised flow correctly:
- Inserts the signature element via DOM (lines 1031-1049)
- Calls
addAllReferencesto populate SignedInfo (line 1052)- Sets
this.signatureNode(line 1054)- Computes the signature value (lines 1067-1086)
- Inserts the SignatureValue element (lines 1074/1083)
The use of DOM-based insertion (lines 1032, 1034, 1041, 1048) avoids string manipulation and namespace issues. Both async and sync flows are handled consistently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
537-572: Clarify that custom Objects are embedded within the Signature element.The new example section is well-structured, but it would help users to explicitly state that Objects added via the
objectsoption are embedded as<ds:Object>children of the<ds:Signature>element (per the W3C xmldsig-core1 spec), not as siblings or independent elements. Additionally, document that when referencing these Objects viaaddReference, the xpath must use theIdattribute specified in the object'sattributesconfiguration, and users are responsible for ensuring Id uniqueness.Consider adding a clarifying sentence like: "The custom Object is embedded as a child of the Signature element. When referencing it with
addReference, use an xpath that matches the Id you specified in the object's attributes (e.g.,//*[@Id='Object1'])."
537-572: Minor: Align digest and signature algorithm choices across examples.Line 565 specifies
digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1"while the constructor at line 550 usesrsa-sha256. For consistency and to guide users toward modern choices, consider updating the reference to use SHA256 as well:digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🔇 Additional comments (1)
README.md (1)
269-274: Example code throughout the README still uses oldaddReferencesignature.The documentation now shows the new object-based signature
addReference({ xpath, transforms, digestAlgorithm, id, type }), but existing examples at lines 78–82, 399–403, 496–500, and 525–529 still use the old positional parameter style. If this is a breaking change, all examples should be updated for consistency. If backward compatibility is maintained, explicitly document that both calling styles work.
3 participants
Added support for custom
<Object>elements within the<Signature>element via a newgetObjectContent?(): Array<{ content: string; attributes?: ObjectAttributes; }> | nullmethod.This is specified in xmldsig-core1/#sec-Object
Modified the signature creation process to sign and reference these internal objects.
These changes are also a step towards XAdES support.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores