validate structuredQuery shape before json access in bundle decoder#16251
validate structuredQuery shape before json access in bundle decoder#16251alhudz wants to merge 1 commit into
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds validation checks to bundle_serializer.cc to ensure that the from clause is an array and that the limit object contains a valid integer value, along with corresponding unit tests. The reviewer suggested optimizing the limit object lookup by using find instead of contains and at to avoid redundant lookups.
| } else if (limit_object.is_object() && limit_object.contains("value") && | ||
| limit_object.at("value").is_number_integer()) { | ||
| return limit_object.at("value").get<int32_t>(); | ||
| } |
There was a problem hiding this comment.
Using contains followed by multiple at calls results in redundant lookups in the JSON object. We can optimize this by using find to perform a single lookup.
| } else if (limit_object.is_object() && limit_object.contains("value") && | |
| limit_object.at("value").is_number_integer()) { | |
| return limit_object.at("value").get<int32_t>(); | |
| } | |
| } else if (limit_object.is_object()) { | |
| auto it = limit_object.find("value"); | |
| if (it != limit_object.end() && it->is_number_integer()) { | |
| return it->get<int32_t>(); | |
| } | |
| } |
Firestorebundle query decoding (reached vialoadBundle, where the bundle is commonly fetched from a CDN) reads parsed JSON without checking its shape, so a malformed bundle throws an uncaughtnlohmannexception instead of failing the reader.Repro: load a named query whose
structuredQuery.fromis a non-array, e.g."from":"colls", or whoselimitis an object with novalue, e.g."limit":{}.Expected: the reader reports a decode failure.
Actual:
DecodeCollectionSourcecallsget_ref<const std::vector<json>&>()with nois_array()guard and throwstype_error.303;DecodeLimitcallslimit_object.at("value")with no presence guard and throwsout_of_range.403. Neither is caught on the decode path, so the process terminates.Fix: guard both reads the way every sibling accessor in this file and in
json_reader.ccalready does, so malformed input fails the reader instead of throwing.Added two regressions in
bundle_serializer_test.cc.