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.

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

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

Conversation

CYBAI
Copy link
Member

@CYBAI CYBAI commented Feb 27, 2023

When we use the parser::Input::new, it will initialize the input iterator with trimming both whitespaces and C0 controls. However, for scheme, we should only trim whitespaces. Thus, with moving from ::new to ::trim_tab_and_newlines could fix the issue.

This PR will fix #815.

@CYBAI CYBAI force-pushed the avoid-trim-c0-scheme branch from d330e70 to cbc1b50 Compare February 27, 2023 10:32
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (edeaea7) 82.74% compared to head (9c16caa) 82.72%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #816      +/-   ##
==========================================
- Coverage   82.74%   82.72%   -0.02%     
==========================================
  Files          20       20              
  Lines        3303     3305       +2     
==========================================
+ Hits         2733     2734       +1     
- Misses        570      571       +1     
Impacted Files Coverage Δ
url/src/lib.rs 76.41% <100.00%> (+0.06%) ⬆️
data-url/tests/wpt.rs 84.41% <0.00%> (-1.30%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

url/tests/unit.rs Outdated Show resolved Hide resolved
@CYBAI CYBAI force-pushed the avoid-trim-c0-scheme branch 2 times, most recently from 806225f to f6aaf80 Compare March 1, 2023 05:20
@CYBAI CYBAI force-pushed the avoid-trim-c0-scheme branch from f6aaf80 to 9c16caa Compare March 5, 2023 01:27
@CYBAI CYBAI changed the title Trim whitespaces only while setting scheme Ignore set_scheme when input contains trailing/leading C0 controls Mar 5, 2023
@CYBAI CYBAI requested a review from valenting March 5, 2023 01:29
@@ -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.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably 1158370) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set scheme with non-tab/newline C0 controls should result in no-op
4 participants