Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add directory redirection handling to cowboy_static #1588

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

richcarl
Copy link

@richcarl richcarl commented Nov 9, 2022

Directory accesses with no trailing slash will by default be redirected (301) to their canonical slashed version. New handler option 'directory_index' can be set to 'false' to disable, or to another status code such as 302.

Directory accesses with a trailing slash can be redirected (301) to an index file. This is off by default. New handler option 'directory_index' can be set to 'true' or a status code such as 302, for the default "index.html", or to a binary string such as <<"index.php">>, or to a pair {Code, Filename}.

Motivation: I think cowboy_static is the correct place for handling this. In principle, it's not a confirmed directory access until the request has been handled to cowboy_static, and it has checked what the file path points to. Furthermore, the feature ought to work also for archives. Hence, doing this in a middleware module (which I did at first to verify my approach) duplicates effort, and runs the risk of doing something different from what cowboy_static does, or perform a redirection on a path that should not be handled as a static file.

I also feel that these features are a minimal requirement for using cowboy to serve a static web site, and that cowboy should handle it out of the box rather than requiring a user to learn how to implement middleware modules. The code is straightforward and does not complicate the module much.

Example route for a static site:

{"/[...]", cowboy_static, {dir, "webroot", [{directory_index, true}]}}

See also #1346.

Directory accesses with no trailing slash will by default be redirected
(301) to their canonical slashed version. New handler option 'directory_index'
can be set to 'false' to disable, or to another status code such as 302.

Directory accesses with a trailing slash can be redirected (301) to an
index file. This is off by default. New handler option 'directory_index' can
be set to 'true' or a status code such as 302, for the default "index.html",
or to a binary string such as <<"index.php">>, or to a pair {Code, Filename}.
@essen
Copy link
Member

essen commented Nov 9, 2022

I agree that Cowboy should support this out of the box, but I still disagree that this is where it should be done. I do not agree that there is a need to check for directory access on disk before looking for an index file. If we take nginx for example, it works like this: https://nginx.org/en/docs/http/ngx_http_index_module.html

The ngx_http_index_module module processes requests ending with the slash character (‘/’).

The server sees the URI ends with / and then checks whether any of the provided index files exist and serves it if that is the case. I think that this is the correct approach: to only consider the URI when making this decision. Nginx also mentions:

It should be noted that using an index file causes an internal redirect

Which is what I believe should happen in most cases, not a 3xx redirect (but it should be configurable).

So the solution of a URI rewrite handler seems more appropriate to me, and that handler can easily take one or more function that would match the URI and rewrite it internally (or externally), and we can even provide a function built-in for the common cases such as index files or forced redirects.

The outline in #1346 is probably not good enough though because the fun should take a full URI (binary? map?) and not just a path, and maybe allow an extra argument for configurability, but the general idea is still valid IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants