-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Prevent improper files (like /dev/random) from being used as file arguments #10733
Conversation
I don't think the arg parsing is the right place to do this. We should fundamentally disallow opening non-files so it's consistent with :open/:vsplit and any other way a user would open a path |
That is a good point, I'll see if I can get this to work in a more interoperable way and will amend the PR |
This reverts commit c123944.
By making changes to the Document::open method I have the :open command working as expected, however when I revert the prior changes to argument parsing the application errors out ungracefully:
In this case, maybe it would be better to keep both checks so that the application does not crash when it gets an invalid input? |
You probably need to handle that error and just create an empty scratch buffer if the file can't open. |
done, thank you all for the suggestions and advice! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good to me but it seems like a nice opportunity for a small refactor since we start to care about what error type is returned by Editor::open
. Since we care about "which error" happened let's switch away from returning an anyhow::Error
and introduce a custom error type. We can write a pretty small one that doesn't need too many changes to the code that uses it with thiserror
(which we already depend on in other crates):
#[derive(Debug, Error)]
pub enum DocumentOpenError {
#[error("path must be a regular file, symlink or directory")]
IrregularFile,
#[error(transparent)]
IoError(#[from] io::Error),
}
Document::open
and all of the functions called within actually only return std::io::Error
s at the moment so this refactor is mostly about changing the return types of those functions from Result<Whatever, anyhow::Error>
to Result<Whatever, io::Error>
. Then when we see that the file isn't a regular file in Document::open
we can return Err(DocumentOpenError::IrregularFile)
and we can match on that error exactly in application.rs
.
Thanks for the review! I've taken a stab at the refactoring, although let me know if more changes are needed, as I'm not too sure if some of the changes I made are ideal (especially those in application.rs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I just have some small nits
Accept suggestion in review. Co-authored-by: Michael Davis <[email protected]>
… individual packages.
…uments (helix-editor#10733) * Implement check before adding path to files * fix problem where directories were removed from args.files * Revert "Implement check before adding path to files" This reverts commit c123944. * Dissallow opening of irregular non-symlink files * Fixed issue with creating new file from command line * Fixed linting error. * Optimized regularity check as suggested in review * Created DocumentOpenError Sum Type to switch on in Application * Forgot cargo fmt * Update helix-term/src/application.rs Accept suggestion in review. Co-authored-by: Michael Davis <[email protected]> * Moved thiserror version configuration to the workspace instead of the individual packages. --------- Co-authored-by: Michael Davis <[email protected]>
…uments (helix-editor#10733) * Implement check before adding path to files * fix problem where directories were removed from args.files * Revert "Implement check before adding path to files" This reverts commit c123944. * Dissallow opening of irregular non-symlink files * Fixed issue with creating new file from command line * Fixed linting error. * Optimized regularity check as suggested in review * Created DocumentOpenError Sum Type to switch on in Application * Forgot cargo fmt * Update helix-term/src/application.rs Accept suggestion in review. Co-authored-by: Michael Davis <[email protected]> * Moved thiserror version configuration to the workspace instead of the individual packages. --------- Co-authored-by: Michael Davis <[email protected]>
As per the suggestions in #10726 this pull request changes the argument parsing to only add a PathBuf to args.files if it is either a file or a symlink. This has been tested on the scenarios provided in the issue and simply opens an empty buffer as if no argument was provided in those cases. (instead of freezing)