Repository navigation
perf(xmldsig): execute each signed Reference's transforms once - #369
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aef391a099
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
polaz
force-pushed
the
perf/#367-single-transform-run
branch
from
November 2, 2026 10:47
aef391a to
714f52d
Compare
Signing ran every Reference's transform chain twice: dependency analysis executed it to learn which mutable values reach its output and discarded the result, then the digest pass executed it again. - The analysis terminal digests the output when the settled dependencies are empty and no step may read character data outside its input; the digest pass takes that value instead of executing the chain again - The speculative analysis made before committing generated canonicalization keeps its plan after that commit, since the commit only changes nodes it tracks; a KeyInfo commit, which may add an ID, still triggers a full analysis - The aggregate external resource test now admits one dereference at an allowance equal to the resource size and refuses one byte less Closes #367
XSLT reads only its input octets and its own stylesheet, and Base64 decodes the text of the nodes it is given, so a DigestValue element is in their input exactly when its future text is. References using them no longer need a parse of the document with placeholder values to find their dependencies. With the analysis now producing digests, an analysis made before the writer's KeyInfo could spend the operation's allowances on a result that is discarded: - The KeyInfo is spliced before the analysis; one that is not inert (it may add an ID or a namespace scope) is committed first and the analysis runs once after it, and the splice is reused by that commit - An inert KeyInfo keeps the analysis plan across the commit - A spent resource limit in the speculative analysis ends the operation instead of being retried after the commit
- Dependency analysis stops a chain once it is known to depend on a value signing fills: before an octet-consuming step (XSLT, Base64) whose input already holds a tracked node, or after a dependency lost its provenance. The output was discarded anyway; a stylesheet no longer runs on a DigestValue not yet filled, where it could fail although the digest pass after the fill succeeds - A writer KeyInfo that removes an element carrying an ID is not inert: the plan made before the splice is no longer kept across its commit and the splice is not deferred, so a Reference to the removed ID is refused instead of keeping a digest of a target the signed document lacks Regression tests cover a Manifest stylesheet that refuses an empty DigestValue and a KeyName ID dropped by the writer, committed and deferred.
polaz
force-pushed
the
perf/#367-single-transform-run
branch
from
November 2, 2026 11:27
714f52d to
50f52d6
Compare
polaz
enabled auto-merge (squash)
November 2, 2026 11:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Testing
Workspace tests with all features (including the xmlsec1 and libxslt oracles), the PKCS#11 suite, Clippy, feature-profile lints, the no-std target, the fuzz build and the compatibility ledgers pass on macOS; Linux and Windows run in CI.
Stacked on #366.
Closes #367