Skip to content
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

Merged
merged 1 commit into from
Mar 31, 2017
Merged

src: fix base64 decoding #11995

merged 1 commit into from
Mar 31, 2017

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Mar 22, 2017

Make sure trailing garbage is not treated as a valid Base64 character.

Fixes: #11987

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

src

cc @bnoordhuis @jorangreef

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Mar 22, 2017
@seishun
Copy link
Contributor Author

seishun commented Mar 22, 2017

Do let me know if someone has a better idea for the test.

@@ -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);
Copy link
Member

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @seishun and @addaleax.

The test is good. Agree on checking the actual bytes:

assert.deepStrictEqual(Buffer.from('w0  ', 'base64'), Buffer.from([195]));

Copy link
Member

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]));

Copy link
Member

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='
);

@jasnell jasnell added this to the 8.0.0 milestone Mar 27, 2017
@addaleax
Copy link
Member

@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. ;)

Buffer.from('3v6YpUFyK0/hitA2tCDIfdYKw0 g ', 'base64').toString('base64'),
'3v6YpUFyK0/hitA2tCDIfdYKw0g='
);

Copy link
Contributor Author

@seishun seishun Mar 31, 2017

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.

Copy link
Member

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…

@addaleax
Copy link
Member

@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.

@aqrln
Copy link
Contributor

aqrln commented Mar 31, 2017

@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 :)

Make sure trailing garbage is not treated as a valid base64 character.

Fixes: #11987
PR-URL: #11995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@seishun
Copy link
Contributor Author

seishun commented Mar 31, 2017

Landed in 7e0c3ab.

@seishun seishun closed this Mar 31, 2017
@seishun seishun merged commit 7e0c3ab into nodejs:master Mar 31, 2017
@seishun seishun deleted the base64-fix branch March 31, 2017 20:02
@jasnell jasnell mentioned this pull request Apr 4, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
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>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: #11987
PR-URL: #11995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 18, 2017

Cherry-picked to v6. Needed manual backport for v4: 83a856af6d

please lmk if you see any issues

MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: #11987
PR-URL: #11995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Make sure trailing garbage is not treated as a valid base64 character.

Fixes: #11987
PR-URL: #11995
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced Apr 19, 2017
MylesBorins added a commit that referenced this pull request May 2, 2017
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
MylesBorins added a commit that referenced this pull request May 2, 2017
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
MylesBorins added a commit that referenced this pull request May 2, 2017
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
MylesBorins added a commit that referenced this pull request May 2, 2017
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
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    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>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    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>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    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>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    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>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
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
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
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
@seishun seishun mentioned this pull request Jun 14, 2017
2 tasks
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buffer.from('base64') decodes some white space as data
8 participants