fix(fs): improve file utime checks (#30872)

Improves the checks on FsFile.prototype.utime

Author: dsherret

cli/rt/file_system.rs

@@ -1254,6 +1254,12 @@ impl FileBackedVfsFile {
 
 #[async_trait::async_trait(?Send)]
 impl deno_io::fs::File for FileBackedVfsFile {
+  fn maybe_path(&self) -> Option<&Path> {
+    // ok because a vfs file will never be written to and this
+    // method is only used for checking write permissions
+    None
+  }
+
   fn read_sync(self: Rc<Self>, buf: &mut [u8]) -> FsResult<usize> {
     self.read_to_buf(buf).map_err(Into::into)
   }

ext/fs/lib.rs

@@ -178,8 +178,8 @@ deno_core::extension!(deno_fs,
     op_fs_funlock_sync,
     op_fs_ftruncate_sync,
     op_fs_file_truncate_async,
-    op_fs_futime_sync,
-    op_fs_futime_async,
+    op_fs_futime_sync<P>,
+    op_fs_futime_async<P>,
 
   ],
   esm = [ "30_fs.js" ],

ext/fs/ops.rs

@@ -1868,7 +1868,7 @@ pub async fn op_fs_file_truncate_async(
 }
 
 #[op2(fast)]
-pub fn op_fs_futime_sync(
+pub fn op_fs_futime_sync<P: FsPermissions + 'static>(
   state: &mut OpState,
   #[smi] rid: ResourceId,
   #[number] atime_secs: i64,
@@ -1878,12 +1878,19 @@ pub fn op_fs_futime_sync(
 ) -> Result<(), FsOpsError> {
   let file =
     FileResource::get_file(state, rid).map_err(FsOpsErrorKind::Resource)?;
+  if let Some(path) = file.maybe_path() {
+    _ = state.borrow::<P>().check_open(
+      Cow::Borrowed(path),
+      OpenAccessKind::WriteNoFollow,
+      "Deno.FsFile.prototype.utimeSync()",
+    )?;
+  }
   file.utime_sync(atime_secs, atime_nanos, mtime_secs, mtime_nanos)?;
   Ok(())
 }
 
 #[op2(async)]
-pub async fn op_fs_futime_async(
+pub async fn op_fs_futime_async<P: FsPermissions + 'static>(
   state: Rc<RefCell<OpState>>,
   #[smi] rid: ResourceId,
   #[number] atime_secs: i64,
@@ -1893,6 +1900,13 @@ pub async fn op_fs_futime_async(
 ) -> Result<(), FsOpsError> {
   let file = FileResource::get_file(&state.borrow(), rid)
     .map_err(FsOpsErrorKind::Resource)?;
+  if let Some(path) = file.maybe_path() {
+    _ = state.borrow().borrow::<P>().check_open(
+      Cow::Borrowed(path),
+      OpenAccessKind::WriteNoFollow,
+      "Deno.FsFile.prototype.utime()",
+    )?;
+  }
   file
     .utime_async(atime_secs, atime_nanos, mtime_secs, mtime_nanos)
     .await?;

ext/fs/std_fs.rs

@@ -85,15 +85,21 @@ impl FileSystem for RealFs {
     options: OpenOptions,
   ) -> FsResult<Rc<dyn File>> {
     let std_file = open_with_checked_path(options, path)?;
-    Ok(Rc::new(StdFileResourceInner::file(std_file)))
+    Ok(Rc::new(StdFileResourceInner::file(
+      std_file,
+      Some(path.to_path_buf()),
+    )))
   }
   async fn open_async<'a>(
     &'a self,
     path: CheckedPathBuf,
     options: OpenOptions,
   ) -> FsResult<Rc<dyn File>> {
     let std_file = open_with_checked_path(options, &path.as_checked_path())?;
-    Ok(Rc::new(StdFileResourceInner::file(std_file)))
+    Ok(Rc::new(StdFileResourceInner::file(
+      std_file,
+      Some(path.to_path_buf()),
+    )))
   }
 
   fn mkdir_sync(

ext/io/fs.rs

@@ -3,6 +3,7 @@
 use std::borrow::Cow;
 use std::fmt::Formatter;
 use std::io;
+use std::path::Path;
 #[cfg(unix)]
 use std::process::Stdio as StdStdio;
 use std::rc::Rc;
@@ -207,6 +208,10 @@ impl FsStat {
 
 #[async_trait::async_trait(?Send)]
 pub trait File {
+  /// Provides the path of the file, which is used for checking
+  /// metadata permission updates.
+  fn maybe_path(&self) -> Option<&Path>;
+
   fn read_sync(self: Rc<Self>, buf: &mut [u8]) -> FsResult<usize>;
   async fn read(self: Rc<Self>, limit: usize) -> FsResult<BufView> {
     let buf = BufMutView::new(limit);

ext/io/lib.rs

@@ -15,6 +15,8 @@ use std::os::fd::AsRawFd;
 use std::os::unix::io::FromRawFd;
 #[cfg(windows)]
 use std::os::windows::io::FromRawHandle;
+use std::path::Path;
+use std::path::PathBuf;
 #[cfg(unix)]
 use std::process::Stdio as StdStdio;
 use std::rc::Rc;
@@ -255,8 +257,9 @@ deno_core::extension!(deno_io,
           StdioPipeInner::Inherit => StdFileResourceInner::new(
             StdFileResourceKind::Stdin(stdin_state),
             STDIN_HANDLE.try_clone().unwrap(),
+            None,
           ),
-          StdioPipeInner::File(pipe) => StdFileResourceInner::file(pipe),
+          StdioPipeInner::File(pipe) => StdFileResourceInner::file(pipe, None),
         }),
         "stdin".to_string(),
       ));
@@ -267,8 +270,9 @@ deno_core::extension!(deno_io,
           StdioPipeInner::Inherit => StdFileResourceInner::new(
             StdFileResourceKind::Stdout,
             STDOUT_HANDLE.try_clone().unwrap(),
+            None,
           ),
-          StdioPipeInner::File(pipe) => StdFileResourceInner::file(pipe),
+          StdioPipeInner::File(pipe) => StdFileResourceInner::file(pipe, None),
         }),
         "stdout".to_string(),
       ));
@@ -279,8 +283,9 @@ deno_core::extension!(deno_io,
           StdioPipeInner::Inherit => StdFileResourceInner::new(
             StdFileResourceKind::Stderr,
             STDERR_HANDLE.try_clone().unwrap(),
+            None,
           ),
-          StdioPipeInner::File(pipe) => StdFileResourceInner::file(pipe),
+          StdioPipeInner::File(pipe) => StdFileResourceInner::file(pipe, None),
         }),
         "stderr".to_string(),
       ));
@@ -494,21 +499,27 @@ pub struct StdFileResourceInner {
   // to occur at a time
   cell_async_task_queue: Rc<TaskQueue>,
   handle: ResourceHandleFd,
+  maybe_path: Option<PathBuf>,
 }
 
 impl StdFileResourceInner {
-  pub fn file(fs_file: StdFile) -> Self {
-    StdFileResourceInner::new(StdFileResourceKind::File, fs_file)
+  pub fn file(fs_file: StdFile, maybe_path: Option<PathBuf>) -> Self {
+    StdFileResourceInner::new(StdFileResourceKind::File, fs_file, maybe_path)
   }
 
-  fn new(kind: StdFileResourceKind, fs_file: StdFile) -> Self {
+  fn new(
+    kind: StdFileResourceKind,
+    fs_file: StdFile,
+    maybe_path: Option<PathBuf>,
+  ) -> Self {
     // We know this will be an fd
     let handle = ResourceHandle::from_fd_like(&fs_file).as_fd_like().unwrap();
     StdFileResourceInner {
       kind,
       handle,
       cell: RefCell::new(Some(fs_file)),
       cell_async_task_queue: Default::default(),
+      maybe_path,
     }
   }
 
@@ -659,6 +670,10 @@ impl StdFileResourceInner {
 
 #[async_trait::async_trait(?Send)]
 impl crate::fs::File for StdFileResourceInner {
+  fn maybe_path(&self) -> Option<&Path> {
+    self.maybe_path.as_deref()
+  }
+
   fn write_sync(self: Rc<Self>, buf: &[u8]) -> FsResult<usize> {
     // Rust will line buffer and we don't want that behavior
     // (see https://github.com/denoland/deno/issues/948), so flush stdout and stderr.
@@ -1101,6 +1116,7 @@ impl crate::fs::File for StdFileResourceInner {
         cell: RefCell::new(Some(inner.try_clone()?)),
         cell_async_task_queue: Default::default(),
         handle: self.handle,
+        maybe_path: self.maybe_path.clone(),
       })),
       None => Err(FsError::FileBusy),
     }

tests/unit/utime_test.ts

@@ -28,6 +28,28 @@ Deno.test(
   },
 );
 
+Deno.test(
+  { permissions: { read: true, write: true } },
+  async function fsFileUtimeFailPermissions() {
+    const testDir = Deno.makeTempDirSync();
+    const filename = testDir + "/file.txt";
+    Deno.writeTextFileSync(filename, "");
+    Deno.permissions.revokeSync({ name: "write" });
+    using file = await Deno.open(filename, {
+      read: true,
+      write: false,
+    });
+
+    const atime = 1000;
+    const mtime = 50000;
+    await assertRejects(
+      () => file.utime(atime, mtime),
+      Deno.errors.NotCapable,
+      "Requires write access to",
+    );
+  },
+);
+
 Deno.test(
   { permissions: { read: true, write: true } },
   function futimeSyncSuccess() {
@@ -49,6 +71,28 @@ Deno.test(
   },
 );
 
+Deno.test(
+  { permissions: { read: true, write: true } },
+  function fsFileUtimeSyncFailPermissions() {
+    const testDir = Deno.makeTempDirSync();
+    const filename = testDir + "/file.txt";
+    Deno.writeTextFileSync(filename, "");
+    Deno.permissions.revokeSync({ name: "write" });
+    using file = Deno.openSync(filename, {
+      read: true,
+      write: false,
+    });
+
+    const atime = 1000;
+    const mtime = 50000;
+    assertThrows(
+      () => file.utimeSync(atime, mtime),
+      Deno.errors.NotCapable,
+      "Requires write access to",
+    );
+  },
+);
+
 Deno.test(
   { permissions: { read: true, write: true } },
   function utimeSyncFileSuccess() {