fix: omit <Transforms> element when no transforms are specified - #542
fix: omit <Transforms> element when no transforms are specified#542msheby wants to merge 4 commits into
Conversation
When addReference is called without transforms (or with an empty transforms array), createReferences previously always emitted an empty <Transforms></Transforms> element. This is invalid under SMPTE ST 430-3 §8.2, which requires the Transforms field to be absent when no transformations apply. Changes: - Make `transforms` optional on the Reference interface - Remove the addReference guard that threw on empty/absent transforms - Guard <Transforms> emission in createReferences so the element is only written when at least one transform is present - Apply C14N fallback in getCanonXml for the empty-transforms case so sign and verify use the same canonical form (matching the existing loadReference behavior) Closes node-saml#540
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReferences now support absent or empty transforms. Signature generation omits the ChangesReference transform handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This change omits an invalid empty Transforms element while preserving signing behavior for references with transforms; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant SignatureGeneration
participant getCanonXml
participant DigestAlgorithm
participant SignatureVerification
SignatureGeneration->>getCanonXml: canonicalize referenced DOM node without transforms
getCanonXml->>DigestAlgorithm: provide canonical XML
DigestAlgorithm-->>SignatureGeneration: return DigestValue
SignatureVerification->>getCanonXml: canonicalize referenced DOM node
getCanonXml->>DigestAlgorithm: provide canonical XML
DigestAlgorithm-->>SignatureVerification: compare digest
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ 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: 2
🧹 Nitpick comments (1)
test/signature-unit-tests.spec.ts (1)
1077-1100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the empty-array input explicitly.
This test covers an omitted
transformsproperty only. The API also promises identical serialization fortransforms: [].Add a second case, or parameterize this test, with
transforms: []. Keep the assertion that noTransformselement exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/signature-unit-tests.spec.ts` around lines 1077 - 1100, Extend the “omits Transforms element when no transforms are specified” test to also call addReference with transforms explicitly set to an empty array. Preserve the existing assertion that the generated signature contains no Transforms element, either by adding a second case or parameterizing the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/signed-xml.ts`:
- Line 434: Update the ancestor-namespace lookup in addAllReferences to pass the
current matched node to findAncestorNs instead of ref.xpath, ensuring each
reference uses its own namespace scope. Add a test covering multiple matched
nodes under different namespace declarations and verify their generated
signature validates successfully.
In `@test/signature-unit-tests.spec.ts`:
- Around line 911-912: Update the assertions around ref.transforms to avoid
non-null assertions: assert the complete expected transform list directly using
the existing transform value, while preserving the expectation that it contains
exactly one canonicalization transform.
---
Nitpick comments:
In `@test/signature-unit-tests.spec.ts`:
- Around line 1077-1100: Extend the “omits Transforms element when no transforms
are specified” test to also call addReference with transforms explicitly set to
an empty array. Preserve the existing assertion that the generated signature
contains no Transforms element, either by adding a second case or parameterizing
the test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e14fcfee-aee2-42f7-8d98-17f02170b629
📒 Files selected for processing (3)
src/signed-xml.tssrc/types.tstest/signature-unit-tests.spec.ts
Using ! on optional properties triggers the no-non-null-assertion ESLint rule. Collapsing the two separate length/index checks into a single deep.equal is also more readable.
…test The previous test only verified that a missing transforms property suppresses the <Transforms> element. Parameterize the test to also cover transforms: [], which the API treats identically.
…ing xpath getCanonReferenceXml passed ref.xpath to findAncestorNs, which always uses docSubset[0] — the first XPath match. When addAllReferences creates multiple references for the same xpath pattern and those matched elements live under different ancestor namespace scopes, every reference beyond the first was digested with the wrong namespace context, producing a signature that verifiers would reject. Fix: add findAncestorNsForNode(element) to utils and call it with the node already in scope instead of re-executing the XPath. Also extract the shared deduplication/filtering logic into buildAncestorNsForElement to avoid code duplication between the two public helpers. Adds a regression test: two <item> elements under sibling <section> elements that each declare a different namespace prefix. The fix makes sign+verify round-trip correctly; the old code would fail verification for the second reference.
1 participant
When
addReferenceis called without transforms (or with an empty transforms array),createReferencespreviously always emitted an empty<Transforms></Transforms>element. This is invalid under SMPTE ST 430-3 §8.2, which requires theTransformsfield to be absent when no transformations apply.Changes:
transformsoptional on theReferenceinterfaceaddReferenceguard that threw on empty/absent transforms<Transforms>emission increateReferencesso the element is only written when at least one transform is presentgetCanonXmlfor the empty-transforms case so sign and verify use the same canonical form (matching the existingloadReferencebehavior)Closes #540
Summary by CodeRabbit
New Features
Transformselement when no transforms are configured.Bug Fixes