Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/sandbox/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use std::path::PathBuf;

use crate::file::replace_path;

#[cfg(target_os = "linux")]
mod landlock;
#[cfg(target_os = "macos")]
Expand Down Expand Up @@ -65,9 +67,11 @@ impl SandboxConfig {
pub fn resolve_paths(&mut self) {
let cwd = std::env::current_dir().unwrap_or_default();
let resolve = |paths: &mut Vec<PathBuf>| {
paths.retain(|p| !p.as_os_str().is_empty());
for p in paths.iter_mut() {
*p = replace_path(&*p);
if p.is_relative() {
*p = cwd.join(&p);
*p = cwd.join(&*p);
}
// Canonicalize to resolve symlinks (e.g., /var -> /private/var on macOS)
if let Ok(canonical) = p.canonicalize() {
Comment on lines 69 to 77

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

If an allowed path is empty or resolves to an empty string (for example, due to an empty template variable), it will be treated as a relative path and resolve to the current working directory (cwd). This can silently grant read/write access to the entire working directory, which is a security risk.

We should filter out empty paths from the allowed lists before resolving them.

Suggested change
let resolve = |paths: &mut Vec<PathBuf>| {
for p in paths.iter_mut() {
*p = replace_path(&*p);
if p.is_relative() {
*p = cwd.join(&p);
*p = cwd.join(&*p);
}
// Canonicalize to resolve symlinks (e.g., /var -> /private/var on macOS)
if let Ok(canonical) = p.canonicalize() {
let resolve = |paths: &mut Vec<PathBuf>| {
paths.retain(|p| !p.as_os_str().is_empty());
for p in paths.iter_mut() {
*p = replace_path(&*p);
if p.is_relative() {
*p = cwd.join(&*p);
}
// Canonicalize to resolve symlinks (e.g., /var -> /private/var on macOS)
if let Ok(canonical) = p.canonicalize() {

Expand Down Expand Up @@ -300,4 +304,18 @@ mod tests {
assert!(!filtered.contains_key("OTHER_VAR"));
assert!(filtered.contains_key("PATH")); // default key
}

#[test]
fn test_resolve_paths_drops_empty_paths() {
let mut config = SandboxConfig {
allow_read: vec![PathBuf::new()],
allow_write: vec![PathBuf::from("")],
..Default::default()
};

config.resolve_paths();

assert!(config.allow_read.is_empty());
assert!(config.allow_write.is_empty());
}
}
44 changes: 44 additions & 0 deletions src/task/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,10 @@ impl Task {
}

fn has_render_templates(&self) -> bool {
fn path_contains_template(path: &Path) -> bool {
path.to_str().is_some_and(contains_template_syntax)
}

let deps_have_template = |deps: &[TaskDep]| {
deps.iter().any(|dep| {
contains_template_syntax(&dep.task)
Expand Down Expand Up @@ -1400,6 +1404,8 @@ impl Task {
.shell
.as_ref()
.is_some_and(|s| contains_template_syntax(s))
|| self.allow_read.iter().any(|p| path_contains_template(p))
|| self.allow_write.iter().any(|p| path_contains_template(p))
|| tools_have_template
}

Expand Down Expand Up @@ -1481,6 +1487,26 @@ impl Task {
{
*shell = render_str(&mut tera, shell, &tera_ctx)?;
}
let mut render_sandbox_paths = |paths: &mut Vec<PathBuf>| -> Result<()> {
let mut rendered = Vec::with_capacity(paths.len());
for p in paths.drain(..) {
if let Some(path) = p.to_str()
&& contains_template_syntax(path)
{
let path = render_str(&mut tera, path, &tera_ctx)?;
if !path.trim().is_empty() {
rendered.push(PathBuf::from(path));
}
} else {
rendered.push(p);
}
}
*paths = rendered;
Ok(())
};
// Tilde expansion is applied later when task and CLI sandbox paths are normalized.
render_sandbox_paths(&mut self.allow_read)?;
render_sandbox_paths(&mut self.allow_write)?;
for (_, v) in &mut self.tools {
match v {
TaskToolValue::String(s) => {
Expand Down Expand Up @@ -2279,6 +2305,24 @@ mod tests {
}
}

#[tokio::test]
async fn test_render_sandbox_allow_paths() {
let config = Config::get().await.unwrap();
let mut task = Task {
allow_read: vec![Path::new("{{ env.HOME }}/read").into()],
allow_write: vec![
Path::new("{{ \"\" }}").into(),
Path::new("{{ env.HOME }}/write").into(),
],
..Default::default()
};

task.render(&config, Path::new(".")).await.unwrap();

assert_eq!(task.allow_read, vec![crate::env::HOME.join("read")]);
assert_eq!(task.allow_write, vec![crate::env::HOME.join("write")]);
}

#[test]
#[cfg(unix)]
fn test_name_from_path() {
Expand Down
50 changes: 40 additions & 10 deletions src/task/task_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::cmd::CmdLineRunner;
use crate::config::{Config, Settings, env_directive::EnvDirective};
use crate::duration;
use crate::env_diff::EnvDiff;
use crate::file::{display_path, is_executable};
use crate::file::{display_path, is_executable, replace_path};
use crate::sandbox::SandboxConfig;
use crate::task::TaskKey;
use crate::task::task_context_builder::TaskContextBuilder;
Expand Down Expand Up @@ -52,6 +52,20 @@ async fn acquire_runtime_lock(interactive: bool) -> RuntimeLockGuard<'static> {
}
}

fn resolve_task_sandbox_path(p: &Path, task_base: Option<&Path>) -> PathBuf {
if p.as_os_str().is_empty() {
return PathBuf::new();
}
let p = replace_path(p);
if p.is_absolute() {
p
} else if let Some(base) = task_base {
base.join(p)
} else {
p
}
}

/// Configuration for TaskExecutor
pub struct TaskExecutorConfig {
pub force: bool,
Expand Down Expand Up @@ -143,15 +157,8 @@ impl TaskExecutor {
config: &Arc<Config>,
) -> Result<SandboxConfig> {
let task_base = task.dir(config).await?;
let resolve_task_path = |p: &PathBuf| -> PathBuf {
if p.is_absolute() {
p.clone()
} else if let Some(base) = &task_base {
base.join(p)
} else {
p.clone()
}
};
let resolve_task_path =
|p: &PathBuf| -> PathBuf { resolve_task_sandbox_path(p, task_base.as_deref()) };
let mut sandbox = SandboxConfig {
deny_read: task.deny_all || task.deny_read || self.sandbox.deny_read,
deny_write: task.deny_all || task.deny_write || self.sandbox.deny_write,
Expand Down Expand Up @@ -1532,6 +1539,29 @@ mod tests {
env
}

#[test]
fn test_resolve_task_sandbox_path_expands_home_before_task_base() {
let resolved =
resolve_task_sandbox_path(Path::new("~/sandbox-path"), Some(Path::new("/task/base")));

assert_eq!(resolved, crate::dirs::HOME.join("sandbox-path"));
}

#[test]
fn test_resolve_task_sandbox_path_uses_task_base_for_relative_paths() {
let resolved =
resolve_task_sandbox_path(Path::new("sandbox-path"), Some(Path::new("/task/base")));

assert_eq!(resolved, PathBuf::from("/task/base/sandbox-path"));
}

#[test]
fn test_resolve_task_sandbox_path_preserves_empty_paths_for_filtering() {
let resolved = resolve_task_sandbox_path(Path::new(""), Some(Path::new("/task/base")));

assert_eq!(resolved, PathBuf::new());
}

#[test]
#[cfg(windows)]
fn test_maybe_convert_env_for_msys_shell_converts_for_bash() {
Expand Down
Loading