feat(server): do path joins more safely

[RealOrangeOne]

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]

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 ⚠️

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

Additional details and impacted files

@@            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	Coverage Δ
unit-tests	71.20% <94.59%> (+0.32%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tessus]

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

[tessus]

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]

Yup, I also think that we should handle errors.

[RealOrangeOne]

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]

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]

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]

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]

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]

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]

Thanks for your contribution!