Respect idAttribute when generating signatures. #508
Conversation
|
Not sure if this needs enhancements to provide support for namespaced id's but then the whole idAttribute has to be changed. But in the long run this can then make the idMode parameter obsolete ... |
|
@tkalmar , can you add a test showing how this behavior is currently broken? |
WalkthroughAdds namespaced ID support and generalizes ID handling across signing: id attributes can now be plain strings or NamespacedId objects; findAttr and ID assignment logic were updated; a KNOWN_NAMESPACED_IDS constant and NamespacedId type were introduced; tests updated to exercise configurable idAttribute. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as signing code
participant SignedXml as SignedXml
participant DOM as XML Element
Note over SignedXml: idAttributes can be string or NamespacedId
Caller->>SignedXml: computeSignature(...)
SignedXml->>DOM: locate reference element
SignedXml->>SignedXml: ensureHasId(element)
alt existing id found
SignedXml-->>SignedXml: use existing attribute (string or namespaced)
else no id
SignedXml->>DOM: set attribute
Note right of DOM #DFF0D8: If NamespacedId, set xmlns + prefixed name\nOtherwise set plain attribute
DOM-->>SignedXml: element updated with new id
end
SignedXml->>SignedXml: create Reference using found/generated id
SignedXml-->>Caller: signature added
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/signed-xml.ts (1)
1332-1332: Consider validating the idAttribute value in the constructor.The change correctly uses
this.idAttributes[0]to respect custom ID attributes. However, there's no validation when a customidAttributeis provided in the constructor (lines 155-157). Invalid values (empty string, whitespace, invalid XML name characters) could cause issues during signing.Consider adding validation in the constructor:
if (idAttribute) { + if (typeof idAttribute !== 'string' || !idAttribute.trim()) { + throw new Error('idAttribute must be a non-empty string'); + } this.idAttributes.unshift(idAttribute); }test/signature-unit-tests.spec.ts (1)
88-88: Fix TypeScript formatting.Type annotations should have spaces around the colon and type union operator.
Apply this diff:
- function verifyAddsId(mode, nsMode, idAttribute:string|undefined = undefined) { + function verifyAddsId(mode, nsMode, idAttribute: string | undefined = undefined) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/signed-xml.ts(1 hunks)test/signature-unit-tests.spec.ts(2 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.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.
📚 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/signature-unit-tests.spec.tssrc/signed-xml.ts
🧬 Code graph analysis (1)
test/signature-unit-tests.spec.ts (1)
src/signed-xml.ts (1)
SignedXml(30-1422)
🔇 Additional comments (1)
test/signature-unit-tests.spec.ts (1)
125-127: Good test coverage for custom ID attributes.The new test case effectively verifies that custom
idAttributevalues are respected during signing, using 'myIdAttribute' to ensure the generated IDs use the configured attribute name instead of the default 'Id'.
|
The code looks OK @tkalmar , but I'm not sure what the use-case is. What is the problem that is solved by writing a different attribute for ID other than the traditional |
|
@cjbarth we are working with an customer endpoint which validates our signed xml, at the moment we have to replace |
|
So, you don't want a different ID, you want a namespace. Do you have a sample XML that you could share? A proper fix would probably be to read the namespace from the existing XML and use that. |
|
The proper solution would be to make idAttribute/idAttributes of type |
|
@cjbarth @shunkica for xml:id no namespace is required as the |
|
@tkalmar The XMLDsig spec doesn't prescribe using xml:id specifically—it just requires Id-type attributes. We understand your use case, but I think If there are going to be changes to this it should be to support fully qualified ID attributes. A simple string-based approach is too narrow and doesn't guarantee proper namespace-aware attribute creation. Proposal: Modify this PR to accept idAttribute as either:
For your use case, you'd pass: This would properly create a namespaced attribute and accommodate any ID attribute convention—whether it's xml:id, |
|
@shunkica A first draft is done. |
There was a problem hiding this comment.
Actionable comments posted: 3
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)
546-546: Fix XPath construction for NamespacedId (duplicate issue).Similar to line 517, when
attris aNamespacedId, the XPath expression will be malformed. ExtractlocalNamebefore constructing the XPath.Apply this fix:
for (const attr of this.idAttributes) { + const attrName = typeof attr === "string" ? attr : attr.localName; - const tmp_elemXpath = `//*[@*[local-name(.)='${attr}']='${uri}']`; + const tmp_elemXpath = `//*[@*[local-name(.)='${attrName}']='${uri}']`; const tmp_elem = xpath.select(tmp_elemXpath, doc);
🧹 Nitpick comments (2)
test/signature-unit-tests.spec.ts (1)
127-129: Consider adding test coverage for NamespacedId.The new test validates custom string
idAttribute(e.g.,"myIdAttribute"), which addresses the primary use case from issue #33. However, the PR also introducesNamespacedIdsupport. Consider adding a test case that uses aNamespacedIdobject (e.g.,xml:idorwsu:Id) to ensure complete coverage of the new functionality.Example test structure:
it("signer adds increasing namespaced id attributes to elements", function () { const xml = '<root><x xmlns="ns"></x><y attr="value"></y><z><w></w></z></root>'; const sig = new SignedXml({ idAttribute: { prefix: "xml", localName: "id", nameSpaceURI: "http://www.w3.org/XML/1998/namespace" } }); // ... rest of test });src/signed-xml.ts (1)
1321-1334: Namespace prefix declaration is handled correctly by DOM spec; no duplicate issues.While the code unconditionally declares the namespace prefix without checking if it already exists, the DOM spec's setAttributeNS updates existing attributes with the same namespaceURI+localName rather than creating duplicates, and implementations avoid emitting duplicate prefix→URI declarations during serialization. This means the current implementation works correctly and won't create problematic duplicate declarations.
That said, explicitly checking for an existing prefix before declaration would improve code clarity:
// Optional improvement: check if prefix already bound if (!node.lookupNamespaceURI(this.idAttributes[0].prefix)) { node.setAttributeNS( "http://www.w3.org/2000/xmlns/", `xmlns:${this.idAttributes[0].prefix}`, this.idAttributes[0].nameSpaceURI, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/constants.ts(1 hunks)src/signed-xml.ts(6 hunks)src/types.ts(1 hunks)src/utils.ts(2 hunks)test/signature-unit-tests.spec.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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: 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
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.
📚 Learning: 2025-10-22T21:50:05.454Z
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.
Applied to files:
src/types.tstest/signature-unit-tests.spec.tssrc/signed-xml.tssrc/utils.ts
📚 Learning: 2025-10-22T21:50:05.454Z
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?.
Applied to files:
src/types.tssrc/signed-xml.ts
📚 Learning: 2025-10-22T21:03:38.354Z
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.
Applied to files:
src/types.tstest/signature-unit-tests.spec.tssrc/signed-xml.ts
📚 Learning: 2025-10-25T14:41:13.728Z
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.
Applied to files:
src/signed-xml.ts
🧬 Code graph analysis (4)
test/signature-unit-tests.spec.ts (1)
src/signed-xml.ts (1)
SignedXml(32-1422)
src/signed-xml.ts (2)
src/types.ts (1)
NamespacedId(65-72)src/constants.ts (1)
KNOWN_NAMESPACED_IDS(4-18)
src/constants.ts (1)
src/types.ts (1)
NamespacedId(65-72)
src/utils.ts (1)
src/types.ts (1)
NamespacedId(65-72)
🔇 Additional comments (10)
src/types.ts (2)
62-72: LGTM! Well-structured interface for namespaced attributes.The
NamespacedIdinterface correctly models XML namespaced attributes with clear field names and documentation.
79-79: LGTM! Backward-compatible API extension.The union type
string | NamespacedIdmaintains backward compatibility while enabling namespaced ID support as requested in issue #33.src/utils.ts (2)
23-30: LGTM! Clean handling of union type.The implementation correctly handles both
stringandNamespacedIdtypes using a type guard, extracting the appropriatelocalNameandnamespacevalues for attribute matching.
20-20: Confirm this breaking API change is intentional.
findAttris re-exported fromindex.tsas part of the public API. The signature change from(element, localName, namespace?)to(element, id: string | NamespacedId)is a breaking change that will impact external users. All internal call sites have been correctly updated. Verify this change is intentional and ensure proper version/changelog documentation if this is a major release.src/constants.ts (1)
4-11: LGTM! Correctly models WS-Security namespaced ID.The
wssecurityentry accurately represents thewsu:Idattribute from the WS-Security Utility 1.0 specification, centralizing what was previously hardcoded logic.src/signed-xml.ts (4)
11-11: LGTM! Necessary imports for namespaced ID support.The imports of
NamespacedIdandKNOWN_NAMESPACED_IDSenable the new functionality throughout the class.Also applies to: 30-30
34-34: Public API change: idAttributes now supports NamespacedId.The type change to
(string | NamespacedId)[]extends the public API to support namespaced ID attributes. This is backward compatible for reading but may affect code that writes to this property.
160-162: LGTM! Cleaner wssecurity mode initialization.Using
KNOWN_NAMESPACED_IDS["wssecurity"]centralizes the WS-Security ID attribute configuration, making the code more maintainable than hardcoded values.
1309-1312: LGTM! Simplified attribute checking.The change to pass
idAttributedirectly tofindAttrsimplifies the code and aligns with the updatedfindAttrsignature.test/signature-unit-tests.spec.ts (1)
117-119: Test helper verifyAddsId does not accept NamespacedId; no issue found.The test helper's type signature explicitly restricts
idAttributetostring | undefined(line 88), and all test calls pass only string or undefined values. The XPath construction is correct for these accepted types. The broaderNamespacedIdsupport exists inSignedXmlOptions(src/types.ts line 79) but was intentionally not included in the test helper's signature—this is a deliberate design choice, not an oversight.
| xml: { | ||
| prefix: "xml", | ||
| localName: "id", | ||
| nameSpaceURI: "http://www.w3.org/XML/1998/namespace", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Supports the xml:id use case.
The xml entry correctly models the xml:id attribute, addressing the customer requirement mentioned in the PR comments where they need to generate signatures with xml:id instead of Id.
Verify that the xml constant is actually used in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for usage of KNOWN_NAMESPACED_IDS["xml"]
rg -n 'KNOWN_NAMESPACED_IDS\["xml"\]' --type ts
rg -n "KNOWN_NAMESPACED_IDS\['xml'\]" --type tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for broader KNOWN_NAMESPACED_IDS usage patterns
echo "=== Search for KNOWN_NAMESPACED_IDS usage ==="
rg -n 'KNOWN_NAMESPACED_IDS' --type ts -C 2
echo ""
echo "=== View the constants.ts file to see the structure ==="
fd 'constants.ts' --type f | xargs head -30Length of output: 1317
🏁 Script executed:
#!/bin/bash
# Search for idMode usage to understand if xml is supposed to be supported
echo "=== Search for idMode string patterns ==="
rg -n 'idMode\s*===|idMode\s*=\s*["\']' --type ts -C 1
echo ""
echo "=== Check full idMode logic in signed-xml.ts ==="
rg -n 'idMode' src/signed-xml.ts -C 3Length of output: 110
🏁 Script executed:
#!/bin/bash
# Search for idMode usage with simpler patterns
echo "=== Search for idMode assignments and checks ==="
rg -n 'idMode' src/signed-xml.ts -B 2 -A 2
echo ""
echo "=== Search for any xml mode or xml:id references ==="
rg -n 'xml:id|xml.*mode|mode.*xml' --type tsLength of output: 815
Add implementation logic to complete the xml:id support feature.
The xml constant has been added to KNOWN_NAMESPACED_IDS, but the feature is incomplete and currently dead code. To make this functional:
-
Update the
idModetype atsrc/signed-xml.ts:33to include"xml":idMode?: "wssecurity" | "xml"; -
Add the corresponding handler in the constructor (after line 162):
if (idMode === "xml") { this.idAttributes.unshift(KNOWN_NAMESPACED_IDS["xml"]); }
Without these changes, the xml constant cannot be used and the customer's xml:id use case won't function.
🤖 Prompt for AI Agents
In src/constants.ts around lines 13-17 and in src/signed-xml.ts update the
idMode type and constructor to enable xml:id handling: add "xml" to the idMode
union at or near line 33 (idMode?: "wssecurity" | "xml";) and in the constructor
after line 162, detect idMode === "xml" and unshift KNOWN_NAMESPACED_IDS["xml"]
onto this.idAttributes so the xml namespaced id is actually used; this wires the
new KNOWN_NAMESPACED_IDS entry into runtime behavior.
| const elemId = | ||
| typeof attr === "string" | ||
| ? elem.getAttribute(attr) | ||
| : elem.getAttribute(`${attr.prefix}:${attr.localName}`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider using getAttributeNS for namespaced attributes.
The code constructs a prefixed attribute name (${attr.prefix}:${attr.localName}) for NamespacedId attributes, but DOM's getAttributeNS(namespaceURI, localName) is the standard method for reading namespaced attributes. This could lead to issues if the prefix varies or is absent.
Apply this pattern for reading namespaced attributes:
const elemId =
typeof attr === "string"
? elem.getAttribute(attr)
- : elem.getAttribute(`${attr.prefix}:${attr.localName}`);
+ : elem.getAttributeNS(attr.nameSpaceURI, attr.localName);This ensures correct attribute resolution regardless of prefix usage in the document.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/signed-xml.ts around lines 512 to 515, the code builds a prefixed name to
read a namespaced attribute which will break if the document uses a different or
no prefix; change the branch that handles non-string attr to call
elem.getAttributeNS(attr.namespaceURI, attr.localName) instead of
elem.getAttribute(`${attr.prefix}:${attr.localName}`), and if getAttributeNS
returns null/undefined fall back to the prefixed lookup only as a last resort;
also guard for the case attr.namespaceURI may be missing (then use the old
getAttribute behavior).
| ? elem.getAttribute(attr) | ||
| : elem.getAttribute(`${attr.prefix}:${attr.localName}`); | ||
| if (uri === elemId) { | ||
| ref.xpath = `//*[@*[local-name(.)='${attr}']='${uri}']`; |
There was a problem hiding this comment.
Fix XPath construction for NamespacedId.
When attr is a NamespacedId object, the template literal ${attr} will stringify it as "[object Object]", producing an invalid XPath. Extract the localName for proper XPath construction.
Apply this fix:
+ const attrName = typeof attr === "string" ? attr : attr.localName;
- if (uri === elemId) {
- ref.xpath = `//*[@*[local-name(.)='${attr}']='${uri}']`;
+ if (uri === elemId) {
+ ref.xpath = `//*[@*[local-name(.)='${attrName}']='${uri}']`;
break; // found the correct element, no need to check further
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/signed-xml.ts around line 517, the XPath construction uses `${attr}`
which stringifies a NamespacedId to "[object Object]"; change it to detect when
attr is a NamespacedId (e.g. an object with a localName property) and use its
localName instead of the object in the template literal so the xpath becomes
`//*[@*[local-name(.)='${localName}']='${uri}']`; ensure the code handles both
string and NamespacedId inputs before assigning ref.xpath.
|
This does conflict with PR #519 where a functionality like this is already supported. |
|
@shunkica @cjbarth |
|
@tkalmar , if @shunkica is able to continue working on he PR, given its size, it would seem reasonable to get that landed first and see what parts of that we can leverage to meet your needs. I'm not thrilled about solving your need for a namespace by allowing for string replacement. Even your current PR seems to fudge the concept of a namespace as some of the comments by coderabbitai points out. If @shunkica is not able to continue, then we'll see what we can do to get you in sooner. |
3 participants
Fixes #33
small fix to respect user provided idAttribute when generating id's
Summary by CodeRabbit