buffer: atob throw error when the input value is invalid #42662

austinkelleher · 2022-04-08T21:13:43Z

Description

The specification of atob has various different conditions that we need
to abide by.

See: https://infra.spec.whatwg.org/#forgiving-base64-decode

Considerations

Each of the new cases that are handled added could be considered breaking changes. Consider the following tests run directly from the Chrome console (Version 100.0.4896.75 (Official Build) (arm64)):

atob(undefined)
VM85:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.
    at <anonymous>:1:1

atob(false)
VM123:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.
    at <anonymous>:1:1

atob(1)
VM152:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.

atob(false)
VM187:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.

atob(0)
VM217:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.
    at <anonymous>:1:1

atob('a')
VM234:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.
    at <anonymous>:1:1

atob('a ')
VM243:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.
    at <anonymous>:1:1

atob(' a')
VM257:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.
    at <anonymous>:1:1
    
atob('aaaaa')
VM283:1 Uncaught DOMException: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded.
    at <anonymous>:1:1

lib/buffer.js

austinkelleher · 2022-04-08T21:21:06Z

Additional note - I'm happy to split this change into multiple PRs if it's helpful. 😄

zloirock · 2022-04-08T21:46:32Z

atob now immediately throws when undefined, false, or a number is supplied

It should be converted to a string, so

atob(1234) === '×mø'

is correct

austinkelleher · 2022-04-08T21:59:58Z

@zloirock Thank you for catching...I'll update the PR.

test/parallel/test-btoa-atob.js

The specification of `atob` has various different conditions that we need to abide by. The specific changes that were made: * `atob` now immediately throws when `undefined`, `false`, or a `number` is supplied * `atob` now strips ASCII whitespace before attempting to decode * `atob` now validates that the code point's length divided by 4 leaves a remainder that is not 1 See: https://infra.spec.whatwg.org/#forgiving-base64-decode Fixes: nodejs#42646

* Use approach of counting non-ASCII whitespace characters instead of removing the characters via string replacement * Additional atob validation tests have been added

lib/buffer.js

test/parallel/test-btoa-atob.js

lib/buffer.js

test/parallel/test-btoa-atob.js

* Chrome and Firefox both use `InvalidCharacterError` when the string to be decoded is not correctly encoded * Remove unnecessary atob test

aduh95

Suggestion: we could keep one array and save a lookup per char.Given the fact that this is a legacy API for which performance are not really a concern, you can ignore this suggestion if you think it makes the code harder to read.

lib/buffer.js

test/parallel/test-btoa-atob.js

* Small optimization in atob validation by assuming positions of whitespace characters in allowed chars array * Improved tests for ASCII whitespace characters * Explicitly validate properties of `DOMException` in tests

lib/buffer.js

nodejs-github-bot · 2022-04-09T15:09:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43410/

austinkelleher · 2022-04-11T12:03:33Z

It seems that some of the tests may have failed intermittently unless I'm missing something. Anything that I'm missing? If not, can someone please re-trigger? Thanks!

nodejs-github-bot · 2022-04-11T12:33:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43433/

austinkelleher · 2022-04-11T19:12:36Z

It looks like all checks have passed except for the pending node-test-linux-linked-debug. When I review the Jenkins job output, it looks like that job has actually succeeded?

aduh95 · 2022-04-12T14:35:19Z

Landed in 7533d08, thanks for the contribution 🎉

danielleadams · 2022-06-27T01:06:04Z

@austinkelleher should this land in v16.x? If so, do you mind creating a backport PR for this to v16.x-staging? (docs) Thanks!

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Apr 8, 2022

austinkelleher commented Apr 8, 2022

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

austinkelleher commented Apr 8, 2022

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

austinkelleher force-pushed the 42646-buffer-validation branch from a924347 to 473c352 Compare April 8, 2022 21:19

austinkelleher force-pushed the 42646-buffer-validation branch from 473c352 to 2ad1a00 Compare April 8, 2022 21:23

aduh95 reviewed Apr 8, 2022

View reviewed changes

test/parallel/test-btoa-atob.js Outdated Show resolved Hide resolved

austinkelleher added 2 commits April 9, 2022 08:53

buffer: simplify atob approach of disregarding whitespace

08e4c75

* Use approach of counting non-ASCII whitespace characters instead of removing the characters via string replacement * Additional atob validation tests have been added

austinkelleher force-pushed the 42646-buffer-validation branch from 1c109ce to 08e4c75 Compare April 9, 2022 12:54

austinkelleher changed the title ~~buffer: atob throw error when the input string is invalid~~ buffer: atob throw error when the input value is invalid Apr 9, 2022

austinkelleher commented Apr 9, 2022

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

aduh95 reviewed Apr 9, 2022

View reviewed changes

test/parallel/test-btoa-atob.js Outdated Show resolved Hide resolved

austinkelleher commented Apr 9, 2022

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

austinkelleher commented Apr 9, 2022

View reviewed changes

test/parallel/test-btoa-atob.js Outdated Show resolved Hide resolved

buffer: switch to using InvalidCharacterError on encoding error

a85b856

* Chrome and Firefox both use `InvalidCharacterError` when the string to be decoded is not correctly encoded * Remove unnecessary atob test

aduh95 reviewed Apr 9, 2022

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

lib/buffer.js Outdated Show resolved Hide resolved

lib/buffer.js Outdated Show resolved Hide resolved

aduh95 reviewed Apr 9, 2022

View reviewed changes

test/parallel/test-btoa-atob.js Outdated Show resolved Hide resolved

aduh95 reviewed Apr 9, 2022

View reviewed changes

test/parallel/test-btoa-atob.js Outdated Show resolved Hide resolved

buffer: update character validation approach in atob

a5828d8

* Small optimization in atob validation by assuming positions of whitespace characters in allowed chars array * Improved tests for ASCII whitespace characters * Explicitly validate properties of `DOMException` in tests

austinkelleher force-pushed the 42646-buffer-validation branch from 689d51f to a5828d8 Compare April 9, 2022 14:26

aduh95 reviewed Apr 9, 2022

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

aduh95 reviewed Apr 9, 2022

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

buffer: improve comments in atob function

48d6cee

aduh95 approved these changes Apr 9, 2022

View reviewed changes

lib/buffer.js Show resolved Hide resolved

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2022

buffer: revive comment about code point length validation in atob

5cee1b4

This was referenced Apr 10, 2022

CI Reliability 2022-04-10 nodejs/reliability#247

Open

CI Reliability 2022-04-11 nodejs/reliability#248

Open

marsonya approved these changes Apr 11, 2022

View reviewed changes

github-actions bot mentioned this pull request Apr 12, 2022

CI Reliability 2022-04-12 nodejs/reliability#249

Open

29 tasks

aduh95 merged commit 7533d08 into nodejs:master Apr 12, 2022

This was referenced Apr 13, 2022

CI Reliability 2022-04-13 nodejs/reliability#250

Open

CI Reliability 2022-04-14 nodejs/reliability#251

Open

BethGriggs mentioned this pull request Apr 14, 2022

2022-04-19, Version 18.0.0 (Current) #42262

Merged

github-actions bot mentioned this pull request Apr 15, 2022

CI Reliability 2022-04-15 nodejs/reliability#252

Open

16 tasks

juanarbol mentioned this pull request May 31, 2022

2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268

Closed

danielleadams added the backport-requested-v16.x label Jun 27, 2022

codebytere mentioned this pull request Aug 23, 2022

fix: Node.js atob input validation electron/electron#35415

Merged

3 tasks

targos removed the backport-requested-v16.x label Sep 16, 2022

juanarbol mentioned this pull request Oct 11, 2022

V16.18.0 proposal #44886

Merged

chee mentioned this pull request Feb 20, 2025

atob and btoa should not be marked as legacy #57145

Closed

Uh oh!

buffer: atob throw error when the input value is invalid #42662

buffer: atob throw error when the input value is invalid #42662

Uh oh!

Conversation

austinkelleher commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Considerations

Uh oh!

Uh oh!

Uh oh!

austinkelleher commented Apr 8, 2022

Uh oh!

zloirock commented Apr 8, 2022

Uh oh!

austinkelleher commented Apr 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 9, 2022

Uh oh!

austinkelleher commented Apr 11, 2022

Uh oh!

nodejs-github-bot commented Apr 11, 2022

Uh oh!

austinkelleher commented Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Apr 12, 2022

Uh oh!

danielleadams commented Jun 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

austinkelleher commented Apr 8, 2022 •

edited

Loading

austinkelleher commented Apr 11, 2022 •

edited

Loading