From 780930ec7dd0a334e7ea944e7454e18c9bdb12fc Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Wed, 10 Apr 2024 17:55:14 -0400 Subject: [PATCH 1/4] read symlink --- helix-view/src/document.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index f26ba8b97851..3587263e5792 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -900,8 +900,9 @@ impl Document { "Path is read only" )); } + let write_path = std::fs::read_link(&path).unwrap_or_else(|_| path.clone()); let backup = if path.exists() { - let path_ = path.clone(); + let path_ = write_path.clone(); // hacks: we use tempfile to handle the complex task of creating // non clobbered temporary path for us we don't want // the whole automatically delete path on drop thing @@ -925,7 +926,7 @@ impl Document { }; let write_result: anyhow::Result<_> = async { - let mut dst = tokio::fs::File::create(&path).await?; + let mut dst = tokio::fs::File::create(&write_path).await?; to_writer(&mut dst, encoding_with_bom_info, &text).await?; Ok(()) } @@ -934,14 +935,13 @@ impl Document { if let Some(backup) = backup { if write_result.is_err() { // restore backup - let _ = tokio::fs::rename(&backup, &path) + let _ = tokio::fs::rename(&backup, &write_path) .await .map_err(|e| log::error!("Failed to restore backup on write failure: {e}")); } else { // copy metadata and delete backup - let path_ = path.clone(); let _ = tokio::task::spawn_blocking(move || { - let _ = copy_metadata(&backup, &path_) + let _ = copy_metadata(&backup, &write_path) .map_err(|e| log::error!("Failed to copy metadata on write: {e}")); let _ = std::fs::remove_file(backup) .map_err(|e| log::error!("Failed to remove backup file on write: {e}")); From 6e0b4d30478c42c8aca593b735746b96309d5f57 Mon Sep 17 00:00:00 2001 From: Kirawi <67773714+kirawi@users.noreply.github.com> Date: Wed, 10 Apr 2024 19:15:38 -0400 Subject: [PATCH 2/4] use tokio::fs::read_link Co-authored-by: Pascal Kuthe --- helix-view/src/document.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 3587263e5792..59b926b81f9a 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -900,7 +900,7 @@ impl Document { "Path is read only" )); } - let write_path = std::fs::read_link(&path).unwrap_or_else(|_| path.clone()); + let write_path = tokio::fs::read_link(&path).await.unwrap_or_else(|_| path.clone()); let backup = if path.exists() { let path_ = write_path.clone(); // hacks: we use tempfile to handle the complex task of creating From 0adc9a89c1516f42d3ddca56a89220928096f1af Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Thu, 11 Apr 2024 11:35:08 -0400 Subject: [PATCH 3/4] add integration test --- helix-term/tests/test/commands/write.rs | 38 +++++++++++++++++++++++++ helix-view/src/document.rs | 4 ++- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index c9280bbd49d5..df5e1b8f706f 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -529,6 +529,44 @@ async fn test_write_all_insert_final_newline_do_not_add_if_unmodified() -> anyho Ok(()) } +#[tokio::test(flavor = "multi_thread")] +async fn test_symlink_write() -> anyhow::Result<()> { + #[cfg(unix)] + use std::os::unix::fs::symlink; + #[cfg(not(unix))] + use std::os::windows::fs::symlink_file as symlink; + + let dir = tempfile::tempdir()?; + + let mut file = tempfile::NamedTempFile::new_in(&dir)?; + let symlink_path = dir.path().join("linked"); + symlink(file.path(), &symlink_path)?; + + let mut app = helpers::AppBuilder::new() + .with_file(&symlink_path, None) + .build()?; + + test_key_sequence( + &mut app, + Some("ithe gostak distims the doshes:w"), + None, + false, + ) + .await?; + + reload_file(&mut file).unwrap(); + let mut file_content = String::new(); + file.as_file_mut().read_to_string(&mut file_content)?; + + assert_eq!( + LineFeedHandling::Native.apply("the gostak distims the doshes"), + file_content + ); + assert!(symlink_path.is_symlink()); + + Ok(()) +} + async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 59b926b81f9a..4b3b02f977c9 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -900,7 +900,9 @@ impl Document { "Path is read only" )); } - let write_path = tokio::fs::read_link(&path).await.unwrap_or_else(|_| path.clone()); + let write_path = tokio::fs::read_link(&path) + .await + .unwrap_or_else(|_| path.clone()); let backup = if path.exists() { let path_ = write_path.clone(); // hacks: we use tempfile to handle the complex task of creating From ed77676f854d0c9ce2c79026862448017466f2c8 Mon Sep 17 00:00:00 2001 From: kirawi <67773714+kirawi@users.noreply.github.com> Date: Thu, 11 Apr 2024 16:39:48 -0400 Subject: [PATCH 4/4] add readonly test --- helix-term/tests/test/commands/write.rs | 37 +++++++++++++++++++++++++ helix-term/tests/test/helpers.rs | 12 ++++++++ helix-view/src/document.rs | 8 +++--- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/helix-term/tests/test/commands/write.rs b/helix-term/tests/test/commands/write.rs index df5e1b8f706f..350f34aab27e 100644 --- a/helix-term/tests/test/commands/write.rs +++ b/helix-term/tests/test/commands/write.rs @@ -567,6 +567,43 @@ async fn test_symlink_write() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread")] +async fn test_symlink_write_fail() -> anyhow::Result<()> { + #[cfg(unix)] + use std::os::unix::fs::symlink; + #[cfg(not(unix))] + use std::os::windows::fs::symlink_file as symlink; + + let dir = tempfile::tempdir()?; + + let file = helpers::new_readonly_tempfile_in_dir(&dir)?; + let symlink_path = dir.path().join("linked"); + symlink(file.path(), &symlink_path)?; + + let mut app = helpers::AppBuilder::new() + .with_file(&symlink_path, None) + .build()?; + + test_key_sequence( + &mut app, + Some("ihello:wq"), + Some(&|app| { + let mut docs: Vec<_> = app.editor.documents().collect(); + assert_eq!(1, docs.len()); + + let doc = docs.pop().unwrap(); + assert_eq!(Some(&path::normalize(&symlink_path)), doc.path()); + assert_eq!(&Severity::Error, app.editor.get_status().unwrap().1); + }), + false, + ) + .await?; + + assert!(symlink_path.is_symlink()); + + Ok(()) +} + async fn edit_file_with_content(file_content: &[u8]) -> anyhow::Result<()> { let mut file = tempfile::NamedTempFile::new()?; diff --git a/helix-term/tests/test/helpers.rs b/helix-term/tests/test/helpers.rs index d7205b0c046a..70b3f4022c00 100644 --- a/helix-term/tests/test/helpers.rs +++ b/helix-term/tests/test/helpers.rs @@ -305,6 +305,18 @@ pub fn new_readonly_tempfile() -> anyhow::Result { Ok(file) } +/// Creates a new temporary file in the directory that is set to read only. Useful for +/// testing write failures. +pub fn new_readonly_tempfile_in_dir( + dir: impl AsRef, +) -> anyhow::Result { + let mut file = tempfile::NamedTempFile::new_in(dir)?; + let metadata = file.as_file().metadata()?; + let mut perms = metadata.permissions(); + perms.set_readonly(true); + file.as_file_mut().set_permissions(perms)?; + Ok(file) +} pub struct AppBuilder { args: Args, config: Config, diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 4b3b02f977c9..d44b4240c7f2 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -893,16 +893,16 @@ impl Document { } } } + let write_path = tokio::fs::read_link(&path) + .await + .unwrap_or_else(|_| path.clone()); - if readonly(&path) { + if readonly(&write_path) { bail!(std::io::Error::new( std::io::ErrorKind::PermissionDenied, "Path is read only" )); } - let write_path = tokio::fs::read_link(&path) - .await - .unwrap_or_else(|_| path.clone()); let backup = if path.exists() { let path_ = write_path.clone(); // hacks: we use tempfile to handle the complex task of creating