Added option restrict-upload-dir #858

jonasdiemer · 2022-08-02T14:11:20Z

To fix #842

svenstaro · 2022-08-02T14:13:36Z

Cool, before I do a proper review, could you add a test to check that other directories are now indeed forbidden?

jonasdiemer · 2022-08-02T14:20:38Z

Sure, let me have a look into testing in Rust :)

svenstaro · 2022-08-02T14:29:01Z

You can take a look at the other tests concerning the upload. It should be somewhat straight forward. Just remember: If you messed up and the test doesn't pick it up, this is a security issue so we need to be really careful here. :)

jonasdiemer · 2022-08-02T15:41:22Z

@svenstaro thanks for the suggestion. Fixed an issue (running tests is actually helpful :)) and added a test similar to existing one uploading_files_is_prevented.

svenstaro

Good stuff! This will probably require a few rounds of reviews until we get there but i think for your first foray into Rust this is already looking pretty good.

tests/upload_files.rs

src/file_upload.rs

src/args.rs

jonasdiemer · 2022-08-03T11:04:03Z

Reworked the solution to use PathBuf and with this also fixed subdirectories not working.

Edit: improved (hopefully) the style a bit using iter().any().

@svenstaro I'd appreciate some feedback (esp. on improving the style). Thanks again! :)

src/file_upload.rs

src/args.rs

tests/upload_files.rs

svenstaro · 2022-08-11T03:14:17Z

I'd be happy to merge as-is. Please update the README.md with the new output of miniserve --help and fix the formatting issue and we're good to merge.

src/config.rs

svenstaro

Found a few tiny things but once we have that, we're good to go. Great job so far!

src/file_upload.rs

svenstaro · 2022-08-11T03:17:43Z

src/args.rs

+    /// Allowed upload directories (together with -u)
+    /// 
+    /// If this is set, uploads are only allowed into the provided directories. 
+    #[clap(long, requires = "file-upload", value_hint = ValueHint::FilePath)]


If you like, you can choose a short letter for this as well as it'd be annoying to write it out many times otherwise.

I'd go for 'A' (as in allowed...), but I am wondering if we might just want to re-purpose -u...

You mean something like -u alone would allow for uploading everywhere and then -u /tmp/lol would allow uploading only to that dir? If that's what you're thinking, I appreciate the conciseness but I can't help but wonder whether that wouldn't make -u pretty hard to parse (is it even still possible?) and somewhat confusing to users perhaps. Thoughts?

-A is a fine choice, I think.

You mean something like -u alone would allow for uploading everywhere and then -u /tmp/lol would allow uploading only to that dir? If that's what you're thinking, I appreciate the conciseness but I can't help but wonder whether that wouldn't make -u pretty hard to parse (is it even still possible?) and somewhat confusing to users perhaps. Thoughts?

Exactly, that was my thinking (after seeing so many command line shorts already taken ;)). Not sure about the parsing, though, I'd give it a try...

Does it parse this correctly? -u --some-other-flag? Would it try to parse the stuff behind -u as a PathBuf?

Just tried -u --verbose and it worked as expected (i.e. verbose output and allowed_upload_dir == Some([]) (i.e. it would allow uploads everywhere)

I've just committed a version that uses -u to define allowed directories. @svenstaro, please check it out and let me know what you think (I am also fine to use the previous version with -A as a shortcut).

I like the -u with min_values. Makes more sense. Might want to try how it does in windows shell. It should work in bash because we've to use ./-path when a file starts with - (e.g. touch ./-h) otherwise it'll be parsed as cli argument.

Unfortunately, I cannot try on Windows.

Directories starting with - would also need the ./ prefix, which doesn't work with my implementation (yet) - will have a look.

tests/upload_files.rs

svenstaro

Hey, I think we're almost there now. Only some tiny stuff now. I'd like to put this into the next miniserve release, too! Do you have some time to finish it up soon so we can land it with the release?

tests/upload_files.rs

svenstaro · 2022-08-15T00:06:43Z

src/args.rs

+    /// Allowed upload directories (together with -u)
+    /// 
+    /// If this is set, uploads are only allowed into the provided directories. 
+    #[clap(long, requires = "file-upload", value_hint = ValueHint::FilePath)]


-A is a fine choice, I think.

jonasdiemer · 2022-08-16T06:17:29Z

Hey, I think we're almost there now. Only some tiny stuff now. I'd like to put this into the next miniserve release, too! Do you have some time to finish it up soon so we can land it with the release?

Except for the decision for using -u or -A (and some cleanup in case we go with -u), it should be ready.

svenstaro · 2022-08-17T02:21:35Z

Please run cargo fmt to fix CI.

jonasdiemer · 2022-08-17T08:33:14Z

Fixed formatting and also sanitized paths so that specification of `-u ./some_dir' also works.

svenstaro · 2022-08-22T10:21:36Z

There's a small lint to fix there.

jonasdiemer · 2022-08-24T13:50:39Z

Looks like CICD on Windows fails, might have to do with the tests that include a dash in the directory name. Unfortunately, I don't have a windows machine to test on...

Atreyagaurav · 2022-08-24T16:19:40Z

I also don't have a Windows machine to build miniserve to test, but I made a colleague try to make a directory named "-h" and it was a success (I tried both with "New Folder" button and mkdir in cmd. cd -h & cd ./-hworked fine, but dir ./-h gave Invalid switch - "h".

I don't know if it's a good idea, but we might be able to use cfg to only test that on Linux? At least for now...

svenstaro · 2022-08-24T20:29:45Z

I'd be fine if we don't run that test on Windows.

jonasdiemer · 2022-08-30T15:06:44Z

As CI has test cases 3+4 failing, I am not sure it's related to the dot. Rather, it could be related to sub-directories (and possibly different path separator for Windows). I've pushed another commit that tries to fix the paths for windows, let's see if it passes CI. Suggestions welcome (struggling a bit as a noob without access to the target platform).

svenstaro · 2022-08-30T16:05:25Z

tests/upload_files.rs

 fn uploading_files_to_allowed_dir_works(
    #[case] server: TestServer,
-    #[case] upload_dirs: Vec<&str>,


Does it work if you take these as Path or OsPath? That might be a better way that just works in Windows as well.

tests/upload_files.rs

svenstaro · 2022-09-18T17:47:25Z

It seems like there's a conflict with the recent QR stuff merge. Could you rebase?

Co-authored-by: Sven-Hendrik Haase <svenstaro@gmail.com>

jonasdiemer · 2022-09-18T18:28:05Z

Rebase done, still need to fix the failing test for windows platform.

svenstaro · 2022-09-18T23:14:25Z

tests/upload_files.rs

+#[case(server(&["-u", "someDir"]), vec!["someDir".to_string()])]
+#[case(server(&["-u", "./-someDir"]), vec!["./-someDir".to_string()])]
+#[case(server(&["-u", if cfg!(windows) {r"someDir\some_sub_dir"} else {"someDir/some_sub_dir"}]),
+  vec!["someDir/some_sub_dir".to_string()])]
+#[case(server(&["-u", if cfg!(windows) {r"someDir\some_sub_dir"} else {"someDir/some_sub_dir"}, 
+                "-u", if cfg!(windows) {r"someDir\some_other_dir"} else {"someDir/some_other_dir"}]), 
+       vec!["someDir/some_sub_dir".to_string(), "someDir/some_other_dir".to_string()])]
+fn uploading_files_to_allowed_dir_works(
+    #[case] server: TestServer,
+    #[case] upload_dirs: Vec<String>,


I still don't really understand why you can't use a std::path::Path here as it will take care of the conversion for you on the operating systems. Wouldn't that be more convenient?

Yeah, I was thinking the same, but thought I'd stick first with they way other tests (see e.g. l. 179) handled it. Since that didn't work in this case (CI still failing), I will try the approach with std::path::Path.

We should probably change other tests as well. :)

Since it still doesn't work, I'd also be ok with you simply disabling the failing test cases on Windows.

Yup, and kind of hard to make progress without access to a windows machine. Not sure I'll find time to setup something (e.g. on Azure/CodeSpaces)...

@svenstaro, I don't know how I would disable a test-case for a specific platform. Could you give me a pointer?

Also noticed, that the feature doesn't work on windows with subdirectories (likely due to \ vs /) - found out by running miniserve.exe from publish_release action on an old windows laptop. Pushed a fix, waiting for Action workflow to verify.

Looks like I found a solution - latest commit passes CI on all platforms.

I assume the warning (the following packages contain code that will be rejected by a future version of Rust: winapi v0.2.8) has nothing to do with my PR.

From my perspective, the PR should be ready to merge now - please let me know if I overlooked something.

Re the 500 errors, I might have a stab at that if time permits.

Well, the error handling was easier to fix than I thought - fixed it (also for the case when overwrite_files is not set, i.e. DuplicateFileError).

svenstaro · 2022-09-20T00:31:08Z

At last! Outstanding work getting it to this state. It took a while but I hope you appreciate that for security stuff we need to go the extra mile. :)

jonasdiemer · 2022-09-20T05:28:16Z

Thanks a lot for your guidance (and patience), I leaned a lot and it was fun!

svenstaro requested changes Aug 2, 2022

View reviewed changes

tests/upload_files.rs Outdated Show resolved Hide resolved

tests/upload_files.rs Outdated Show resolved Hide resolved

tests/upload_files.rs Show resolved Hide resolved

src/file_upload.rs Outdated Show resolved Hide resolved

src/args.rs Outdated Show resolved Hide resolved

jonasdiemer requested a review from svenstaro August 3, 2022 12:53

svenstaro requested changes Aug 3, 2022

View reviewed changes

src/file_upload.rs Outdated Show resolved Hide resolved

src/file_upload.rs Outdated Show resolved Hide resolved

src/args.rs Outdated Show resolved Hide resolved

svenstaro reviewed Aug 3, 2022

View reviewed changes

tests/upload_files.rs Outdated Show resolved Hide resolved

tests/upload_files.rs Show resolved Hide resolved

svenstaro reviewed Aug 11, 2022

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

svenstaro requested changes Aug 11, 2022

View reviewed changes

svenstaro reviewed Aug 11, 2022

View reviewed changes

tests/upload_files.rs Outdated Show resolved Hide resolved

svenstaro requested changes Aug 15, 2022

View reviewed changes

jonasdiemer requested a review from svenstaro August 17, 2022 08:32

jonasdiemer closed this Aug 30, 2022

jonasdiemer reopened this Aug 30, 2022

svenstaro reviewed Aug 30, 2022

View reviewed changes

svenstaro reviewed Sep 18, 2022

View reviewed changes

tests/upload_files.rs Outdated Show resolved Hide resolved

jonasdiemer force-pushed the restrict-upload-dir branch from ec83ba7 to 753eed8 Compare September 18, 2022 18:14

Added option restrict-upload-dir

2662c59

jonasdiemer and others added 17 commits September 18, 2022 20:25

Avoid error message during testing of restricted

36b041a

Renamed option for more clarity

550ae01

Update src/file_upload.rs

54738df

Co-authored-by: Sven-Hendrik Haase <svenstaro@gmail.com>

renaming of option for clarity in test

40b7d26

clarity of comment

ed925ea

Co-authored-by: Sven-Hendrik Haase <svenstaro@gmail.com>

Clarity of comment

751bf58

Co-authored-by: Sven-Hendrik Haase <svenstaro@gmail.com>

Moved use to global

c1c7079

test case with two allowed dirs

0049bb2

check status code when restricted; fix formatting

384c2b8

Use argument -u instead of --allowed-upload-dir

e2ae526

sanitize allowed upload paths for cases like ./dir

5404e4f

cargo fmt

d905b68

Test dir with starting -

092b619

Improved output of -h

0d722da

fix lint errors

234422c

trying to handle paths in a way that works for windows

7e4d1cf

Fixed formatting

d34b411

jonasdiemer force-pushed the restrict-upload-dir branch from 753eed8 to d34b411 Compare September 18, 2022 18:27

Fixing (hopefully) issue with path on Windows

734f55d

svenstaro reviewed Sep 18, 2022

View reviewed changes

jonasdiemer added 3 commits September 19, 2022 10:22

Switched to using Path in test to handle windows platform.

851c8d9

Changed handling of allowed path to fix Windows

6577af2

Fixed clippy issue (single-char string to char)

5d11d6a

jonasdiemer requested a review from svenstaro September 19, 2022 14:21

jonasdiemer added 2 commits September 19, 2022 16:43

Return 403 instead of 500 for upload errs

20a055d

Merge branch 'svenstaro:master' into restrict-upload-dir

8aae523

svenstaro merged commit 5a68df1 into svenstaro:master Sep 20, 2022

Added option restrict-upload-dir #858

Added option restrict-upload-dir #858

Conversation

jonasdiemer commented Aug 2, 2022

svenstaro commented Aug 2, 2022

jonasdiemer commented Aug 2, 2022

svenstaro commented Aug 2, 2022

jonasdiemer commented Aug 2, 2022

svenstaro left a comment

Choose a reason for hiding this comment

jonasdiemer commented Aug 3, 2022 • edited Loading

svenstaro commented Aug 11, 2022

svenstaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenstaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasdiemer commented Aug 16, 2022

svenstaro commented Aug 17, 2022

jonasdiemer commented Aug 17, 2022

svenstaro commented Aug 22, 2022

jonasdiemer commented Aug 24, 2022

Atreyagaurav commented Aug 24, 2022

svenstaro commented Aug 24, 2022

jonasdiemer commented Aug 30, 2022 • edited Loading

svenstaro Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

svenstaro commented Sep 18, 2022

jonasdiemer commented Sep 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenstaro commented Sep 20, 2022

jonasdiemer commented Sep 20, 2022

jonasdiemer commented Aug 3, 2022 •

edited

Loading

jonasdiemer commented Aug 30, 2022 •

edited

Loading

svenstaro Aug 30, 2022 •

edited

Loading