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

fix(NODE-6123): utf8 validation is insufficiently strict #676

Merged
merged 8 commits into from
Apr 30, 2024

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Apr 23, 2024

Description

Outside of web, our toUtf8 function was insufficiently strict and allowed overlong encodings.

What is changing?

Change our functionality to use jsTextDecoder to double check utf8 input when a replacement character is detected.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

Drivers wide initiative to make UTF-8 validation strict and consistent.

UTF-8 validation now throws a BSONError on overlong encodings in Node.js

Specifically, this affects deserialize when utf8 validation is enabled, which is the default.

An overlong encoding is when the number of bytes in an encoding is inflated by padding the code point with leading 0s (see here for more information).

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@billouboq
Copy link
Contributor

Does it impact the performance ?

src/validate_utf8.ts Outdated Show resolved Hide resolved
@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-6123/stricter-toUTF8-validation branch from 0f34306 to 3768409 Compare April 24, 2024 19:41
@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-6123/stricter-toUTF8-validation branch from 3768409 to 8d2ed1e Compare April 24, 2024 19:50
@aditi-khare-mongoDB aditi-khare-mongoDB added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Apr 25, 2024
@nbbeeken nbbeeken marked this pull request as ready for review April 25, 2024 15:34
src/validate_utf8.ts Outdated Show resolved Hide resolved
src/validate_utf8.ts Outdated Show resolved Hide resolved
src/validate_utf8.ts Outdated Show resolved Hide resolved
test/node/byte_utils.test.ts Outdated Show resolved Hide resolved
test/node/byte_utils.test.ts Outdated Show resolved Hide resolved
src/utils/node_byte_utils.ts Outdated Show resolved Hide resolved
etc/rollup/rollup-plugin-require-vendor/require_vendor.mjs Outdated Show resolved Hide resolved
src/parser/deserializer.ts Outdated Show resolved Hide resolved
src/utils/node_byte_utils.ts Outdated Show resolved Hide resolved
src/parse_utf8.ts Outdated Show resolved Hide resolved
test/node/byte_utils.test.ts Outdated Show resolved Hide resolved
test/node/byte_utils.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

a few more small things

@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-6123): utf8 validation is not strict enough fix(NODE-6123): utf8 validation is insufficiently enough Apr 30, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-6123): utf8 validation is insufficiently enough fix(NODE-6123): utf8 validation is insufficiently strict Apr 30, 2024
@nbbeeken nbbeeken merged commit ae8bac7 into main Apr 30, 2024
5 checks passed
@nbbeeken nbbeeken deleted the NODE-6123/stricter-toUTF8-validation branch April 30, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants