Fix Id attribute handling in addAllReferences #521
Conversation
WalkthroughReplaced manual per-node ID extraction in addAllReferences with a single call to Changes
Sequence Diagram(s)sequenceDiagram
participant SignedXml as SignedXml.addAllReferences
participant Ensure as SignedXml.ensureHasId
participant Node as XML Node
rect #E6F5FF
SignedXml->>Node: iterate target nodes
SignedXml->>Ensure: ensureHasId(node)
Ensure-->>SignedXml: id (existing or generated)
SignedXml->>SignedXml: set ref.uri = "#" + id
SignedXml->>SignedXml: set ref.targetUri = id
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-22T21:50:05.441ZApplied to files:
🧬 Code graph analysis (2)test/signature-unit-tests.spec.ts (1)
test/signature-object-tests.spec.ts (1)
🔇 Additional comments (4)
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 (1)
test/signature-object-tests.spec.ts (1)
370-393: Consider adding assertions to verify Id generation.The test validates that signing doesn't throw when an Object lacks an Id, but it doesn't assert the expected behavior. Consider adding checks for:
- The generated Id on the Data element
- The Reference URI pointing to that Id
This would make the test more meaningful and catch potential regressions.
Example assertions to add:
sig.computeSignature(xml); +const signedXml = sig.getSignedXml(); +const doc = new xmldom.DOMParser().parseFromString(signedXml); + +// Verify that Data element got an Id +const dataEl = select1Ns("/root/ds:Signature/ds:Object/Data[@Id]", doc); +isDomNode.assertIsElementNode(dataEl); + +// Verify Reference URI points to the generated Id +const refEl = select1Ns("/root/ds:Signature/ds:SignedInfo/ds:Reference", doc); +isDomNode.assertIsElementNode(refEl); +expect(refEl.getAttribute("URI")).to.match(/^#_\d+$/);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/signed-xml.ts(1 hunks)test/signature-object-tests.spec.ts(1 hunks)test/signature-unit-tests.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-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.
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.
📚 Learning: 2025-10-22T21:50:05.441Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.441Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
test/signature-unit-tests.spec.tssrc/signed-xml.tstest/signature-object-tests.spec.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-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.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
🧬 Code graph analysis (1)
test/signature-object-tests.spec.ts (1)
src/signed-xml.ts (1)
SignedXml(30-1422)
🔇 Additional comments (4)
src/signed-xml.ts (1)
1138-1141: LGTM! Clean refactoring to centralize ID handling.The delegation to
ensureHasIdsimplifies the code while maintaining all existing behavior:
- Supports prefixed Id attributes (e.g.,
ns:Id)- Handles WS-Security mode with namespaced IDs
- Generates IDs when missing
- Maintains the
ref.urimutation as documented in learningstest/signature-unit-tests.spec.ts (1)
1407-1436: LGTM! Good test coverage for prefixed Id attributes.The test properly validates that prefixed Id attributes (e.g.,
ns:Id="unique-id") are correctly recognized byensureHasIdand used in the Reference URI, ensuring the refactoring handles namespaced attributes as expected.test/signature-object-tests.spec.ts (2)
397-433: LGTM! Thorough test for KeyInfo with explicit Id.The test properly validates that:
- KeyInfo elements with explicit Ids (via
keyInfoAttributes) are correctly referenced- The Reference URI points to the provided Id
- The resulting signature is cryptographically valid
435-465: LGTM! Good coverage for autogenerated KeyInfo Id.The test validates that KeyInfo elements without explicit Ids get auto-generated Ids via
ensureHasId, and that references to them work correctly. This complements the explicit Id test and ensures the refactoring handles both scenarios.
…ode-saml#520) - Run ensureHasId(node) in addAllReferences to: 1. have consistent Id matching logic as the initial pass 2. add Id attributes to elements which are present inside the Signature itself (KeyInfo, Object) - Added tests for autogenerated ids within the Signature and prefixed Ids
shunkica
force-pushed
the
id-attribute-handling
branch
from
0dc9d9c to
f7dbece
Compare
2 participants
Resolves issue #520
Summary by CodeRabbit
Bug Fixes
Tests