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

Prevent improper files (like /dev/random) from being used as file arguments #10733

Merged
merged 12 commits into from
Jun 18, 2024

Conversation

TiredTumblrina
Copy link
Contributor

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)

@pascalkuthe
Copy link
Member

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

@TiredTumblrina
Copy link
Contributor Author

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

@TiredTumblrina
Copy link
Contributor Author

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:

> hx /dev/random
Error: unable to create new application

Caused by:
    0: open '/dev/random'
    1: Path argument /dev/random should lead is not a regular file or a symlink.

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?

@kirawi
Copy link
Member

kirawi commented May 12, 2024

You probably need to handle that error and just create an empty scratch buffer if the file can't open.

@TiredTumblrina
Copy link
Contributor Author

done, thank you all for the suggestions and advice!

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels May 15, 2024
Copy link
Member

@the-mikedavis the-mikedavis left a 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::Errors 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.

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 15, 2024
@TiredTumblrina
Copy link
Contributor Author

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).

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 15, 2024
Copy link
Member

@the-mikedavis the-mikedavis left a 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

helix-view/Cargo.toml Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 17, 2024
TiredTumblrina and others added 3 commits June 17, 2024 18:36
Accept suggestion in review.

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
@the-mikedavis the-mikedavis removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 18, 2024
@the-mikedavis the-mikedavis added the S-waiting-on-review Status: Awaiting review from a maintainer. label Jun 18, 2024
@pascalkuthe pascalkuthe merged commit 94a9c81 into helix-editor:master Jun 18, 2024
6 checks passed
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
…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 <mcarsondavis@gmail.com>

* Moved thiserror version configuration to the workspace instead of the individual packages.

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
mxxntype pushed a commit to mxxntype/helix that referenced this pull request Aug 14, 2024
…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 <mcarsondavis@gmail.com>

* Moved thiserror version configuration to the workspace instead of the individual packages.

---------

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants