mirror of
https://github.com/nushell/nushell.git
synced 2025-06-19 00:17:02 +02:00
Mitigate the poor interaction between ndots expansion and non-path strings (#13218)
# Description @hustcer reported that slashes were disappearing from external args since #13089: ``` $> ossutil ls oss://abc/b/c Error: invalid cloud url: "oss:/abc/b/c", please make sure the url starts with: "oss://" $> ossutil ls 'oss://abc/b/c' Error: oss: service returned error: StatusCode=403, ErrorCode=UserDisable, ErrorMessage="UserDisable", RequestId=66791EDEFE87B73537120838, Ec=0003-00000801, Bucket=abc, Object= ``` I narrowed this down to the ndots handling, since that does path parsing and path reconstruction in every case. I decided to change that so that it only activates if the string contains at least `...`, since that would be the minimum trigger for ndots, and also to not activate it if the string contains `://`, since it's probably undesirable for a URL. Kind of a hack, but I'm not really sure how else we decide whether someone wants ndots or not. # User-Facing Changes - bare strings not containing ndots are not modified - bare strings containing `://` are not modified # Tests + Formatting Added tests to prevent regression.
This commit is contained in:
parent
4509944988
commit
43e349a274
@ -64,7 +64,9 @@ impl Command for External {
|
|||||||
|
|
||||||
let expanded_name = match &name {
|
let expanded_name = match &name {
|
||||||
// Expand tilde and ndots on the name if it's a bare string / glob (#13000)
|
// Expand tilde and ndots on the name if it's a bare string / glob (#13000)
|
||||||
Value::Glob { no_expand, .. } if !*no_expand => expand_ndots(expand_tilde(&*name_str)),
|
Value::Glob { no_expand, .. } if !*no_expand => {
|
||||||
|
expand_ndots_safe(expand_tilde(&*name_str))
|
||||||
|
}
|
||||||
_ => Path::new(&*name_str).to_owned(),
|
_ => Path::new(&*name_str).to_owned(),
|
||||||
};
|
};
|
||||||
|
|
||||||
@ -294,7 +296,7 @@ fn expand_glob(
|
|||||||
// For an argument that doesn't include the GLOB_CHARS, just do the `expand_tilde`
|
// For an argument that doesn't include the GLOB_CHARS, just do the `expand_tilde`
|
||||||
// and `expand_ndots` expansion
|
// and `expand_ndots` expansion
|
||||||
if !arg.contains(GLOB_CHARS) {
|
if !arg.contains(GLOB_CHARS) {
|
||||||
let path = expand_ndots(expand_tilde(arg));
|
let path = expand_ndots_safe(expand_tilde(arg));
|
||||||
return Ok(vec![path.into()]);
|
return Ok(vec![path.into()]);
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -582,6 +584,21 @@ fn escape_cmd_argument(arg: &Spanned<OsString>) -> Result<Cow<'_, OsStr>, ShellE
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Expand ndots, but only if it looks like it probably contains them, because there is some lossy
|
||||||
|
/// path normalization that happens.
|
||||||
|
fn expand_ndots_safe(path: impl AsRef<Path>) -> PathBuf {
|
||||||
|
let string = path.as_ref().to_string_lossy();
|
||||||
|
|
||||||
|
// Use ndots if it contains at least `...`, since that's the minimum trigger point, and don't
|
||||||
|
// use it if it contains ://, because that looks like a URL scheme and the path normalization
|
||||||
|
// will mess with that.
|
||||||
|
if string.contains("...") && !string.contains("://") {
|
||||||
|
expand_ndots(path)
|
||||||
|
} else {
|
||||||
|
path.as_ref().to_owned()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod test {
|
mod test {
|
||||||
use super::*;
|
use super::*;
|
||||||
@ -610,7 +627,7 @@ mod test {
|
|||||||
assert_eq!(actual, expected);
|
assert_eq!(actual, expected);
|
||||||
|
|
||||||
let actual = expand_glob("./a.txt", cwd, Span::unknown(), &None).unwrap();
|
let actual = expand_glob("./a.txt", cwd, Span::unknown(), &None).unwrap();
|
||||||
let expected: Vec<OsString> = vec![Path::new(".").join("a.txt").into()];
|
let expected = &["./a.txt"];
|
||||||
assert_eq!(actual, expected);
|
assert_eq!(actual, expected);
|
||||||
|
|
||||||
let actual = expand_glob("[*.txt", cwd, Span::unknown(), &None).unwrap();
|
let actual = expand_glob("[*.txt", cwd, Span::unknown(), &None).unwrap();
|
||||||
|
@ -229,6 +229,38 @@ fn external_command_escape_args() {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn external_command_ndots_args() {
|
||||||
|
let actual = nu!(r#"
|
||||||
|
nu --testbin cococo foo/. foo/.. foo/... foo/./bar foo/../bar foo/.../bar ./bar ../bar .../bar
|
||||||
|
"#);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
actual.out,
|
||||||
|
if cfg!(windows) {
|
||||||
|
// Windows is a bit weird right now, where if ndots has to fix something it's going to
|
||||||
|
// change everything to backslashes too. Would be good to fix that
|
||||||
|
r"foo/. foo/.. foo\..\.. foo/./bar foo/../bar foo\..\..\bar ./bar ../bar ..\..\bar"
|
||||||
|
} else {
|
||||||
|
r"foo/. foo/.. foo/../.. foo/./bar foo/../bar foo/../../bar ./bar ../bar ../../bar"
|
||||||
|
}
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn external_command_url_args() {
|
||||||
|
// If ndots is not handled correctly, we can lose the double forward slashes that are needed
|
||||||
|
// here
|
||||||
|
let actual = nu!(r#"
|
||||||
|
nu --testbin cococo http://example.com http://example.com/.../foo //foo
|
||||||
|
"#);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
actual.out,
|
||||||
|
"http://example.com http://example.com/.../foo //foo"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
#[cfg_attr(
|
#[cfg_attr(
|
||||||
not(target_os = "linux"),
|
not(target_os = "linux"),
|
||||||
|
Loading…
x
Reference in New Issue
Block a user