Fix transform processing regression

[cjbarth]

There was a bug introduced in the 4.x release of xml-crypto whereby transforms weren't added in all the cases they were supposed to.

This includes code from the comment by @srd90.

Closes #378

[codecov]

Codecov Report

Merging #379 (0ecf235) into master (fa2922f) will increase coverage by 0.14%.
The diff coverage is 70.83%.

@@            Coverage Diff             @@
##           master     #379      +/-   ##
==========================================
+ Coverage   72.84%   72.98%   +0.14%     
==========================================
  Files           9        9              
  Lines         880      881       +1     
  Branches      234      235       +1     
==========================================
+ Hits          641      643       +2     
  Misses        149      149              
+ Partials       90       89       -1     

Files Changed	Coverage Δ
src/enveloped-signature.ts	81.81% <ø> (ø)
src/c14n-canonicalization.ts	52.10% <50.00%> (+1.68%)	⬆️
src/signed-xml.ts	74.33% <63.63%> (+0.17%)	⬆️
src/exclusive-canonicalization.ts	77.34% <81.81%> (-1.23%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[srd90]

This is not exactly fix for regression but maybe there should be guard against situation where signature does not have any references. I.e. if this code block

xml-crypto/src/signed-xml.ts

Lines 473 to 480 in 2e32d50

	validateReferences(doc: Document) {
	for (const ref of this.references) {
	if (!this.validateReference(ref, doc)) {
	return false;
	}
	}
	return true;
	}

would have had something like this (in pseudo code) it would have prevented this ( #378 ) particular signature bypass situation in published versions [4.0.0, 4.1.0].

  validateReferences(doc: Document) { 
+   if ( ! Array.isArray(this.references) || this.references.length <= 0) {
+       throw new Error("Missing reference(s)");
+       // or return false ....
+   }
    for (const ref of this.references) { 
      if (!this.validateReference(ref, doc)) { 
        return false; 
      } 
    } 
    return true; 
  } 

Links to spec:

https://www.w3.org/TR/2013/REC-xmldsig-core1-20130411/#sec-SignedInfo says:

The structure of SignedInfo includes the canonicalization algorithm, a signature algorithm, and one or more references. ...

and https://www.w3.org/TR/2013/REC-xmldsig-core1-20130411/#sec-Reference says:

Reference is an element that may occur one or more times. ...

[srd90]

line 171 already handles xpath.isComment(node)

not related to regression bug: How this and other C14N implementations would handle XML CDATA sections (in which case node type might might be CDATA or something like that):

<library>
  <book id="bookid">
    <name><![CDATA[this & that < foo]]></name>
  </book>
</library>

instead of

<library>
  <book id="bookid">
    <name>this &amp; that &lt; foo</name>
  </book>
</library>

[cjbarth]

Fixed.

Regarding CDATA, MDN clarifies that we should be handling more cases where there is a .data property: https://developer.mozilla.org/en-US/docs/Web/API/CharacterData

[cjbarth]

I didn't like my original fix. There is no value in complicating the code just for the sake of types. I've used a different method for now and we can revisit types later on.

[cjbarth]

This is not exactly fix for regression but maybe there should be guard against situation where signature does not have any references.

I like that suggestion. I wonder if @DiegoMajluf could help with a test or two for these cases.

[DiegoMajluf]

@cjbarth thanks for your work. I will make some test with my testing documents, I'm still facing some issues with the canonicalization. I will let you know.

[DiegoMajluf]

@cjbarth @srd90, I have tested PR 379 with some of my production documents, and all of them have passed successfully. Thank you very much once again.

However, as I mentioned, I'm still encountering issues related to canonicalization (or transform algorithm; I'm not very clear on the difference yet). From what I've gathered so far, it seems to be connected to the presence or absence of namespace attributes on the root element. Nevertheless, I believe this might belongs to a different issue.

I will create a new issue report when I can build a more specific situation.

[LoneRifle]

lgtm, unblocking this for now