Repository: servo/rust-url (rust)

Issue:
Cannot parse usernames in path urls.
In rust-postgres, there's a test that generates a url like so:

``` rust
let mut url = Url::from_file_path(&Path::new(unix_socket_directory)).unwrap();
*url.username_mut().unwrap() = "postgres".to_string();                         
url.scheme = "postgres".to_string();                                           
```

When printed, the url is:

```
postgres://postgres@/var/run/postgresql
```

But when this url is parsed, it parses to a url with an empty host (a parser error).

Changing the  scheme can break things
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?



Discussion:
You are trying to parse an URL with an authority (scheme://authority), an authority must include a non-empty host.

Oh wait, no, I stand corrected:

```
      reg-name    = *( unreserved / pct-encoded / sub-delims )
```

FWIW

``` js
new URL("postgres://postgres@/var/run/postgresql").username === ""
```

is `true` for both Firefox and Chrome

@alexcrichton Not sure why I didn’t respond to this a year ago, sorry. To use "non-standard" schemes like `postgres:` you probably want a custom "scheme type mapper" https://servo.github.io/rust-url/url/struct.UrlParser.html#method.scheme_type_mapper .

@nox Where is that quote from? This crates implements http://url.spec.whatwg.org/ which sometimes diverges from the various relevant IETF RFCs.

@frewsxcv The Rust API of this crate and its behavior is not necessarily the same as https://url.spec.whatwg.org/#api (which is designed for JavaScript and web compat) as long as it enables Servo to implement it on top.

> The Rust API of this crate and its behavior is not necessarily the same as https://url.spec.whatwg.org/#api (which is designed for JavaScript and web compat) as long as it enables Servo to implement it on top.

I thought about that possibility, but wasn't sure. Thanks for clarifying :+1: 

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.

This same issue affects some of the upcoming [HSTS code in servo](https://github.com/samfoo/servo/blob/hsts-preload/components/net/hsts.rs#L124) 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.

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

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.

``` rust
#[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
```

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.

``` rust
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.

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.


Patch:
diff --git a/src/lib.rs b/src/lib.rs
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -266,6 +266,7 @@ impl Url {
     /// Methods of the `Url` struct assume a number of invariants.
     /// This checks each of these invariants and panic if one is not met.
     /// This is for testing rust-url itself.
+    #[doc(hidden)]
     pub fn assert_invariants(&self) {
         macro_rules! assert {
             ($x: expr) => {
@@ -848,7 +849,7 @@ impl Url {
         let old_suffix_pos = if opt_new_port.is_some() { self.path_start } else { self.host_end };
         let suffix = self.slice(old_suffix_pos..).to_owned();
         self.serialization.truncate(self.host_start as usize);
-        if !self.has_host() {
+        if !self.has_authority() {
             debug_assert!(self.slice(self.scheme_end..self.host_start) == ":");
             debug_assert!(self.username_end == self.host_start);
             self.serialization.push('/');


Test patch:
diff --git a/tests/unit.rs b/tests/unit.rs
--- a/tests/unit.rs
+++ b/tests/unit.rs
@@ -233,3 +233,29 @@ fn test_form_serialize() {
         .finish();
     assert_eq!(encoded, "foo=%C3%A9%26&bar=&foo=%23");
 }
+
+#[test]
+/// https://github.com/servo/rust-url/issues/25
+fn issue_25() {
+    let filename = if cfg!(windows) { r"C:\run\pg.sock" } else { "/run/pg.sock" };
+    let mut url = Url::from_file_path(filename).unwrap();
+    url.assert_invariants();
+    url.set_scheme("postgres").unwrap();
+    url.assert_invariants();
+    url.set_host(Some("")).unwrap();
+    url.assert_invariants();
+    url.set_username("me").unwrap();
+    url.assert_invariants();
+    let expected = format!("postgres://me@/{}run/pg.sock", if cfg!(windows) { "C:/" } else { "" });
+    assert_eq!(url.as_str(), expected);
+}
+
+#[test]
+/// https://github.com/servo/rust-url/issues/61
+fn issue_61() {
+    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();
+}