Changing the scheme can break things

[Manishearth]

If you change the scheme from a non-relative one to a relative one without regenerating the scheme data, the Url object stops working properly. For example, parsing view-source+http://google.com and then using slice() on url.scheme to make it http again does not change the corresponding scheme data.

Perhaps there should be a set_scheme() method for setting the scheme safely?

[SimonSapin]

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.

[samfoo]

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.

[SimonSapin]

@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.

[samfoo]

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

[SimonSapin]

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.

[samfoo]

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.

[SimonSapin]

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.

[SimonSapin]

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.