`xpath` dependency "problem"

[cjbarth]

@markstos @LoneRifle , I'd like to get your thoughts on an issue that I see as I port this code over to TypeScript. I note that some types in the xpath dependency are wrong, specifically xpath.selectWithResolver() isn't present in xpath.d.ts. I was going to make a PR to add it, and then I noted that the last commit was several years go.

I poked around the issues and PRs to see if that was just because the project was already "done" and didn't really need any updates and I found an issue with a security vulnerability that hasn't been addressed, even though there is a PR to partly address it.

A little more digging shows that someone took it upon themselves to re-write this entire thing in TypeScript, but appears to have abandoned it.

I also found several performance improvements that people have suggested here and here, at least. @nosvalds made a fork to incorporate those into the codebase for his use, but he doesn't seem to be active on GitHub anymore. However, @cleydyr, who contributed these very nice improvements does seem active on GitHub, but his fork isn't any better maintained, not even including his own performance improvements.

So, it seems this project, which we depend on, is now an unmaintained security risk. Any thoughts on what we might do about it? Should it also be part of the node-saml organization, perhaps as a fork? I don't want to take on any more, but it does seem that if we want to maintain the security of this project, we have to do a little like what Auth0 ended up doing.

Adding @yaronn and @JLRishe in case they have anything to say on this matter since @yaronn wrote the original library and @JLRishe apparently maintains the fork we use. I'm also adding @karfau, because he said he is one of the maintainers, so maybe he can offer some insight.

[karfau]

Sadly that statement of mine is confusing (I now made a tiny update to the op make that explicit).
I'm the maintainer of the xmldom aka @xmldom/xmldom package, so I'm willing to support any fork that tries to bring that dependency up-to-date, but that's all I will be able to do.

PS: There is a recent development in xmldom related to xpath if I understood it correctly: xmldom/xmldom#488 which has been done by @zorkow

[JLRishe]

Hello @cjbarth

Thank you for tagging me.

To address your comments:

• Vulnerability warning
  • In its current state, the project uses the old xmldom package in its unit tests and in the getting started guide. While this is not ideal, and needs to be updated, the actual library has no external dependencies, so I think referring to the package of a security risk, on account of the xmldom warnings, would be unfounded, as they have no bearing on anything that imports and uses this package.
• Unmerged performance changes
  • I have just added a comment to #108 to explain why it can't be merged in its current state. It uses a non-DOM-standard feature of the xmldom package, and this xpath package is intended to work with any standards-compliant DOM implementation, not just xmldom.
  • Regarding #107 , this seems to be relying on a JS engine-specific (V8) implementation detail in order to achieve its performance gains (reversing an array and using .pop() instead of using .unshift() without reversing the array). While I'm not entirely opposed to merging this change, I don't really have the wherewithal to evaluate it.

While I am sorry that I haven't given more attention to this project, hopefully the above provides some insight as to why more action hasn't been taken on these three matters, and allays your concerns about any security vulnerability.

I would be more amenable to incorporating performance improvements that are algorithmic in nature, rather than taking advantage of implementation details.

Incidentally, the change that @karfau mentioned here actually offers some performance improvements to anyone using this xpath package with xmldom and make #108 largely irrelevant, so I'm going to add a further comment there about that.

I'll be happy to answer any other questions you might have.

[cjbarth]

• In its current state, the project uses the old xmldom package in its unit tests and in the getting started guide. While this is not ideal, and needs to be updated, the actual library has no external dependencies, so I think referring to the package of a security risk, on account of the xmldom warnings, would be unfounded, as they have no bearing on anything that imports and uses this package.

In that case, I'll see if I can move the dependency do devDependencies.

• Regarding #107 , this seems to be relying on a JS engine-specific (V8) implementation detail in order to achieve its performance gains (reversing an array and using .pop() instead of using .unshift() without reversing the array). While I'm not entirely opposed to merging this change, I don't really have the wherewithal to evaluate it.

I'm pretty sure this isn't just Node. Constantly pushing something to the front of an array requires that every element in the array be reindexed, which takes a while. If this were a linked list, I could see your argument, but in every language appending to an array is faster then pushing to an array. The reverse would then just be a one-time hit instead of a the burden of moving the array around all those times in a loop.

[JLRishe]

In that case, I'll see if I can move the dependency do devDependencies.

Not quite sure whose devDependencies you're referring to here, but xmldom is already in xpath's devDependencies. Its package.json doesn't have a dependencies list.

Constantly pushing something to the front of an array requires that every element in the array be reindexed, which takes a while.

Ok, that makes sense. I will try to get 107 incorporated along with the xmldom dependency updates within the next week.

[cjbarth]

Not quite sure whose devDependencies you're referring to here, but xmldom is already in xpath's devDependencies. Its package.json doesn't have a dependencies list.

That would be my mistake. You're already doing everything correctly regarding that. I appreciate your thoroughness here.

[cjbarth]

I note that some types in the xpath dependency are wrong, specifically xpath.selectWithResolver() isn't present in xpath.d.ts. I was going to make a PR to add it

And here the PR is: goto100/xpath#116

[karfau]

Incidentally, the change that @karfau mentioned here actually offers some performance improvements to anyone using this xpath package with xmldom and make #108 largely irrelevant, so I'm going to add a further comment there about that.

Please be aware that this change is currently, only avaklable under the next version (currently 0.9.0-beta8) and will officially be part of the latest when 0.9.0 is going to be released.
(I'm not able to provide an ETA for that, but there are not many things left to be done in the milestone.)

[JLRishe]

@karfau Thank you very much for that clarification. That's good to know.

[JLRishe]

@cjbarth Thank you for submitting the PR for the .d.ts changes. Please let me know when you're done making any changes.

A few updates/things to note:

I have merged a PR that updates the getting started guide to use @xmldom/xmldom instead of xmldom

I have added a comment to #107. It looks like he used his master branch as the source branch in that PR and has since made others to it, so either he would need to remedy that or I would need to cherry pick the changes

Incidentally, the proposed change in #107 is in the code that parses the XPath strings, not part that evaluates the expressions (parsing and evaluation are independent operations). If you are evaluating the same expression many times (and a quick glance at your project suggests that is the case), then I strongly advise parsing the XPaths just once, and reusing the parsed expressions. This would essentially make #107 a moot point for you.

There is an API for doing that, with details here. Another benefit of this API is that it allows explicitly indicating the result type (number, string, nodeset).

[cjbarth]

@JLRishe has been very responsive about this issue in general. It seems that he is responding to necessary issues. He has been active on several issues over at xpath and the most important changes have been made already. Thank you @JLRishe ! The open PRs aren't critical and have been partially worked around by the creating of the @xmldom/is-dom-node package.

Closing...