fix: preserve unreserved path ID characters#867
Conversation
Fixes googleworkspace#842 Co-Authored-By: bridge <bridge@nousresearch.com>
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modifies the URL path segment encoding logic to correctly handle Apps Script IDs. By switching from a strict non-alphanumeric encoding set to one that respects RFC 3986 unreserved characters, the changes ensure that IDs containing hyphens and underscores are not unnecessarily percent-encoded, preventing potential issues with generated API URLs. Highlights
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. Footnotes
|
|
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. |
![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 encode_path_segment to preserve RFC 3986 unreserved characters (-, ., _, ~) during percent-encoding. Feedback suggests defining the encode set by starting from NON_ALPHANUMERIC and removing the unreserved characters, rather than manually building it from CONTROLS. This prevents other characters like ! or & from being left unencoded, which could lead to unexpected behavior.
| use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS}; | ||
|
|
||
| const PATH_SEGMENT_ENCODE_SET: &AsciiSet = &CONTROLS | ||
| .add(b' ') | ||
| .add(b'"') | ||
| .add(b'#') | ||
| .add(b'%') | ||
| .add(b'<') | ||
| .add(b'>') | ||
| .add(b'?') | ||
| .add(b'`') | ||
| .add(b'{') | ||
| .add(b'}') | ||
| .add(b'/') | ||
| .add(b':') | ||
| .add(b';') | ||
| .add(b'=') | ||
| .add(b'@') | ||
| .add(b'[') | ||
| .add(b'\\') | ||
| .add(b']') | ||
| .add(b'^') | ||
| .add(b'|'); | ||
|
|
||
| utf8_percent_encode(s, PATH_SEGMENT_ENCODE_SET).to_string() |
There was a problem hiding this comment.
By manually constructing PATH_SEGMENT_ENCODE_SET starting from CONTROLS and adding specific characters, several characters that were previously encoded by NON_ALPHANUMERIC (such as !, $, &, ', (, ), *, +, ,) are now left unencoded. This could lead to unexpected behavior or bugs if a path segment contains these characters (for example, + might be decoded to a space by some servers, or & could be misinterpreted as a query parameter separator).
Instead, we can define PATH_SEGMENT_ENCODE_SET by starting from NON_ALPHANUMERIC and removing the RFC 3986 unreserved characters (-, ., _, ~). This is much cleaner, less error-prone, and ensures all other non-alphanumeric characters remain safely encoded.
| use percent_encoding::{utf8_percent_encode, AsciiSet, CONTROLS}; | |
| const PATH_SEGMENT_ENCODE_SET: &AsciiSet = &CONTROLS | |
| .add(b' ') | |
| .add(b'"') | |
| .add(b'#') | |
| .add(b'%') | |
| .add(b'<') | |
| .add(b'>') | |
| .add(b'?') | |
| .add(b'`') | |
| .add(b'{') | |
| .add(b'}') | |
| .add(b'/') | |
| .add(b':') | |
| .add(b';') | |
| .add(b'=') | |
| .add(b'@') | |
| .add(b'[') | |
| .add(b'\\') | |
| .add(b']') | |
| .add(b'^') | |
| .add(b'|'); | |
| utf8_percent_encode(s, PATH_SEGMENT_ENCODE_SET).to_string() | |
| use percent_encoding::{utf8_percent_encode, AsciiSet, NON_ALPHANUMERIC}; | |
| const PATH_SEGMENT_ENCODE_SET: &AsciiSet = &NON_ALPHANUMERIC | |
| .remove(b'-') | |
| .remove(b'.') | |
| .remove(b'_') | |
| .remove(b'~'); | |
| utf8_percent_encode(s, PATH_SEGMENT_ENCODE_SET).to_string() |
build_create_template_url embeds template_id into a query string via raw format!(), not a .query() builder. encode_path_segment (this PR's prior commit) deliberately leaves & and + unencoded since both are legal in a bare path segment — but neither is safe in a query string: an unencoded & splits the value into two parameters (silently truncating the intended template_id), and + is commonly decoded server-side as a space. validate_resource_name (which template_id passes through first) does not reject & or +, so this was reachable from --template-id input. Confirmed by compiling and running url::Url::parse against the actual encode set before this fix. Add a fork-local encode_query_value in modelarmor.rs rather than reusing/exporting validate::encode_path_segment's encode set, so that function can stay a byte-for-byte match with the upstream fix (googleworkspace#867) it backports.
encode_path_segment percent-encoded every non-alphanumeric character, including RFC 3986 unreserved characters (-, ., _, ~). This mangled IDs that legitimately contain them — most visibly Apps Script scriptIds (base64url-derived, so - and _ are common), producing a URL the Apps Script Execution API rejects with 404 (upstream googleworkspace#842, fix already open upstream as PR googleworkspace#867 — this backports the identical encode set so a future upstream merge stays conflict-free). Switch from an allowlist (NON_ALPHANUMERIC) to a denylist covering only characters that can alter URL structure (space, control chars, #, %, /, :, ?, etc.) — unreserved characters now pass through untouched. This also fixes a latent bug in the fork's own modelarmor helper: it was sending region names like "us-central1" as "us%2Dcentral1", which is the same failure mode as googleworkspace#842. Updates existing tests whose expectations encoded the old (wrong) behavior: the path-traversal test now checks the real invariant (`/` is encoded, so `..` can't reintroduce a path separator) instead of asserting `..` disappears — `.` is unreserved and intentionally stays visible.
build_create_template_url embeds template_id into a query string via raw format!(), not a .query() builder. encode_path_segment (this PR's prior commit) deliberately leaves & and + unencoded since both are legal in a bare path segment — but neither is safe in a query string: an unencoded & splits the value into two parameters (silently truncating the intended template_id), and + is commonly decoded server-side as a space. validate_resource_name (which template_id passes through first) does not reject & or +, so this was reachable from --template-id input. Confirmed by compiling and running url::Url::parse against the actual encode set before this fix. Add a fork-local encode_query_value in modelarmor.rs rather than reusing/exporting validate::encode_path_segment's encode set, so that function can stay a byte-for-byte match with the upstream fix (googleworkspace#867) it backports.
) * fix: preserve RFC 3986 unreserved characters in encode_path_segment encode_path_segment percent-encoded every non-alphanumeric character, including RFC 3986 unreserved characters (-, ., _, ~). This mangled IDs that legitimately contain them — most visibly Apps Script scriptIds (base64url-derived, so - and _ are common), producing a URL the Apps Script Execution API rejects with 404 (upstream googleworkspace#842, fix already open upstream as PR googleworkspace#867 — this backports the identical encode set so a future upstream merge stays conflict-free). Switch from an allowlist (NON_ALPHANUMERIC) to a denylist covering only characters that can alter URL structure (space, control chars, #, %, /, :, ?, etc.) — unreserved characters now pass through untouched. This also fixes a latent bug in the fork's own modelarmor helper: it was sending region names like "us-central1" as "us%2Dcentral1", which is the same failure mode as googleworkspace#842. Updates existing tests whose expectations encoded the old (wrong) behavior: the path-traversal test now checks the real invariant (`/` is encoded, so `..` can't reintroduce a path separator) instead of asserting `..` disappears — `.` is unreserved and intentionally stays visible. * fix: encode & and + in modelarmor create-template query value build_create_template_url embeds template_id into a query string via raw format!(), not a .query() builder. encode_path_segment (this PR's prior commit) deliberately leaves & and + unencoded since both are legal in a bare path segment — but neither is safe in a query string: an unencoded & splits the value into two parameters (silently truncating the intended template_id), and + is commonly decoded server-side as a space. validate_resource_name (which template_id passes through first) does not reject & or +, so this was reachable from --template-id input. Confirmed by compiling and running url::Url::parse against the actual encode set before this fix. Add a fork-local encode_query_value in modelarmor.rs rather than reusing/exporting validate::encode_path_segment's encode set, so that function can stay a byte-for-byte match with the upstream fix (googleworkspace#867) it backports.
2 participants
Summary
Fixes #842
Preserve RFC 3986 unreserved characters in path parameter encoding so Apps Script IDs containing
-and_stay valid inscript scripts runURLs.Changes
NON_ALPHANUMERICencode set with a path-segment encode set that leaves-,.,_, and~intact./,?,#,%,@, and=percent-encoded.Test Plan
cargo fmt -- --checkcargo test -q validate::tests::test_encode_path_segment_preserves_unreserved_script_id(blocked in this environment: linkerccis not installed, so Rust build scripts for dependencies fail before tests compile)cargo test -q(same linker blocker)