guard IsLocalResourceName against names missing documents segment#16396
guard IsLocalResourceName against names missing documents segment#16396isl-Ramzi 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. |
|
/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 updates IsLocalResourceName in serializer.cc to ensure that the resource path contains at least 5 segments and that the fifth segment is "documents", preventing assertions when stripping the prefix. It also adds unit tests to verify that decoding fails when the "documents" segment is missing. The review feedback suggests using path.size() >= 5 instead of path.size() > 4 for consistency with other parts of the codebase, and improving the formatting of the JSON string in the new unit test.
| std::string json( | ||
| R"({"name":"projects/p/databases/default", | ||
| "readTime":{"seconds":"0","nanos":0}})"); |
There was a problem hiding this comment.
The JSON string in the raw string literal can be formatted with proper indentation to improve readability.
| std::string json( | |
| R"({"name":"projects/p/databases/default", | |
| "readTime":{"seconds":"0","nanos":0}})"); | |
| std::string json( | |
| R"({ | |
| "name": "projects/p/databases/default", | |
| "readTime": {"seconds": "0", "nanos": 0} | |
| })"); |
There was a problem hiding this comment.
Moot now — that test is gone on the rebase, since DecodeNonDocumentKeyResourceNameFails from #16221 already covers the same name.
|
Thank you for this PR! Adding the bounds and segment validation directly into We currently have another PR in progress that we need to merge into the Once the other PR is merged, we will revisit this to get your holistic fix integrated. Thanks again for your excellent work on this! |
|
Thank you for the update! No problem at all—I’ll hold off on this PR until the other one is merged into main. Once that's done, I'll be happy to rebase or make any necessary updates to help get this fix integrated. Thanks for the review, and I appreciate the feedback! |
|
Hi @isl-Ramzi The other PR has already been merged, so the file on the main branch has changed. Would you mind rebasing your branch onto the latest |
Require the five-segment `projects/{p}/databases/{d}/documents` prefix in
`Serializer::IsLocalResourceName` so a shorter name such as
`projects/{p}/databases/{d}` cannot reach a `PopFirst(5)` on a four-segment
path. Valid document names always carry the `documents` segment, so
valid-input behavior is unchanged.
With the check at the predicate, the equivalent per-caller checks added in
firebase#16221 are redundant and are folded back into it:
- `BundleSerializer::DecodeName` drops its size/segment test.
- `Serializer::IsLocalDocumentKey` returns to
`DocumentKey::IsDocumentKey(resource.PopFirst(5))`, now that the
predicate guarantees the prefix.
Add a regression test for a `referenceValue` that stops before `documents`,
which reaches the predicate through `IsLocalDocumentKey`. The short-name case
for documents and document metadata is already covered by
`DecodeNonDocumentKeyResourceNameFails`.
e34d62a to
5f00a2c
Compare
|
Rebased onto latest main. Now that #16221 has landed, the crash itself is already fixed at the call sites, so this is down to consolidating those checks into the predicate. Three things worth a look:
I dropped my document-metadata test because If you would rather keep the checks where #16221 put them and close this out, that is fine by me too. |
| return IsLocalResourceName(resource) && resource.size() >= 5 && | ||
| resource[4] == "documents" && (resource.size() - 5) % 2 == 0; | ||
| return IsLocalResourceName(resource) && | ||
| DocumentKey::IsDocumentKey(resource.PopFirst(5)); |
There was a problem hiding this comment.
Could we revert this to avoid the memory allocation from resource.PopFirst(5)? DocumentKey::IsDocumentKey only checks the size parity, so copying the path isn't necessary.
Also, since you added the size >= 5 and "documents" checks to IsLocalResourceName above, we can simplify this function down to just:
bool Serializer::IsLocalDocumentKey(absl::string_view path) const {
auto resource = ResourcePath::FromStringView(path);
return IsLocalResourceName(resource) && (resource.size() - 5) % 2 == 0;
}There was a problem hiding this comment.
Done, reverted to your non-allocating form. Since IsLocalResourceName now guarantees the five-segment documents prefix, the size parity check on its own is enough here, so IsLocalDocumentKey is back to just IsLocalResourceName(resource) && (resource.size() - 5) % 2 == 0. No more PopFirst copy.
3 participants
A bundle whose document name matches the local project and database but stops before the
documentssegment, such asprojects/<id>/databases/<db>, passesIsLocalResourceNameand then reachesPopFirst(5)on a four-segment path, which trips aHARD_ASSERTand aborts the process. The same predicate gatesDecodeNameandIsLocalDocumentKey(the latter reachable from a document field'sreferenceValue), and bundle payloads handed toloadBundleare untrusted. Require the five-segment documents prefix inIsLocalResourceNameso short names are rejected the wayExtractLocalPathFromResourceNamealready rejects them.