From 94a9c81eb07e03fd7c439e8239041a864106c2b4 Mon Sep 17 00:00:00 2001 From: TiredTumblrina <144416919+TiredTumblrina@users.noreply.github.com> Date: Tue, 18 Jun 2024 12:14:17 -0400 Subject: [PATCH] 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 * Moved thiserror version configuration to the workspace instead of the individual packages. --------- Co-authored-by: Michael Davis --- Cargo.lock | 1 + Cargo.toml | 1 + helix-dap/Cargo.toml | 2 +- helix-lsp/Cargo.toml | 2 +- helix-term/src/application.rs | 38 +++++++++++++++++++++++------------ helix-term/src/commands.rs | 1 + helix-term/src/ui/mod.rs | 2 +- helix-view/Cargo.toml | 2 +- helix-view/src/document.rs | 29 ++++++++++++++++++++------ helix-view/src/editor.rs | 6 ++++-- 10 files changed, 59 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8c8e9748..c2f2735d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1535,6 +1535,7 @@ dependencies = [ "serde_json", "slotmap", "tempfile", + "thiserror", "tokio", "tokio-stream", "toml", diff --git a/Cargo.toml b/Cargo.toml index 6206281b..e3ee1031 100644 --- a/Cargo.toml +++ b/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" diff --git a/helix-dap/Cargo.toml b/helix-dap/Cargo.toml index 3521f589..c37340cc 100644 --- a/helix-dap/Cargo.toml +++ b/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" diff --git a/helix-lsp/Cargo.toml b/helix-lsp/Cargo.toml index c6c94b84..ab9251eb 100644 --- a/helix-lsp/Cargo.toml +++ b/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 diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index f71eed20..9adc764c 100644 --- a/helix-term/src/application.rs +++ b/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 - .open(&file, action) - .context(format!("open '{}'", file.to_string_lossy()))?; + let doc_id = match editor.open(&file, action) { + // Ignore irregular files during application init. + 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{}.", - nr_of_files, - 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); + + // if all files were invalid, replace with empty buffer + if nr_of_files == 0 { + editor.new_file(Action::VerticalSplit); + } else { + editor.set_status(format!( + "Loaded {} file{}.", + nr_of_files, + 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); } diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index 5fc83235..67955aec 100644 --- a/helix-term/src/commands.rs +++ b/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, diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index 5211c2e2..6a4655fd 100644 --- a/helix-term/src/ui/mod.rs +++ b/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, diff --git a/helix-view/Cargo.toml b/helix-view/Cargo.toml index 987746a2..a19357f7 100644 --- a/helix-view/Cargo.toml +++ b/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"] } diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 23b597a3..a56cbc2f 100644 --- a/helix-view/src/document.rs +++ b/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, } +#[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( 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( 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>>, config: Arc>, - ) -> Result { + ) -> Result { + // 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 = - std::fs::File::open(path).context(format!("unable to open {:?}", path))?; + let mut file = std::fs::File::open(path)?; from_reader(&mut file, encoding)? } else { let line_ending: LineEnding = config.load().default_line_ending.into(); diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 635f7261..3eeb4830 100644 --- a/helix-view/src/editor.rs +++ b/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 { + pub fn open(&mut self, path: &Path, action: Action) -> Result { let path = helix_stdx::path::canonicalize(path); let id = self.document_by_path(&path).map(|doc| doc.id);