From 35a0f7a36991ab7ec3b4da5c6e681de240d58526 Mon Sep 17 00:00:00 2001
From: Himadri Bhattacharjee <lavafroth@protonmail.com>
Date: Sat, 4 May 2024 06:47:50 +0530
Subject: [PATCH] fix: prevent relative directory traversal from crashing
 (#12438)

<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

- fixes #11922
- fixes #12203

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->
This is a rewrite for some parts of the recursive completion system. The
Rust `std::path` structures often ignores things like a trailing `.`
because for a complete path, it implies the current directory. We are
replacing the use of some of these structs for Strings.

A side effect is the slashes being normalized in Windows. For example if
we were to type `foo/bar/b`, it would complete it to `foo\bar\baz`
because a backward slash is the main separator in windows.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

Relative paths are preserved. `..`s in the paths won't eagerly show
completions from the parent path. For example, `asd/foo/../b` will now
complete to `asd/foo/../bar` instead of `asd/bar`.

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
---
 .../src/completions/completion_common.rs      | 191 +++++++++---------
 crates/nu-cli/tests/completions.rs            |  21 +-
 2 files changed, 117 insertions(+), 95 deletions(-)

diff --git a/crates/nu-cli/src/completions/completion_common.rs b/crates/nu-cli/src/completions/completion_common.rs
index 5f927976a2..4a8b0d57ac 100644
--- a/crates/nu-cli/src/completions/completion_common.rs
+++ b/crates/nu-cli/src/completions/completion_common.rs
@@ -7,40 +7,65 @@ use nu_protocol::{
     Span,
 };
 use nu_utils::get_ls_colors;
-use std::{
-    ffi::OsStr,
-    path::{is_separator, Component, Path, PathBuf, MAIN_SEPARATOR as SEP},
+use std::path::{
+    is_separator, Component, Path, PathBuf, MAIN_SEPARATOR as SEP, MAIN_SEPARATOR_STR,
 };
 
+#[derive(Clone, Default)]
+pub struct PathBuiltFromString {
+    parts: Vec<String>,
+    isdir: bool,
+}
+
 fn complete_rec(
-    partial: &[String],
+    partial: &[&str],
+    built: &PathBuiltFromString,
     cwd: &Path,
     options: &CompletionOptions,
     dir: bool,
     isdir: bool,
-) -> Vec<PathBuf> {
+) -> Vec<PathBuiltFromString> {
     let mut completions = vec![];
 
-    if let Ok(result) = cwd.read_dir() {
-        for entry in result.filter_map(|e| e.ok()) {
-            let entry_name = entry.file_name().to_string_lossy().into_owned();
-            let path = entry.path();
+    if let Some((&base, rest)) = partial.split_first() {
+        if (base == "." || base == "..") && (isdir || !rest.is_empty()) {
+            let mut built = built.clone();
+            built.parts.push(base.to_string());
+            built.isdir = true;
+            return complete_rec(rest, &built, cwd, options, dir, isdir);
+        }
+    }
 
-            if !dir || path.is_dir() {
-                match partial.first() {
-                    Some(base) if matches(base, &entry_name, options) => {
-                        let partial = &partial[1..];
-                        if !partial.is_empty() || isdir {
-                            completions.extend(complete_rec(partial, &path, options, dir, isdir));
-                            if entry_name.eq(base) {
-                                break;
-                            }
+    let mut built_path = cwd.to_path_buf();
+    for part in &built.parts {
+        built_path.push(part);
+    }
+
+    let Ok(result) = built_path.read_dir() else {
+        return completions;
+    };
+
+    for entry in result.filter_map(|e| e.ok()) {
+        let entry_name = entry.file_name().to_string_lossy().into_owned();
+        let entry_isdir = entry.path().is_dir();
+        let mut built = built.clone();
+        built.parts.push(entry_name.clone());
+        built.isdir = entry_isdir;
+
+        if !dir || entry_isdir {
+            match partial.split_first() {
+                Some((base, rest)) => {
+                    if matches(base, &entry_name, options) {
+                        if !rest.is_empty() || isdir {
+                            completions
+                                .extend(complete_rec(rest, &built, cwd, options, dir, isdir));
                         } else {
-                            completions.push(path)
+                            completions.push(built);
                         }
                     }
-                    None => completions.push(path),
-                    _ => {}
+                }
+                None => {
+                    completions.push(built);
                 }
             }
         }
@@ -48,33 +73,23 @@ fn complete_rec(
     completions
 }
 
+#[derive(Debug)]
 enum OriginalCwd {
     None,
-    Home(PathBuf),
-    Some(PathBuf),
-    // referencing a single local file
-    Local(PathBuf),
+    Home,
+    Prefix(String),
 }
 
 impl OriginalCwd {
-    fn apply(&self, p: &Path) -> String {
-        let mut ret = match self {
-            Self::None => p.to_string_lossy().into_owned(),
-            Self::Some(base) => pathdiff::diff_paths(p, base)
-                .unwrap_or(p.to_path_buf())
-                .to_string_lossy()
-                .into_owned(),
-            Self::Home(home) => match p.strip_prefix(home) {
-                Ok(suffix) => format!("~{}{}", SEP, suffix.to_string_lossy()),
-                _ => p.to_string_lossy().into_owned(),
-            },
-            Self::Local(base) => Path::new(".")
-                .join(pathdiff::diff_paths(p, base).unwrap_or(p.to_path_buf()))
-                .to_string_lossy()
-                .into_owned(),
+    fn apply(&self, mut p: PathBuiltFromString) -> String {
+        match self {
+            Self::None => {}
+            Self::Home => p.parts.insert(0, "~".to_string()),
+            Self::Prefix(s) => p.parts.insert(0, s.clone()),
         };
 
-        if p.is_dir() {
+        let mut ret = p.parts.join(MAIN_SEPARATOR_STR);
+        if p.isdir {
             ret.push(SEP);
         }
         ret
@@ -116,79 +131,67 @@ pub fn complete_item(
             };
             get_ls_colors(ls_colors_env_str)
         });
+
+    let mut cwd = cwd_pathbuf.clone();
+    let mut prefix_len = 0;
     let mut original_cwd = OriginalCwd::None;
-    let mut components_vec: Vec<Component> = Path::new(&partial).components().collect();
 
-    // Path components that end with a single "." get normalized away,
-    // so if the partial path ends in a literal "." we must add it back in manually
-    if partial.ends_with('.') && partial.len() > 1 {
-        components_vec.push(Component::Normal(OsStr::new(".")));
-    };
-    let mut components = components_vec.into_iter().peekable();
-
-    let mut cwd = match components.peek().cloned() {
+    let mut components = Path::new(&partial).components().peekable();
+    match components.peek().cloned() {
         Some(c @ Component::Prefix(..)) => {
             // windows only by definition
             components.next();
             if let Some(Component::RootDir) = components.peek().cloned() {
                 components.next();
             };
-            [c, Component::RootDir].iter().collect()
+            cwd = [c, Component::RootDir].iter().collect();
+            prefix_len = c.as_os_str().len();
+            original_cwd = OriginalCwd::Prefix(c.as_os_str().to_string_lossy().into_owned());
         }
         Some(c @ Component::RootDir) => {
             components.next();
-            PathBuf::from(c.as_os_str())
+            // This is kind of a hack. When joining an empty string with the rest,
+            // we add the slash automagically
+            cwd = PathBuf::from(c.as_os_str());
+            prefix_len = 1;
+            original_cwd = OriginalCwd::Prefix(String::new());
         }
         Some(Component::Normal(home)) if home.to_string_lossy() == "~" => {
             components.next();
-            original_cwd = OriginalCwd::Home(home_dir().unwrap_or(cwd_pathbuf.clone()));
-            home_dir().unwrap_or(cwd_pathbuf)
-        }
-        Some(Component::CurDir) => {
-            components.next();
-            original_cwd = match components.peek().cloned() {
-                Some(Component::Normal(_)) | None => OriginalCwd::Local(cwd_pathbuf.clone()),
-                _ => OriginalCwd::Some(cwd_pathbuf.clone()),
-            };
-            cwd_pathbuf
-        }
-        _ => {
-            original_cwd = OriginalCwd::Some(cwd_pathbuf.clone());
-            cwd_pathbuf
+            cwd = home_dir().unwrap_or(cwd_pathbuf);
+            prefix_len = 1;
+            original_cwd = OriginalCwd::Home;
         }
+        _ => {}
     };
 
-    let mut partial = vec![];
+    let after_prefix = &partial[prefix_len..];
+    let partial: Vec<_> = after_prefix
+        .strip_prefix(is_separator)
+        .unwrap_or(after_prefix)
+        .split(is_separator)
+        .filter(|s| !s.is_empty())
+        .collect();
 
-    for component in components {
-        match component {
-            Component::Prefix(..) => unreachable!(),
-            Component::RootDir => unreachable!(),
-            Component::CurDir => {}
-            Component::ParentDir => {
-                if partial.pop().is_none() {
-                    cwd.pop();
-                }
-            }
-            Component::Normal(c) => partial.push(c.to_string_lossy().into_owned()),
-        }
-    }
-
-    complete_rec(partial.as_slice(), &cwd, options, want_directory, isdir)
-        .into_iter()
-        .map(|p| {
-            let path = original_cwd.apply(&p);
-            let style = ls_colors.as_ref().map(|lsc| {
-                lsc.style_for_path_with_metadata(
-                    &path,
-                    std::fs::symlink_metadata(&path).ok().as_ref(),
-                )
+    complete_rec(
+        partial.as_slice(),
+        &PathBuiltFromString::default(),
+        &cwd,
+        options,
+        want_directory,
+        isdir,
+    )
+    .into_iter()
+    .map(|p| {
+        let path = original_cwd.apply(p);
+        let style = ls_colors.as_ref().map(|lsc| {
+            lsc.style_for_path_with_metadata(&path, std::fs::symlink_metadata(&path).ok().as_ref())
                 .map(lscolors::Style::to_nu_ansi_term_style)
                 .unwrap_or_default()
-            });
-            (span, escape_path(path, want_directory), style)
-        })
-        .collect()
+        });
+        (span, escape_path(path, want_directory), style)
+    })
+    .collect()
 }
 
 // Fix files or folders with quotes or hashes
diff --git a/crates/nu-cli/tests/completions.rs b/crates/nu-cli/tests/completions.rs
index 611dbfd35e..a22d770010 100644
--- a/crates/nu-cli/tests/completions.rs
+++ b/crates/nu-cli/tests/completions.rs
@@ -334,7 +334,26 @@ fn partial_completions() {
     let suggestions = completer.complete(&target_dir, target_dir.len());
 
     // Create the expected values
-    let expected_paths: Vec<String> = vec![file(dir.join("final_partial").join("somefile"))];
+    let expected_paths: Vec<String> = vec![
+        file(
+            dir.join("partial_a")
+                .join("..")
+                .join("final_partial")
+                .join("somefile"),
+        ),
+        file(
+            dir.join("partial_b")
+                .join("..")
+                .join("final_partial")
+                .join("somefile"),
+        ),
+        file(
+            dir.join("partial_c")
+                .join("..")
+                .join("final_partial")
+                .join("somefile"),
+        ),
+    ];
 
     // Match the results
     match_suggestions(expected_paths, suggestions);