Repository navigation
Description
Summary
Found while chasing a codecov/patch failure on #571. Three separate things in src/signed-xml.ts, worst first. They could reasonably be split into two issues — (1) is a behaviour question, (2) and (3) are test hygiene — but they surfaced together and share a cause, so they are written up together.
1. Two documented constructor options have no test at all
| Option | Documented | Never executes |
|---|---|---|
implicitTransforms |
README §Caring for Implicit transform, with a worked example, plus the options list | src/signed-xml.ts:766-767 — the forEach that appends them to a reference's transforms |
idAttribute |
README options list, line 251 | src/signed-xml.ts:162 — this.idAttributes.unshift(idAttribute) |
grep -o implicitTransforms test/*.spec.ts returns nothing. idAttribute returns two hits, but both are a local variable of that name in an unrelated assertion, not the option being passed.
So the package documents two options, one of them with its own README section, and has no evidence either of them works. This is the part worth acting on.
2. A test passes on the wrong error
signer appends signature to a non-existing reference node in test/signature-unit-tests.spec.ts reads as coverage for the the following xpath cannot be used because it was not found path. It is not:
- it never sets
canonicalizationAlgorithm, socomputeSignaturethrows out ofcreateSignedInfowithMissing canonicalizationAlgorithm when trying to create signed info for XMLbefore thelocation.referencelookup is reached; - the only assertion is
expect(err).not.to.be.an.instanceof(TypeError), which everyErrorsatisfies.
Coverage confirms the path it names never runs: src/signed-xml.ts:1026-1033 is uncovered on master.
Fix is to set canonicalizationAlgorithm and assert on the message, at which point it covers what it claims.
3. Error contracts are broadly unverified
src/signed-xml.ts has 529 statements, 70 uncovered (13.2%). Of the uncovered, 32 are throw sites:
| Method | Uncovered throws |
|---|---|
loadReference |
5 — every could not find …DigestMethod/Algorithm/DigestValue… guard |
getCanonSignedInfoXml |
5 — No signature found., Missing canonicalizationAlgorithm…, could not find SignedInfo…, multiple/whole xml dom guards |
checkSignature |
5 |
computeSignature |
5 |
addReference |
3 — including digestAlgorithm is required and transforms must contain at least one transform algorithm |
findSignatureAlgorithm |
2 — including signatureAlgorithm is required |
findCanonicalizationAlgorithm, findHashAlgorithm, calculateSignatureValue, validateElementAgainstReferences, loadSignature, namespace resolver |
1 each (7 total) |
Two of these are the errors AGENTS.md singles out as deliberate API design — signatureAlgorithm is required and digestAlgorithm is required exist specifically so nobody silently inherits SHA-1 — and neither has a test.
Also uncovered, and not an error path: the string branch of validateElementAgainstReferences (src/signed-xml.ts:500-502). The method is public and accepts Element | string; only the Element form is exercised.
One caveat on the number
17 of the 70 uncovered statements are callback-form branches that #571 removes, so the count drops on its own when that lands. Worth re-measuring after it rather than chasing 13.2% today.
Why this matters here
AGENTS.md already states the standard: "Uncovered code indicates either inadequately tested public behavior or code that may be unnecessary; determine which." For (1) the answer is inadequately tested public behaviour. For (3) these are the messages that tell a caller their configuration is unsafe, in a library where failing closed is the whole point — an error that stops being thrown, or starts saying something else, is a silent change to the contract.
Suggested scope
- (1) is the one with real risk and deserves its own change.
- (2) is a two-line fix.
- (3) is best done after feat!: give asynchronous work its own entry points #571 lands, and probably incrementally rather than as one sweep.
🤖 Generated with Claude Code
Activity
cjbarth commentedon Sep 28, 2026
Parts 1 and 2 don't need to wait for #571. They only add tests, so they can land on
masterfor 6.3.3. The issue stays on 7.0 for part 3.For
idAttribute, test what 6.x promises. When verifying, an element whose ID is in that attribute is found. When signing, that ID is reused rather than a new one being added. Don't assert the name of the attribute that signing adds to an element without an ID. That's still hard-coded toIdinensureHasId(), and #508 changes it in 7.0.[-]`implicitTransforms` and `idAttribute` have no tests, error contracts are unverified, and one test passes on the wrong error[/-][+]`implicitTransforms` and `idAttribute` have no tests[/+]cjbarth commentedon Sep 28, 2026
Splitting this. Part 2 is done: #599 replaced the test that passed on the wrong error. Part 1 is tests only, so it moves to 6.3.3, and this issue now covers just that; I have retitled it to match. Part 3 is now #630, on 7.0, with coverage measured again on
master.