feat!: shrink the public export surface to what it means to publish - #569
feat!: shrink the public export surface to what it means to publish#569cjbarth wants to merge 4 commits into
Conversation
`index.ts` did `export * from "./utils"`, so every helper written for `signed-xml.ts` to use became public API at the same time. That is how `findChilds` stayed published with no callers and how each new helper joined the surface without anyone choosing to publish it. The export list is explicit now, and it names the four helpers a consumer has a real use for: `derToPem`, `pemToDer` and `normalizePem` for key-format conversion, and `findAncestorNs`, which is already part of the custom canonicalization contract in practice — three projects on GitHub import it alongside `C14nCanonicalization` to canonicalize a document subset by hand. See the survey on node-saml#551. `validateDigestValue` goes internal despite the "check before dropping" note. Nobody imports it, and publishing the constant-time comparison is what invites someone to reimplement it; it belongs behind the API that already uses it. `BASE64_REGEX` and `PEM_FORMAT_REGEX` lose their `export` keyword outright, since no sibling module uses them. The rest stay exported from `utils.ts` for their siblings but are no longer re-exported to consumers — a sibling and a consumer reaching a helper through the same `export` keyword is what caused this, and `index.ts` is the only place that can now widen the surface. `index.ts` only governs the barrel, though. With no `exports` map every file under `lib/` was reachable by path, and consumers do reach: one published package imports `xml-crypto/lib/hash-algorithms.js` for a class we never exported. Declare an `exports` map naming the entry point and `package.json` and nothing else, so a deep import fails loudly instead of quietly depending on the build layout. The bundled algorithm classes stay reachable through the `HashAlgorithms`, `SignatureAlgorithms` and `CanonicalizationAlgorithms` registries that name them. `test/public-api-tests.spec.ts` pins both the runtime export names and the declared subpaths, so widening either is a deliberate edit rather than a side effect of adding a helper or a file. The README documented an `xpath` export that 6.x does not have — the verification example destructured it and would have thrown. Point the examples at the `xpath` package instead, whose `select()` takes the expression first, and replace that section with the actual export list. BREAKING CHANGE: `findAttr`, `findChildren`, `findChilds`, `isDescendantOf`, `isArrayHasLength`, `encodeSpecialCharactersInAttribute`, `encodeSpecialCharactersInText`, `validateDigestValue`, `BASE64_REGEX`, `EXTRACT_X509_CERTS` and `PEM_FORMAT_REGEX` are no longer exported, and subpaths into `lib/` no longer resolve. See the Upgrading section of the README for replacements. Closes node-saml#551 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe package now exposes an explicit public API, blocks unlisted deep imports through its ChangesPublic API surface
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to The 7.0 release narrows the public API and blocks unsupported deep imports while documenting migration paths. The change is ready to merge with normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR implements the 7.0 export reduction, preserves selected types and utilities, removes internal helpers and constants, adds export-surface safeguards, and updates the README. It does not show the required changelog updates, and the provided changes do not independently demonstrate the preceding 6.x deprecation work described in issue Resolution Add changelog entries for the removed exports and their replacements. Provide evidence that the required 6.x deprecation phase was completed before removing the names in 7.0, or include the missing deprecation changes if they are part of this PR [ Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
package.jsonParsing error: ESLint was configured to run on Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #569 +/- ##
==========================================
+ Coverage 75.95% 76.91% +0.96%
==========================================
Files 9 9
Lines 1048 1057 +9
Branches 273 275 +2
==========================================
+ Hits 796 813 +17
+ Misses 144 138 -6
+ Partials 108 106 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`findChilds` is an alias of `findChildren` with no caller in `src/`, so withdrawing it from the barrel still left dead code behind for no one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 7.0 upgrade table told callers of the removed encoders to use an XML serializer, but a serializer does not escape the way C14N requires, so a custom canonicalizer that followed the advice would change its digests. Point them at `C14nCanonicalization` and `ExclusiveCanonicalization`, as the 6.x deprecation notice now does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 7.0 upgrade table said `crypto.timingSafeEqual()`'s length-mismatch exception "counts as unequal". It throws, so following the advice turns an ordinary mismatch, possibly caused by untrusted XML, into an exception where `validateDigestValue()` returned `false`. Check the lengths first, as the 6.x deprecation notice now says. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes #551
index.tsdidexport * from "./utils", so every helper written forsigned-xml.tsto use became public API at the same time. That is howfindChildsstayed published with no callers, and how each new helper joined the surface without anyone choosing to publish it.The keep list is informed by the consumer survey on the issue, which checked what dependents and public code actually import rather than what we call.
What stays
derToPem,pemToDer,normalizePempemToDerhas a confirmed importer, and three separate projects hand-rolled anormalizePem, which says the need is real and the export merely undiscoverablefindAncestorNsakira-io/node-efatura,kacheablecr-jpg/nervus-backendanddressoria/sri-signing-workerallimport { C14nCanonicalization, findAncestorNs } from "xml-crypto"What goes
findAttr,findChildren,findChilds,isDescendantOf,isArrayHasLength,encodeSpecialCharactersInAttribute,encodeSpecialCharactersInText,validateDigestValue,BASE64_REGEX,EXTRACT_X509_CERTS,PEM_FORMAT_REGEX. None has a confirmed importer; every code-search hit was a same-named local helper, a fork, a vendored copy, or a CVE-reproduction corpus.validateDigestValuegoes internal despite the issue's "check before dropping" note. Nobody imports it, so the risk of pushing someone toward a naive===is not realised — and publishing the constant-time comparison is itself what invites reimplementation. It belongs behind the API that already uses it.BASE64_REGEXandPEM_FORMAT_REGEXlose theexportkeyword outright since no sibling uses them. The rest stay exported fromutils.tsfor their siblings but are no longer re-exported to consumers — a sibling and a consumer reaching a helper through the sameexportkeyword is the root cause, andindex.tsis now the only place that can widen the surface.lib/was public tooindex.tsonly governs the barrel. With noexportsmap, every file underlib/was reachable by path, and consumers do reach:shunkica/fiskalizacija2-jsdoesfor a class we never exported. So
package.jsonnow declares anexportsmap naming the entry point andpackage.jsonand nothing else. Verified against a real package resolution: the barrel resolves,xml-crypto/lib/hash-algorithms.jsfails withERR_PACKAGE_PATH_NOT_EXPORTED,xml-crypto/package.jsonstill resolves.mainandtypesstay for resolvers that ignoreexports.The bundled algorithm classes remain reachable through the registries that name them, which the README now documents:
Guard against regrowth
test/public-api-tests.spec.tspins both the runtime export names and the declared subpaths, so widening either shows up as a reviewable diff rather than a side effect of adding a helper or a file. Confirmed it fails onmaster, listing exactly the names withdrawn here.README fix found along the way
The README documented an
xpathexport that 6.x does not have —require("xml-crypto").xpathisundefined, so the verification example would have thrown onselect(...). The examples now use thexpathpackage directly, whoseselect()takes the expression first; verified the corrected example validatestest/static/valid_signature.xml. That section is replaced with the actual export list.Ordering
#567 deprecates every name withdrawn here, with a runtime warning, and is meant to land on 6.x first. This branch will need a rebase after that.
Verification
npm run build && npm test && npm run lintclean; 243 passing (241 + 2).🤖 Generated with Claude Code
Summary by CodeRabbit
lib/imports are no longer supported.SignedXmlregistries.