-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve error messages related to FS access #17526
Comments
@aapoalas , I like to contribute on this issue. please assign it to me. |
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. |
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 #[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(
¤t_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)
} |
It seems that the error bubble's up from when 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 |
@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}"))
} |
Since #18404 is merged, should this issue be closed? |
When a file is not found with, at least,
Deno.realPathSync
, the following error is printed: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
. TheDeno
object should probably have itsToStringTag
symbol get reassigned so that the stack would actually printDeno.realPathSync
, just for consistency's sake.The text was updated successfully, but these errors were encountered: