Skip to content

CDRIVER-3895: Remove dots/dollars validation for insert/replace docs #801

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.

Sign up for GitHub

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

Merged
merged 22 commits into from
Jun 16, 2021
Merged

CDRIVER-3895: Remove dots/dollars validation for insert/replace docs #801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
95bcb21
CDRIVER-3895: Update CRUD, transactions, and unified spec tests
jmikola Jun 7, 2021
fc62d89
Skip unified CRUD tests related to CDRIVER-3630
jmikola Jun 7, 2021
2dddab2
Apply clang-format
jmikola Jun 7, 2021
f9f5dde
Ensure upsertedIds is always present in BulkWriteResult
jmikola Jun 8, 2021
b30fa08
clang-format
jmikola Jun 8, 2021
63e5118
Allow array or document for updateOne/Many update arg
jmikola Jun 8, 2021
c48e538
Remove dot/dollar restrictions for insert/replace validation
jmikola Jun 8, 2021
e4324be
Remove redundant replace doc validation for legacy updates
jmikola Jun 8, 2021
29c50fb
Test success and failure for legacy update code path
jmikola Jun 8, 2021
e86d47c
Revert "Skip tests with dots and dollars in field names on 5.0+ (#795)"
jmikola Jun 11, 2021
3891867
Revise bulk and collection tests for allowing dots/dollars
jmikola Jun 11, 2021
52c2954
clang-format
jmikola Jun 11, 2021
6441b0e
Allow methods in bulk_op_append to fail so expectError can be evaluated
jmikola Jun 11, 2021
16488d6
Skip unified tests due to schema version and outstanding issues
jmikola Jun 11, 2021
e025cb0
Fix typo
jmikola Jun 12, 2021
798a71a
Remove superfluous bson_reader_destroy
jmikola Jun 14, 2021
ad17e84
Ensure WC is set on database and collection entities
jmikola Jun 15, 2021
253045f
Update src/libmongoc/src/mongoc/mongoc-write-command-legacy.c
jmikola Jun 16, 2021
e870d34
Update src/libmongoc/tests/test-mongoc-collection.c
jmikola Jun 16, 2021
985f830
Update src/libmongoc/tests/test-mongoc-write-commands.c
jmikola Jun 16, 2021
fb67cdf
Update src/libmongoc/tests/test-mongoc-write-commands.c
jmikola Jun 16, 2021
931a8f7
Use mock_server_with_auto_hello
jmikola Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/libmongoc/src/mongoc/mongoc-util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,11 @@

const bson_validate_flags_t _mongoc_default_insert_vflags =
BSON_VALIDATE_UTF8 | BSON_VALIDATE_UTF8_ALLOW_NULL |
BSON_VALIDATE_EMPTY_KEYS | BSON_VALIDATE_DOT_KEYS |
BSON_VALIDATE_DOLLAR_KEYS;
BSON_VALIDATE_EMPTY_KEYS;

const bson_validate_flags_t _mongoc_default_replace_vflags =
BSON_VALIDATE_UTF8 | BSON_VALIDATE_UTF8_ALLOW_NULL |
BSON_VALIDATE_EMPTY_KEYS | BSON_VALIDATE_DOT_KEYS |
BSON_VALIDATE_DOLLAR_KEYS;
BSON_VALIDATE_EMPTY_KEYS;
Copy link
Member Author

Choose a reason for hiding this comment

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

With this change all three vflags are identical. The vflags aren't part of the public API, so we could have consolidated them into one value; however, I think leaving them as-is is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, leaving them as-is seems OK.


const bson_validate_flags_t _mongoc_default_update_vflags =
BSON_VALIDATE_UTF8 | BSON_VALIDATE_UTF8_ALLOW_NULL |
Expand Down Expand Up @@ -324,6 +322,8 @@ _mongoc_validate_replace (const bson_t *doc,
bson_error_t *error)
{
bson_error_t validate_err;
bson_iter_t iter;
const char *key;

if (vflags == BSON_VALIDATE_NONE) {
return true;
Expand All @@ -338,6 +338,27 @@ _mongoc_validate_replace (const bson_t *doc,
return false;
}

if (!bson_iter_init (&iter, doc)) {
bson_set_error (error,
MONGOC_ERROR_BSON,
MONGOC_ERROR_BSON_INVALID,
"replace document is corrupt");
return false;
}

while (bson_iter_next (&iter)) {
key = bson_iter_key (&iter);
if (key[0] == '$') {
bson_set_error (error,
MONGOC_ERROR_COMMAND,
MONGOC_ERROR_COMMAND_INVALID_ARG,
"Invalid key '%s': replace prohibits $ operators",
key);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is basically the inverse of the existing logic in _mongoc_validate_update. We technically only need to check the first key since the server would raise an error (as discussed in the CRUD spec), but I opted to keep this the same as update validation. If anyone feels strongly about this, feel free to open a new CDRIVER ticket to propose a future change to both validation methods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only reason I can see not to validate all fields is performance. But the previous behavior was checking for all fields (including nested fields) with bson_validate_with_error, and performance was not a known issue. So I am OK with checking all top-level fields.


return false;
}
}

return true;
}

Expand Down
57 changes: 18 additions & 39 deletions src/libmongoc/src/mongoc/mongoc-write-command-legacy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,16 +380,13 @@ _mongoc_write_command_update_legacy (mongoc_write_command_t *command,
int32_t max_bson_obj_size;
mongoc_rpc_t rpc;
uint32_t request_id = 0;
bson_iter_t subiter, subsubiter;
bson_t doc;
bson_iter_t subiter;
bson_t update, selector;
const uint8_t *data = NULL;
uint32_t len = 0;
size_t err_offset;
bool val = false;
char *ns;
int vflags = (BSON_VALIDATE_UTF8 | BSON_VALIDATE_UTF8_ALLOW_NULL |
BSON_VALIDATE_DOLLAR_KEYS | BSON_VALIDATE_DOT_KEYS);
bool r;
bson_reader_t *reader;
const bson_t *bson;
bool eof;
Expand All @@ -406,45 +403,19 @@ _mongoc_write_command_update_legacy (mongoc_write_command_t *command,

max_bson_obj_size = mongoc_server_stream_max_bson_obj_size (server_stream);

reader =
bson_reader_new_from_data (command->payload.data, command->payload.len);
while ((bson = bson_reader_read (reader, &eof))) {
if (bson_iter_init (&subiter, bson) && bson_iter_find (&subiter, "u") &&
BSON_ITER_HOLDS_DOCUMENT (&subiter)) {
bson_iter_document (&subiter, &len, &data);
BSON_ASSERT (bson_init_static (&doc, data, len));

if (bson_iter_init (&subsubiter, &doc) &&
bson_iter_next (&subsubiter) &&
(bson_iter_key (&subsubiter)[0] != '$') &&
!bson_validate (
&doc, (bson_validate_flags_t) vflags, &err_offset)) {
result->failed = true;
bson_set_error (error,
MONGOC_ERROR_BSON,
MONGOC_ERROR_BSON_INVALID,
"update document is corrupt or contains "
"invalid keys including $ or .");
bson_reader_destroy (reader);
EXIT;
}
} else {
result->failed = true;
bson_set_error (error,
MONGOC_ERROR_BSON,
MONGOC_ERROR_BSON_INVALID,
"updates is malformed.");
bson_reader_destroy (reader);
EXIT;
}
}

ns = bson_strdup_printf ("%s.%s", database, collection);

bson_reader_destroy (reader);
reader =
bson_reader_new_from_data (command->payload.data, command->payload.len);
while ((bson = bson_reader_read (reader, &eof))) {
/* ensure the document has "q" and "u" document fields in that order */
r = (bson_iter_init (&subiter, bson) && bson_iter_find (&subiter, "q") &&
BSON_ITER_HOLDS_DOCUMENT (&subiter) &&
bson_iter_find (&subiter, "u") &&
BSON_ITER_HOLDS_DOCUMENT (&subiter));

BSON_ASSERT (r);
Copy link
Member Author

Choose a reason for hiding this comment

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

Checking that both fields exist is necessary since we don't explicitly fetch the fields later. For whatever reason, _mongoc_write_command_update_legacy iterates through all fields and could fail to fully construct the command if one was absent. In practice, I doubt that would ever happen since the BSON for this is constructed elsewhere in libmongoc and doesn't originate from the user.

Note: I opted not to test for this assert failure, since it really isn't possible to trigger it from the public API.


request_id = ++client->cluster.request_id;

rpc.header.msg_len = 0;
Expand All @@ -459,6 +430,10 @@ _mongoc_write_command_update_legacy (mongoc_write_command_t *command,
while (bson_iter_next (&subiter)) {
if (strcmp (bson_iter_key (&subiter), "u") == 0) {
bson_iter_document (&subiter, &len, &data);

BSON_ASSERT (data);
BSON_ASSERT (len >= 5);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice. Asserting the document is at least as large as an empty document is a good safety check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shamelessly copied from _mongoc_write_command_delete_legacy.


if (len > max_bson_obj_size) {
_mongoc_write_command_too_large_error (
error, 0, len, max_bson_obj_size);
Expand All @@ -472,6 +447,10 @@ _mongoc_write_command_update_legacy (mongoc_write_command_t *command,
BSON_ASSERT (bson_init_static (&update, data, len));
} else if (strcmp (bson_iter_key (&subiter), "q") == 0) {
bson_iter_document (&subiter, &len, &data);

BSON_ASSERT (data);
BSON_ASSERT (len >= 5);

if (len > max_bson_obj_size) {
_mongoc_write_command_too_large_error (
error, 0, len, max_bson_obj_size);
Expand Down
36 changes: 35 additions & 1 deletion src/libmongoc/tests/bsonutil/bson-parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ typedef enum {
BSON_PARSER_BOOL,
BSON_PARSER_DOC,
BSON_PARSER_ARRAY,
BSON_PARSER_ARRAY_OR_DOC,
Copy link
Member Author

Choose a reason for hiding this comment

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

This was necessary for accepting documents or pipeline arrays for update operations.

BSON_PARSER_ANY,
BSON_PARSER_WRITE_CONCERN,
BSON_PARSER_READ_CONCERN,
Expand Down Expand Up @@ -62,6 +63,8 @@ parser_type_to_string (bson_parser_type_t ptype)
return "DOC";
case BSON_PARSER_ARRAY:
return "ARRAY";
case BSON_PARSER_ARRAY_OR_DOC:
return "ARRAY or DOC";
case BSON_PARSER_ANY:
return "ANY";
case BSON_PARSER_WRITE_CONCERN:
Expand Down Expand Up @@ -240,7 +243,8 @@ bson_parser_entry_destroy (bson_parser_entry_t *entry, bool with_parsed_fields)
{
if (with_parsed_fields) {
if (entry->ptype == BSON_PARSER_DOC ||
entry->ptype == BSON_PARSER_ARRAY) {
entry->ptype == BSON_PARSER_ARRAY ||
entry->ptype == BSON_PARSER_ARRAY_OR_DOC) {
bson_t **out;

out = (bson_t **) entry->out;
Expand Down Expand Up @@ -399,6 +403,23 @@ bson_parser_array_optional (bson_parser_t *parser,
bson_parser_add_entry (parser, key, (void *) out, BSON_PARSER_ARRAY, true);
}

void
bson_parser_array_or_doc (bson_parser_t *parser, const char *key, bson_t **out)
{
*out = NULL;
bson_parser_add_entry (
parser, key, (void *) out, BSON_PARSER_ARRAY_OR_DOC, false);
}
void
bson_parser_array_or_doc_optional (bson_parser_t *parser,
const char *key,
bson_t **out)
{
*out = NULL;
bson_parser_add_entry (
parser, key, (void *) out, BSON_PARSER_ARRAY_OR_DOC, true);
}

void
bson_parser_bool (bson_parser_t *parser, const char *key, bool **out)
{
Expand Down Expand Up @@ -579,6 +600,19 @@ entry_marshal (bson_parser_entry_t *entry,
*out = bson_copy (&tmp);
}

else if (ptype == BSON_PARSER_ARRAY_OR_DOC) {
bson_t tmp;
bson_t **out = (bson_t **) entry->out;

if (btype != BSON_TYPE_ARRAY && btype != BSON_TYPE_DOCUMENT) {
marshal_error (key, btype, ptype, error);
goto done;
}

bson_iter_bson (iter, &tmp);
*out = bson_copy (&tmp);
}

else if (ptype == BSON_PARSER_ANY) {
bson_val_t **out = (bson_val_t **) entry->out;

Expand Down
7 changes: 7 additions & 0 deletions src/libmongoc/tests/bsonutil/bson-parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,13 @@ bson_parser_array (bson_parser_t *bp, const char *key, bson_t **out);
void
bson_parser_array_optional (bson_parser_t *bp, const char *key, bson_t **out);

void
bson_parser_array_or_doc (bson_parser_t *bp, const char *key, bson_t **out);
void
bson_parser_array_or_doc_optional (bson_parser_t *bp,
const char *key,
bson_t **out);

void
bson_parser_bool (bson_parser_t *bp, const char *key, bool **out);
void
Expand Down
Loading