fix: preserve unreserved path ID characters#865
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 prevent unnecessary percent-encoding of RFC 3986 unreserved characters. By replacing the overly restrictive encoding set, the change ensures that identifiers containing dashes and underscores remain intact, which is critical for maintaining valid URLs for Apps Script operations. 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. |
|
Superseded by #867, which is rebased onto upstream main to avoid the fork branch conflict. |
![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 in src/validate.rs to preserve RFC 3986 unreserved characters (-, ., _, ~) instead of encoding all non-alphanumeric characters. The reviewer pointed out that manually constructing the encode set from CONTROLS is error-prone and leaves characters like & unencoded, potentially allowing query parameter injection. A cleaner and safer approach was suggested to start from NON_ALPHANUMERIC and remove the four unreserved characters.
| 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.
Defining the encode set by manually adding individual characters to CONTROLS is verbose, error-prone, and accidentally leaves characters like & (and other sub-delimiters like +, !, $, etc.) unencoded. Leaving & unencoded in a path segment allows query parameter injection (e.g., if an ID contains ?fields=name&alt=json, the &alt=json portion will be treated as a query parameter of the outer request rather than part of the path segment).
Instead, define the encode set by starting from NON_ALPHANUMERIC and removing only the four RFC 3986 unreserved characters (-, ., _, ~). This is much cleaner, safer, and perfectly aligns with the goal of leaving only those four characters intact.
| 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() |
1 participant
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)