Skip to content

Commit

Permalink
fix(windows): avoid inadvertly running exes from cwd (starship#2885)
Browse files Browse the repository at this point in the history
On Windows when running commands with their name instead of the path with Command::new, executable with that name from the current working directory will be executed.

This PR replaces all instances of Command::new with a new create_command function which will first resolve any executable paths and avoid this issue.
  • Loading branch information
davidkna authored Jul 16, 2021
1 parent e1fc137 commit 1eaf996
Show file tree
Hide file tree
Showing 19 changed files with 126 additions and 93 deletions.
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ pub fn module<'a>(context: &'a Context) -> Option<Module<'a>> {
}
```

If using `context.exec_cmd` isn't possible, please use `crate::utils::create_command` instead of `std::process::Command::new`.

## Logging

Debug logging in starship is done with our custom logger implementation.
Expand Down
2 changes: 2 additions & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# std::process::Command::new may inadvertly run executables from the current working directory
disallowed-methods = ["std::process::Command::new"]
12 changes: 0 additions & 12 deletions src/bug_report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,18 +265,6 @@ mod tests {
assert!(link.contains("No+Starship+config"));
}

#[test]
fn test_get_shell_info() {
env::remove_var("STARSHIP_SHELL");
let unknown_shell = get_shell_info();
assert_eq!(UNKNOWN_SHELL, &unknown_shell.name);

env::set_var("STARSHIP_SHELL", "fish");

let fish_shell = get_shell_info();
assert_eq!("fish", &fish_shell.name);
}

#[test]
#[cfg(not(windows))]
fn test_get_config_path() {
Expand Down
5 changes: 2 additions & 3 deletions src/configure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use std::env;
use std::ffi::OsString;
use std::io::ErrorKind;
use std::process;
use std::process::Command;

use crate::config::RootModuleConfig;
use crate::config::StarshipConfig;
Expand Down Expand Up @@ -136,9 +135,9 @@ pub fn write_configuration(table: &mut Table) {
pub fn edit_configuration() {
let config_path = get_config_path();
let editor_cmd = shell_words::split(&get_editor()).expect("Unmatched quotes found in $EDITOR.");
let editor_path = which::which(&editor_cmd[0]).expect("Unable to locate editor in $PATH.");

let command = Command::new(editor_path)
let command = utils::create_command(&editor_cmd[0])
.expect("Unable to locate editor in $PATH.")
.args(&editor_cmd[1..])
.arg(config_path)
.status();
Expand Down
5 changes: 2 additions & 3 deletions src/init/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::utils::create_command;
use std::ffi::OsStr;
use std::path::{Path, PathBuf};
use std::{env, io};
Expand Down Expand Up @@ -51,9 +52,7 @@ impl StarshipPath {
return self.sprint();
}
let str_path = self.str_path()?;
let res = std::process::Command::new("cygpath.exe")
.arg(str_path)
.output();
let res = create_command("cygpath").and_then(|mut cmd| cmd.arg(str_path).output());
let output = match res {
Ok(output) => output,
Err(e) => {
Expand Down
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![warn(clippy::disallowed_method)]

#[macro_use]
extern crate shadow_rs;

Expand Down
2 changes: 2 additions & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![warn(clippy::disallowed_method)]

use clap::crate_authors;
use std::io;
use std::time::SystemTime;
Expand Down
15 changes: 10 additions & 5 deletions src/modules/custom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::time::Instant;

use super::{Context, Module, RootModuleConfig};

use crate::{configs::custom::CustomConfig, formatter::StringFormatter};
use crate::{configs::custom::CustomConfig, formatter::StringFormatter, utils::create_command};

/// Creates a custom module with some configuration
///
Expand Down Expand Up @@ -100,7 +100,7 @@ fn get_shell<'a, 'b>(shell_args: &'b [&'a str]) -> (std::borrow::Cow<'a, str>, &
#[cfg(not(windows))]
fn shell_command(cmd: &str, shell_args: &[&str]) -> Option<Output> {
let (shell, shell_args) = get_shell(shell_args);
let mut command = Command::new(shell.as_ref());
let mut command = create_command(shell.as_ref()).ok()?;

command
.args(shell_args)
Expand All @@ -118,6 +118,7 @@ fn shell_command(cmd: &str, shell_args: &[&str]) -> Option<Output> {
"Could not launch command with given shell or STARSHIP_SHELL env variable, retrying with /usr/bin/env sh"
);

#[allow(clippy::disallowed_method)]
Command::new("/usr/bin/env")
.arg("sh")
.stdin(Stdio::piped())
Expand All @@ -141,14 +142,17 @@ fn shell_command(cmd: &str, shell_args: &[&str]) -> Option<Output> {
Some(std::borrow::Cow::Borrowed(shell_args[0])),
&shell_args[1..],
)
} else if let Ok(env_shell) = std::env::var("STARSHIP_SHELL") {
} else if let Some(env_shell) = std::env::var("STARSHIP_SHELL")
.ok()
.filter(|s| !cfg!(test) && !s.is_empty())
{
(Some(std::borrow::Cow::Owned(env_shell)), &[] as &[&str])
} else {
(None, &[] as &[&str])
};

if let Some(forced_shell) = shell {
let mut command = Command::new(forced_shell.as_ref());
let mut command = create_command(forced_shell.as_ref()).ok()?;

command
.args(shell_args)
Expand All @@ -169,7 +173,8 @@ fn shell_command(cmd: &str, shell_args: &[&str]) -> Option<Output> {
);
}

let command = Command::new("cmd.exe")
let command = create_command("cmd")
.ok()?
.arg("/C")
.arg(cmd)
.stdin(Stdio::piped())
Expand Down
4 changes: 2 additions & 2 deletions src/modules/directory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,14 +298,14 @@ fn to_fish_style(pwd_dir_length: usize, dir_string: String, truncated_dir_string
mod tests {
use super::*;
use crate::test::ModuleRenderer;
use crate::utils::create_command;
use crate::utils::home_dir;
use ansi_term::Color;
#[cfg(not(target_os = "windows"))]
use std::os::unix::fs::symlink;
#[cfg(target_os = "windows")]
use std::os::windows::fs::symlink_dir as symlink;
use std::path::Path;
use std::process::Command;
use std::{fs, io};
use tempfile::TempDir;

Expand Down Expand Up @@ -443,7 +443,7 @@ mod tests {
}

fn init_repo(path: &Path) -> io::Result<()> {
Command::new("git")
create_command("git")?
.args(&["init"])
.current_dir(path)
.output()
Expand Down
4 changes: 2 additions & 2 deletions src/modules/dotnet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,10 +345,10 @@ enum FileType {
mod tests {
use super::*;
use crate::test::ModuleRenderer;
use crate::utils::create_command;
use ansi_term::Color;
use std::fs::{self, OpenOptions};
use std::io::{self, Write};
use std::process::Command;
use tempfile::{self, TempDir};

#[test]
Expand Down Expand Up @@ -551,7 +551,7 @@ mod tests {
let repo_dir = tempfile::tempdir()?;

if is_repo {
Command::new("git")
create_command("git")?
.args(&["init", "--quiet"])
.current_dir(repo_dir.path())
.output()?;
Expand Down
20 changes: 10 additions & 10 deletions src/modules/git_branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,9 @@ fn get_first_grapheme(text: &str) -> &str {
mod tests {
use ansi_term::Color;
use std::io;
use std::process::Command;

use crate::test::{fixture_repo, FixtureProvider, ModuleRenderer};
use crate::utils::create_command;

#[test]
fn show_nothing_on_empty_dir() -> io::Result<()> {
Expand Down Expand Up @@ -270,12 +270,12 @@ mod tests {
fn test_works_with_unborn_default_branch() -> io::Result<()> {
let repo_dir = tempfile::tempdir()?;

Command::new("git")
create_command("git")?
.args(&["init"])
.current_dir(&repo_dir)
.output()?;

Command::new("git")
create_command("git")?
.args(&["symbolic-ref", "HEAD", "refs/heads/main"])
.current_dir(&repo_dir)
.output()?;
Expand All @@ -297,7 +297,7 @@ mod tests {
fn test_render_branch_only_attached_on_branch() -> io::Result<()> {
let repo_dir = fixture_repo(FixtureProvider::Git)?;

Command::new("git")
create_command("git")?
.args(&["checkout", "-b", "test_branch"])
.current_dir(repo_dir.path())
.output()?;
Expand Down Expand Up @@ -325,7 +325,7 @@ mod tests {
fn test_render_branch_only_attached_on_detached() -> io::Result<()> {
let repo_dir = fixture_repo(FixtureProvider::Git)?;

Command::new("git")
create_command("git")?
.args(&["checkout", "@~1"])
.current_dir(&repo_dir.path())
.output()?;
Expand All @@ -348,12 +348,12 @@ mod tests {
fn test_works_in_bare_repo() -> io::Result<()> {
let repo_dir = tempfile::tempdir()?;

Command::new("git")
create_command("git")?
.args(&["init", "--bare"])
.current_dir(&repo_dir)
.output()?;

Command::new("git")
create_command("git")?
.args(&["symbolic-ref", "HEAD", "refs/heads/main"])
.current_dir(&repo_dir)
.output()?;
Expand All @@ -379,7 +379,7 @@ mod tests {
// fn test_git_dir_env_variable() -> io::Result<()> {let repo_dir =
// tempfile::tempdir()?;

// Command::new("git")
// create_command("git")?
// .args(&["init"])
// .current_dir(&repo_dir)
// .output()?;
Expand Down Expand Up @@ -424,7 +424,7 @@ mod tests {
) -> io::Result<()> {
let repo_dir = fixture_repo(FixtureProvider::Git)?;

Command::new("git")
create_command("git")?
.args(&["checkout", "-b", branch_name])
.current_dir(repo_dir.path())
.output()?;
Expand Down Expand Up @@ -463,7 +463,7 @@ mod tests {
) -> io::Result<()> {
let repo_dir = fixture_repo(FixtureProvider::Git)?;

Command::new("git")
create_command("git")?
.args(&["checkout", "-b", branch_name])
.current_dir(repo_dir.path())
.output()?;
Expand Down
32 changes: 16 additions & 16 deletions src/modules/git_commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,10 @@ fn id_to_hex_abbrev(bytes: &[u8], len: usize) -> String {
#[cfg(test)]
mod tests {
use ansi_term::Color;
use std::process::Command;
use std::{io, str};

use crate::test::{fixture_repo, FixtureProvider, ModuleRenderer};
use crate::utils::create_command;

#[test]
fn show_nothing_on_empty_dir() -> io::Result<()> {
Expand All @@ -128,7 +128,7 @@ mod tests {
fn test_render_commit_hash() -> io::Result<()> {
let repo_dir = fixture_repo(FixtureProvider::Git)?;

let mut git_output = Command::new("git")
let mut git_output = create_command("git")?
.args(&["rev-parse", "HEAD"])
.current_dir(&repo_dir.path())
.output()?
Expand Down Expand Up @@ -160,7 +160,7 @@ mod tests {
fn test_render_commit_hash_len_override() -> io::Result<()> {
let repo_dir = fixture_repo(FixtureProvider::Git)?;

let mut git_output = Command::new("git")
let mut git_output = create_command("git")?
.args(&["rev-parse", "HEAD"])
.current_dir(&repo_dir.path())
.output()?
Expand Down Expand Up @@ -207,12 +207,12 @@ mod tests {
fn test_render_commit_hash_only_detached_on_detached() -> io::Result<()> {
let repo_dir = fixture_repo(FixtureProvider::Git)?;

Command::new("git")
create_command("git")?
.args(&["checkout", "@~1"])
.current_dir(&repo_dir.path())
.output()?;

let mut git_output = Command::new("git")
let mut git_output = create_command("git")?
.args(&["rev-parse", "HEAD"])
.current_dir(&repo_dir.path())
.output()?
Expand Down Expand Up @@ -240,15 +240,15 @@ mod tests {
fn test_render_commit_hash_with_tag_enabled() -> io::Result<()> {
let repo_dir = fixture_repo(FixtureProvider::Git)?;

let mut git_commit = Command::new("git")
let mut git_commit = create_command("git")?
.args(&["rev-parse", "HEAD"])
.current_dir(&repo_dir.path())
.output()?
.stdout;
git_commit.truncate(7);
let commit_output = str::from_utf8(&git_commit).unwrap().trim();

let git_tag = Command::new("git")
let git_tag = create_command("git")?
.args(&["describe", "--tags", "--exact-match", "HEAD"])
.current_dir(&repo_dir.path())
.output()?
Expand Down Expand Up @@ -283,25 +283,25 @@ mod tests {
fn test_render_commit_hash_only_detached_on_detached_with_tag_enabled() -> io::Result<()> {
let repo_dir = fixture_repo(FixtureProvider::Git)?;

Command::new("git")
create_command("git")?
.args(&["checkout", "@~1"])
.current_dir(&repo_dir.path())
.output()?;

Command::new("git")
create_command("git")?
.args(&["tag", "tagOnDetached", "-m", "Testing tags on detached"])
.current_dir(&repo_dir.path())
.output()?;

let mut git_commit = Command::new("git")
let mut git_commit = create_command("git")?
.args(&["rev-parse", "HEAD"])
.current_dir(&repo_dir.path())
.output()?
.stdout;
git_commit.truncate(7);
let commit_output = str::from_utf8(&git_commit).unwrap().trim();

let git_tag = Command::new("git")
let git_tag = create_command("git")?
.args(&["describe", "--tags", "--exact-match", "HEAD"])
.current_dir(&repo_dir.path())
.output()?
Expand Down Expand Up @@ -337,33 +337,33 @@ mod tests {

let repo_dir = fixture_repo(FixtureProvider::Git)?;

let mut git_commit = Command::new("git")
let mut git_commit = create_command("git")?
.args(&["rev-parse", "HEAD"])
.current_dir(&repo_dir.path())
.output()?
.stdout;
git_commit.truncate(7);
let commit_output = str::from_utf8(&git_commit).unwrap().trim();

Command::new("git")
create_command("git")?
.args(&["tag", "v2", "-m", "Testing tags v2"])
.current_dir(&repo_dir.path())
.output()?;

// Wait one second between tags
thread::sleep(time::Duration::from_millis(1000));

Command::new("git")
create_command("git")?
.args(&["tag", "v0", "-m", "Testing tags v0", "HEAD~1"])
.current_dir(&repo_dir.path())
.output()?;

Command::new("git")
create_command("git")?
.args(&["tag", "v1", "-m", "Testing tags v1"])
.current_dir(&repo_dir.path())
.output()?;

let git_tag = Command::new("git")
let git_tag = create_command("git")?
.args(&[
"for-each-ref",
"--contains",
Expand Down
Loading

0 comments on commit 1eaf996

Please sign in to comment.