Add option to HTTP::StaticFileHandler initializer to disable directory listing #4403
Conversation
joaodiogocosta
force-pushed
the
static-file-handler-optional-directory-listing
branch
from
9c03dd7 to
70cdd3f
Compare
| @@ -14,9 +14,10 @@ class HTTP::StaticFileHandler | |||
| # If *fallthrough* is `false`, this handler does not call next handler when | |||
| # request method is neither GET or HEAD, then serves `405 Method Not Allowed`. | |||
| # Otherwise, it calls next handler. | |||
| def initialize(public_dir : String, fallthrough = true) | |||
| def initialize(public_dir : String, fallthrough = true, list_dirs = true) | |||
There was a problem hiding this comment.
A mention of list_dirs in the docs above would be necessary.
There was a problem hiding this comment.
Sure, forgot that. Done.
joaodiogocosta
force-pushed
the
static-file-handler-optional-directory-listing
branch
from
70cdd3f to
ac3e536
Compare
|
As this feature is commonly referred to as directory listing, I would suggest to name the flag argument |
|
I wonder if there shouldn't be two handlers: one for listing and navigating directory indexes, and another for serving static files. |
|
@straight-shoota that was my first thought. I ended up renaming it to |
|
@ysbaddaden the first time I saw mentioned the static file handler, I imagined it just as it is, an handler that serves static files and directories. What I mean is that, for me, it's implicit that an handler that handles static files, also handles directories. This is good user experience. If the handlers are to be split, I foresee a lot of the same logic in both places (knowing that one can extract the shared bit, of course). Another downside is perhaps the fact that it would be a breaking change of the current API. |
| @@ -49,16 +53,17 @@ class HTTP::StaticFileHandler | |||
|
|
|||
| file_path = File.join(@public_dir, expanded_path) | |||
| is_dir = Dir.exists? file_path | |||
| is_file = File.exists?(file_path) && !is_dir | |||
There was a problem hiding this comment.
Minor improvement: changing the order means one less system call for dirs: !is_dir && File.exists?(file_path)
There was a problem hiding this comment.
@straight-shoota nice catch!
|
I don't think splitting this into two handlers would be a good idea. As @joaodiogocosta mentioned, most of the logic is the same. There are only a few specific parts for the directory listing apart from the ECR template. |
joaodiogocosta
force-pushed
the
static-file-handler-optional-directory-listing
branch
from
ac3e536 to
8c1dca7
Compare
|
Well, serving assets is checking whether the file exists and serve it. Indexing directories is checking whether a directory exists and list its contents. I don't see much overlap, except for expanding request.path from the public path. |
|
I thought of proposing to split them, but it's hard to imagine that someone will want directory listing without static file. Splitting them will mean to configure two handlers in the same way. Although currently is just the directory to serve, if we add in the future the path to where be mounted it will be more setting to duplicate if the handlers are split. I like the |
joaodiogocosta
force-pushed
the
static-file-handler-optional-directory-listing
branch
from
8c1dca7 to
5768fc6
Compare
|
@bcardiff agree. As a user, like me, it just seems strange to have to set up separate handlers for this.
|
|
LGTM too. @bcardiff, anything else left before merging? |
|
Great 👍 |
|
Thanks @joaodiogocosta ! Happy first contribution :-) |
|
Yey! Looking forward to use this in Soil 👍 |
This PR follows the discussion on #4398.
Reason:
At this time, using
HTTP::StaticFileHandlerone can configure which directory to serve static files from by passing its path to the constructor:This will serve files from
public/in the root path/and currently there is no way to change it. Additionally, it will display by default a directory list if a path to a directory is given.In some cases, such as web frameworks, it's preferable not to let the user list files under a public directory for reasons like security.
In this sense, this PR adds an option to
HTTP::StaticFileHandlerthat enables the user to turn off directory listing, preserving the default behaviour.Code Changes:
Dir.exists?call, resulting in one less system call.HTTP::StaticFileHandler. The name of the argument isdirectory_listingand defaults to true, as it was before.The default behaviour remains the same.