Skip to content

Commit

Permalink
fix: Detect ambiguous usernames, maybe in all dangerous cases
Browse files Browse the repository at this point in the history
This makes all the unit tests pass, as well as the journey tests
that run locally. My guess is that the journey tests that run only
on CI may pass as well.

It also passes manual testing of specific cases of concern, by
running `cargo install --path .` and then attempting the commands:

    gix clone 'ssh://-Fconfigfile@example.com/abc'
    gix clone -- '-Fconfigfile@example.com:abc/def'

Previously, these passed -F... option arguments to ssh. Now they do
not, instead refusing to run ssh and outputting:

    Error: Username '-Fconfigfile' could be mistaken for a command-line argument

However, gix_transport::client::blocking_io::ssh::connect remains
unchanged. It does not appear to use the username argument. If it
should use it, then when that is added, it will also need to check
for dangerous usernames. But it may be that the username is never
needed just to pass `-G` for that check.
  • Loading branch information
EliahKagan committed Apr 12, 2024
1 parent 6ba46f9 commit 7896c36
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 3 deletions.
13 changes: 10 additions & 3 deletions gix-transport/src/client/blocking_io/ssh/program_kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
}
Expand All @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions gix-url/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
}

Expand Down

0 comments on commit 7896c36

Please sign in to comment.