-
- Notifications
You must be signed in to change notification settings - Fork 61
sign: throw on unsupported padding scheme #66
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
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.