Skip to content
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

fix: do not panic running invalid file specifier #25530

Merged

Conversation

yazan-abdalrahman
Copy link
Contributor

@yazan-abdalrahman yazan-abdalrahman commented Sep 9, 2024

This update prevents a panic in specifier_to_file_path on Windows by validating the file URL before calling to_file_path(). It ensures that:

Paths are checked to be absolute and are converted if needed.
The path is verified to exist before passing it to to_file_path(), ensuring invalid paths don't cause the application to crash.
This fix gracefully handles invalid or non-standard file URLs, avoiding panics during path resolution.

fix #20056

Co-authored-by: Bedis Nbiba bedisnbiba@gmail.com

@yazan-abdalrahman
Copy link
Contributor Author

@bartlomieju @dsherret
Please verify this solution again. I overwrite it

… handle-invalid-path-error-denoland#20062

# Conflicts:
#	cli/graph_util.rs
#	cli/lsp/config.rs
#	runtime/permissions/lib.rs
@dsherret dsherret changed the title fix: handle invalid path error fix: do not panic running invalid file specifier Sep 17, 2024
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@dsherret dsherret enabled auto-merge (squash) September 17, 2024 18:09
file:/// URL, it should successfully resolve to a valid file path
auto-merge was automatically disabled September 18, 2024 09:58

Head branch was pushed to by a user without write access

@yazan-abdalrahman
Copy link
Contributor Author

yazan-abdalrahman commented Sep 18, 2024

@dsherret

Why don’t we check if the file exists directly within the specifier_to_file_path method instead of handling it separately in places that use this method?

The current approach in specifier_to_file_path checks only for the validity of the URL format but does not ensure that the file it points to exists. This leads to issues in different parts of the code where it didn't handle invalid or non-existent paths might cause panics, such as when running scripts.

For example, I insert this code to handle the running script to prevent it from panic if file doesn't exist like deno run 'file:///a/b.ts' , but u revert it.

  for path in &start_paths {
           if !path.exists() || !path.is_dir() || path == Path::new("/") {
             let path_str = match &flags.subcommand {
               DenoSubcommand::Run(run) => &run.script,
               _ => path.to_str().unwrap_or("Invalid path"),
             };
             return Err(deno_core::error::uri_error(format!(
               "Invalid file path.\n  Specifier: {path_str}"
             )));
           }
         }
		       if let Ok(file_path) = specifier_to_file_path(&module_specifier) {
              if let Some(parent_path) = file_path.parent() {
                Some(vec![parent_path.to_path_buf()])
              } else {
                Some(vec![file_path.to_path_buf()])
              }

I think we should handle the file existence check directly within specifier_to_file_path. so, we avoid handling this check in multiple places, reducing the risk of missing it and preventing potential panics when paths don’t exist. This centralizes the validation, ensuring both the specifier format is valid and the file exists in one step, simplifying the overall error handling.

If we centralize the file existence check in specifier_to_file_path, we'll need to update tests that currently use valid specifiers but point to non-existent files. These tests should be modified to add actual files to the test data to ensure they pass.

@dsherret
Copy link
Member

Why don’t we check if the file exists directly within the specifier_to_file_path method instead of handling it separately in places that use this method?

That would be slow. Additionally, we don't always use specifier_to_file_path with paths that exist.

For example, I insert this code to handle the running script to prevent it from panic if file doesn't exist like deno run 'file:///a/b.ts' , but u revert it.

That code didn't have a test that failed after it was removed and I don't believe it's the right approach (instead stuff should fail when it goes to load the file)

@dsherret dsherret merged commit bed4647 into denoland:main Sep 18, 2024
17 checks passed
@dsherret dsherret mentioned this pull request Oct 2, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Panic when trying to run invalid file
2 participants