feat(server): do path joins more safely

RealOrangeOne commented 6 months ago

Description

Check that a joined path is still a child of the original path.

Motivation and Context

When joining paths, there's little protection against directory traversal. From testing, Actix seems to nicely sanitises these in the request, which is great, but it's still better to add a little protection.

How Has This Been Tested?

Unit tests have been added, which pass.

Changelog Entry

Types of Changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Documentation (no code change)
[ ] Refactor (refactoring production code)
[ ] Other

Checklist:

[ ] My code follows the code style of this project.
[ ] I have updated the documentation accordingly.
[ ] I have formatted the code with rustfmt.
[ ] I checked the lints with clippy.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

codecov-commenter commented 6 months ago

Codecov Report

Attention: Patch coverage is 94.59459% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 71.20%. Comparing base (db971e6) to head (0f83176).

Files	Patch %	Lines
src/main.rs	0.00%	2 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #247 +/- ## ========================================== + Coverage 70.88% 71.20% +0.32% ========================================== Files 11 11 Lines 625 639 +14 ========================================== + Hits 443 455 +12 - Misses 182 184 +2 ``` | [Flag](https://app.codecov.io/gh/orhun/rustypaste/pull/247/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Orhun+Parmaks%C4%B1z) | Coverage Δ | | |---|---|---| | [unit-tests](https://app.codecov.io/gh/orhun/rustypaste/pull/247/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Orhun+Parmaks%C4%B1z) | `71.20% <94.59%> (+0.32%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Orhun+Parmaks%C4%B1z#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tessus commented 6 months ago

@orhun it seems that you have to approve the CI for the 2 PRs.

tessus commented 6 months ago

But what I could ascertain from a quick look at the code is that prior to this PR errors were handled gracefully. After this PR rustypaste will panic. Not sure I like this.

orhun commented 6 months ago

Yup, I also think that we should handle errors.

RealOrangeOne commented 6 months ago

There's only 1 panic this introduces, and it's during server startup. Sure, I could facade it to convert a path issue to IoResult, and let Rust deal with returning that one, or I can call expect. The latter felt fine for a quick change.

tessus commented 6 months ago

What about the occurrences of PasteType::Url.get_path(&config.server.upload_path).unwrap()?

Isn't it true that these can panic?

P.S.: but I also noticed the change from fs::create_dir_all(paste_type.get_path(&server_config.upload_path))? to use an expect, which is probably what you meant in your previous comment.

RealOrangeOne commented 6 months ago

There are other uses of unwrap, but those are strictly in tests, which I've not considered to be an issue. If a test panics, it's functionally the same as a failure, which is what we want.

Yes, the fs::create_dir_all is the only new place the application could panic.

tessus commented 6 months ago

but those are strictly in tests

Oops, once again the gh diff failed me again. all the test headers were not in the code, thus I didn't see that those were tests. All good then.

Yes, the fs::create_dir_all is the only new place the application could panic.

I would rather see an error message than a panic. But it's up to orhun.

orhun commented 6 months ago

Yes, it would be better to print out an error message than panicking. Also, there are some lints about the usage of unwrap in tests 🙂 we can simply replace them with expect.

orhun commented 6 months ago

Looks like there is a test failure:

 ---- util::tests::test_safe_join_path stdout ----
thread 'util::tests::test_safe_join_path' panicked at src/util.rs:211:9:
assertion failed: safe_path_join("/foo", "/foobar").is_ok()
stack backtrace:

orhun / rustypaste