Improve error messages related to FS access

[aapoalas]

When a file is not found with, at least, Deno.realPathSync, the following error is printed:

error: Uncaught (in promise) NotFound: No such file or directory (os error 2)
      return Deno.realPathSync(join(baseDir, filePath));
                  ^
    at Object.realPathSync (deno:runtime/js/30_fs.js:146:16)

There are two unfortunate parts with this error message:

First and foremost: "No such file or directory" tells the error, but not what it's about. Seeing this error, I have no idea what file or directory the error relates to. It would generally be preferable (at least in my opinion) for the error message to contain not only what it was trying to do (the function being called) and the error it ran into (NotFound) but also what it was operating on (the name of the file).

The second much less important unfortunate part is the Object.realPathSync. The Deno object should probably have its ToStringTag symbol get reassigned so that the stack would actually print Deno.realPathSync, just for consistency's sake.

[Abhishek2nayak]

@aapoalas , I like to contribute on this issue. please assign it to me.

[jeiea]

It may be a trivial concern, but is it okay if the exposed parameter contains sensitive information like secret?

[aapoalas]

It may be a trivial concern, but is it okay if the exposed parameter contains sensitive information like secret?

I was thinking about this as well but decided in the end that no: If an API user has the permission to know the filename they want to open / read / write, then they have the permission to see it in an error message. And if a library user wants to stop the filename from leaking out, then it should catch the errors and possibly rethrow with a "sanitised" error.

[ghost]

My compile time is a little slow and I am just leaving this here for reference for myself / anyone else attempting to tackle the issue, as I am calling it a night. It took me a moment to track down where this was happening at.

realPathSync calls into the Rust function in runtime\ops\fs.rs. line 1169.

#[op]
fn op_realpath_sync(
  state: &mut OpState,
  path: String,
) -> Result<String, AnyError> {
  let path = PathBuf::from(&path);
  let permissions = state.borrow_mut::<Permissions>();
  println!("testing {}", path.display());
  permissions.read.check(&path, Some("Deno.realPathSync()"))?;
  if path.is_relative() {
    permissions.read.check_blind(
      &current_dir()?,
      "CWD",
      "Deno.realPathSync()",
    )?;
  }

  debug!("op_realpath_sync {}", path.display());
  // corresponds to the realpath on Unix and
  // CreateFile and GetFinalPathNameByHandle on Windows
  let realpath = canonicalize_path(&path)?;
  let realpath_str = into_string(realpath.into_os_string())?;
  Ok(realpath_str)
}

[ghost]

It seems that the error bubble's up from when op_realpath_sync calls into canonicalize_path

let realpath = canonicalize_path(&path)?;

An error handler here catches the error and allows customization of the error being displayed to the user. This also seems to the exact case for op_realpath_async. The other methods that I have looked at seem to have some appropriate error handling where as these two do not. Perhaps we could put together a list that we feel needs improvement?

[ghost]

@aapoalas Oh ic, creating a generalized function like below in that PR just you mentioned was something I was considering. I might wait until that get's closed out to continue with this. Thanks for the info!

fn create_err_mapper(desc: String) -> impl Fn(Error) -> Error {
  move |err: Error| Error::new(err.kind(), format!("{err}, {desc}"))
}

[andrewnester]

Since https://github.com/denoland/deno/pull/18404 is merged, should this issue be closed?