[CDRIVER-6017] BSON Validation Refactor #2026

vector-of-bool · 2025-06-03T04:11:14Z

This changeset is a large refactor of the bson_validate implementation.

Existing validation test cases were rewritten to use a uniform code generation mechanism that notes the purpose of each case. Many more validation scenarios were added.

The BSON_VALIDATE_UTF8 flags have been removed from the default flags used by the CRUD API. Because of a bug, UTF-8 validation wasn't being applied to the CRUD API, so removing the flag is necessary since it would otherwise be a breaking change.

The new validation is written as a straightforward recursive algorithm, and doesn't use the error-prone bson_visit API.

The following changes are of note:

The error code BSON_VALIDATE_CORRUPT is now set on corruption. Previously, this was set to BSON_VALIDATE_NONE, which had a value of zero. This was very confusing, as it was also set to zero in case of success.
Doc comments are added to better explain the behavior of the validation flags.
The "error offset" parameter is now adjusted properly in the case of document nesting.
Error message strings have been updated and made more explanatory.
BSON data that has over 1000 levels of nesting is now rejected, unconditionally.

src/libmongoc/CMakeLists.txt

src/libbson/src/bson/validate.c

src/libbson/tests/test-bson.c

src/libbson/src/bson/validate.c

src/libbson/tests/validate-tests.py

src/libbson/src/bson/validate.c

kevinAlbs

Left minor comments. I found the test generator easy to read and modify. And I like the move away from bson_iter_visit_all. LGTM

src/libbson/src/bson/bson-types.h

src/libbson/tests/validate-tests.py

src/libbson/src/bson/validate.c

kevinAlbs · 2025-06-04T19:43:27Z

src/libbson/src/bson/validate.c

+   return false;
+}
+
+// Validate an element's key string according to the valiation rules


Suggested change

// Validate an element's key string according to the valiation rules

// Validate an element's key string according to the validation rules

kevinAlbs · 2025-06-04T19:44:31Z

src/libbson/src/bson/validate.c

+   // Check for special keys
+   if (self->params->check_special_dollar_keys) {
+      // dollar-keys are checked during the startup of _validate_doc. If we get here, there's a problem.
+      require_with_error (key[0] != '$', iter->off, BSON_VALIDATE_DOLLAR_KEYS, "Disallowed element key: \"%s\"", key);


Suggested change

require_with_error (key[0] != '$', iter->off, BSON_VALIDATE_DOLLAR_KEYS, "Disallowed element key: \"%s\"", key);

require_with_error (key[0] != '$', iter->off, BSON_VALIDATE_DOLLAR_KEYS, "Disallowed '$' in element key: \"%s\"", key);

Suggest including offending character in error.

kevinAlbs · 2025-06-04T19:45:50Z

src/libbson/src/bson/validate.c

+   }
+
+   if (!self->params->allow_dot_in_keys) {
+      require_with_error (!strstr (key, "."), iter->off, BSON_VALIDATE_DOT_KEYS, "Disallowed element key: \"%s\"", key);


Suggested change

require_with_error (!strstr (key, "."), iter->off, BSON_VALIDATE_DOT_KEYS, "Disallowed element key: \"%s\"", key);

require_with_error (!strstr (key, "."), iter->off, BSON_VALIDATE_DOT_KEYS, "Disallowed '.' in element key: \"%s\"", key);

The new `bson_validate` implementation does not make use of the error-prone `bson_visit` APIs. Instead, it is written as a simple recursive validator. The new validator respects requests for UTF-8 validation properly.

The existing test cases used BSON files, and didn't have any commentary on what they were actually testing. New test cases are generated from a Python shorthand and contain the tested bytes inline, with a distinct test case for each actual validation scenario.

These include small changes in spelling, param assertions, and small cleanups Co-authored-by: Kevin Albertson <[email protected]> Co-authored-by: Ezra Chung <[email protected]>

- Add `bson/validate-private.h` to contain the declaration of the validation impl. - Add BSON_VALIDATE_MAX_NESTING_DEPTH to represent the validation limit. - Change the nesting limit to 100, to match the behavior of Server

vector-of-bool · 2025-06-05T20:46:25Z

Accidental rebase plz ignore.

I believe I've address all outstanding change requests, but may have missed some as the GitHub UI has been very uncooperative this week.

eramongodb

Minor feedback remaining; otherwise, LGTM.

eramongodb · 2025-06-05T20:50:34Z

src/libbson/src/bson/validate.c

+   BSON_ASSERT_PARAM (bson);
+
+   // The depth limit of 100 is chosen to match the limit enforced by MongoDB server.
+   // Refer: https://www.mongodb.com/docs/manual/reference/limits/#mongodb-limit-Nested-Depth-for-BSON-Documen


Suggested change

// Refer: https://www.mongodb.com/docs/manual/reference/limits/#mongodb-limit-Nested-Depth-for-BSON-Documen

// Refer: https://www.mongodb.com/docs/manual/reference/limits/#mongodb-limit-Nested-Depth-for-BSON-Documents

End of URL is clipped.

eramongodb · 2025-06-05T20:52:25Z

src/libbson/src/bson/validate-private.h

+ * @return true If the given document has no validation errors
+ * @return false Otherwise
+ */
+extern bool


Suggested change

extern bool

bool

extern should (now) be unnecessary.

eramongodb · 2025-06-05T20:53:54Z

src/libbson/src/bson/bson-types.h

+    * The depth limit of 100 is chosen to match the limit enforced by MongoDB server.
+    * Refer: https://www.mongodb.com/docs/manual/reference/limits/#mongodb-limit-Nested-Depth-for-BSON-Document
+    */
+   BSON_VALIDATION_MAX_NESTING_DEPTH = 100,


Suggested change

BSON_VALIDATION_MAX_NESTING_DEPTH = 100,

BSON_VALIDATE_MAX_NESTING_DEPTH = 100,

All other validation-related constants use a prefix of BSON_VALIDATE_*.

Do we really need to expose this in the public API? Can it be moved to <bson/validate-private.h> instead? Test code is able to include private headers.

I put it in the public header because I think it might be useful to users to know the limit. Because it's public, I intentionally chose the differing name to prevent a user from thinking that this is meaningfull:

bson_validate(d, BSON_VALIDATE_UTF8 | BSON_VALIDATE_MAX_NESTING_DEPTH, NULL);

It may actually be better to have a function bson_get_max_nesting_depth(), in case it is changed in the future and may vary depending on what library they load at runtime?

Thank you for the explanation.

It might be useful to users to know the limit.

I'm not sure about that, but in that case, perhaps BSON_MAX_NESTING_DEPTH via bson/bson.h (alongside BSON_MAX_SIZE) might be better, since it is not a limit that is restricted to validation only? (Apparently we have #define BCON_STACK_MAX 100 in bson/bcon.h, but that doesn't seem to be meant to be used outside of the component.)

It may actually be better to have a function bson_get_max_nesting_depth()

I do not expect the limit to change anytime soon, so even if we expose it in public API, I do not mind leaving it as a compile-time constant.

vector-of-bool · 2025-06-06T18:09:45Z

Note for future readers:

The validation limit was set to 1000, as it is possible that users want to use documents deeper than are accepted by the server, and it is also possible that the server can accept documents with deeper nesting levels. The purpose of the limit is mostly to prevent deep recursion, not to prevent invalid data.

* New BSON validation routine rewrite The new `bson_validate` implementation does not make use of the error-prone `bson_visit` APIs. Instead, it is written as a simple recursive validator. The new validator respects requests for UTF-8 validation properly. * Stop validating at 1000 depth, preventing stack overflow * Replace most BSON validation tests with generated ones The existing test cases used BSON files, and didn't have any commentary on what they were actually testing. New test cases are generated from a Python shorthand and contain the tested bytes inline, with a distinct test case for each actual validation scenario. * Disable UTF-8 validation by default on CRUD APIs * Document and tweak the value of BSON_VALIDATE_CORRUPT * Add test cases related to the overlong null encoding * Tweak JS scope validation to permit more obj keys * Add a NEWS entry for validation changes. * Allow `-private.h` headers to not include the prelude header --------- Co-authored-by: Kevin Albertson <[email protected]> Co-authored-by: Ezra Chung <[email protected]>

vector-of-bool requested a review from a team as a code owner June 3, 2025 04:11

vector-of-bool requested a review from kevinAlbs June 3, 2025 04:11

kevinAlbs requested a review from eramongodb June 3, 2025 12:31

kevinAlbs mentioned this pull request Jun 3, 2025

[CDRIVER-4448] Fix interaction of bson_visit with bson_validate #1090

Closed

eramongodb requested changes Jun 4, 2025

View reviewed changes

kevinAlbs approved these changes Jun 4, 2025

View reviewed changes

vector-of-bool and others added 24 commits June 4, 2025 18:09

New BSON validation routine rewrite

29f7182

The new `bson_validate` implementation does not make use of the error-prone `bson_visit` APIs. Instead, it is written as a simple recursive validator. The new validator respects requests for UTF-8 validation properly.

Tweak validation to differentiate between invalid utf-8 and null chars

5f7930d

Stop validating at 1000 depth, preventing stack overflow

e564c0d

Various validation commentary and cleanup

a190ed8

Disable UTF-8 validation by default on CRUD APIs

0ff823f

Update test cases that check error message strings

9d6a13d

Minor fixes and tweaks from PR comments

a8cf5bd

These include small changes in spelling, param assertions, and small cleanups Co-authored-by: Kevin Albertson <[email protected]> Co-authored-by: Ezra Chung <[email protected]>

Regen validation tests

1ec7968

Document and tweak the value of BSON_VALIDATE_CORRUPT

45d6140

#undef some macros that we define privately

2c0778f

Mark some parameters as required via assertions

7f1e2cf

Remove dup strlen()

98e6a2d

Note the unreachability of some tags in element validation

ce5053a

Tweak limits, and a private header for validation

cb3b51c

- Add `bson/validate-private.h` to contain the declaration of the validation impl. - Add BSON_VALIDATE_MAX_NESTING_DEPTH to represent the validation limit. - Change the nesting limit to 100, to match the behavior of Server

Add inline metadata for the validation test generator

90e27a9

Note the validation of $id as an arbitrary value

07da508

Include offending char in bad key error messages

0f0f117

Tweak message when doc iteration fails to start

62df9b1

Add test cases related to the overlong null encoding

f654398

Tweak depth validation by 1

0e0eaf8

Cleanup on test generator

bfd38da

Another test case for Binary type 2

c9cfdc2

Tweak JS scope validation to permit more obj keys

99e014f

Add a NEWS entry for validation changes.

f0f60b5

vector-of-bool force-pushed the CDRIVER-6017-validate-fixes branch from 7a48211 to f0f60b5 Compare June 5, 2025 20:45

vector-of-bool requested a review from eramongodb June 5, 2025 20:46

Allow -private.h headers to not include the prelude header

4ed67e8

eramongodb approved these changes Jun 5, 2025

View reviewed changes

vector-of-bool added 3 commits June 6, 2025 10:11

More minnor tweaks

6a59899

Make the validation depth private and much higher

f74f69f

Outdated comment

b25d6fc

vector-of-bool merged commit d184525 into mongodb:master Jun 6, 2025
37 of 39 checks passed

vector-of-bool mentioned this pull request Jun 6, 2025

[CDRIVER-6017] BSON Validation Refactor (#2026) (Cherry-pick for 1.30.x) #2031

Open

vector-of-bool mentioned this pull request Jun 6, 2025

[CDRIVER-6017] BSON Validation Refactor (#2026) (Cherry-pick for 2.0.x) #2032

Open

	// Validate an element's key string according to the valiation rules
	// Validate an element's key string according to the validation rules

	require_with_error (key[0] != '$', iter->off, BSON_VALIDATE_DOLLAR_KEYS, "Disallowed element key: \"%s\"", key);
	require_with_error (key[0] != '$', iter->off, BSON_VALIDATE_DOLLAR_KEYS, "Disallowed '$' in element key: \"%s\"", key);

	require_with_error (!strstr (key, "."), iter->off, BSON_VALIDATE_DOT_KEYS, "Disallowed element key: \"%s\"", key);
	require_with_error (!strstr (key, "."), iter->off, BSON_VALIDATE_DOT_KEYS, "Disallowed '.' in element key: \"%s\"", key);

	// Refer: https://www.mongodb.com/docs/manual/reference/limits/#mongodb-limit-Nested-Depth-for-BSON-Documen
	// Refer: https://www.mongodb.com/docs/manual/reference/limits/#mongodb-limit-Nested-Depth-for-BSON-Documents

	BSON_VALIDATION_MAX_NESTING_DEPTH = 100,
	BSON_VALIDATE_MAX_NESTING_DEPTH = 100,

[CDRIVER-6017] BSON Validation Refactor #2026

[CDRIVER-6017] BSON Validation Refactor #2026

Uh oh!

Conversation

vector-of-bool commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinAlbs 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!

kevinAlbs Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

kevinAlbs Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

vector-of-bool commented Jun 5, 2025

Uh oh!

eramongodb left a comment

Choose a reason for hiding this comment

Uh oh!

eramongodb Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

eramongodb Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

eramongodb Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

vector-of-bool Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

eramongodb Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vector-of-bool commented Jun 6, 2025

Uh oh!

Uh oh!

Uh oh!

eramongodb Jun 6, 2025 •

edited

Loading