possibility of removing xmldom-fork-fixed dependency and moving to xmldom 0.1.19

[ploer]

Xmldom clarified its license to a dual MIT/LGPL license several versions ago; however, the version of xmldom used in xml-crypto does not include that clarification.

It would be great to move to the latest xmldom (0.1.19), so that organizations that can't accept a LGPL license can still use xml-crypto. (We use xml-crypto in passport-saml, and this issue comes up there).

I notice that if I remove xmldom-fork-fixed and update xmldom, there is a single test case that starts failing. I haven't found a repository with the xmldom-fork-fixed changes, but it seems like there must be something in there that hasn't been submitted back to xmldom.

Is there anything I could do to help get this fixed?

[yaronn]

Hi @ploer

I'm not sure what that change was but I'm very disappointed of myself for not committing the repository for it. I do remember that newer versions of xmldom broke some tests (regardless of any fixed I have made on the fork) so at that time I wanted to freeze the xmldom version with a few minor fixes I had to add. I think it might be close to my xmldom fork in github. If you try the bellow instead of xmldom-fork-fixed do tests pass?

git+https://github.com/yaronn/xmldom.git

That repo has a few concrete changes which are easy to track. Otherwise the solution will be to compare the npm package code. From my perspective any change that I have made to xmldom can be under any license you want.

I will probably not have time to check it myself in the next week or two but if that does not work for you I will take a look when I can.

Thanks!

[KevinHorvatin]

I tried this and a number of tests failed. Unfortunately I didn't record the failing tests.

[ploer]

@yaronn, thanks for the helpful background, and sorry it took me so long to get back to this.

It looks to me like if I use your fork the tests do indeed pass (@KevinHorvatin, any chance there was something else going on in the environment you tested in?).

It looks like those changes mostly consist of adding an ignoreWhitepace option. Seems like something we might be able to submit back, or maybe there is some alternative we could use. I'll look into it in a little more detail.

[ploer]

If I'm understanding things correctly, it looks like the ignoreWhitespace option is only used in two of the tests, and is only needed for one of them ("windows store signature"). It does not appear to be exposed to consumers of the library or be used in the actual implementation of signature checking.

It's not entirely clear to me if the input to that particular test case is even legitimate, since the whitespace has to be removed for the signature to validate. (and XML signatures are supposed to be whitespace-sensitve, right?)

So perhaps a simpler solution would be to change that test artifact (windows_store_signature.xml) to not have extraneous whitespace? Making that change locally is sufficient to get the tests to pass using just the main branch [email protected].

I'm happy to submit a PR with that change if it sounds appropriate.

[yaronn]

the whitespace patch is because of this:

http://webservices20.blogspot.co.il/2013/06/validating-windows-mobile-app-store.html

which is actually based on a defect someone opened:

#23

I agree that the unit test as it is now actually checks xmldom-forked-fixed rather than xml-crypto but it is a valid end-to-end scenario to check. what do you think about reverting to the official xmldom, but still keep xmldom-forked-fixed as a dev dependency for unit tests. will this work license wise?

[ploer]

Ah, interesting, thanks for the background.

What would you think of just adding a naive transform inside the test along the lines of xml = xml.replace(/>\s*</g, '><');, and then add a comment saying "to correctly handle windows store signatures, use a library such as xmldom-fork-fixed with the ignoreWhiteSpace option".

This would leave the test artifact as-is, documents what you need to do if you have those use cases, and verifies that the library correctly handles the signature once whitespace is stripped. It seems to me that that's basically what is being accomplished in the current arrangement, but would allow removing the xmldom-fork-fixed dependency.

[ploer]

I've created PR #42 to show what I had in mind, and seems like it would address this issue if that could work for you.

[yaronn]

Looks good, I have merged and published it. Thanks!

[ploer]

Thank you, really appreciate it!