Prevent improper files (like /dev/random) from being used as file arguments (#10733)

* Implement check before adding path to files

* fix problem where directories were removed from args.files

* Revert "Implement check before adding path to files"

This reverts commit c123944d9b2e125cc0e17e61d53aaffe09234baf.

* Dissallow opening of irregular non-symlink files

* Fixed issue with creating new file from command line

* Fixed linting error.

* Optimized regularity check as suggested in review

* Created DocumentOpenError Sum Type to switch on in Application

* Forgot cargo fmt

* Update helix-term/src/application.rs

Accept suggestion in review.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

* Moved thiserror version configuration to the workspace instead of the individual packages.

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

Cargo.lock

@@ -1535,6 +1535,7 @@ dependencies = [
  "serde_json",
  "slotmap",
  "tempfile",
+ "thiserror",
  "tokio",
  "tokio-stream",
  "toml",

Cargo.toml

@@ -40,6 +40,7 @@ package.helix-term.opt-level = 2
 tree-sitter = { version = "0.22" }
 nucleo = "0.2.0"
 slotmap = "1.0.7"
+thiserror = "1.0"
 
 [workspace.package]
 version = "24.3.0"

helix-dap/Cargo.toml

@@ -20,8 +20,8 @@ anyhow = "1.0"
 log = "0.4"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
-thiserror = "1.0"
 tokio = { version = "1", features = ["rt", "rt-multi-thread", "io-util", "io-std", "time", "process", "macros", "fs", "parking_lot", "net", "sync"] }
+thiserror.workspace = true
 
 [dev-dependencies]
 fern = "0.6"

helix-lsp/Cargo.toml

@@ -26,9 +26,9 @@ log = "0.4"
 lsp-types = { version = "0.95" }
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
-thiserror = "1.0"
 tokio = { version = "1.38", features = ["rt", "rt-multi-thread", "io-util", "io-std", "time", "process", "macros", "fs", "parking_lot", "sync"] }
 tokio-stream = "0.1.15"
 parking_lot = "0.12.3"
 arc-swap = "1"
 slotmap.workspace = true
+thiserror.workspace = true

helix-term/src/application.rs

@@ -9,7 +9,7 @@ use helix_lsp::{
 use helix_stdx::path::get_relative_path;
 use helix_view::{
     align_view,
-    document::DocumentSavedEventResult,
+    document::{DocumentOpenError, DocumentSavedEventResult},
     editor::{ConfigEvent, EditorEvent},
     graphics::Rect,
     theme,
@@ -186,9 +186,15 @@ impl Application {
                             Some(Layout::Horizontal) => Action::HorizontalSplit,
                             None => Action::Load,
                         };
-                        let doc_id = editor
+                        let doc_id = match editor.open(&file, action) {
-                            .open(&file, action)
+                            // Ignore irregular files during application init.
-                            .context(format!("open '{}'", file.to_string_lossy()))?;
+                            Err(DocumentOpenError::IrregularFile) => {
+                                nr_of_files -= 1;
+                                continue;
+                            }
+                            Err(err) => return Err(anyhow::anyhow!(err)),
+                            Ok(doc_id) => doc_id,
+                        };
                         // with Action::Load all documents have the same view
                         // NOTE: this isn't necessarily true anymore. If
                         // `--vsplit` or `--hsplit` are used, the file which is
@@ -199,15 +205,21 @@ impl Application {
                         doc.set_selection(view_id, pos);
                     }
                 }
-                editor.set_status(format!(
+
-                    "Loaded {} file{}.",
+                // if all files were invalid, replace with empty buffer
-                    nr_of_files,
+                if nr_of_files == 0 {
-                    if nr_of_files == 1 { "" } else { "s" } // avoid "Loaded 1 files." grammo
+                    editor.new_file(Action::VerticalSplit);
-                ));
+                } else {
-                // align the view to center after all files are loaded,
+                    editor.set_status(format!(
-                // does not affect views without pos since it is at the top
+                        "Loaded {} file{}.",
-                let (view, doc) = current!(editor);
+                        nr_of_files,
-                align_view(doc, view, Align::Center);
+                        if nr_of_files == 1 { "" } else { "s" } // avoid "Loaded 1 files." grammo
+                    ));
+                    // align the view to center after all files are loaded,
+                    // does not affect views without pos since it is at the top
+                    let (view, doc) = current!(editor);
+                    align_view(doc, view, Align::Center);
+                }
             } else {
                 editor.new_file(Action::VerticalSplit);
             }

helix-term/src/commands.rs

@@ -69,6 +69,7 @@ use crate::job::{self, Jobs};
 use std::{
     cmp::Ordering,
     collections::{HashMap, HashSet},
+    error::Error,
     fmt,
     future::Future,
     io::Read,

helix-term/src/ui/mod.rs

@@ -29,7 +29,7 @@ pub use text::Text;
 
 use helix_view::Editor;
 
-use std::path::PathBuf;
+use std::{error::Error, path::PathBuf};
 
 pub fn prompt(
     cx: &mut crate::commands::Context,

helix-view/Cargo.toml

@@ -50,7 +50,7 @@ toml = "0.8"
 log = "~0.4"
 
 parking_lot = "0.12.3"
-
+thiserror.workspace = true
 
 [target.'cfg(windows)'.dependencies]
 clipboard-win = { version = "5.3", features = ["std"] }

helix-view/src/document.rs

@@ -1,4 +1,4 @@
-use anyhow::{anyhow, bail, Context, Error};
+use anyhow::{anyhow, bail, Error};
 use arc_swap::access::DynAccess;
 use arc_swap::ArcSwap;
 use futures_util::future::BoxFuture;
@@ -12,6 +12,7 @@ use helix_core::text_annotations::{InlineAnnotation, Overlay};
 use helix_lsp::util::lsp_pos_to_pos;
 use helix_stdx::faccess::{copy_metadata, readonly};
 use helix_vcs::{DiffHandle, DiffProviderRegistry};
+use thiserror;
 
 use ::parking_lot::Mutex;
 use serde::de::{self, Deserialize, Deserializer};
@@ -21,6 +22,7 @@ use std::cell::Cell;
 use std::collections::HashMap;
 use std::fmt::Display;
 use std::future::Future;
+use std::io;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::sync::{Arc, Weak};
@@ -117,6 +119,14 @@ pub struct SavePoint {
     revert: Mutex<Transaction>,
 }
 
+#[derive(Debug, thiserror::Error)]
+pub enum DocumentOpenError {
+    #[error("path must be a regular file, simlink, or directory")]
+    IrregularFile,
+    #[error(transparent)]
+    IoError(#[from] io::Error),
+}
+
 pub struct Document {
     pub(crate) id: DocumentId,
     text: Rope,
@@ -380,7 +390,7 @@ fn apply_bom(encoding: &'static encoding::Encoding, buf: &mut [u8; BUF_SIZE]) ->
 pub fn from_reader<R: std::io::Read + ?Sized>(
     reader: &mut R,
     encoding: Option<&'static Encoding>,
-) -> Result<(Rope, &'static Encoding, bool), Error> {
+) -> Result<(Rope, &'static Encoding, bool), io::Error> {
     // These two buffers are 8192 bytes in size each and are used as
     // intermediaries during the decoding process. Text read into `buf`
     // from `reader` is decoded into `buf_out` as UTF-8. Once either
@@ -523,7 +533,7 @@ fn read_and_detect_encoding<R: std::io::Read + ?Sized>(
     reader: &mut R,
     encoding: Option<&'static Encoding>,
     buf: &mut [u8],
-) -> Result<(&'static Encoding, bool, encoding::Decoder, usize), Error> {
+) -> Result<(&'static Encoding, bool, encoding::Decoder, usize), io::Error> {
     let read = reader.read(buf)?;
     let is_empty = read == 0;
     let (encoding, has_bom) = encoding
@@ -685,11 +695,18 @@ impl Document {
         encoding: Option<&'static Encoding>,
         config_loader: Option<Arc<ArcSwap<syntax::Loader>>>,
         config: Arc<dyn DynAccess<Config>>,
-    ) -> Result<Self, Error> {
+    ) -> Result<Self, DocumentOpenError> {
+        // If the path is not a regular file (e.g.: /dev/random) it should not be opened.
+        if path
+            .metadata()
+            .map_or(false, |metadata| !metadata.is_file())
+        {
+            return Err(DocumentOpenError::IrregularFile);
+        }
+
         // Open the file if it exists, otherwise assume it is a new file (and thus empty).
         let (rope, encoding, has_bom) = if path.exists() {
-            let mut file =
+            let mut file = std::fs::File::open(path)?;
-                std::fs::File::open(path).context(format!("unable to open {:?}", path))?;
             from_reader(&mut file, encoding)?
         } else {
             let line_ending: LineEnding = config.load().default_line_ending.into();

helix-view/src/editor.rs

@@ -1,6 +1,8 @@
 use crate::{
     align_view,
-    document::{DocumentSavedEventFuture, DocumentSavedEventResult, Mode, SavePoint},
+    document::{
+        DocumentOpenError, DocumentSavedEventFuture, DocumentSavedEventResult, Mode, SavePoint,
+    },
     graphics::{CursorKind, Rect},
     handlers::Handlers,
     info::Info,
@@ -1677,7 +1679,7 @@ impl Editor {
     }
 
     // ??? possible use for integration tests
-    pub fn open(&mut self, path: &Path, action: Action) -> Result<DocumentId, Error> {
+    pub fn open(&mut self, path: &Path, action: Action) -> Result<DocumentId, DocumentOpenError> {
         let path = helix_stdx::path::canonicalize(path);
         let id = self.document_by_path(&path).map(|doc| doc.id);