-
-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sign: throw on unsupported padding scheme #66
Conversation
Do not silently apply the wrong padding scheme.
8a55c7a
to
350ef83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thanks! i'm happy to make the changes i suggested as part of the rebase i'll do once i land another PR that's queued up, and then this can be landed too :-)
350ef83
to
b6578f2
Compare
yay, the tests are doing their job :-) there's some failures, where older node isn't throwing an exception where the tests expect it i think? |
b6578f2
to
3ef6cc0
Compare
Actually, it seems that the check of the error message was too strict; I replaced it by a regex. |
the 3 remaining failures seem legitimate |
3ef6cc0
to
fb871f0
Compare
First of all, the test messages were wrong: I had swapped 'browser' and 'node'. Old versions of Node.js do not support the padding option and therefore ignore it and never throw. Programs written against old Node versions will not include the padding option and therefore are not affected by the new throw statement in browserify. So I let the test run only for recent Node versions. |
fb871f0
to
e02aab6
Compare
OK, it turned out the Interestingly, this means the passphrase tests did run on many of the old Node versions tested, just not on 4.9 and 7.10. And they succeeded! Still, I now disabled these tests for major versions <= 10, as seemed to be intended. |
e02aab6
to
dcd81c6
Compare
I now ran the passphrase tests on 4.9 on my machine, and they run just fine, so it seems better to simply enable them always. People might be depending on them working on old node versions. |
Good catch! Let's switch those checks to use semver (v6), rather than string munging. |
dcd81c6
to
ce4e773
Compare
Weird; I'm unable to reproduce the failure on node 8.0. On my machine, the test suite runs fine on node 8.0. |
It works on node 8 latest, but fails for me on node 8.2, for example. Support for the padding option was indeed added in 8.0, though, so maybe it was broken until v8.6? i'll update the skip logic. |
ce4e773
to
8767739
Compare
Do not silently apply the wrong padding scheme. Would have saved me hours of head-scratching and screen-staring.