If save-ing with non-existing parent dir, return directory_not_found (#15961)

This commit is contained in:
liquid-dragons
2025-06-29 17:15:15 +02:00
committed by GitHub
parent a4a3c514ba
commit c795f16143
4 changed files with 147 additions and 18 deletions

View File

@ -91,7 +91,8 @@ impl Command for Save {
PipelineData::ByteStream(stream, metadata) => { PipelineData::ByteStream(stream, metadata) => {
check_saving_to_source_file(metadata.as_ref(), &path, stderr_path.as_ref())?; check_saving_to_source_file(metadata.as_ref(), &path, stderr_path.as_ref())?;
let (file, stderr_file) = get_files(&path, stderr_path.as_ref(), append, force)?; let (file, stderr_file) =
get_files(engine_state, &path, stderr_path.as_ref(), append, force)?;
let size = stream.known_size(); let size = stream.known_size();
let signals = engine_state.signals(); let signals = engine_state.signals();
@ -201,7 +202,8 @@ impl Command for Save {
stderr_path.as_ref(), stderr_path.as_ref(),
)?; )?;
let (mut file, _) = get_files(&path, stderr_path.as_ref(), append, force)?; let (mut file, _) =
get_files(engine_state, &path, stderr_path.as_ref(), append, force)?;
for val in ls { for val in ls {
file.write_all(&value_to_bytes(val)?) file.write_all(&value_to_bytes(val)?)
.map_err(&from_io_error)?; .map_err(&from_io_error)?;
@ -226,7 +228,8 @@ impl Command for Save {
input_to_bytes(input, Path::new(&path.item), raw, engine_state, stack, span)?; input_to_bytes(input, Path::new(&path.item), raw, engine_state, stack, span)?;
// Only open file after successful conversion // Only open file after successful conversion
let (mut file, _) = get_files(&path, stderr_path.as_ref(), append, force)?; let (mut file, _) =
get_files(engine_state, &path, stderr_path.as_ref(), append, force)?;
file.write_all(&bytes).map_err(&from_io_error)?; file.write_all(&bytes).map_err(&from_io_error)?;
file.flush().map_err(&from_io_error)?; file.flush().map_err(&from_io_error)?;
@ -422,13 +425,14 @@ fn prepare_path(
} }
} }
fn open_file(path: &Path, span: Span, append: bool) -> Result<File, ShellError> { fn open_file(
let file: Result<File, nu_protocol::shell_error::io::ErrorKind> = match (append, path.exists()) engine_state: &EngineState,
{ path: &Path,
(true, true) => std::fs::OpenOptions::new() span: Span,
.append(true) append: bool,
.open(path) ) -> Result<File, ShellError> {
.map_err(|err| err.into()), let file: std::io::Result<File> = match (append, path.exists()) {
(true, true) => std::fs::OpenOptions::new().append(true).open(path),
_ => { _ => {
// This is a temporary solution until `std::fs::File::create` is fixed on Windows (rust-lang/rust#134893) // This is a temporary solution until `std::fs::File::create` is fixed on Windows (rust-lang/rust#134893)
// A TOCTOU problem exists here, which may cause wrong error message to be shown // A TOCTOU problem exists here, which may cause wrong error message to be shown
@ -438,22 +442,51 @@ fn open_file(path: &Path, span: Span, append: bool) -> Result<File, ShellError>
deprecated, deprecated,
reason = "we don't get a IsADirectory error, so we need to provide it" reason = "we don't get a IsADirectory error, so we need to provide it"
)] )]
Err(nu_protocol::shell_error::io::ErrorKind::from_std( Err(std::io::ErrorKind::IsADirectory.into())
std::io::ErrorKind::IsADirectory,
))
} else { } else {
std::fs::File::create(path).map_err(|err| err.into()) std::fs::File::create(path)
} }
#[cfg(not(target_os = "windows"))] #[cfg(not(target_os = "windows"))]
std::fs::File::create(path).map_err(|err| err.into()) std::fs::File::create(path)
} }
}; };
file.map_err(|err_kind| ShellError::Io(IoError::new(err_kind, span, PathBuf::from(path)))) match file {
Ok(file) => Ok(file),
Err(err) => {
// In caase of NotFound, search for the missing parent directory.
// This also presents a TOCTOU (or TOUTOC, technically?)
if err.kind() == std::io::ErrorKind::NotFound {
if let Some(missing_component) =
path.ancestors().skip(1).filter(|dir| !dir.exists()).last()
{
// By looking at the postfix to remove, rather than the prefix
// to keep, we are able to handle relative paths too.
let components_to_remove = path
.strip_prefix(missing_component)
.expect("Stripping ancestor from a path should never fail")
.as_os_str()
.as_encoded_bytes();
return Err(ShellError::Io(IoError::new(
ErrorKind::DirectoryNotFound,
engine_state
.span_match_postfix(span, components_to_remove)
.map(|(pre, _post)| pre)
.unwrap_or(span),
PathBuf::from(missing_component),
)));
}
}
Err(ShellError::Io(IoError::new(err, span, PathBuf::from(path))))
}
}
} }
/// Get output file and optional stderr file /// Get output file and optional stderr file
fn get_files( fn get_files(
engine_state: &EngineState,
path: &Spanned<PathBuf>, path: &Spanned<PathBuf>,
stderr_path: Option<&Spanned<PathBuf>>, stderr_path: Option<&Spanned<PathBuf>>,
append: bool, append: bool,
@ -467,7 +500,7 @@ fn get_files(
.transpose()?; .transpose()?;
// Only if both files can be used open and possibly truncate them // Only if both files can be used open and possibly truncate them
let file = open_file(path, path_span, append)?; let file = open_file(engine_state, path, path_span, append)?;
let stderr_file = stderr_path_and_span let stderr_file = stderr_path_and_span
.map(|(stderr_path, stderr_path_span)| { .map(|(stderr_path, stderr_path_span)| {
@ -480,7 +513,7 @@ fn get_files(
inner: vec![], inner: vec![],
}) })
} else { } else {
open_file(stderr_path, stderr_path_span, append) open_file(engine_state, stderr_path, stderr_path_span, append)
} }
}) })
.transpose()?; .transpose()?;

View File

@ -536,6 +536,39 @@ fn parent_redirection_doesnt_affect_save() {
}) })
} }
#[test]
fn save_missing_parent_dir() {
Playground::setup("save_test_24", |dirs, sandbox| {
sandbox.with_files(&[]);
let actual = nu!(
cwd: dirs.root(),
r#"'hello' | save save_test_24/foobar/hello.txt"#,
);
assert!(actual.err.contains("nu::shell::io::directory_not_found"));
assert!(actual.err.contains("foobar' does not exist"));
})
}
#[test]
fn save_missing_ancestor_dir() {
Playground::setup("save_test_24", |dirs, sandbox| {
sandbox.with_files(&[]);
std::fs::create_dir(dirs.test().join("foo"))
.expect("should have been able to create subdir for test");
let actual = nu!(
cwd: dirs.root(),
r#"'hello' | save save_test_24/foo/bar/baz/hello.txt"#,
);
assert!(actual.err.contains("nu::shell::io::directory_not_found"));
assert!(actual.err.contains("bar' does not exist"));
})
}
#[test] #[test]
fn force_save_to_dir() { fn force_save_to_dir() {
let actual = nu!(cwd: "crates/nu-command/tests/commands", r#" let actual = nu!(cwd: "crates/nu-command/tests/commands", r#"

View File

@ -768,6 +768,30 @@ impl EngineState {
&[0u8; 0] &[0u8; 0]
} }
/// If the span's content starts with the given prefix, return two subspans
/// corresponding to this prefix, and the rest of the content.
pub fn span_match_prefix(&self, span: Span, prefix: &[u8]) -> Option<(Span, Span)> {
let contents = self.get_span_contents(span);
if contents.starts_with(prefix) {
span.split_at(prefix.len())
} else {
None
}
}
/// If the span's content ends with the given postfix, return two subspans
/// corresponding to the rest of the content, and this postfix.
pub fn span_match_postfix(&self, span: Span, prefix: &[u8]) -> Option<(Span, Span)> {
let contents = self.get_span_contents(span);
if contents.ends_with(prefix) {
span.split_at(span.len() - prefix.len())
} else {
None
}
}
/// Get the global config from the engine state. /// Get the global config from the engine state.
/// ///
/// Use [`Stack::get_config()`] instead whenever the `Stack` is available, as it takes into /// Use [`Stack::get_config()`] instead whenever the `Stack` is available, as it takes into

View File

@ -132,6 +132,45 @@ impl Span {
Self::new(self.start - offset, self.end - offset) Self::new(self.start - offset, self.end - offset)
} }
/// Return length of the slice.
pub fn len(&self) -> usize {
self.end - self.start
}
/// Indicate if slice has length 0.
pub fn is_empty(&self) -> bool {
self.start == self.end
}
/// Return another span fully inside the [`Span`].
///
/// `start` and `end` are relative to `self.start`, and must lie within the `Span`.
/// In other words, both `start` and `end` must be `<= self.len()`.
pub fn subspan(&self, offset_start: usize, offset_end: usize) -> Option<Self> {
let len = self.len();
if offset_start > len || offset_end > len || offset_start > offset_end {
None
} else {
Some(Self::new(
self.start + offset_start,
self.start + offset_end,
))
}
}
/// Return two spans that split the ['Span'] at the given position.
pub fn split_at(&self, offset: usize) -> Option<(Self, Self)> {
if offset < self.len() {
Some((
Self::new(self.start, self.start + offset),
Self::new(self.start + offset, self.end),
))
} else {
None
}
}
pub fn contains(&self, pos: usize) -> bool { pub fn contains(&self, pos: usize) -> bool {
self.start <= pos && pos < self.end self.start <= pos && pos < self.end
} }