Merge pull request #2060 from yuja/push-urolxnurwtsn

fix: username in scp-like url is no longer percent-encoded (#2056)

Author: Byron

gix-url/src/lib.rs

@@ -315,11 +315,23 @@ fn percent_encode(s: &str) -> Cow<'_, str> {
 /// Serialization
 impl Url {
     /// Write this URL losslessly to `out`, ready to be parsed again.
-    pub fn write_to(&self, mut out: &mut dyn std::io::Write) -> std::io::Result<()> {
-        if !(self.serialize_alternative_form && (self.scheme == Scheme::File || self.scheme == Scheme::Ssh)) {
-            out.write_all(self.scheme.as_str().as_bytes())?;
-            out.write_all(b"://")?;
+    pub fn write_to(&self, out: &mut dyn std::io::Write) -> std::io::Result<()> {
+        // Since alternative form doesn't employ any escape syntax, password and
+        // port number cannot be encoded.
+        if self.serialize_alternative_form
+            && (self.scheme == Scheme::File || self.scheme == Scheme::Ssh)
+            && self.password.is_none()
+            && self.port.is_none()
+        {
+            self.write_alternative_form_to(out)
+        } else {
+            self.write_canonical_form_to(out)
         }
+    }
+
+    fn write_canonical_form_to(&self, out: &mut dyn std::io::Write) -> std::io::Result<()> {
+        out.write_all(self.scheme.as_str().as_bytes())?;
+        out.write_all(b"://")?;
         match (&self.user, &self.host) {
             (Some(user), Some(host)) => {
                 out.write_all(percent_encode(user).as_bytes())?;
@@ -337,9 +349,31 @@ impl Url {
             (Some(_user), None) => unreachable!("BUG: should not be possible to have a user but no host"),
         }
         if let Some(port) = &self.port {
-            write!(&mut out, ":{port}")?;
+            write!(out, ":{port}")?;
+        }
+        out.write_all(&self.path)?;
+        Ok(())
+    }
+
+    fn write_alternative_form_to(&self, out: &mut dyn std::io::Write) -> std::io::Result<()> {
+        match (&self.user, &self.host) {
+            (Some(user), Some(host)) => {
+                out.write_all(user.as_bytes())?;
+                assert!(
+                    self.password.is_none(),
+                    "BUG: cannot serialize password in alternative form"
+                );
+                out.write_all(b"@")?;
+                out.write_all(host.as_bytes())?;
+            }
+            (None, Some(host)) => {
+                out.write_all(host.as_bytes())?;
+            }
+            (None, None) => {}
+            (Some(_user), None) => unreachable!("BUG: should not be possible to have a user but no host"),
         }
-        if self.serialize_alternative_form && self.scheme == Scheme::Ssh {
+        assert!(self.port.is_none(), "BUG: cannot serialize port in alternative form");
+        if self.scheme == Scheme::Ssh {
             out.write_all(b":")?;
         }
         out.write_all(&self.path)?;

gix-url/tests/fixtures/make_baseline.sh

@@ -15,7 +15,7 @@ tests_windows=()
 for path in "repo" "re:po" "re/po"; do
   # normal urls
   for protocol in "ssh+git" "git+ssh" "git" "ssh"; do
-    for host in "host" "user@host" "user@[::1]" "user@::1"; do
+    for host in "host" "user@host" "user_name@host" "user@[::1]" "user@::1"; do
       for port_separator in "" ":"; do
         tests+=("$protocol://$host$port_separator/$path")
 
@@ -42,7 +42,7 @@ for path in "repo" "re:po" "re/po"; do
     tests+=("./$protocol:$host/~$path")
   done
   # SCP like urls
-  for host in "host" "[::1]"; do
+  for host in "user@name@host" "user_name@host" "host" "[::1]"; do
     tests+=("$host:$path")
     tests+=("$host:/~$path")
   done

gix-url/tests/url/baseline.rs

@@ -84,7 +84,7 @@ fn run() {
     }
 
     assert!(
-        failure_count_reserialization <= 42,
+        failure_count_reserialization <= 63,
         "the number of reserialization errors should ideally get better, not worse - if this panic is not due to regressions but to new passing test cases, you can set this check to {failure_count_reserialization}"
     );
     assert_eq!(failure_count_roundtrips, 0, "there should be no roundtrip errors");
@@ -185,8 +185,8 @@ mod baseline {
 
         pub fn max_num_failures(&self) -> usize {
             match self {
-                Kind::Unix => 165,
-                Kind::Windows => 171,
+                Kind::Unix => 198,
+                Kind::Windows => 198 + 6,
             }
         }
 

gix-url/tests/url/parse/ssh.rs

@@ -1,6 +1,6 @@
 use gix_url::Scheme;
 
-use crate::parse::{assert_url, assert_url_roundtrip, url, url_alternate};
+use crate::parse::{assert_url, assert_url_roundtrip, url, url_alternate, url_with_pass};
 
 #[test]
 fn without_user_and_without_port() -> crate::Result {
@@ -184,6 +184,30 @@ fn scp_like_with_windows_path_and_port_thinks_port_is_part_of_path() -> crate::R
     Ok(())
 }
 
+#[test]
+fn scp_like_with_non_alphanumeric_username() -> crate::Result {
+    let url = assert_url(
+        "_user.name@host.xz:C:/path",
+        url_alternate(Scheme::Ssh, "_user.name", "host.xz", None, b"C:/path"),
+    )?
+    .to_bstring();
+    assert_eq!(url, "_user.name@host.xz:C:/path");
+    Ok(())
+}
+
+// Git passes the non-path part "user@name@host.xz" to OpenSSH, and the ssh
+// command interprets it as user = "user@name", host = "host.xz".
+#[test]
+fn scp_like_with_username_including_at() -> crate::Result {
+    let url = assert_url(
+        "user@name@host.xz:path",
+        url_alternate(Scheme::Ssh, "user@name", "host.xz", None, b"path"),
+    )?
+    .to_bstring();
+    assert_eq!(url, "user@name@host.xz:path");
+    Ok(())
+}
+
 // Git does not care that the host is named `file`, it still treats it as an SCP url.
 // I btw tested this, yes you can really clone a repository from there, just `git init`
 // in the directory above your home directory on the remote machine.
@@ -193,3 +217,19 @@ fn strange_scp_like_with_host_named_file() -> crate::Result {
     assert_eq!(url.to_bstring(), "file:..");
     Ok(())
 }
+
+#[test]
+fn bad_alternative_form_with_password() -> crate::Result {
+    let url = url_with_pass(Scheme::Ssh, "user", "password", "host.xz", None, b"/")
+        .serialize_alternate_form(true)
+        .to_bstring();
+    assert_eq!(url, "ssh://user:password@host.xz/");
+    Ok(())
+}
+
+#[test]
+fn bad_alternative_form_with_port() -> crate::Result {
+    let url = url_alternate(Scheme::Ssh, None, "host.xz", 21, b"/").to_bstring();
+    assert_eq!(url, "ssh://host.xz:21/");
+    Ok(())
+}