guard IsLocalResourceName against names missing documents segment#16396
guard IsLocalResourceName against names missing documents segment#16396isl-Ramzi 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 |
![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.
| return IsValidResourceName(path) && path.size() > 4 && | ||
| path[1] == database_id_.project_id() && | ||
| path[3] == database_id_.database_id() && path[4] == "documents"; |
There was a problem hiding this comment.
For consistency with other parts of the codebase (such as ValidateDocumentKeyPath which checks resource_name.size() < 5 and ExtractLocalPathFromResourceName which checks resource_name.size() <= 4), consider using path.size() >= 5 instead of path.size() > 4. This directly aligns with the 5-segment prefix requirement described in the comments.
| return IsValidResourceName(path) && path.size() > 4 && | |
| path[1] == database_id_.project_id() && | |
| path[3] == database_id_.database_id() && path[4] == "documents"; | |
| return IsValidResourceName(path) && path.size() >= 5 && | |
| path[1] == database_id_.project_id() && | |
| path[3] == database_id_.database_id() && path[4] == "documents"; |
| 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} | |
| })"); |
|
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! |
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.