Review changes with  

Changed Files

File	Status
src/directory_listing.rs	1% smaller
Cargo.lock	Unsupported file format
Cargo.toml	Unsupported file format
docs/content/configuration/command-line-arguments.md	Unsupported file format
docs/content/configuration/config-file.md	Unsupported file format
docs/content/configuration/environment-variables.md	Unsupported file format
docs/content/features/directory-listing.md	Unsupported file format
src/directory_listing_download.rs	0% smaller
src/handler.rs	0% smaller
src/lib.rs	0% smaller
src/server.rs	0% smaller
src/settings/cli.rs	0% smaller
src/settings/file.rs	0% smaller
src/settings/mod.rs	0% smaller
src/static_files.rs	0% smaller
src/testing.rs	0% smaller
tests/dir_listing.rs	0% smaller
tests/dir_listing_download.rs	0% smaller
tests/static_files.rs	0% smaller

This is an early draft of the feature, implementing a prototype for #67.

Disclaimer: I do not have much experience in async Rust (or even that much experience in Rust in general), my implementation might be subpar/unidiomatic. All kind of feedback is very much appreciated.

Sorry for the delay, here are a few comments on the feature. I will add more if needed during the next days.
Regarding the code, I will comment on that directly.

But let me know what your thoughts are on this.

--

• UPDATE: I feel like query param is the sanest way to go, unless I missed any other caveats or considerations.

Agree, let's go with a query param for the directory download.
I suggest this query: /?download that will be the short version of /?download=tarball. Let's support only the first (/?download) for now. If we want to add zip or another format in the future then we can just extend the query to be like the second (/?download=tarball|zipball|etc).

• Since this will technically "list directory", if an option is used to toggle this feature, it should respect the directory listing option and possibly warn user/error on conflicting combination.

What do you mean exactly by "conflicting combination"?
The question is, do we want to make this directory download feature dependent on the directory listing, or should this feature be standalone and also complement the directory listing instead (if enabled)?

For example, I think this feature will be behind an option flag like directory-download=tarball (just an idea), but it could work regardless of the directory listing. Of course, if the directory listing is enabled with the download, we have to display the link in the HTML template.

About the link text on the directory listing.

I believe we can just say "download tarball" or "download tar.gz" whatever fits better. Something like:

Sorry for the delay

Hey, please take your time, don't worry about it! Thanks for the review too!

I suggest this query: /?download that will be the short version of /?download=tarball.

Yeap sure, download will do just fine. It is not too relevant for now, but does tarball always equate to gzipped tar? What if we want to support XZ or bzip2?

What do you mean exactly by "conflicting combination"?

IMO in some cases, directory-listing will be treated as a security toggle, i.e. in no way expose the list of content in the directory. Thus, if somehow the user disabled directory-listing but enabled directory-download, I guess that will be conflicting, in the sense that the content can now be leaked by downloading the tarball?

I believe we can just say "download tarball" or "download tar.gz" whatever fits better.

Yeap sure, sounds great!

Yeap sure, download will do just fine. It is not too relevant for now, but does tarball always equate to gzipped tar? What if we want to support XZ or bzip2?

Good question, I was thinking of only supporting in the future tar.gz and zip as those two are the popular ones (Unix/Windows).
To be consistent, let's forget the tarball or zipball thing and use the extension as a reference.
We will use /?download (which means tar.gz) and later expand it to /?download=tar.gz or /?download=zip if needed in the future.

The text link can also be download tar.gz.

IMO in some cases, directory-listing will be treated as a security toggle, i.e. in no way expose the list of content in the directory. Thus, if somehow the user disabled directory-listing but enabled directory-download, I guess that will be conflicting, in the sense that the content can now be leaked by downloading the tarball?

I got it. In that case, let's make it dependent on directory-listing=true, so if a user needs download functionality, the user has to enable it like this:

$ static-web-server -p 1234 -d ./public \
    --directory-listing --directory-listing-download=tar.gz

The --directory-listing-download option should be empty by default (which means it is disabled).
This will also allow us later if we need to support zip, to just expand it to a list like --directory-listing-download=tar.gz,zip.

I think it's almost ready for review, just a few questions/confirmations:

• This feature should probably respect the ignore_hidden_files option as well, to implement that I think we have to replace the append_dir_all with our own implementation. Do we want to do this in this commit as well?
• I tried dancing with clap and the current state is the best I can do:
  • --directory-listing-download=targz to enable
  • --directory-listing-download=none to disable (mixing none with other non-none values will panic, should we handle this differently as well?, I can't find a way to do it all in clap, yet)
  • future: --directory-listing-download=targz,zip,...
  • this flag requires -z (or the long one)
• on error handling:
  • I think we can still try our best to respond if error occurs when making the header (most likely due to code changes/upstream behavior changes), instead of responding with 500?
  • For compression task, the only way to signal error is by calling sender.abort(), which IMO doesn't do much anyway. The most likely error seems to be due to broken pipe, i.e. user aborted the download midway. In that situation, there is nothing we can do since the session has already been closed anyway.

• This feature should probably respect the ignore_hidden_files option as well, to implement that I think we have to replace the append_dir_all with our own implementation. Do we want to do this in this commit as well?

Yes, since ignore_hidden_files skips dotfiles, we have to implement it. It may not be a big deal to implement it. See https://docs.rs/async-tar/latest/src/async_tar/builder.rs.html#585.

• I tried dancing with clap and the current state is the best I can do:
  • --directory-listing-download=targz to enable
  • --directory-listing-download=none to disable (mixing none with other non-none values will panic, should we handle this differently as well?, I can't find a way to do it all in clap, yet)
  • future: --directory-listing-download=targz,zip,...
  • this flag requires -z (or the long one)

What if we represent the directory_listing_download as Vec<Option<DirDownloadFmt>> and remove the enum's None case of DirDownloadFmt (not tested)?

I will investigate from my side as well. It would be much better to support an enum, but if not possible, we could use a simple String instead, where an empty string means disabled. Remember that if we add zip archive support, we could use both, like --directory-listing-download=targz,zip. If we use a string, then only check for s.is_empty() or s == "targz".
In the future, we could read just the string value and split it by a comma separator.

• on error handling:

  • I think we can still try our best to respond if error occurs when making the header (most likely due to code changes/upstream behavior changes), instead of responding with 500?
  • For compression task, the only way to signal error is by calling sender.abort(), which IMO doesn't do much anyway. The most likely error seems to be due to broken pipe, i.e. user aborted the download midway. In that situation, there is nothing we can do since the session has already been closed anyway.

This sounds fine to me. If we do not panic the server thread, we are fine.

What if we represent the directory_listing_download as Vec<Option<DirDownloadFmt>> and remove the enum's None case of DirDownloadFmt (not tested)?

Do we absolutely need to allow explicitly disabling using --directory-listing-download=, or will the lack of said option be sufficient? If that's the case, None can be removed.