Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ignore set_scheme when input contains trailing/leading C0 controls #816

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Ignore set_scheme when input contains trailing/leading C0 controls #816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83c19dd
Ignore set_scheme when input contains trailing/leading C0 controls
CYBAI Feb 27, 2023
9c16caa
Import related wpt setters test
CYBAI Mar 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions url/src/lib.rs
View file
Open in desktop
Expand Up @@ -2313,6 +2313,12 @@ impl Url {
/// ```
#[allow(clippy::result_unit_err, clippy::suspicious_operation_groupings)]
pub fn set_scheme(&mut self, scheme: &str) -> Result<(), ()> {
// If the given scheme contains leading or trailing C0 controls,
// we'll ignore the set_scheme operation.
if scheme.trim_matches(|ch| ch <= ' ').len() != scheme.len() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two problems here:

  1. I think we should early exit if the scheme contains a Non-tab/newline C0 controls - not just leading or trailing
  2. This would still match tabs and newlines.

Please add a test for one of these chars in the middle of the scheme.
Also a test that setting it to https\n still works.

return Ok(());
}

let mut parser = Parser::for_setter(String::new());
let remaining = parser.parse_scheme(parser::Input::new(scheme))?;
let new_scheme_type = SchemeType::from(&parser.serialization);
Expand Down
33 changes: 33 additions & 0 deletions url/tests/setters_tests.json
View file
Open in desktop
Expand Up @@ -270,6 +270,39 @@
"protocol": "https:",
"port": ""
}
},
{
"comment": "Non-tab/newline C0 controls result in no-op",
"href": "http://test/",
"new_value": "https\u0000",
"expected": {
"href": "http://test/",
"protocol": "http:"
}
},
{
"href": "http://test/",
"new_value": "https\u000C",
"expected": {
"href": "http://test/",
"protocol": "http:"
}
},
{
"href": "http://test/",
"new_value": "https\u000E",
"expected": {
"href": "http://test/",
"protocol": "http:"
}
},
{
"href": "http://test/",
"new_value": "https\u0020",
"expected": {
"href": "http://test/",
"protocol": "http:"
}
}
],
"username": [
Expand Down
11 changes: 11 additions & 0 deletions url/tests/unit.rs
View file
Open in desktop
Expand Up @@ -147,6 +147,17 @@ fn new_path_windows_fun() {
}
}

#[test]
fn set_scheme_with_leading_or_trailing_c0_controls_should_result_in_noop() {
let mut url: Url = "http://test".parse().unwrap();

let controls = ["\u{0000}", "\u{000C}", "\u{000E}", "\u{0020}"];
for control in controls.iter() {
assert!(url.set_scheme(&format!("{}https", control)).is_ok());
assert!(url.set_scheme(&format!("https{}", control)).is_ok());
}
}

#[test]
fn new_directory_paths() {
if cfg!(unix) {
Expand Down