Skip to content

Make removesNodes required on CanonicalizationOrTransformationAlgorithm #558

Description

@cjbarth

Summary

Make removesNodes required on CanonicalizationOrTransformationAlgorithm. It shipped as optional in 6.x because it could not be required without breaking every custom algorithm; 7.0 is where that gets fixed.

Background

computeSignature() rejects a reference that encloses the signature when every transform on it provably cannot remove the Signature — otherwise the digest covers a Signature whose SignatureValue is still empty, and the result can never verify. Each algorithm declares that about itself:

readonly removesNodes = false; // canonicalization: renders the node-set, drops nothing
readonly removesNodes = true;  // enveloped-signature: removes the Signature

The problem with optional

Three states exist today, and the third is the bad one:

removesNodes Meaning Check
false provably preserves the Signature applies
true may remove it defers
undefined we assume it may remove it defers

That third row is a default making a security-relevant decision for the implementer. Someone writing a pure canonicalization transform gets no protection and is never told — they did not choose to opt out, they just did not know the field existed. This is the shape AGENTS.md warns about: "Don't add a default for anything security-relevant. Throw instead."

Requiring it removes the row. Every algorithm states what it does, and the check is exact for custom algorithms and built-ins alike.

Scope

  • Change removesNodes?: boolean to removesNodes: boolean in src/types.ts.
  • Drop the algo != null && / === false defensiveness in SignedXml.preservesEveryNode(); the value is then always present.
  • Note the break in the changelog with the migration below.

Migration

Add one line to any custom canonicalization or transform:

class MyTransform implements CanonicalizationOrTransformationAlgorithm {
  readonly removesNodes = false; // or true, if it drops nodes from the node-set
  process(node: Node) { /* ... */ }
  getAlgorithmName() { return "http://MyTransformation"; }
}

Pick false only if the algorithm returns the same node-set it was given. If it filters nodes at all — an XPath transform, say — use true, which is also the conservative choice: it keeps the current deferring behaviour.

TypeScript implementors get a compile error naming the missing property, so the break is loud and mechanical. JavaScript implementors get no such signal, which is worth a prominent changelog entry: for them the field is silently undefined, and the runtime behaviour is unchanged from 6.x rather than newly broken.

Worth deciding alongside

Whether preservesEveryNode() should throw on an unregistered algorithm rather than returning false. Today an unknown URI defers here and then fails later in findCanonicalizationAlgorithm() with "canonicalization algorithm is not supported". That ordering is fine, but if the errors are ever reworked, the two paths should agree on which one reports first.

Related

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions