Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add safe.directories config #10736

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
5 changes: 3 additions & 2 deletions src/cargo/util/config/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,10 @@ impl ConfigKey {

/// Rewinds this `ConfigKey` back to the state it was at before the last
/// `push` method being called.
pub fn pop(&mut self) {
let (_part, env) = self.parts.pop().unwrap();
pub fn pop(&mut self) -> String {
let (part, env) = self.parts.pop().unwrap();
self.env.truncate(env);
part
}

/// Returns the corresponding environment variable key for this
Expand Down
149 changes: 148 additions & 1 deletion src/cargo/util/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ use crate::core::compiler::rustdoc::RustdocExternMap;
use crate::core::shell::Verbosity;
use crate::core::{features, CliUnstable, Shell, SourceId, Workspace};
use crate::ops;
use crate::util::errors::CargoResult;
use crate::util::errors::{ownership_error, CargoResult};
use crate::util::toml as cargo_toml;
use crate::util::validate_package_name;
use crate::util::{FileLock, Filesystem, IntoUrl, IntoUrlWithBase, Rustc};
Expand Down Expand Up @@ -186,6 +186,7 @@ pub struct Config {
doc_extern_map: LazyCell<RustdocExternMap>,
progress_config: ProgressConfig,
env_config: LazyCell<EnvConfig>,
safe_directories: LazyCell<HashSet<PathBuf>>,
/// This should be false if:
/// - this is an artifact of the rustc distribution process for "stable" or for "beta"
/// - this is an `#[test]` that does not opt in with `enable_nightly_features`
Expand Down Expand Up @@ -284,6 +285,7 @@ impl Config {
doc_extern_map: LazyCell::new(),
progress_config: ProgressConfig::default(),
env_config: LazyCell::new(),
safe_directories: LazyCell::new(),
nightly_features_allowed: matches!(&*features::channel(), "nightly" | "dev"),
}
}
Expand Down Expand Up @@ -1057,6 +1059,103 @@ impl Config {
}
}

/// Checks if the given path is owned by a different user.
fn check_safe_dir(&self, path: &Path, safe_directories: &HashSet<PathBuf>) -> CargoResult<()> {
if !self.safe_directories_enabled() {
return Ok(());
}
paths::validate_ownership(path, safe_directories).map_err(|e| {
match e.downcast_ref::<paths::OwnershipError>() {
Some(e) => {
let mut to_add = e.path.parent().unwrap();
if to_add.file_name().and_then(|f| f.to_str()) == Some(".cargo") {
to_add = to_add.parent().unwrap();
}
ownership_error(e, "config files", to_add, self)
}
None => e,
}
})
}

/// Returns whether or not the nightly-only safe.directories behavior is enabled.
pub fn safe_directories_enabled(&self) -> bool {
// Because the config is loaded before the CLI options are available
// (primarily to handle aliases), this can't be gated on a `-Z` flag.
// So for now, this is only enabled via an environment variable. This
// has some downsides (such as not supporting the "allow" list), but
// should be fine for a one-off exception.
self.env
.get("CARGO_UNSTABLE_SAFE_DIRECTORIES")
.map(|s| s.as_str())
== Some("true")
&& self.nightly_features_allowed
}

/// Returns the `safe.directories` config setting.
pub fn safe_directories(&self) -> CargoResult<&HashSet<PathBuf>> {
self.safe_directories.try_borrow_with(|| {
if !self.safe_directories_enabled() {
return Ok(HashSet::new());
}
let mut safe_directories: HashSet<PathBuf> = self
.get_list("safe.directories")?
.map(|dirs| {
dirs.val
.iter()
.map(|(s, def)| def.root(self).join(s))
.collect()
})
.unwrap_or_default();
self.extend_safe_directories_env(&mut safe_directories);
Ok(safe_directories)
})
}

fn extend_safe_directories_env(&self, safe_directories: &mut HashSet<PathBuf>) {
// Note: Unlike other Cargo environment variables, this does not
// assume paths relative to the current directory (only absolute paths
// are supported). This is intended as an extra layer of safety.
Comment on lines +1116 to +1118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I didn't see this in the RFC
  • For my use of git's equivalent, I did use a . though I did it through -c rather than an env variable
  • We are only enforcing this contract by the fact that the a . won't match anything, correct? Should we more explicitly tell the user, either via a warning or error, that their safe directory setting will do nothing?

if let Some(dirs) = self.env.get("CARGO_SAFE_DIRECTORIES") {
safe_directories.extend(env::split_paths(dirs));
}
if let Some(dirs) = self.env.get("RUSTUP_SAFE_DIRECTORIES") {
safe_directories.extend(env::split_paths(dirs));
}
}

/// Loads the `safe.directories` setting directly from a `ConfigValue`
/// (which should be a Table of the root of the config file).
fn safe_directories_from_cv(
&self,
root: Option<&ConfigValue>,
) -> CargoResult<HashSet<PathBuf>> {
if !self.safe_directories_enabled() {
return Ok(HashSet::new());
}
let mut safe_directories: HashSet<PathBuf> = root
.map(|root| root.get("safe.directories"))
.transpose()?
.flatten()
.map(|cv| cv.list("safe.directories"))
.transpose()?
.map(|dirs| {
dirs.iter()
.map(|(dir, def)| {
if dir != "*" {
def.root(self).join(dir)
} else {
PathBuf::from(dir)
}
})
.collect()
})
.unwrap_or_default();

self.extend_safe_directories_env(&mut safe_directories);
Ok(safe_directories)
}

fn load_file(&self, path: &Path, includes: bool) -> CargoResult<ConfigValue> {
self._load_file(path, &mut HashSet::new(), includes)
}
Expand Down Expand Up @@ -1281,6 +1380,7 @@ impl Config {
.merge(tmp_table, true)
.with_context(|| format!("failed to merge --config argument `{arg}`"))?;
}
reject_cli_unsupported(&loaded_args)?;
Ok(loaded_args)
}

Expand Down Expand Up @@ -1370,6 +1470,7 @@ impl Config {
} else {
None
};
let safe_directories = self.safe_directories_from_cv(home_cv.as_ref())?;

let mut result = Vec::new();

Expand All @@ -1380,7 +1481,17 @@ impl Config {
continue;
}
if let Some(path) = self.get_file_path(&dot_cargo, "config", true)? {
self.check_safe_dir(&path, &safe_directories)?;
let cv = self._load_file(&path, &mut seen, includes)?;
if let Some(safe) = cv.get("safe.directories")? {
bail!(
"safe.directories may only be configured from Cargo's home directory\n\
Found `safe.directories` in {}\n\
Cargo's home directory is {}\n",
safe.definition(),
home.display()
);
}
result.push(cv);
}
}
Expand Down Expand Up @@ -1982,6 +2093,28 @@ impl ConfigValue {
self.definition()
)
}

/// Retrieve a `ConfigValue` using a dotted key notation.
///
/// This is similar to `Config::get`, but can be used directly on a root
/// `ConfigValue::Table`. This does *not* look at environment variables.
fn get(&self, key: &str) -> CargoResult<Option<&ConfigValue>> {
let mut key = ConfigKey::from_str(key);
let last = key.pop();
let (mut table, _def) = self.table("")?;
let mut key_so_far = ConfigKey::new();
for part in key.parts() {
key_so_far.push(part);
match table.get(part) {
Some(cv) => match cv {
CV::Table(t, _def) => table = t,
_ => cv.expected("table", &key_so_far.to_string())?,
},
None => return Ok(None),
}
}
Ok(table.get(&last))
}
}

pub fn homedir(cwd: &Path) -> Option<PathBuf> {
Expand Down Expand Up @@ -2473,3 +2606,17 @@ macro_rules! drop_eprint {
$crate::__shell_print!($config, err, false, $($arg)*)
);
}

/// Rejects config entries set on the CLI that are not supported.
fn reject_cli_unsupported(root: &CV) -> CargoResult<()> {
let (root, _) = root.table("")?;
// safe.directories cannot be set on the CLI because the config is loaded
// before the CLI is parsed (primarily to handle aliases).
if let Some(cv) = root.get("safe") {
let (safe, _) = cv.table("safe")?;
if safe.contains_key("directories") {
bail!("safe.directories cannot be set via the CLI");
}
}
Ok(())
}