Repository navigation
[ENHANCEMENT]: Make signature-emission template methods protected so subclasses can customize output without type suppressions #539
Description
Activity
That is a lot of changes and I'm not sure they would all be required. First, #540 should address part of this concern. For the others, can you cite which SMPTE standards require a transform to be applied, but not specified? That seems broken on their end.
For your
getKeyInfoconcern, that is why we have an interface forgetKeyInfoContent. Does that work for you or am I missing something?Are there specific tests you have in mind that are made more complex by our current structure?
Thanks for the quick response!
SMPTE ST 430-3:2012 §8.2 is the relevant normative text — it states verbatim: "The Transforms field of the Reference elements shall be omitted, meaning the bytes of the referenced node are hashed without any transformations." So the spec doesn't require a transform applied implicitly; it requires
<Transforms>to be absent entirely. The problem is the output side: xml-crypto currently emits<Transforms></Transforms>when addReference is called with an empty array, which violates this. You're right that #540 is the fix for that — we agree it covers this concern and we've submitted a PR for #538 as well.On getKeyInfoContent: That interface controls the inner content but not the indentation of the outer
<KeyInfo>wrapper element itself. Our override is purely cosmetic — D-Cinema XML files are frequently read manually by operators and engineers, so consistent indentation throughout the signature block is useful in practice. That said, if this is the only concern with getKeyInfo, we'd be happy to drop it from the request.On scope: You're right that not all 11 methods are strictly necessary. The minimum set that would let us eliminate the as unknown as type casts in our subclass is:
- getCanonXml — fixes a C14N serialization bug (self-closing elements after enveloped-signature); notably this method isn't declared in the type definitions at all today
- getCanonReferenceXml — fixes the duplicate default namespace issue from
findNSPrefixreturns only the first xmlns declaration → duplicate default xmlns in non-exclusive C14N when subset declares anyxmlns:*#538 - createSignedInfo, createSignature, createReferences — pretty-printing for human-readable ETM output
- findCanonicalizationAlgorithm, findSignatureAlgorithm — called from the above overrides
We're happy to narrow the PR to just these if that's preferable.
On tests: The main complexity today is that algorithm lookups (findCanonicalizationAlgorithm, findSignatureAlgorithm) are private, so unit tests that want to inject a deterministic stub algorithm can't do so cleanly without the same as unknown as workarounds. Making them protected would allow straightforward test subclasses.
What tests are you trying to write? Would they be better suited to being directly in this project? That might make several things easier.
As for pretty-printing, I feel like that can easily be done on the reviewer's copy of the XML while leaving the signed copy alone.
As for the first two issues, is this something we can include in this project?
One thing to weigh before this lands in 6.4: each method made
protectedbecomes part of the semver contract for subclasses, and 7.0 is moving the other way (#618 makesC14nCanonicalization'srenderNs()andprocessInner()private). So the set should be the smallest one a concrete use needs.On the uses so far: the SMPTE
<Transforms>case was left to #540, but see my correction there.addReference()refuses emptytransformstoday. The questions from 11 August are also still open: which tests you want to write, and whether pretty-printing can be done on a reviewer's copy.
Is your feature request related to a problem? Please describe...
Summary
SignedXmlhas a natural template-method shape —computeSignaturecallscreateSignedInfo, which callscreateReferences, which callsgetCanonReferenceXml, which calls the (public)getCanonXml; separatelycomputeSignaturecallsgetKeyInfoandcreateSignature. In v6 every one of those intermediate methods is declaredprivate, which makes the class impossible to subclass cleanly in strict TypeScript: any override triggers TS4114 (must have 'override' modifier) and addingoverridetriggers TS2415 (private in base class). The only way to keep the subclass compiling is@ts-expect-erroror an intersection-type cast — both of which sidestep type safety rather than express the design.The proposed change is a one-line-per-method visibility change from
privatetoprotectedon the methods that participate in the template. No behavior change; consumers who don't subclass see no difference. Consumers who do subclass can now express legitimate customizations in typed code.Motivating cases
Pretty-printing / spec-mandated formatting of
SignedInfoand its children. Post-processing the emitted signature to reformat<SignedInfo>isn't safe — non-exclusive C14N preserves inter-element whitespace as text nodes, so any post-hoc reformat changes the canonical form and invalidates the already-computedSignatureValue. The only correct place to intervene is inside the same chain that produces the pre-canonicalization string, which today means overridingcreateSignedInfo(private) orcreateReferences(private).Reference-emission customization. Some SMPTE standards require that Reference elements not contain a
<Transforms>child at all. The digest still has to be computed under C14N; only the emission changes. Doing this cleanly needs acreateReferencesoverride, which is currently blocked.Custom
getKeyInfooutput (e.g. emitting a full X.509 certificate chain rather than a single cert) — thegetKeyInfoContentcallback covers the content, but a subclass that wants to alter attribute ordering, whitespace, or attachIdattributes to the outer<KeyInfo>element needs the wrapping method.Repro-friendly algorithm lookups in tests. The
findSignatureAlgorithm/findCanonicalizationAlgorithm/findHashAlgorithmhelpers are the natural stub points for deterministic tests of subclass logic; keeping themprivateforces test subclasses into the same visibility gymnastics.Describe teh solution you'd like...
Concrete ask
Change visibility from
privatetoprotectedon the following methods insrc/signed-xml.ts(line numbers from v6.1.2):getCanonSignedInfoXmlcalculateSignatureValue; entry point for SignedInfo C14NgetCanonReferenceXmlcalculateSignatureValuefindSignatureAlgorithmfindCanonicalizationAlgorithmfindHashAlgorithmloadReferencecreateReferencesgetKeyInfo<KeyInfo>wrappercreateReferences<Reference>elementsensureHasIdcreateSignedInfo<SignedInfo>createSignature<Signature>elementThe state fields those methods touch (
this.signatureNode,this.references,this.signatureValue, etc.) can stayprivate— subclasses that need them can be added incrementally.Compatibility
private → protectedis a source-compatible widening. Existing consumers that don't subclass are unaffected. Existing subclasses (if any) that were already relying on TypeScript workarounds can drop those workarounds. The compiled JavaScript is byte-identical.Related
findNSPrefixreturns only the first xmlns declaration → duplicate default xmlns in non-exclusive C14N when subset declares anyxmlns:*#538 (findNSPrefixreturns only the firstxmlns[:*]attribute).Describe the alternatives you've considered...
Forking the type declarations -- ugh.