Use the tsscmp module for older Node.js compatibility

[dougwilson]

This is a followup on my comment #24 (comment) such that Express and related modules can continue to use this module instead of needing to switch to something else for Node.js compatibility.

Please feel free to reject it if you rather not add a dependency, though. Express can fork this module and depend on a fork if needed, so no big deal, but just wanted to offer this up in case you'd like to incorporate it into the module 👍

The tsscmp module was directed by a Node.js core maintainer and is a user-land port of the algo in Node.js

[dougwilson]

/cc @natevw

[natevw]

First, my apologies for not coordinating this better in the first place! I had somehow gotten the impression that Express was no longer using this by default, and so didn't check whether updating the engine dependency here would cause a mismatch. I see now that it's a direct dependency of the main package! 🙁

I would like to avoid any dependencies, especially since node now has this built-in. As detailed in my #24 (comment) response, there is no particularly urgent need to migrate from >=1.0.4 to 1.1.0.

So in general I would simply tell users to require 1.0.x if they need to support "node": "<6.6.0". It looks like express already pins the dependency to 1.0.6 exactly, so perhaps leaving that is the simplest solution here too?

[natevw]

I would like to avoid any dependencies

That said, this patch looks good to me and the library has the benefit not only of working on all engines but also getting rid of the somewhat precarious way we're handling buffer lengths atm.

So I'm not entirely opposed to this as a solution if it's clearly the best path forward for express, but just want to make sure it's needed.

[natevw]

Could this be "~1.0.5" in case tsscmp gets updated?

[dougwilson]

Hi @natevw I'm sorry for the confusion. There certainly isn't a need to coordinate a non-security fix, and I wouldn't expect the packages I depend on to feed obligated to coordinate every release :) That is just what comes with depending on packages (and why I just depend on a single, specific version, as you ended up noticing -- so I can see what the changes were and update it without a flood of issues potentially, which I'm sure would have happened here if suddenly existing Express servers stopped working below Node.js 6.6.0 👍 ).

I was working off of a security vulnerability report disclosed to the Express project in good faith. The user agrees now that it is not a security issue. He also told me how he came to the conclusion that 1.1.0 was a security release and it came down to the fact the commit 5fb33f0 visible in the commit history is titled "Use timingSafeEqual instead of hashing to prevent time attack" which made it sound like the commit is necessary to prevent a timing attack.

I agree that 1.1.0 does not contain a security fix, though hopefully the explanation I got from that user may shed some light on why it was thought to have one. Express is happy to continue to use the 1.0.x series for the time being, so don't feel like you need to accept this pull request. I made the change to the dependency version as requested just in case you wanted it, but please, feel free to "no thanks" my pull request, no feelings either way.

[jodevsa]

@dougwilson @natevw I am so sorry for naming the PR as if it were a security fix!, this may have been due to my bad language and bad understanding of timing attacks. TBH, i thought the function introduced in >V6.0.0; handled timing attacks in a better way than the existing code @natevw implemented, but looks like it holds the same logic.

So sorry for the mess i created :(

[natevw]

@jodevsa No worries! The irony is that I though the switch would finally put to rest all the confusion and surprise re. the original hashing, but… :sadtrombone: Oh well/LOL, and it's encouraging that there's lots of watchful eyes on this library.

@dougwilson Sounds like Express is content to leave things sit for now, so let's do that unless something comes up with 1.0.6. Closing this PR "without prejudice", i.e. would be willing to re-open it if there's a compelling need from a major consumer of this library.