Changing the scheme can break things about rust-url HOT 8 CLOSED

I’m a bit reluctant, since it’s not obvious what the semantics of changing the scheme like this should be.

For the case of view-source+ specifically, I wonder if you wouldn’t be better detecting and off cutting of the prefix before parsing as an URL.

from rust-url.

This same issue affects some of the upcoming HSTS code in servo where a Url is changed from http to https.

If it's not reasonable to mutate the scheme, does it make more sense to make scheme private and require a function call to read it? This way you can't create a Url instance that has an invalid state.

from rust-url.

@samfoo http and https are both "relative" schemes, though. What’s the problem you’re seeing with HSTS? It might not be the same as the original issue here.

from rust-url.

A simple test case is probably the easiest way to show one of the problems (that the default port doesn't change). This is specifically the problem with the pull request for HSTS. I'm not sure how much other internal state the Url keeps that could potentially cause a problem.

#[test]
fn test_changing_protocol_should_not_retain_the_old_default_port() {
    let mut http_url = Url::parse("http://mozilla.org").unwrap();
    http_url.scheme = "https".to_string();

    assert!(http_url.port().is_none());
    assert_eq!(http_url.port_or_default().unwrap(), 443)
}

---- hsts::test_changing_protocol_should_not_retain_the_old_default_port stdout ----
    thread 'hsts::test_changing_protocol_should_not_retain_the_old_default_port' panicked at 'assertion failed: `(left == right)` (left: `80`, right: `443`)', /Users/sam/projects/servo/tests/unit/net/hsts.rs:19

from rust-url.

I’m thinking of refactoring the parser and data structure to fix a bunch of things including this.

But in the mean time, you can work around this issue by using url.defaul_port = Some(443) when changing the scheme to HTTPS.

from rust-url.

secure_url.relative_scheme_data_mut()
            .map(|scheme_data| {
                scheme_data.default_port = Some(443);
            });

Is the best I could come up with, since default_port is on the the scheme data, not the Url. But that is better than serialising and re-parsing which is the strategy I took up 'til now.

from rust-url.

Yes, I think this is the best we can do without changing the data structure. There could be a set_scheme method, but it’d just do this internally.

from rust-url.

This passes in 1.0:

    let mut url = Url::parse("http://mozilla.org").unwrap();
    url.set_scheme("https").unwrap();
    assert_eq!(url.port(), None);
    assert_eq!(url.port_or_known_default(), Some(443));
    url.assert_invariants();

#187 adds it as a test case.

from rust-url.