From 6cbe65d3f25c5c00fb9248a5558ac74f6e77ff06 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Apr 2024 21:51:37 +0000 Subject: [PATCH 01/25] Reorder tests and add username assertion placeholders --- gix-url/tests/access/mod.rs | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/gix-url/tests/access/mod.rs b/gix-url/tests/access/mod.rs index b437834577..accfa47c94 100644 --- a/gix-url/tests/access/mod.rs +++ b/gix-url/tests/access/mod.rs @@ -30,6 +30,17 @@ mod canonicalized { } } +#[test] +fn user() -> crate::Result { + let mut url = gix_url::parse("https://user:password@host/path".into())?; + + assert_eq!(url.user(), Some("user")); + assert_eq!(url.set_user(Some("new-user".into())), Some("user".into())); + assert_eq!(url.user(), Some("new-user")); + + Ok(()) +} + #[test] fn password() -> crate::Result { let mut url = gix_url::parse("https://user:password@host/path".into())?; @@ -42,12 +53,16 @@ fn password() -> crate::Result { } #[test] -fn user() -> crate::Result { - let mut url = gix_url::parse("https://user:password@host/path".into())?; +fn user_argument_safe() -> crate::Result { + let url = gix_url::parse("ssh://-Fconfigfile@foo/bar".into())?; - assert_eq!(url.user(), Some("user")); - assert_eq!(url.set_user(Some("new-user".into())), Some("user".into())); - assert_eq!(url.user(), Some("new-user")); + // FIXME: Add the critical assertions for the user argument here. + + assert_eq!(url.host(), Some("foo")); + assert_eq!(url.host_argument_safe(), Some("foo")); + + assert_eq!(url.path, "/bar"); + assert_eq!(url.path_argument_safe(), Some("/bar".into())); Ok(()) } @@ -55,20 +70,30 @@ fn user() -> crate::Result { #[test] fn host_argument_safe() -> crate::Result { let url = gix_url::parse("ssh://-oProxyCommand=open$IFS-aCalculator/foo".into())?; + + // FIXME: Add assertions for the user argument here. + assert_eq!(url.host(), Some("-oProxyCommand=open$IFS-aCalculator")); assert_eq!(url.host_argument_safe(), None); + assert_eq!(url.path, "/foo"); assert_eq!(url.path_argument_safe(), Some("/foo".into())); + Ok(()) } #[test] fn path_argument_safe() -> crate::Result { let url = gix_url::parse("ssh://foo/-oProxyCommand=open$IFS-aCalculator".into())?; + + // FIXME: Add assertions for the user argument here. + assert_eq!(url.host(), Some("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); + assert_eq!(url.path, "/-oProxyCommand=open$IFS-aCalculator"); assert_eq!(url.path_argument_safe(), None); + Ok(()) } From 1bdfdd98c9a82eee7ac40b38fe05d54f1f93237e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Apr 2024 22:15:45 +0000 Subject: [PATCH 02/25] Prepare for adding Url::user_argument_safe - Harmonize minor nearby related inconsistencies in documentation. - Add the new assertions that don't require the new method (these are less important than the forthcoming ones that will call it). --- gix-url/src/lib.rs | 19 +++++++++++-------- gix-url/tests/access/mod.rs | 9 ++++++--- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index a810ad66c7..fba3ffe6d7 100644 --- a/gix-url/src/lib.rs +++ b/gix-url/src/lib.rs @@ -164,15 +164,17 @@ impl Url { /// Access impl Url { - /// Returns the user mentioned in the url, if present. + /// Return the user mentioned in the URL, if present. pub fn user(&self) -> Option<&str> { self.user.as_deref() } - /// Returns the password mentioned in the url, if present. + + /// Return the password mentioned in the url, if present. pub fn password(&self) -> Option<&str> { self.password.as_deref() } - /// Returns the host mentioned in the url, if present. + + /// Return the host mentioned in the URL, if present. /// /// # Security-Warning /// @@ -191,7 +193,7 @@ impl Url { self.host().filter(|host| !looks_like_argument(host.as_bytes())) } - /// Return the path of this URL *and* if it can't be mistaken for a command-line argument. + /// Return the path of this URL *if* it can't be mistaken for a command-line argument. /// Note that it always begins with a slash, which is ignored for this comparison. /// /// Use this method if the path is going to be passed to a command-line application. @@ -201,11 +203,12 @@ impl Url { .and_then(|truncated| (!looks_like_argument(truncated)).then_some(self.path.as_ref())) } - /// Returns true if the path portion of the url is `/`. + /// Return true if the path portion of the URL is `/`. pub fn path_is_root(&self) -> bool { self.path == "/" } - /// Returns the actual or default port for use according to the url scheme. + + /// Return the actual or default port for use according to the URL scheme. /// Note that there may be no default port either. pub fn port_or_default(&self) -> Option { self.port.or_else(|| { @@ -227,7 +230,7 @@ fn looks_like_argument(b: &[u8]) -> bool { /// Transformation impl Url { - /// Turn a file url like `file://relative` into `file:///root/relative`, hence it assures the url's path component is absolute, using + /// Turn a file URL like `file://relative` into `file:///root/relative`, hence it assures the URL's path component is absolute, using /// `current_dir` if necessary. pub fn canonicalized(&self, current_dir: &std::path::Path) -> Result { let mut res = self.clone(); @@ -287,7 +290,7 @@ impl Url { /// Deserialization impl Url { - /// Parse a URL from `bytes` + /// Parse a URL from `bytes`. pub fn from_bytes(bytes: &BStr) -> Result { parse(bytes) } diff --git a/gix-url/tests/access/mod.rs b/gix-url/tests/access/mod.rs index accfa47c94..3aba542604 100644 --- a/gix-url/tests/access/mod.rs +++ b/gix-url/tests/access/mod.rs @@ -56,7 +56,8 @@ fn password() -> crate::Result { fn user_argument_safe() -> crate::Result { let url = gix_url::parse("ssh://-Fconfigfile@foo/bar".into())?; - // FIXME: Add the critical assertions for the user argument here. + assert_eq!(url.user(), Some("-Fconfigfile")); + // FIXME: Add the critical user_argument_safe assertion here. assert_eq!(url.host(), Some("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); @@ -71,7 +72,8 @@ fn user_argument_safe() -> crate::Result { fn host_argument_safe() -> crate::Result { let url = gix_url::parse("ssh://-oProxyCommand=open$IFS-aCalculator/foo".into())?; - // FIXME: Add assertions for the user argument here. + assert_eq!(url.user(), None); + // FIXME: Add the user_argument_safe assertion here. assert_eq!(url.host(), Some("-oProxyCommand=open$IFS-aCalculator")); assert_eq!(url.host_argument_safe(), None); @@ -86,7 +88,8 @@ fn host_argument_safe() -> crate::Result { fn path_argument_safe() -> crate::Result { let url = gix_url::parse("ssh://foo/-oProxyCommand=open$IFS-aCalculator".into())?; - // FIXME: Add assertions for the user argument here. + assert_eq!(url.user(), None); + // FIXME: Add the user_argument_safe assertion here. assert_eq!(url.host(), Some("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); From db40382328c373258aa3bd5f9551511a42af6be5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Apr 2024 22:38:59 +0000 Subject: [PATCH 03/25] feat: Add `Url::user_argument_safe()` This returns `None` if the username begins with a `-`, which would confuse command-line applications. It is analogous to the `Url::host_argument_safe()` and `Url::path_argument_safe()` methods (introduced in d80b5f6), but for usernames rather than hosts or paths. --- gix-url/src/lib.rs | 14 ++++++++++++++ gix-url/tests/access/mod.rs | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index fba3ffe6d7..f0373c521b 100644 --- a/gix-url/src/lib.rs +++ b/gix-url/src/lib.rs @@ -165,10 +165,24 @@ impl Url { /// Access impl Url { /// Return the user mentioned in the URL, if present. + /// + /// # Security-Warning + /// + /// URLs allow usernames to start with `-` which makes it possible to mask command-line arguments as username which then leads to + /// the invocation of programs from an attacker controlled URL. See for details. + /// + /// If this value is going to be used in a command-line application, call [Self::user_argument_safe()] instead. pub fn user(&self) -> Option<&str> { self.user.as_deref() } + /// Return the user from this URL if present *and* if it can't be mistaken for a command-line argument. + /// + /// Use this method if the user or a portion of the URL that begins with it will be passed to a command-line application. + pub fn user_argument_safe(&self) -> Option<&str> { + self.user().filter(|user| !looks_like_argument(user.as_bytes())) + } + /// Return the password mentioned in the url, if present. pub fn password(&self) -> Option<&str> { self.password.as_deref() diff --git a/gix-url/tests/access/mod.rs b/gix-url/tests/access/mod.rs index 3aba542604..1296db3a21 100644 --- a/gix-url/tests/access/mod.rs +++ b/gix-url/tests/access/mod.rs @@ -57,7 +57,7 @@ fn user_argument_safe() -> crate::Result { let url = gix_url::parse("ssh://-Fconfigfile@foo/bar".into())?; assert_eq!(url.user(), Some("-Fconfigfile")); - // FIXME: Add the critical user_argument_safe assertion here. + assert_eq!(url.user_argument_safe(), None); assert_eq!(url.host(), Some("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); From 169a698ce82df559253c0389a6fa6921a136bc9e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Apr 2024 23:08:39 +0000 Subject: [PATCH 04/25] Expand and introduce tests of more combinations - Add assertions where user_argument_safe returns None, but because there really is no username, rather than it starting with a `-`. - Add tests where more parts are present. (Most existing tests did not have the optional username part. Most also did/do not have the optional password part, but there's no password_argument_safe method, and probably no need for one because a password is not passed explicitly on the command line as an argument or the front of a larger argument--hopefully never, but if so, then it would be an operand argument following an option argument and would be prevented from being interpreted as an option that way.) - Add comments to clarify the assertions that specifically verify that unsafe parts of URLs are rejected when using *_argument_safe methods. --- gix-url/tests/access/mod.rs | 43 ++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/gix-url/tests/access/mod.rs b/gix-url/tests/access/mod.rs index 1296db3a21..a052611b9e 100644 --- a/gix-url/tests/access/mod.rs +++ b/gix-url/tests/access/mod.rs @@ -57,7 +57,7 @@ fn user_argument_safe() -> crate::Result { let url = gix_url::parse("ssh://-Fconfigfile@foo/bar".into())?; assert_eq!(url.user(), Some("-Fconfigfile")); - assert_eq!(url.user_argument_safe(), None); + assert_eq!(url.user_argument_safe(), None); // An unsafe username is blocked. assert_eq!(url.host(), Some("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); @@ -73,10 +73,10 @@ fn host_argument_safe() -> crate::Result { let url = gix_url::parse("ssh://-oProxyCommand=open$IFS-aCalculator/foo".into())?; assert_eq!(url.user(), None); - // FIXME: Add the user_argument_safe assertion here. + assert_eq!(url.user_argument_safe(), None); // As there is no user. See all_argument_safe_valid(). assert_eq!(url.host(), Some("-oProxyCommand=open$IFS-aCalculator")); - assert_eq!(url.host_argument_safe(), None); + assert_eq!(url.host_argument_safe(), None); // An unsafe host string is blocked. assert_eq!(url.path, "/foo"); assert_eq!(url.path_argument_safe(), Some("/foo".into())); @@ -89,13 +89,46 @@ fn path_argument_safe() -> crate::Result { let url = gix_url::parse("ssh://foo/-oProxyCommand=open$IFS-aCalculator".into())?; assert_eq!(url.user(), None); - // FIXME: Add the user_argument_safe assertion here. + assert_eq!(url.user_argument_safe(), None); // As there is no user. See all_argument_safe_valid(). assert_eq!(url.host(), Some("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); assert_eq!(url.path, "/-oProxyCommand=open$IFS-aCalculator"); - assert_eq!(url.path_argument_safe(), None); + assert_eq!(url.path_argument_safe(), None); // An unsafe path is blocked. + + Ok(()) +} + +#[test] +fn all_argument_safe_allowed() -> crate::Result { + let url = gix_url::parse("ssh://user.name@example.com/path/to/file".into())?; + + assert_eq!(url.user(), Some("user.name")); + assert_eq!(url.user_argument_safe(), Some("user.name")); + + assert_eq!(url.host(), Some("example.com")); + assert_eq!(url.host_argument_safe(), Some("example.com")); + + assert_eq!(url.path, "/path/to/file"); + assert_eq!(url.path_argument_safe(), Some("/path/to/file".into())); + + Ok(()) +} + +#[test] +fn all_argument_safe_disallowed() -> crate::Result { + let all_bad = "ssh://-Fconfigfile@-oProxyCommand=open$IFS-aCalculator/-oProxyCommand=open$IFS-aCalculator"; + let url = gix_url::parse(all_bad.into())?; + + assert_eq!(url.user(), Some("-Fconfigfile")); + assert_eq!(url.user_argument_safe(), None); // An unsafe username is blocked. + + assert_eq!(url.host(), Some("-oProxyCommand=open$IFS-aCalculator")); + assert_eq!(url.host_argument_safe(), None); // An unsafe host string is blocked. + + assert_eq!(url.path, "/-oProxyCommand=open$IFS-aCalculator"); + assert_eq!(url.path_argument_safe(), None); // An unsafe path is blocked. Ok(()) } From e8e2463ad1e2c3b9f604b744d554cfa6684eca2f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Apr 2024 23:19:34 +0000 Subject: [PATCH 05/25] Clarify circumstances when *_argument_safe should be used --- gix-url/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index f0373c521b..23b7cf59fb 100644 --- a/gix-url/src/lib.rs +++ b/gix-url/src/lib.rs @@ -90,7 +90,7 @@ pub struct Url { /// URLs allow paths to start with `-` which makes it possible to mask command-line arguments as path which then leads to /// the invocation of programs from an attacker controlled URL. See for details. /// - /// If this value is going to be used in a command-line application, call [Self::path_argument_safe()] instead. + /// If this value is ever going to be passed to a command-line application, call [Self::path_argument_safe()] instead. pub path: BString, } @@ -171,7 +171,7 @@ impl Url { /// URLs allow usernames to start with `-` which makes it possible to mask command-line arguments as username which then leads to /// the invocation of programs from an attacker controlled URL. See for details. /// - /// If this value is going to be used in a command-line application, call [Self::user_argument_safe()] instead. + /// If this value is ever going to be passed to a command-line application, call [Self::user_argument_safe()] instead. pub fn user(&self) -> Option<&str> { self.user.as_deref() } @@ -195,7 +195,7 @@ impl Url { /// URLs allow hosts to start with `-` which makes it possible to mask command-line arguments as host which then leads to /// the invocation of programs from an attacker controlled URL. See for details. /// - /// If this value is going to be used in a command-line application, call [Self::host_argument_safe()] instead. + /// If this value is ever going to be passed to a command-line application, call [Self::host_argument_safe()] instead. pub fn host(&self) -> Option<&str> { self.host.as_deref() } From 03563456f5b8a6bd94d9c571c1623c41363052d6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Apr 2024 23:57:15 +0000 Subject: [PATCH 06/25] Fix ambiguous host journey test snapshot names The committed snapshot out of the output was renamed in f72ecce to fix a spelling error. This updates references to it from gix.sh. --- tests/journey/gix.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/journey/gix.sh b/tests/journey/gix.sh index ce359b134d..5d4e0e2231 100644 --- a/tests/journey/gix.sh +++ b/tests/journey/gix.sh @@ -347,7 +347,7 @@ title "gix commit-graph" ) (with "an ambiguous ssh host which could be mistaken for an argument" it "fails without trying to pass it to command-line programs" && { - WITH_SNAPSHOT="$snapshot/fail-ambigous-host" \ + WITH_SNAPSHOT="$snapshot/fail-ambiguous-host" \ expect_run $WITH_FAILURE "$exe_plumbing" free pack receive 'ssh://-oProxyCommand=open$IFS-aCalculator/foo' } ) @@ -366,7 +366,7 @@ title "gix commit-graph" snapshot="$snapshot/clone" (with "an ambiguous ssh host which could be mistaken for an argument" it "fails without trying to pass it to command-line programs" && { - WITH_SNAPSHOT="$snapshot/fail-ambigous-host" \ + WITH_SNAPSHOT="$snapshot/fail-ambiguous-host" \ expect_run $WITH_FAILURE "$exe_plumbing" clone 'ssh://-oProxyCommand=open$IFS-aCalculator/foo' } ) From 8c94fd4ee2b85898f8998d93e25d45ec1a0fb4b1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 00:35:50 +0000 Subject: [PATCH 07/25] Add ambiguous path journey tests This also adds a snapshot from running them, which shows the expected failure. (However, in this commit the only snapshot that is added is the one that was actually generated on the non-CI test run. This pertains to the `clone` subcommand. Journey tests of the `receive` subcommand are only run on CI, including the new test. It should have a committed snapshot, but that's not included in this commit.) These differ from the ambiguous host tests, in that ambiguous hosts are dangerous in the `gix` command when they appear in an `ssh://` URL (with no `username@` preceding them), while ambiguous paths are instead dangerous in the `gix` command when they appear in a `username@host:path/repo` "URL". The new tests should pass already, and the test of the `clone` subcommand is confirmed to pass (as noted above, journey tests of the `receive` subcommand are only run on CI). This is because ambiguous hosts and ambiguous paths are already handled. In contrast, ambiguous usernames, which have no journey tests yet, are not yet handled in gix-transport. --- tests/journey/gix.sh | 12 ++++++++++++ .../plumbing/no-repo/pack/clone/fail-ambiguous-path | 1 + 2 files changed, 13 insertions(+) create mode 100644 tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-path diff --git a/tests/journey/gix.sh b/tests/journey/gix.sh index 5d4e0e2231..02fe0ddf43 100644 --- a/tests/journey/gix.sh +++ b/tests/journey/gix.sh @@ -351,6 +351,12 @@ title "gix commit-graph" expect_run $WITH_FAILURE "$exe_plumbing" free pack receive 'ssh://-oProxyCommand=open$IFS-aCalculator/foo' } ) + (with "an ambiguous ssh path which could be mistaken for an argument" + it "fails without trying to pass it to command-line programs" && { + WITH_SNAPSHOT="$snapshot/fail-ambiguous-path" \ + expect_run $WITH_FAILURE "$exe_plumbing" free pack receive 'git@foo:-oProxyCommand=open$IFS-aCalculator/bar' + } + ) fi ) elif [[ "$kind" = "small" ]]; then @@ -370,6 +376,12 @@ title "gix commit-graph" expect_run $WITH_FAILURE "$exe_plumbing" clone 'ssh://-oProxyCommand=open$IFS-aCalculator/foo' } ) + (with "an ambiguous ssh path which could be mistaken for an argument" + it "fails without trying to pass it to command-line programs" && { + WITH_SNAPSHOT="$snapshot/fail-ambiguous-path" \ + expect_run $WITH_FAILURE "$exe_plumbing" clone 'git@foo:-oProxyCommand=open$IFS-aCalculator/bar' + } + ) ) fi (with "the 'index' sub-command" diff --git a/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-path b/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-path new file mode 100644 index 0000000000..5ce09d4e1a --- /dev/null +++ b/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-path @@ -0,0 +1 @@ + Error: The repository path '-oProxyCommand=open$IFS-aCalculator/bar' could be mistaken for a command-line argument \ No newline at end of file From a38f646eddf0cf0f3fde0ad68a15a8481ad7fd4f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 02:16:59 +0000 Subject: [PATCH 08/25] Add expected snapshot for ambiguous path `receive` This test should currently pass, but it, like the other journey tests of the `receive` subcommand, runs only on CI. This is an expected snapshot, so that if the test, code under test, or expectation expressed in the snapshot is wrong, then it will be revealed that there is a problem. This also expresses the expectation to humans, which may help in fixing the new test if it turns out to be wrong. Note however that this does mean that the journey snapshot here is not strictly being used for approval testing. --- .../snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-path | 1 + 1 file changed, 1 insertion(+) create mode 100644 tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-path diff --git a/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-path b/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-path new file mode 100644 index 0000000000..2d3c844e17 --- /dev/null +++ b/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-path @@ -0,0 +1 @@ +Error: The repository path '-oProxyCommand=open$IFS-aCalculator/bar' could be mistaken for a command-line argument \ No newline at end of file From d639b3b0bea7c0426e59e289dc85abe85837590e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 01:09:38 +0000 Subject: [PATCH 09/25] Add ambiguous username journey tests These should not pass yet, because gix-transport and the gix binary do not yet have special handling to reject ambiguous usernames that can be interpreted as command-line options to ssh clients. These new journey tests may need further refinement. --- tests/journey/gix.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/journey/gix.sh b/tests/journey/gix.sh index 02fe0ddf43..6af7cb507a 100644 --- a/tests/journey/gix.sh +++ b/tests/journey/gix.sh @@ -345,6 +345,21 @@ title "gix commit-graph" } ) ) + (with "an ambiguous ssh username which could be mistaken for an argument" + snapshot="$snapshot/fail-ambiguous-username" + (with "explicit ssh (true url with scheme)" + it "fails without trying to pass it to command-line programs" && { + WITH_SNAPSHOT="$snapshot/explicit-ssh" \ + expect_run $WITH_FAILURE "$exe_plumbing" free pack receive 'ssh://-Fconfigfile@foo/bar' + } + ) + (with "implicit ssh (special syntax with no scheme)" + it "fails without trying to pass it to command-line programs" && { + WITH_SNAPSHOT="$snapshot/implicit-ssh" \ + expect_run $WITH_FAILURE "$exe_plumbing" free pack receive -- '-Fconfigfile@foo:bar/baz' + } + ) + ) (with "an ambiguous ssh host which could be mistaken for an argument" it "fails without trying to pass it to command-line programs" && { WITH_SNAPSHOT="$snapshot/fail-ambiguous-host" \ @@ -370,6 +385,21 @@ title "gix commit-graph" if test "$kind" = "max" || test "$kind" = "max-pure"; then (with "the 'clone' sub-command" snapshot="$snapshot/clone" + (with "an ambiguous ssh username which could be mistaken for an argument" + snapshot="$snapshot/fail-ambiguous-username" + (with "explicit ssh (true url with scheme)" + it "fails without trying to pass it to command-line programs" && { + WITH_SNAPSHOT="$snapshot/explicit-ssh" \ + expect_run $WITH_FAILURE "$exe_plumbing" clone 'ssh://-Fconfigfile@foo/bar' + } + ) + (with "implicit ssh (special syntax with no scheme)" + it "fails without trying to pass it to command-line programs" && { + WITH_SNAPSHOT="$snapshot/implicit-ssh" \ + expect_run $WITH_FAILURE "$exe_plumbing" clone -- '-Fconfigfile@foo:bar/baz' + } + ) + ) (with "an ambiguous ssh host which could be mistaken for an argument" it "fails without trying to pass it to command-line programs" && { WITH_SNAPSHOT="$snapshot/fail-ambiguous-host" \ From a51003d2bdb21b42963a46e20f513ed8c2639a2e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 02:30:11 +0000 Subject: [PATCH 10/25] Add expected snapshots for ambiguous username journey tests The tests appear to work, at least partially, in that a test failure, of the kind expected due to gix-transport not yet checking for usernames that ssh clients can interpret as an option argument, is observed now that the current output has snapshots to compare to (the comparison fails, which at this point is intended). Not all tests ran, because of the runner script's fail-fast behavior and because the `receive` subcommand tests only run on CI. While I believe having these snapshots in now will make it eaiser to ensure nothing essential was forgotten, once ambiguous username handling is implemented, this does make these journey tests no longer strictly approval tests (since the snapshots incorporate foreknowledge of the intended behavior). --- .../no-repo/pack/clone/fail-ambiguous-username/explicit-ssh | 1 + .../no-repo/pack/clone/fail-ambiguous-username/implicit-ssh | 1 + .../no-repo/pack/receive/fail-ambiguous-username/explicit-ssh | 1 + .../no-repo/pack/receive/fail-ambiguous-username/implicit-ssh | 1 + 4 files changed, 4 insertions(+) create mode 100644 tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-username/explicit-ssh create mode 100644 tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-username/implicit-ssh create mode 100644 tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-username/explicit-ssh create mode 100644 tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-username/implicit-ssh diff --git a/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-username/explicit-ssh b/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-username/explicit-ssh new file mode 100644 index 0000000000..78d252596a --- /dev/null +++ b/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-username/explicit-ssh @@ -0,0 +1 @@ + Error: Username '-Fconfigfile' could be mistaken for a command-line argument \ No newline at end of file diff --git a/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-username/implicit-ssh b/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-username/implicit-ssh new file mode 100644 index 0000000000..78d252596a --- /dev/null +++ b/tests/snapshots/plumbing/no-repo/pack/clone/fail-ambiguous-username/implicit-ssh @@ -0,0 +1 @@ + Error: Username '-Fconfigfile' could be mistaken for a command-line argument \ No newline at end of file diff --git a/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-username/explicit-ssh b/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-username/explicit-ssh new file mode 100644 index 0000000000..8a9fca8771 --- /dev/null +++ b/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-username/explicit-ssh @@ -0,0 +1 @@ +Error: Username '-Fconfigfile' could be mistaken for a command-line argument \ No newline at end of file diff --git a/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-username/implicit-ssh b/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-username/implicit-ssh new file mode 100644 index 0000000000..8a9fca8771 --- /dev/null +++ b/tests/snapshots/plumbing/no-repo/pack/receive/fail-ambiguous-username/implicit-ssh @@ -0,0 +1 @@ +Error: Username '-Fconfigfile' could be mistaken for a command-line argument \ No newline at end of file From 54286091ebc6e13a8f27f730fa88127e6334cf13 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 04:13:34 +0000 Subject: [PATCH 11/25] Add ambiguous user unit tests, and more for hostname Not all of these tests can pass yet, since gix-transport does not yet detect and refuse to proceed with leading-hypnen usernames. Some pass; those that do not are, as expected: - ambiguous_user_is_disallowed_explicit_ssh - ambiguous_user_is_disallowed_implicit_ssh - ambiguous_user_and_host_remain_disallowed_together_explicit_ssh - ambiguous_user_and_host_remain_disallowed_together_implicit_ssh This also adds AmbiguousUserName in one of the enums that will need to have it, but nothing fails with this error yet; it is introduced now only to facilitate writing unit tests that assert it. --- .../src/client/blocking_io/ssh/mod.rs | 2 + .../src/client/blocking_io/ssh/tests.rs | 45 ++++++++++++++++++- 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 16f47bd25f..00e06582d7 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/gix-transport/src/client/blocking_io/ssh/mod.rs @@ -42,6 +42,8 @@ pub mod invocation { #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { + #[error("Username '{user}' could be mistaken for a command-line argument")] + AmbiguousUserName { user: String }, #[error("Host name '{host}' could be mistaken for a command-line argument")] AmbiguousHostName { host: String }, #[error("The 'Simple' ssh variant doesn't support {function}")] diff --git a/gix-transport/src/client/blocking_io/ssh/tests.rs b/gix-transport/src/client/blocking_io/ssh/tests.rs index 4e4da78070..8fa19d0bb7 100644 --- a/gix-transport/src/client/blocking_io/ssh/tests.rs +++ b/gix-transport/src/client/blocking_io/ssh/tests.rs @@ -144,8 +144,25 @@ mod program_kind { assert!(call_args(kind, "ssh://user@host:43/p", Protocol::V2).ends_with("-P 43 user@host")); } } + #[test] - fn ambiguous_host_is_allowed_with_user() { + fn ambiguous_user_is_disallowed_explicit_ssh() { + assert!(matches!( + try_call(ProgramKind::Ssh, "ssh://-arg@host/p", Protocol::V2), + Err(ssh::invocation::Error::AmbiguousUserName { user }) if user == "-arg" + )) + } + + #[test] + fn ambiguous_user_is_disallowed_implicit_ssh() { + assert!(matches!( + try_call(ProgramKind::Ssh, "-arg@host:p/q", Protocol::V2), + Err(ssh::invocation::Error::AmbiguousUserName { user }) if user == "-arg" + )) + } + + #[test] + fn ambiguous_host_is_allowed_with_user_explicit_ssh() { assert_eq!( call_args(ProgramKind::Ssh, "ssh://user@-arg/p", Protocol::V2), joined(&["ssh", "-o", "SendEnv=GIT_PROTOCOL", "user@-arg"]) @@ -153,13 +170,37 @@ mod program_kind { } #[test] - fn ambiguous_host_is_disallowed() { + fn ambiguous_host_is_allowed_with_user_implicit_ssh() { + assert_eq!( + call_args(ProgramKind::Ssh, "user@-arg:p/q", Protocol::V2), + joined(&["ssh", "-o", "SendEnv=GIT_PROTOCOL", "user@-arg"]) + ); + } + + #[test] + fn ambiguous_host_is_disallowed_without_user() { assert!(matches!( try_call(ProgramKind::Ssh, "ssh://-arg/p", Protocol::V2), Err(ssh::invocation::Error::AmbiguousHostName { host }) if host == "-arg" )); } + #[test] + fn ambiguous_user_and_host_remain_disallowed_together_explicit_ssh() { + assert!(matches!( + try_call(ProgramKind::Ssh, "ssh://-arg@host/p", Protocol::V2), + Err(ssh::invocation::Error::AmbiguousUserName { user }) if user == "-arg" + )); + } + + #[test] + fn ambiguous_user_and_host_remain_disallowed_together_implicit_ssh() { + assert!(matches!( + try_call(ProgramKind::Ssh, "-userarg@-hostarg:p/q", Protocol::V2), + Err(ssh::invocation::Error::AmbiguousUserName { user }) if user == "-userarg" + )); + } + #[test] fn simple_cannot_handle_any_arguments() { assert!(matches!( From f56ad390a5569d0129b7b16632991d18b9ddb4f7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 06:38:19 +0000 Subject: [PATCH 12/25] fix: Prevent usernames with leading `-` from being passed to SSH This detects ambiguous usernames in dangerous cases where they would be passed to external commands to form SSH connections, if they would be misinterpreted as option arguments. This change is analogous to b06a0dd, hardening `gix-transport` and applications that use it against options smuggled in URLs, but for the non-mandatory username portion of a URL, rather than the host and path portions that were covered there. For example, commands like these no longer pass `-F...` options to `ssh`: gix clone 'ssh://-Fconfigfile@example.com/abc' gix clone -- '-Fconfigfile@example.com:abc/def' Instead, they refuse to run `ssh`, producing the error: Error: Username '-Fconfigfile' could be mistaken for a command-line argument --- .../src/client/blocking_io/ssh/program_kind.rs | 13 ++++++++++--- gix-url/src/lib.rs | 7 +++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/gix-transport/src/client/blocking_io/ssh/program_kind.rs b/gix-transport/src/client/blocking_io/ssh/program_kind.rs index 70905829f6..d046db772c 100644 --- a/gix-transport/src/client/blocking_io/ssh/program_kind.rs +++ b/gix-transport/src/client/blocking_io/ssh/program_kind.rs @@ -60,8 +60,12 @@ impl ProgramKind { } } }; - let host_as_ssh_arg = match url.user() { + let host_maybe_with_user_as_ssh_arg = match url.user() { Some(user) => { + // FIXME: See the fixme comment on Url::user_argument_safe() about its return type. + if url.user_argument_safe() != Some(user) { + return Err(ssh::invocation::Error::AmbiguousUserName { user: user.into() }); + } let host = url.host().expect("present in ssh urls"); format!("{user}@{host}") } @@ -75,8 +79,11 @@ impl ProgramKind { } }; - // Try to force ssh to yield english messages (for parsing later) - Ok(prepare.arg(host_as_ssh_arg).env("LANG", "C").env("LC_ALL", "C")) + // Try to force ssh to yield English messages (for parsing later). + Ok(prepare + .arg(host_maybe_with_user_as_ssh_arg) + .env("LANG", "C") + .env("LC_ALL", "C")) } /// Note that the caller has to assure that the ssh program is launched in English by setting the locale. diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index 23b7cf59fb..ff6d5a12f5 100644 --- a/gix-url/src/lib.rs +++ b/gix-url/src/lib.rs @@ -180,6 +180,13 @@ impl Url { /// /// Use this method if the user or a portion of the URL that begins with it will be passed to a command-line application. pub fn user_argument_safe(&self) -> Option<&str> { + // FIXME: A return value of None from this method, or host_argument_safe(), is ambiguous: the user (or host) is + // either present but unsafe, or absent. Furthermore, in practice the value is usually needed even if unsafe, + // in order to report it in an error message. In gix-transport, the ambiguity makes it easy to write a new bug + // while using this interface for user_argument_safe(). In contrast, in host_argument_safe(), the ambiguity is + // much less of a problem, because the host is expected to be present. Yet the host() method must still be + // called when handling the None case, to include it in the error. If possible, both methods should be replaced + // by methods with a richer return type (a new enum). If not, the ambiguity should be prominently documented. self.user().filter(|user| !looks_like_argument(user.as_bytes())) } From 2e7517e964af0a0d74e05049db6bcd2527199cb3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 07:58:58 +0000 Subject: [PATCH 13/25] Comment gix_transport::client::blocking_io::ssh::connect To note that if the `-G` check ever also uses the username, then that has to be checked to ensure it is safe. --- gix-transport/src/client/blocking_io/ssh/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 00e06582d7..7d179951f6 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/gix-transport/src/client/blocking_io/ssh/mod.rs @@ -118,6 +118,8 @@ pub fn connect( .stdin(Stdio::null()) .with_shell() .arg("-G") + // Username affects the stdout from `ssh -G` but may not affect the status. But if + // we end up needing it, it can be added here, with a user_argument_safe() check. .arg(url.host_argument_safe().ok_or_else(|| Error::AmbiguousHostName { host: url.host().expect("set in ssh urls").into(), })?), From beef8d2ce20af8b97fcd85c185cf9373d4897cdb Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 18:06:38 +0000 Subject: [PATCH 14/25] Give `looks_like_argument` a more accurate name This renames it to `looks_like_command_line_option`, because both options and non-option arguments are arguments. The condition it checks for is that something looks like an option and therefore should *not* be used as a command-line argument. --- gix-url/src/lib.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index ff6d5a12f5..55967a4e34 100644 --- a/gix-url/src/lib.rs +++ b/gix-url/src/lib.rs @@ -187,7 +187,8 @@ impl Url { // much less of a problem, because the host is expected to be present. Yet the host() method must still be // called when handling the None case, to include it in the error. If possible, both methods should be replaced // by methods with a richer return type (a new enum). If not, the ambiguity should be prominently documented. - self.user().filter(|user| !looks_like_argument(user.as_bytes())) + self.user() + .filter(|user| !looks_like_command_line_option(user.as_bytes())) } /// Return the password mentioned in the url, if present. @@ -211,7 +212,8 @@ impl Url { /// /// Use this method if the host is going to be passed to a command-line application. pub fn host_argument_safe(&self) -> Option<&str> { - self.host().filter(|host| !looks_like_argument(host.as_bytes())) + self.host() + .filter(|host| !looks_like_command_line_option(host.as_bytes())) } /// Return the path of this URL *if* it can't be mistaken for a command-line argument. @@ -221,7 +223,7 @@ impl Url { pub fn path_argument_safe(&self) -> Option<&BStr> { self.path .get(1..) - .and_then(|truncated| (!looks_like_argument(truncated)).then_some(self.path.as_ref())) + .and_then(|truncated| (!looks_like_command_line_option(truncated)).then_some(self.path.as_ref())) } /// Return true if the path portion of the URL is `/`. @@ -245,7 +247,7 @@ impl Url { } } -fn looks_like_argument(b: &[u8]) -> bool { +fn looks_like_command_line_option(b: &[u8]) -> bool { b.first() == Some(&b'-') } From 29941e66b367ff1eac448ec4024b1a17246ffaac Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 18:12:15 +0000 Subject: [PATCH 15/25] Sketch *_as_argument methods and supporting enum This isn't correct yet, because: - It doesn't compile because of the overcomplicated way I'm doing borrowing. The enum can just have references as type arguments instead. - The enum should be public. (Those are noted here rather than in the next commit that will fix them, because it is not useful to include them in a message generated from it if it is titled as a conventional commit.) In addition, while some existing methods are implemented in terms of these now so there is some test coverage that way, it's far from complete and these really need their own tests. --- gix-url/src/lib.rs | 104 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 84 insertions(+), 20 deletions(-) diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index 55967a4e34..299c181647 100644 --- a/gix-url/src/lib.rs +++ b/gix-url/src/lib.rs @@ -55,6 +55,26 @@ pub fn expand_path(user: Option<&expand_path::ForUser>, path: &BStr) -> Result

for details. +/// +/// # Security Warning +/// +/// This type only expresses known *syntactic* risk. It does not cover other risks, such as passing a personal access +/// token as a username rather than a password in an application that logs usernames. +enum ArgumentSafety<'a, T> { + /// May be safe. There is nothing to pass, so there is nothing dangerous. + Absent, + /// May be safe. The argument does not begin with a `-` and so will not be confused as an option. + Usable(&'a T), + /// Dangerous! Begins with `-` and could be treated as an option. Use the value in error messages only. + Dangerous(&'a T), +} + /// A URL with support for specialized git related capabilities. /// /// Additionally there is support for [deserialization](Url::from_bytes()) and [serialization](Url::to_bstring()). @@ -85,7 +105,7 @@ pub struct Url { pub port: Option, /// The path portion of the URL, usually the location of the git repository. /// - /// # Security-Warning + /// # Security Warning /// /// URLs allow paths to start with `-` which makes it possible to mask command-line arguments as path which then leads to /// the invocation of programs from an attacker controlled URL. See for details. @@ -164,9 +184,9 @@ impl Url { /// Access impl Url { - /// Return the user mentioned in the URL, if present. + /// Return the username mentioned in the URL, if present. /// - /// # Security-Warning + /// # Security Warning /// /// URLs allow usernames to start with `-` which makes it possible to mask command-line arguments as username which then leads to /// the invocation of programs from an attacker controlled URL. See for details. @@ -176,19 +196,28 @@ impl Url { self.user.as_deref() } - /// Return the user from this URL if present *and* if it can't be mistaken for a command-line argument. + /// Classify the username of this URL by whether it is safe to pass as a command-line argument. /// - /// Use this method if the user or a portion of the URL that begins with it will be passed to a command-line application. + /// Use this method instead of [Self::user()] if the host is going to be passed to a command-line application. + /// If the unsafe and absent cases need not be distinguished, [Self::user_argument_safe()] may also be used. + pub fn user_as_argument(&self) -> ArgumentSafety { + match self.user() { + Some(user) if looks_like_command_line_option(user.as_bytes()) => ArgumentSafety::Dangerous(user), + Some(user) => ArgumentSafety::Usable(user), + None => ArgumentSafety::Absent, + } + } + + /// Return the username of this URL if present *and* if it can't be mistaken for a command-line argument. + /// + /// Use this method or [Self::user_as_argument()] instead of [Self::user()] if the host is going to be + /// passed to a command-line application. Prefer [Self::user_as_argument()] unless the unsafe and absent + /// cases need not be distinguished from each other. pub fn user_argument_safe(&self) -> Option<&str> { - // FIXME: A return value of None from this method, or host_argument_safe(), is ambiguous: the user (or host) is - // either present but unsafe, or absent. Furthermore, in practice the value is usually needed even if unsafe, - // in order to report it in an error message. In gix-transport, the ambiguity makes it easy to write a new bug - // while using this interface for user_argument_safe(). In contrast, in host_argument_safe(), the ambiguity is - // much less of a problem, because the host is expected to be present. Yet the host() method must still be - // called when handling the None case, to include it in the error. If possible, both methods should be replaced - // by methods with a richer return type (a new enum). If not, the ambiguity should be prominently documented. - self.user() - .filter(|user| !looks_like_command_line_option(user.as_bytes())) + match self.user_as_argument() { + ArgumentSafety::Usable(user) => Some(user), + _ => None, + } } /// Return the password mentioned in the url, if present. @@ -198,28 +227,63 @@ impl Url { /// Return the host mentioned in the URL, if present. /// - /// # Security-Warning + /// # Security Warning /// /// URLs allow hosts to start with `-` which makes it possible to mask command-line arguments as host which then leads to /// the invocation of programs from an attacker controlled URL. See for details. /// - /// If this value is ever going to be passed to a command-line application, call [Self::host_argument_safe()] instead. + /// If this value is ever going to be passed to a command-line application, call [Self::host_as_argument()] + /// or [Self::host_argument_safe()] instead. pub fn host(&self) -> Option<&str> { self.host.as_deref() } + /// Classify the host of this URL by whether it is safe to pass as a command-line argument. + /// + /// Use this method instead of [Self::host()] if the host is going to be passed to a command-line application. + /// If the unsafe and absent cases need not be distinguished, [Self::host_argument_safe()] may also be used. + pub fn host_as_argument(&self) -> ArgumentSafety { + match self.host() { + Some(host) if looks_like_command_line_option(host.as_bytes()) => ArgumentSafety::Dangerous(host), + Some(host) => ArgumentSafety::Usable(host), + None => ArgumentSafety::Absent, + } + } + /// Return the host of this URL if present *and* if it can't be mistaken for a command-line argument. /// - /// Use this method if the host is going to be passed to a command-line application. + /// Use this method or [Self::host_as_argument()] instead of [Self::host()] if the host is going to be + /// passed to a command-line application. Prefer [Self::host_as_argument()] unless the unsafe and absent + /// cases need not be distinguished from each other. pub fn host_argument_safe(&self) -> Option<&str> { - self.host() - .filter(|host| !looks_like_command_line_option(host.as_bytes())) + match self.host_as_argument() { + ArgumentSafety::Usable(host) => Some(host), + _ => None, + } + } + + /// Classify the path of this URL by whether it is safe to pass as a command-line argument. + /// Note that it always begins with a slash, which is ignored for this classification. + /// + /// Use this method or [Self::path_argument_safe()] instead of [Self::path] if the host is going to be + /// passed to a command-line application, unless it is certain that the leading `/` will always be included. + /// + /// This method never returns an [ArgumentSafety::Absent]. + pub fn path_as_argument(&self) -> ArgumentSafety { + match self.path_argument_safe() { + Some(path) => ArgumentSafety::Usable(path), + None => ArgumentSafety::Dangerous(self.path.as_ref()), + } } /// Return the path of this URL *if* it can't be mistaken for a command-line argument. /// Note that it always begins with a slash, which is ignored for this comparison. /// - /// Use this method if the path is going to be passed to a command-line application. + /// Use this method instead of accessing [Self::path] directly if the path is going to be passed to a + /// command-line application, unless it is certain that the leading `/` will always be included. + /// + /// The result of this method is unambiguous because [Self::path] is not an option type, but + /// [Self::path_as_argument()] may also safely be used. pub fn path_argument_safe(&self) -> Option<&BStr> { self.path .get(1..) From 545799882643532dd0b3e8ab436efd7722c74e3c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 18:27:52 +0000 Subject: [PATCH 16/25] feat: Add `ArgumentSafety` and `Url::*_as_argument()` methods This adds three methods to `Url`: - `Url::user_as_argument` - `Url::host_as_argument` - `Url::path_as_argument` They return `ArgumentSafety` values that distingiush three cases: 1. There is no wrapped value (`Absent`). 2. The wrapped value is usable and presumably safe (`Usable`). 3. The wrapped value is dangerous and should not be passed as a command-line argument because it could be interpreted as an option due to starting with `-`. The value itself may still be useful and safe to include in error messages. `user_as_argument` and `host_as_argument` are the most useful ones, as they serve as alternatives to `user_argument_safe` and `host_argument_safe` whose return values are unambiguous. The `user_argument_safe` and `host_argument_safe` methods don't distinguish between the absence of a username or host, and the presence of a username or host that is unsafe to pass as an argument. `path_as_argument` included for parity, in case it is useful to write code that handles the three cases similarly. However, there is no underlying ambiguity in the return value of the corresponding `path_argument_safe` method, since unlike `user` and `host`, the `path` is not an option type. --- gix-url/src/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index 299c181647..0431f6bab3 100644 --- a/gix-url/src/lib.rs +++ b/gix-url/src/lib.rs @@ -66,13 +66,13 @@ pub fn expand_path(user: Option<&expand_path::ForUser>, path: &BStr) -> Result

{ +pub enum ArgumentSafety { /// May be safe. There is nothing to pass, so there is nothing dangerous. Absent, /// May be safe. The argument does not begin with a `-` and so will not be confused as an option. - Usable(&'a T), + Usable(T), /// Dangerous! Begins with `-` and could be treated as an option. Use the value in error messages only. - Dangerous(&'a T), + Dangerous(T), } /// A URL with support for specialized git related capabilities. @@ -200,7 +200,7 @@ impl Url { /// /// Use this method instead of [Self::user()] if the host is going to be passed to a command-line application. /// If the unsafe and absent cases need not be distinguished, [Self::user_argument_safe()] may also be used. - pub fn user_as_argument(&self) -> ArgumentSafety { + pub fn user_as_argument(&self) -> ArgumentSafety<&str> { match self.user() { Some(user) if looks_like_command_line_option(user.as_bytes()) => ArgumentSafety::Dangerous(user), Some(user) => ArgumentSafety::Usable(user), @@ -242,7 +242,7 @@ impl Url { /// /// Use this method instead of [Self::host()] if the host is going to be passed to a command-line application. /// If the unsafe and absent cases need not be distinguished, [Self::host_argument_safe()] may also be used. - pub fn host_as_argument(&self) -> ArgumentSafety { + pub fn host_as_argument(&self) -> ArgumentSafety<&str> { match self.host() { Some(host) if looks_like_command_line_option(host.as_bytes()) => ArgumentSafety::Dangerous(host), Some(host) => ArgumentSafety::Usable(host), @@ -269,7 +269,7 @@ impl Url { /// passed to a command-line application, unless it is certain that the leading `/` will always be included. /// /// This method never returns an [ArgumentSafety::Absent]. - pub fn path_as_argument(&self) -> ArgumentSafety { + pub fn path_as_argument(&self) -> ArgumentSafety<&BStr> { match self.path_argument_safe() { Some(path) => ArgumentSafety::Usable(path), None => ArgumentSafety::Dangerous(self.path.as_ref()), From 1b0af07ffa122a2199d444b84512300ef578abb5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 19:30:32 +0000 Subject: [PATCH 17/25] Give `ArgumentSafety` traits; test `*_as_argument` methods The three new `Url::*_as_argument` methods were only tested indirectly before (and the path one not at all). This adds tests for them so they have the same direct coverage as the corresponding `*_argument_safe` methods. The `Debug` and `PartialEq` traits added to `ArgumentSafety` are primarily added to allow the new assertions to be written in a more streamlined way (without matching). --- gix-url/src/lib.rs | 1 + gix-url/tests/access/mod.rs | 39 ++++++++++++++++++++++++++++++++----- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index 0431f6bab3..dd424a43a4 100644 --- a/gix-url/src/lib.rs +++ b/gix-url/src/lib.rs @@ -66,6 +66,7 @@ pub fn expand_path(user: Option<&expand_path::ForUser>, path: &BStr) -> Result

{ /// May be safe. There is nothing to pass, so there is nothing dangerous. Absent, diff --git a/gix-url/tests/access/mod.rs b/gix-url/tests/access/mod.rs index a052611b9e..5b76589980 100644 --- a/gix-url/tests/access/mod.rs +++ b/gix-url/tests/access/mod.rs @@ -30,6 +30,8 @@ mod canonicalized { } } +use gix_url::ArgumentSafety; + #[test] fn user() -> crate::Result { let mut url = gix_url::parse("https://user:password@host/path".into())?; @@ -53,81 +55,108 @@ fn password() -> crate::Result { } #[test] -fn user_argument_safe() -> crate::Result { +fn user_argument_safety() -> crate::Result { let url = gix_url::parse("ssh://-Fconfigfile@foo/bar".into())?; assert_eq!(url.user(), Some("-Fconfigfile")); + assert_eq!(url.user_as_argument(), ArgumentSafety::Dangerous("-Fconfigfile")); assert_eq!(url.user_argument_safe(), None); // An unsafe username is blocked. assert_eq!(url.host(), Some("foo")); + assert_eq!(url.host_as_argument(), ArgumentSafety::Usable("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); assert_eq!(url.path, "/bar"); + assert_eq!(url.path_as_argument(), ArgumentSafety::Usable("/bar".into())); assert_eq!(url.path_argument_safe(), Some("/bar".into())); Ok(()) } #[test] -fn host_argument_safe() -> crate::Result { +fn host_argument_safety() -> crate::Result { let url = gix_url::parse("ssh://-oProxyCommand=open$IFS-aCalculator/foo".into())?; assert_eq!(url.user(), None); + assert_eq!(url.user_as_argument(), ArgumentSafety::Absent); assert_eq!(url.user_argument_safe(), None); // As there is no user. See all_argument_safe_valid(). assert_eq!(url.host(), Some("-oProxyCommand=open$IFS-aCalculator")); + assert_eq!( + url.host_as_argument(), + ArgumentSafety::Dangerous("-oProxyCommand=open$IFS-aCalculator") + ); assert_eq!(url.host_argument_safe(), None); // An unsafe host string is blocked. assert_eq!(url.path, "/foo"); + assert_eq!(url.path_as_argument(), ArgumentSafety::Usable("/foo".into())); assert_eq!(url.path_argument_safe(), Some("/foo".into())); Ok(()) } #[test] -fn path_argument_safe() -> crate::Result { +fn path_argument_safety() -> crate::Result { let url = gix_url::parse("ssh://foo/-oProxyCommand=open$IFS-aCalculator".into())?; assert_eq!(url.user(), None); + assert_eq!(url.user_as_argument(), ArgumentSafety::Absent); assert_eq!(url.user_argument_safe(), None); // As there is no user. See all_argument_safe_valid(). assert_eq!(url.host(), Some("foo")); + assert_eq!(url.host_as_argument(), ArgumentSafety::Usable("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); assert_eq!(url.path, "/-oProxyCommand=open$IFS-aCalculator"); + assert_eq!( + url.path_as_argument(), + ArgumentSafety::Dangerous("/-oProxyCommand=open$IFS-aCalculator".into()) + ); assert_eq!(url.path_argument_safe(), None); // An unsafe path is blocked. Ok(()) } #[test] -fn all_argument_safe_allowed() -> crate::Result { +fn all_argument_safety_safe() -> crate::Result { let url = gix_url::parse("ssh://user.name@example.com/path/to/file".into())?; assert_eq!(url.user(), Some("user.name")); + assert_eq!(url.user_as_argument(), ArgumentSafety::Usable("user.name")); assert_eq!(url.user_argument_safe(), Some("user.name")); assert_eq!(url.host(), Some("example.com")); + assert_eq!(url.host_as_argument(), ArgumentSafety::Usable("example.com")); assert_eq!(url.host_argument_safe(), Some("example.com")); assert_eq!(url.path, "/path/to/file"); + assert_eq!(url.path_as_argument(), ArgumentSafety::Usable("/path/to/file".into())); assert_eq!(url.path_argument_safe(), Some("/path/to/file".into())); Ok(()) } #[test] -fn all_argument_safe_disallowed() -> crate::Result { +fn all_argument_safety_not_safe() -> crate::Result { let all_bad = "ssh://-Fconfigfile@-oProxyCommand=open$IFS-aCalculator/-oProxyCommand=open$IFS-aCalculator"; let url = gix_url::parse(all_bad.into())?; assert_eq!(url.user(), Some("-Fconfigfile")); + assert_eq!(url.user_as_argument(), ArgumentSafety::Dangerous("-Fconfigfile")); assert_eq!(url.user_argument_safe(), None); // An unsafe username is blocked. assert_eq!(url.host(), Some("-oProxyCommand=open$IFS-aCalculator")); + assert_eq!( + url.host_as_argument(), + ArgumentSafety::Dangerous("-oProxyCommand=open$IFS-aCalculator") + ); assert_eq!(url.host_argument_safe(), None); // An unsafe host string is blocked. assert_eq!(url.path, "/-oProxyCommand=open$IFS-aCalculator"); + assert_eq!( + url.path_as_argument(), + ArgumentSafety::Dangerous("/-oProxyCommand=open$IFS-aCalculator".into()) + ); assert_eq!(url.path_argument_safe(), None); // An unsafe path is blocked. Ok(()) From 4dda375cb1e2b4c7bc656979ac718f919c391b94 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 20:09:25 +0000 Subject: [PATCH 18/25] Start on using {user,host}_as_argument in prepare_invocation Right now this is more locked down than necessary, serving to check that ArgumentSafety can be easily matched on in scenarios where one may wish to reject both usernames and hosts that are ambiguous. This is verified by all tests passing except these that fail: - ambiguous_host_is_allowed_with_user_explicit_ssh - ambiguous_host_is_allowed_with_user_implicit_ssh Prohibiting ambiguous hosts even when the username is present and cannot itself be confused with a command-line option isn't needed in gix-transport, which passes URLs of the form `user@host` anytime the username is present. However, if an application ever passes the username as a separate argument rather than as part of the netloc with the host, then being able to express that in a natural and non-error-prone way is important. --- .../client/blocking_io/ssh/program_kind.rs | 26 +++++++------------ 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/gix-transport/src/client/blocking_io/ssh/program_kind.rs b/gix-transport/src/client/blocking_io/ssh/program_kind.rs index d046db772c..e3a2575c45 100644 --- a/gix-transport/src/client/blocking_io/ssh/program_kind.rs +++ b/gix-transport/src/client/blocking_io/ssh/program_kind.rs @@ -2,6 +2,8 @@ use std::{ffi::OsStr, io::ErrorKind}; use bstr::{BString, ByteSlice, ByteVec}; +use gix_url::ArgumentSafety::*; + use crate::{ client::{ssh, ssh::ProgramKind}, Protocol, @@ -60,23 +62,13 @@ impl ProgramKind { } } }; - let host_maybe_with_user_as_ssh_arg = match url.user() { - Some(user) => { - // FIXME: See the fixme comment on Url::user_argument_safe() about its return type. - if url.user_argument_safe() != Some(user) { - return Err(ssh::invocation::Error::AmbiguousUserName { user: user.into() }); - } - let host = url.host().expect("present in ssh urls"); - format!("{user}@{host}") - } - None => { - let host = url - .host_argument_safe() - .ok_or_else(|| ssh::invocation::Error::AmbiguousHostName { - host: url.host().expect("ssh host always set").into(), - })?; - host.into() - } + + let host_maybe_with_user_as_ssh_arg = match (url.user_as_argument(), url.host_as_argument()) { + (Usable(user), Usable(host)) => format!("{user}@{host}"), + (Absent, Usable(host)) => host.into(), + (Dangerous(user), _) => Err(ssh::invocation::Error::AmbiguousUserName { user: user.into() })?, + (_, Dangerous(host)) => Err(ssh::invocation::Error::AmbiguousHostName { host: host.into() })?, + (_, Absent) => panic!("BUG: host should always be present in SSH URLs"), }; // Try to force ssh to yield English messages (for parsing later). From 29116236de3ca0ee97f60f1ad4024f74490bb2cd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 20:44:44 +0000 Subject: [PATCH 19/25] Reallow `user@-arg...` in prepare_invocation Since the `user@` prefix makes it safe as gix-transport uses it, and there are test cases specifying that it should be permitted. --- gix-transport/src/client/blocking_io/ssh/program_kind.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/gix-transport/src/client/blocking_io/ssh/program_kind.rs b/gix-transport/src/client/blocking_io/ssh/program_kind.rs index e3a2575c45..7c6f5c8a15 100644 --- a/gix-transport/src/client/blocking_io/ssh/program_kind.rs +++ b/gix-transport/src/client/blocking_io/ssh/program_kind.rs @@ -65,6 +65,7 @@ impl ProgramKind { let host_maybe_with_user_as_ssh_arg = match (url.user_as_argument(), url.host_as_argument()) { (Usable(user), Usable(host)) => format!("{user}@{host}"), + (Usable(user), Dangerous(host)) => format!("{user}@{host}"), // The `user@` makes it safe. (Absent, Usable(host)) => host.into(), (Dangerous(user), _) => Err(ssh::invocation::Error::AmbiguousUserName { user: user.into() })?, (_, Dangerous(host)) => Err(ssh::invocation::Error::AmbiguousHostName { host: host.into() })?, From 524739b9189d007dab2f0dde38ce59dabd20d737 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 22:38:45 +0000 Subject: [PATCH 20/25] Try, so far unsuccessfully, to add missing `-G` test This is to test a code path where a host is passed to an ssh command and it's important to ensure it doesn't start with `-`. This code path uses `-G` to help figure out what SSH client is being used, and most of the time it is not followed. The existing check is sufficiently simple it is most likely correct. But it seems never to to have been covered in automated tests: neither unit tests nor any journey tests. This does not yet manage to test is successfully, because it does not succeed at getting that specific code path to be followed. --- gix-transport/src/client/blocking_io/file.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/gix-transport/src/client/blocking_io/file.rs b/gix-transport/src/client/blocking_io/file.rs index 5f5bf25b0d..49978d936c 100644 --- a/gix-transport/src/client/blocking_io/file.rs +++ b/gix-transport/src/client/blocking_io/file.rs @@ -291,7 +291,7 @@ pub fn connect( mod tests { mod ssh { mod connect { - use crate::{client::blocking_io::ssh::connect, Protocol}; + use crate::{client::blocking_io::ssh, Protocol}; #[test] fn path() { @@ -304,10 +304,24 @@ mod tests { ("user@host.xy:~/repo", "~/repo"), ] { let url = gix_url::parse((*url).into()).expect("valid url"); - let cmd = connect(url, Protocol::V1, Default::default(), false).expect("parse success"); + let cmd = ssh::connect(url, Protocol::V1, Default::default(), false).expect("parse success"); assert_eq!(cmd.path, expected, "the path will be substituted by the remote shell"); } } + + #[test] + fn ambiguous_host_disallowed() { + for url in [ + "ssh://-oProxyCommand=open$IFS-aCalculator/foo", + "user@-oProxyCommand=open$IFS-aCalculator:username/repo", + ] { + let url = gix_url::parse((*url).into()).expect("valid url"); + assert!(matches!( + ssh::connect(url, Protocol::V1, Default::default(), false), + Err(ssh::Error::AmbiguousHostName { host }) if host == "-oProxyCommand=open$IFS-aCalculator", + )); + } + } } } } From 902367fb452787a8fd8305676fa9c5a827646490 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Apr 2024 00:18:36 +0000 Subject: [PATCH 21/25] Test that leading-`-` host names aren't used in `-G` check This test was absent before, but seems important to add to verify that the code there was correct (it was), but also because I plan to refactor that code to use `host_as_argument` rather than the older (in some cases potentially ambiguous) `host_argument_safe`. This is specific to the case where a command like `ssh -G` -- but where `ssh` is an uncertain command such that gix-transport tries to figure out what is -- is used. A netloc is passed as part of that, but the netloc used consists only of a host and not a username, even when this is a step in a longer process that will later use (and check the safety of) the username. So only the host has to be checked (at that stage) to make sure it cannot masquerade as an option argument. --- gix-transport/src/client/blocking_io/file.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gix-transport/src/client/blocking_io/file.rs b/gix-transport/src/client/blocking_io/file.rs index 49978d936c..06ce1cbfbd 100644 --- a/gix-transport/src/client/blocking_io/file.rs +++ b/gix-transport/src/client/blocking_io/file.rs @@ -316,8 +316,13 @@ mod tests { "user@-oProxyCommand=open$IFS-aCalculator:username/repo", ] { let url = gix_url::parse((*url).into()).expect("valid url"); + let options = ssh::connect::Options { + command: Some("unrecognized".into()), + disallow_shell: false, + kind: None, + }; assert!(matches!( - ssh::connect(url, Protocol::V1, Default::default(), false), + ssh::connect(url, Protocol::V1, options, false), Err(ssh::Error::AmbiguousHostName { host }) if host == "-oProxyCommand=open$IFS-aCalculator", )); } From cf59f574f97f9a3ca489c603674d0ec76e498e1e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Apr 2024 20:59:35 +0000 Subject: [PATCH 22/25] Use `Url::host_as_argument()` in `ssh::connect()` Instead of `Url::host_argument_safe()`. This also removes the comment about how the username should be handled if added, mostly because I didn't really need to add that comment. --- gix-transport/src/client/blocking_io/ssh/mod.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 7d179951f6..6820051a5e 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/gix-transport/src/client/blocking_io/ssh/mod.rs @@ -1,5 +1,7 @@ use std::process::Stdio; +use gix_url::ArgumentSafety::*; + use crate::{client::blocking_io, Protocol}; /// The error used in [`connect()`]. @@ -118,11 +120,11 @@ pub fn connect( .stdin(Stdio::null()) .with_shell() .arg("-G") - // Username affects the stdout from `ssh -G` but may not affect the status. But if - // we end up needing it, it can be added here, with a user_argument_safe() check. - .arg(url.host_argument_safe().ok_or_else(|| Error::AmbiguousHostName { - host: url.host().expect("set in ssh urls").into(), - })?), + .arg(match url.host_as_argument() { + Usable(host) => host, + Dangerous(host) => Err(Error::AmbiguousHostName { host: host.into() })?, + Absent => panic!("BUG: host should always be present in SSH URLs"), + }), ); gix_features::trace::debug!(cmd = ?cmd, "invoking `ssh` for feature check"); kind = if cmd.status().ok().map_or(false, |status| status.success()) { From 03fb64ac8fca02bed9786b0e832764c1728e6e0e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Apr 2024 00:38:00 +0000 Subject: [PATCH 23/25] (Re)add a short, more specific comment about user@ --- gix-transport/src/client/blocking_io/ssh/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 6820051a5e..9e5effb471 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/gix-transport/src/client/blocking_io/ssh/mod.rs @@ -124,7 +124,7 @@ pub fn connect( Usable(host) => host, Dangerous(host) => Err(Error::AmbiguousHostName { host: host.into() })?, Absent => panic!("BUG: host should always be present in SSH URLs"), - }), + }), // We omit `user@` with `-G`, so it need not be safe here, but host must be safe. ); gix_features::trace::debug!(cmd = ?cmd, "invoking `ssh` for feature check"); kind = if cmd.status().ok().map_or(false, |status| status.success()) { From 09311b0e0039c5a82c871047ff24336ab1741d47 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 13 Apr 2024 10:32:42 +0200 Subject: [PATCH 24/25] refactor `gix-url` - put more standard-derives on `ArgumentSafety` to allow more convenient usage. - remove `path_as_argument()` method as it wasn't adding anything. This allowed the `ArgumentSafety` wrapper to be simpler. - turn assertion comments into messages. --- gix-url/src/lib.rs | 29 ++++++----------------------- gix-url/tests/access/mod.rs | 33 +++++++++++++++------------------ 2 files changed, 21 insertions(+), 41 deletions(-) diff --git a/gix-url/src/lib.rs b/gix-url/src/lib.rs index dd424a43a4..8468ec80ef 100644 --- a/gix-url/src/lib.rs +++ b/gix-url/src/lib.rs @@ -66,14 +66,14 @@ pub fn expand_path(user: Option<&expand_path::ForUser>, path: &BStr) -> Result

{ +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +pub enum ArgumentSafety<'a> { /// May be safe. There is nothing to pass, so there is nothing dangerous. Absent, /// May be safe. The argument does not begin with a `-` and so will not be confused as an option. - Usable(T), + Usable(&'a str), /// Dangerous! Begins with `-` and could be treated as an option. Use the value in error messages only. - Dangerous(T), + Dangerous(&'a str), } /// A URL with support for specialized git related capabilities. @@ -201,7 +201,7 @@ impl Url { /// /// Use this method instead of [Self::user()] if the host is going to be passed to a command-line application. /// If the unsafe and absent cases need not be distinguished, [Self::user_argument_safe()] may also be used. - pub fn user_as_argument(&self) -> ArgumentSafety<&str> { + pub fn user_as_argument(&self) -> ArgumentSafety<'_> { match self.user() { Some(user) if looks_like_command_line_option(user.as_bytes()) => ArgumentSafety::Dangerous(user), Some(user) => ArgumentSafety::Usable(user), @@ -243,7 +243,7 @@ impl Url { /// /// Use this method instead of [Self::host()] if the host is going to be passed to a command-line application. /// If the unsafe and absent cases need not be distinguished, [Self::host_argument_safe()] may also be used. - pub fn host_as_argument(&self) -> ArgumentSafety<&str> { + pub fn host_as_argument(&self) -> ArgumentSafety<'_> { match self.host() { Some(host) if looks_like_command_line_option(host.as_bytes()) => ArgumentSafety::Dangerous(host), Some(host) => ArgumentSafety::Usable(host), @@ -263,28 +263,11 @@ impl Url { } } - /// Classify the path of this URL by whether it is safe to pass as a command-line argument. - /// Note that it always begins with a slash, which is ignored for this classification. - /// - /// Use this method or [Self::path_argument_safe()] instead of [Self::path] if the host is going to be - /// passed to a command-line application, unless it is certain that the leading `/` will always be included. - /// - /// This method never returns an [ArgumentSafety::Absent]. - pub fn path_as_argument(&self) -> ArgumentSafety<&BStr> { - match self.path_argument_safe() { - Some(path) => ArgumentSafety::Usable(path), - None => ArgumentSafety::Dangerous(self.path.as_ref()), - } - } - /// Return the path of this URL *if* it can't be mistaken for a command-line argument. /// Note that it always begins with a slash, which is ignored for this comparison. /// /// Use this method instead of accessing [Self::path] directly if the path is going to be passed to a /// command-line application, unless it is certain that the leading `/` will always be included. - /// - /// The result of this method is unambiguous because [Self::path] is not an option type, but - /// [Self::path_as_argument()] may also safely be used. pub fn path_argument_safe(&self) -> Option<&BStr> { self.path .get(1..) diff --git a/gix-url/tests/access/mod.rs b/gix-url/tests/access/mod.rs index 5b76589980..ae2269ea2f 100644 --- a/gix-url/tests/access/mod.rs +++ b/gix-url/tests/access/mod.rs @@ -60,14 +60,13 @@ fn user_argument_safety() -> crate::Result { assert_eq!(url.user(), Some("-Fconfigfile")); assert_eq!(url.user_as_argument(), ArgumentSafety::Dangerous("-Fconfigfile")); - assert_eq!(url.user_argument_safe(), None); // An unsafe username is blocked. + assert_eq!(url.user_argument_safe(), None, "An unsafe username is blocked."); assert_eq!(url.host(), Some("foo")); assert_eq!(url.host_as_argument(), ArgumentSafety::Usable("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); assert_eq!(url.path, "/bar"); - assert_eq!(url.path_as_argument(), ArgumentSafety::Usable("/bar".into())); assert_eq!(url.path_argument_safe(), Some("/bar".into())); Ok(()) @@ -79,17 +78,20 @@ fn host_argument_safety() -> crate::Result { assert_eq!(url.user(), None); assert_eq!(url.user_as_argument(), ArgumentSafety::Absent); - assert_eq!(url.user_argument_safe(), None); // As there is no user. See all_argument_safe_valid(). + assert_eq!( + url.user_argument_safe(), + None, + "As there is no user. See all_argument_safe_valid()" + ); assert_eq!(url.host(), Some("-oProxyCommand=open$IFS-aCalculator")); assert_eq!( url.host_as_argument(), ArgumentSafety::Dangerous("-oProxyCommand=open$IFS-aCalculator") ); - assert_eq!(url.host_argument_safe(), None); // An unsafe host string is blocked. + assert_eq!(url.host_argument_safe(), None, "An unsafe host string is blocked"); assert_eq!(url.path, "/foo"); - assert_eq!(url.path_as_argument(), ArgumentSafety::Usable("/foo".into())); assert_eq!(url.path_argument_safe(), Some("/foo".into())); Ok(()) @@ -101,18 +103,18 @@ fn path_argument_safety() -> crate::Result { assert_eq!(url.user(), None); assert_eq!(url.user_as_argument(), ArgumentSafety::Absent); - assert_eq!(url.user_argument_safe(), None); // As there is no user. See all_argument_safe_valid(). + assert_eq!( + url.user_argument_safe(), + None, + "As there is no user. See all_argument_safe_valid()" + ); assert_eq!(url.host(), Some("foo")); assert_eq!(url.host_as_argument(), ArgumentSafety::Usable("foo")); assert_eq!(url.host_argument_safe(), Some("foo")); assert_eq!(url.path, "/-oProxyCommand=open$IFS-aCalculator"); - assert_eq!( - url.path_as_argument(), - ArgumentSafety::Dangerous("/-oProxyCommand=open$IFS-aCalculator".into()) - ); - assert_eq!(url.path_argument_safe(), None); // An unsafe path is blocked. + assert_eq!(url.path_argument_safe(), None, "An unsafe path is blocked"); Ok(()) } @@ -130,7 +132,6 @@ fn all_argument_safety_safe() -> crate::Result { assert_eq!(url.host_argument_safe(), Some("example.com")); assert_eq!(url.path, "/path/to/file"); - assert_eq!(url.path_as_argument(), ArgumentSafety::Usable("/path/to/file".into())); assert_eq!(url.path_argument_safe(), Some("/path/to/file".into())); Ok(()) @@ -150,14 +151,10 @@ fn all_argument_safety_not_safe() -> crate::Result { url.host_as_argument(), ArgumentSafety::Dangerous("-oProxyCommand=open$IFS-aCalculator") ); - assert_eq!(url.host_argument_safe(), None); // An unsafe host string is blocked. + assert_eq!(url.host_argument_safe(), None, "An unsafe host string is blocked"); assert_eq!(url.path, "/-oProxyCommand=open$IFS-aCalculator"); - assert_eq!( - url.path_as_argument(), - ArgumentSafety::Dangerous("/-oProxyCommand=open$IFS-aCalculator".into()) - ); - assert_eq!(url.path_argument_safe(), None); // An unsafe path is blocked. + assert_eq!(url.path_argument_safe(), None, "An unsafe path is blocked"); Ok(()) } From 996310ba1408fe746c6de43cb24dc1e809fd4d57 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 13 Apr 2024 11:10:23 +0200 Subject: [PATCH 25/25] refactor `gix-transport` with minor edits to comments --- gix-transport/src/client/blocking_io/ssh/mod.rs | 2 +- gix-transport/src/client/blocking_io/ssh/program_kind.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 9e5effb471..6820051a5e 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/gix-transport/src/client/blocking_io/ssh/mod.rs @@ -124,7 +124,7 @@ pub fn connect( Usable(host) => host, Dangerous(host) => Err(Error::AmbiguousHostName { host: host.into() })?, Absent => panic!("BUG: host should always be present in SSH URLs"), - }), // We omit `user@` with `-G`, so it need not be safe here, but host must be safe. + }), ); gix_features::trace::debug!(cmd = ?cmd, "invoking `ssh` for feature check"); kind = if cmd.status().ok().map_or(false, |status| status.success()) { diff --git a/gix-transport/src/client/blocking_io/ssh/program_kind.rs b/gix-transport/src/client/blocking_io/ssh/program_kind.rs index 7c6f5c8a15..d7162c09b3 100644 --- a/gix-transport/src/client/blocking_io/ssh/program_kind.rs +++ b/gix-transport/src/client/blocking_io/ssh/program_kind.rs @@ -72,9 +72,9 @@ impl ProgramKind { (_, Absent) => panic!("BUG: host should always be present in SSH URLs"), }; - // Try to force ssh to yield English messages (for parsing later). Ok(prepare .arg(host_maybe_with_user_as_ssh_arg) + // Try to force ssh to yield English messages (for parsing later). .env("LANG", "C") .env("LC_ALL", "C")) }