From f8b0af70ff3d6745e9284639a1b5899c382a564b Mon Sep 17 00:00:00 2001 From: Yash Thakur <45539777+ysthakur@users.noreply.github.com> Date: Thu, 12 Jun 2025 19:26:01 -0400 Subject: [PATCH] Don't make unquoted file/dir paths absolute (#15878) # Description Closes #15848. Currently, we expand unquoted strings to absolute paths if they are of type `path` or `directory`. This PR makes this no longer happen. `~`, `.`, and `..+` are still expanded, but a path like `.../foo/bar/..` will only be turned into `../../foo`, rather than a full absolute path. This is mostly so that paths don't get modified before being sent to known external commands (as in the linked issue). But also, it seems unnecessary to make all unquoted paths absolute. After feedback from @132ikl, this PR also makes it so that unquoted paths are expanded at parse time, so that it matches the runtime behavior. Previously, `path` expressions were turned into strings verbatim, while `directory` expressions were treated as not being const. API change: `nu_path::expansions::expand_path` is now exposed as `nu_path::expand_path`. # User-Facing Changes This has the potential to silently break a lot of scripts. For example, if someone has a command that expects an already-expanded absolute path, changes the current working directory, and then passes the path somewhere, they will now need to use `path expand` to expand the path themselves before changing the current working directory. # Tests + Formatting Just added one test to make sure unquoted `path` arguments aren't made absolute. # After Submitting This is a breaking change, so will need to be mentioned in the release notes. --- crates/nu-engine/src/eval.rs | 40 ---------------------------- crates/nu-engine/src/eval_ir.rs | 14 +++------- crates/nu-path/src/expansions.rs | 7 +++-- crates/nu-path/src/lib.rs | 4 ++- crates/nu-protocol/src/eval_base.rs | 36 ++++++++++++------------- crates/nu-protocol/src/eval_const.rs | 20 -------------- tests/repl/test_custom_commands.rs | 12 +++++++++ 7 files changed, 41 insertions(+), 92 deletions(-) diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 7d54320338..6fecf9dd7a 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -1,7 +1,6 @@ use crate::eval_ir_block; #[allow(deprecated)] use crate::get_full_help; -use nu_path::{AbsolutePathBuf, expand_path_with}; use nu_protocol::{ BlockId, Config, DataSource, ENV_VARIABLE_ID, IntoPipelineData, PipelineData, PipelineMetadata, ShellError, Span, Value, VarId, @@ -404,45 +403,6 @@ impl Eval for EvalRuntime { stack.get_config(engine_state) } - fn eval_filepath( - engine_state: &EngineState, - stack: &mut Stack, - path: String, - quoted: bool, - span: Span, - ) -> Result { - if quoted { - Ok(Value::string(path, span)) - } else { - let cwd = engine_state.cwd(Some(stack))?; - let path = expand_path_with(path, cwd, true); - - Ok(Value::string(path.to_string_lossy(), span)) - } - } - - fn eval_directory( - engine_state: Self::State<'_>, - stack: &mut Self::MutState, - path: String, - quoted: bool, - span: Span, - ) -> Result { - if path == "-" { - Ok(Value::string("-", span)) - } else if quoted { - Ok(Value::string(path, span)) - } else { - let cwd = engine_state - .cwd(Some(stack)) - .map(AbsolutePathBuf::into_std_path_buf) - .unwrap_or_default(); - let path = expand_path_with(path, cwd, true); - - Ok(Value::string(path.to_string_lossy(), span)) - } - } - fn eval_var( engine_state: &EngineState, stack: &mut Stack, diff --git a/crates/nu-engine/src/eval_ir.rs b/crates/nu-engine/src/eval_ir.rs index cfffded6c5..b3c6d0d0d8 100644 --- a/crates/nu-engine/src/eval_ir.rs +++ b/crates/nu-engine/src/eval_ir.rs @@ -1,6 +1,6 @@ use std::{borrow::Cow, fs::File, sync::Arc}; -use nu_path::{AbsolutePathBuf, expand_path_with}; +use nu_path::{expand_path, expand_path_with}; use nu_protocol::{ DataSource, DeclId, ENV_VARIABLE_ID, Flag, IntoPipelineData, IntoSpanned, ListStream, OutDest, PipelineData, PipelineMetadata, PositionalArg, Range, Record, RegId, ShellError, Signals, @@ -888,9 +888,7 @@ fn literal_value( if *no_expand { Value::string(path, span) } else { - let cwd = ctx.engine_state.cwd(Some(ctx.stack))?; - let path = expand_path_with(path, cwd, true); - + let path = expand_path(path, true); Value::string(path.to_string_lossy(), span) } } @@ -904,13 +902,7 @@ fn literal_value( } else if *no_expand { Value::string(path, span) } else { - let cwd = ctx - .engine_state - .cwd(Some(ctx.stack)) - .map(AbsolutePathBuf::into_std_path_buf) - .unwrap_or_default(); - let path = expand_path_with(path, cwd, true); - + let path = expand_path(path, true); Value::string(path.to_string_lossy(), span) } } diff --git a/crates/nu-path/src/expansions.rs b/crates/nu-path/src/expansions.rs index 44e2d12503..36c54e3411 100644 --- a/crates/nu-path/src/expansions.rs +++ b/crates/nu-path/src/expansions.rs @@ -60,7 +60,10 @@ where canonicalize(path) } -fn expand_path(path: impl AsRef, need_expand_tilde: bool) -> PathBuf { +/// Resolve only path components (tilde, ., .., ...+), if possible. +/// +/// Doesn't convert to absolute form or use syscalls. Output may begin with "../" +pub fn expand_path(path: impl AsRef, need_expand_tilde: bool) -> PathBuf { let path = if need_expand_tilde { expand_tilde(path) } else { @@ -77,7 +80,7 @@ fn expand_path(path: impl AsRef, need_expand_tilde: bool) -> PathBuf { /// /// Furthermore, unlike canonicalize(), it does not use sys calls (such as readlink). /// -/// Does not convert to absolute form nor does it resolve symlinks. +/// Converts to absolute form but does not resolve symlinks. /// The input path is specified relative to another path pub fn expand_path_with(path: P, relative_to: Q, expand_tilde: bool) -> PathBuf where diff --git a/crates/nu-path/src/lib.rs b/crates/nu-path/src/lib.rs index cf31a5789f..d9df5abdb8 100644 --- a/crates/nu-path/src/lib.rs +++ b/crates/nu-path/src/lib.rs @@ -10,7 +10,9 @@ mod tilde; mod trailing_slash; pub use components::components; -pub use expansions::{canonicalize_with, expand_path_with, expand_to_real_path, locate_in_dirs}; +pub use expansions::{ + canonicalize_with, expand_path, expand_path_with, expand_to_real_path, locate_in_dirs, +}; pub use helpers::{cache_dir, data_dir, home_dir, nu_config_dir}; pub use path::*; pub use tilde::expand_tilde; diff --git a/crates/nu-protocol/src/eval_base.rs b/crates/nu-protocol/src/eval_base.rs index e642ddbf29..e4523b2ac7 100644 --- a/crates/nu-protocol/src/eval_base.rs +++ b/crates/nu-protocol/src/eval_base.rs @@ -1,4 +1,6 @@ //! Foundational [`Eval`] trait allowing dispatch between const-eval and regular evaluation +use nu_path::expand_path; + use crate::{ BlockId, Config, ENV_VARIABLE_ID, GetSpan, Range, Record, ShellError, Span, Value, VarId, ast::{ @@ -32,9 +34,23 @@ pub trait Eval { Expr::Int(i) => Ok(Value::int(*i, expr_span)), Expr::Float(f) => Ok(Value::float(*f, expr_span)), Expr::Binary(b) => Ok(Value::binary(b.clone(), expr_span)), - Expr::Filepath(path, quoted) => Self::eval_filepath(state, mut_state, path.clone(), *quoted, expr_span), + Expr::Filepath(path, quoted) => { + if *quoted { + Ok(Value::string(path, expr_span)) + } else { + let path = expand_path(path, true); + Ok(Value::string(path.to_string_lossy(), expr_span)) + } + } Expr::Directory(path, quoted) => { - Self::eval_directory(state, mut_state, path.clone(), *quoted, expr_span) + if path == "-" { + Ok(Value::string("-", expr_span)) + } else if *quoted { + Ok(Value::string(path, expr_span)) + } else { + let path = expand_path(path, true); + Ok(Value::string(path.to_string_lossy(), expr_span)) + } } Expr::Var(var_id) => Self::eval_var(state, mut_state, *var_id, expr_span), Expr::CellPath(cell_path) => Ok(Value::cell_path(cell_path.clone(), expr_span)), @@ -329,22 +345,6 @@ pub trait Eval { fn get_config(state: Self::State<'_>, mut_state: &mut Self::MutState) -> Arc; - fn eval_filepath( - state: Self::State<'_>, - mut_state: &mut Self::MutState, - path: String, - quoted: bool, - span: Span, - ) -> Result; - - fn eval_directory( - state: Self::State<'_>, - mut_state: &mut Self::MutState, - path: String, - quoted: bool, - span: Span, - ) -> Result; - fn eval_var( state: Self::State<'_>, mut_state: &mut Self::MutState, diff --git a/crates/nu-protocol/src/eval_const.rs b/crates/nu-protocol/src/eval_const.rs index 7e8cf11b2b..4dcfe40f30 100644 --- a/crates/nu-protocol/src/eval_const.rs +++ b/crates/nu-protocol/src/eval_const.rs @@ -441,26 +441,6 @@ impl Eval for EvalConst { state.get_config().clone() } - fn eval_filepath( - _: &StateWorkingSet, - _: &mut (), - path: String, - _: bool, - span: Span, - ) -> Result { - Ok(Value::string(path, span)) - } - - fn eval_directory( - _: &StateWorkingSet, - _: &mut (), - _: String, - _: bool, - span: Span, - ) -> Result { - Err(ShellError::NotAConstant { span }) - } - fn eval_var( working_set: &StateWorkingSet, _: &mut (), diff --git a/tests/repl/test_custom_commands.rs b/tests/repl/test_custom_commands.rs index bee046bcac..281c496f54 100644 --- a/tests/repl/test_custom_commands.rs +++ b/tests/repl/test_custom_commands.rs @@ -279,6 +279,18 @@ fn path_argument_dont_auto_expand_if_double_quoted() -> TestResult { run_test(r#"def spam [foo: path] { echo $foo }; spam "~/aa""#, "~/aa") } +#[test] +fn path_argument_dont_make_absolute_if_unquoted() -> TestResult { + #[cfg(windows)] + let expected = "..\\bar"; + #[cfg(not(windows))] + let expected = "../bar"; + run_test( + r#"def spam [foo: path] { echo $foo }; spam foo/.../bar"#, + expected, + ) +} + #[test] fn dont_allow_implicit_casting_between_glob_and_string() -> TestResult { let _ = fail_test(