-
-
Notifications
You must be signed in to change notification settings - Fork 415
Implement percent-decoding in simple_url for all URL components #2358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Byron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Keep going, see the comments.
gix-url/src/simple_url.rs
Outdated
| pub password: Option<String>, // Owned to allow percent-decoding | ||
| pub host: Option<String>, // Owned to allow normalization to lowercase | ||
| pub port: Option<u16>, | ||
| pub path: &'a str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot The path should also be percent-decoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 7423682. The path field is now String and gets percent-decoded during parsing, just like username and password.
gix-url/src/simple_url.rs
Outdated
| // Validate username and password don't contain literal whitespace before encoding | ||
| // (percent-encoded whitespace like %20 is allowed and will be decoded) | ||
| if user_str.chars().any(char::is_whitespace) { | ||
| return Err(UrlParseError::InvalidDomainCharacter); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot There should be no extra validation necessary, the percent decoding takes care of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 7423682. Removed all whitespace validation - the percent decoder handles everything now. Literal whitespace is accepted and passed through, while percent-encoded whitespace like %20 gets properly decoded.
Byron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot It seems that percent-decoding will not error if it sees whitespace. Let's check for whitespace specifically, and do so just once at the beginning. The whole URL can't have any whitespace.
Done in commit 0bb3c7e. Added a single whitespace check at the very beginning of |
…nd paths, and add whitespace rejection. Co-authored-by: Byron <63622+Byron@users.noreply.github.com>
0bb3c7e to
a3e1823
Compare
Percent-encoded usernames, passwords, and paths were being decoded outside the
simple_urlmodule inparse.rs, creating duplicated logic and making the parser incomplete.Changes
ParsedUrlstructure: Changedusername,password, andpathfields from&strtoStringto store decoded values directlysimple_url::ParsedUrl::parse(): All URL components (username, password, and path) are now decoded during initial parsing using a newpercent_decode()helperparse.rs: Eliminatedpercent_decoded_utf8()andurl_user()helper functions that were decoding fields already handled bysimple_url%20) is properly decodedExample
All URL components are now decoded consistently, simplifying the codebase and centralizing percent-decoding logic in the
simple_urlmodule while maintaining RFC 3986 compliance.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.