validate resource name format before PopFirst in bundle parsing#16221
validate resource name format before PopFirst in bundle parsing#16221alhudz wants to merge 2 commits 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. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@alhudz Thanks for the PR. Please sign the CLA and we'll take a look |
Done |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation checks to ensure that Firestore resource paths have at least 5 segments and that the 5th segment (index 4) is "documents" before processing them, preventing potential out-of-bounds access. The review comments suggest minor readability improvements to make the size checks (< 5 and >= 5) more intuitive and directly aligned with the 5-segment requirement.
| if (!rpc_serializer_.IsLocalResourceName(path) || path.size() <= 4 || | ||
| path[4] != "documents") { |
There was a problem hiding this comment.
Using path.size() < 5 is slightly more readable here as it directly aligns with the 5 segments required to safely call path.PopFirst(5) later in the function.
| if (!rpc_serializer_.IsLocalResourceName(path) || path.size() <= 4 || | |
| path[4] != "documents") { | |
| if (!rpc_serializer_.IsLocalResourceName(path) || path.size() < 5 || | |
| path[4] != "documents") { |
There was a problem hiding this comment.
Switched to path.size() < 5, reads better against the PopFirst(5) below. Behaviour is the same.
| return IsLocalResourceName(resource) && resource.size() > 4 && | ||
| resource[4] == "documents" && |
There was a problem hiding this comment.
Using resource.size() >= 5 is slightly more intuitive than resource.size() > 4 as it directly corresponds to the 5 segments required for resource.PopFirst(5) and makes the relationship with index 4 (resource[4]) clearer.
| return IsLocalResourceName(resource) && resource.size() > 4 && | |
| resource[4] == "documents" && | |
| return IsLocalResourceName(resource) && resource.size() >= 5 && | |
| resource[4] == "documents" && |
There was a problem hiding this comment.
Done, resource.size() >= 5 now.
|
I will take a look for this next week. |
alhudz
force-pushed
the
bundle-name-popfirst-guard
branch
from
a2da660 to
56c5931
Compare
|
Folded in the two readability tweaks from the review ( |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds path validation to ensure resource paths have at least 5 segments and that the 5th segment is "documents" before processing them, preventing potential out-of-bounds crashes. The reviewer notes that while this prevents a crash in PopFirst(5), DecodeName can still return paths that are not valid document keys, which could still trigger a crash in DecodeDocumentMetadata and DecodeDocument. The reviewer suggests adding validation using DocumentKey::IsDocumentKey in those functions.
|
any update? |
cherylEnkidu
left a comment
cherylEnkidu
left a comment
There was a problem hiding this comment.
Thank you so much for your contribution! The PR looks great overall—there are just a few minor things that could be improved.
| return IsLocalResourceName(resource) && | ||
| return IsLocalResourceName(resource) && resource.size() >= 5 && | ||
| resource[4] == "documents" && | ||
| DocumentKey::IsDocumentKey(resource.PopFirst(5)); |
There was a problem hiding this comment.
The current implementation calls DocumentKey::IsDocumentKey(resource.PopFirst(5)). PopFirst allocates a new ResourcePath vector and copies all segment strings in memory. This allocation is unnecessary because we only need to verify if the number of segments remaining after the database prefix is even.
Suggested Diff
bool Serializer::IsLocalDocumentKey(absl::string_view path) const {
auto resource = ResourcePath::FromStringView(path);
return IsLocalResourceName(resource) && resource.size() >= 5 &&
resource[4] == "documents" &&
- DocumentKey::IsDocumentKey(resource.PopFirst(5));
+ (resource.size() - 5) % 2 == 0;
}| if (!reader.ok()) { | ||
| return {}; | ||
| } | ||
| if (!DocumentKey::IsDocumentKey(path)) { |
There was a problem hiding this comment.
- if (!DocumentKey::IsDocumentKey(path)) {
+ if (path.empty() || !DocumentKey::IsDocumentKey(path)) {
| if (!reader.ok()) { | ||
| return {}; | ||
| } | ||
| if (!DocumentKey::IsDocumentKey(path)) { |
There was a problem hiding this comment.
- if (!DocumentKey::IsDocumentKey(path)) {
+ if (path.empty() || !DocumentKey::IsDocumentKey(path)) {| } | ||
| } | ||
|
|
||
| TEST_F(BundleSerializerTest, DecodeNonDocumentKeyResourceNameFails) { |
There was a problem hiding this comment.
The current DecodeNonDocumentKeyResourceNameFails test only validates collection paths. Could you expand the test suite to verify that:
Short paths (size < 5, e.g., /projects/p/databases/default) fail gracefully instead of crashing.
Root documents paths (which resolve to an empty key) fail gracefully.
for example:
TEST_F(BundleSerializerTest, DecodeNonDocumentKeyResourceNameFails) {
// 1. Collection Path (Odd number of segments, fails key check)
{
ProtoDocument document = TestDocument(ProtoValue());
document.set_name(FullPath("bundle"));
std::string json_string;
MessageToJsonString(document, &json_string);
JsonReader reader;
bundle_serializer.DecodeDocument(reader, Parse(json_string));
EXPECT_NOT_OK(reader.status());
}
// 2. Root Path (Popped path is empty, size 0, must fail)
{
ProtoDocument document = TestDocument(ProtoValue());
document.set_name(FullPath(""));
std::string json_string;
MessageToJsonString(document, &json_string);
JsonReader reader;
bundle_serializer.DecodeDocument(reader, Parse(json_string));
EXPECT_NOT_OK(reader.status());
}
// 3. Short Path (Size < 5, must fail prefix validation instead of crashing PopFirst)
{
ProtoDocument document = TestDocument(ProtoValue());
document.set_name("projects/p/databases/default");
std::string json_string;
MessageToJsonString(document, &json_string);
JsonReader reader;
bundle_serializer.DecodeDocument(reader, Parse(json_string));
EXPECT_NOT_OK(reader.status());
}
// (Duplicate these three cases for DecodeDocumentMetadata below as well)
}3 participants
DecodeName and IsLocalDocumentKey accept a resource name from bundle input and call PopFirst(5) after only checking IsLocalResourceName, which still allows a four-segment name like projects/p/databases/d. That trips the length assertion inside PopFirst. ExtractLocalPathFromResourceName and DocumentKey::FromName already require size() > 4 and a documents segment, so this applies the same format check at the two bundle-reachable sites.