-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
src: fix base64 decoding #11995
src: fix base64 decoding #11995
Conversation
Do let me know if someone has a better idea for the test. |
test/parallel/test-buffer-alloc.js
Outdated
@@ -464,6 +464,9 @@ assert.strictEqual( | |||
// Regression test for https://github.com/nodejs/node/issues/3496. | |||
assert.strictEqual(Buffer.from('=bad'.repeat(1e4), 'base64').length, 0); | |||
|
|||
// Regression test for https://github.com/nodejs/node/issues/11987. | |||
assert.strictEqual(Buffer.from("w0 ", 'base64').length, 1); |
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.
I think a good test would be
assert.deepStrictEqual(Buffer.from('w0 ', 'base64'), Buffer.from('w0', 'base64'));
and/or checking the actual bytes?
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.
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.
Both of the suggested tests are worthwhile...
assert.deepStrictEqual(Buffer.from('w0 ', 'base64'), Buffer.from('w0', 'base64'));
assert.deepStrictEqual(Buffer.from('w0 ', 'base64'), Buffer.from([195]));
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.
And also from the issue:
assert.strictEqual(
Buffer.from('3v6YpUFyK0/hitA2tCDIfdYKw0 g ', 'base64').toString('base64'),
'3v6YpUFyK0/hitA2tCDIfdYKw0g='
);
@seishun Took the liberty of pushing to your branch, hope that’s okay. :) Rebased & tests added from comments. CI: https://ci.nodejs.org/job/node-test-commit/8812/ I’m going to land this if the CI comes back green. @aqrln Heads up, this is going to conflict with your PRs. ;) |
test/parallel/test-buffer-alloc.js
Outdated
Buffer.from('3v6YpUFyK0/hitA2tCDIfdYKw0 g ', 'base64').toString('base64'), | ||
'3v6YpUFyK0/hitA2tCDIfdYKw0g=' | ||
); | ||
|
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.
@addaleax I think adding all these tests is redundant. This one conveys intent the best:
assert.deepStrictEqual(Buffer.from('w0 ', 'base64'),
Buffer.from('w0', 'base64'));
If this test passes but one of the other proposed tests fails, that means base64 encoding is fundamentally broken, which would cause one of the many existing tests to fail.
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.
@seishun It’s your PR and I’m not going to tell you what to do. 😄 But really, having slightly more tests than necessary won’t hurt anyone…
@seishun CI is green … I’d say you can merge this with any non-empty subset of the tests here added. I’d prefer all but every assertion here should be sufficient as a regression test for the particular issue this fixes. |
@addaleax thanks for the heads up! It'll conflict with only one of the PRs, and the conflict will be easy to fix, so I'll rebase it as soon as this one lands :) |
Landed in 7e0c3ab. |
Make sure trailing garbage is not treated as a valid base64 character. Fixes: nodejs#11987 PR-URL: nodejs#11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>
Cherry-picked to v6. Needed manual backport for v4: 83a856af6d please lmk if you see any issues |
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12499
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12497
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12499
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12497
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12499 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12497 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12499 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12497 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs#11947 * (Ben Noordhuis) nodejs#11898 * (jBarz) nodejs#11776 PR-URL: nodejs#12499
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs#11947 * (Ben Noordhuis) nodejs#11898 * (jBarz) nodejs#11776 PR-URL: nodejs#12497
Make sure trailing garbage is not treated as a valid base64 character. Fixes: nodejs/node#11987 PR-URL: nodejs/node#11995 Reviewed-By: Anna Henningsen <anna@addaleax.net>
Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12497
Make sure trailing garbage is not treated as a valid Base64 character.
Fixes: #11987
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
src
cc @bnoordhuis @jorangreef