caddy: Bug: Caddy v2.4.2 with specified rewrite rules causes unexpected 308 redirection

I think I came up with a solution that works in all 3 main cases I’ve tested: only redirect if the base element of the path (the filename) is the same in both the original request and the rewritten request. In other words, do not redirect if the filename in the path was changed/rewritten internally. (The logic being, if the admin wanted to rewrite to the canonical path, they would have.)

This seems to prevent the file server from stepping on the toes of intentional rewrites, while still enforcing path canonicalization when, for example, only the prefix of the path is changed (as in handle_path).

For all the test cases below, I found it helpful to turn on debug mode:

{
    debug
}

Test case 1

This config (used on my Expert Caddy website) used to cause a redirect loop:

localhost

root * /home/matt/Dev/matt.life

templates

# article index should have trailing slash to preserve hrefs
redir /expert-caddy /expert-caddy/
try_files {path} {path}/ {path}.html

# serve all articles from the template
rewrite /expert-caddy/* /expert-caddy/template.html

file_server

(This config could actually be better, probably by specifying template.html as an index file in the file_server, but 🤷‍♂️)

Test case 2

This config is from the Caddy website. Without my proposed change, it would not render pages in the /docs/ section of the site:

localhost

root * src

file_server
templates
encode gzip

try_files {path}.html {path}

redir   /docs/json      /docs/json/
redir   /docs/modules   /docs/modules/
rewrite /docs/json/*    /docs/json/index.html
rewrite /docs/modules/* /docs/modules/index.html
rewrite /docs/*         /docs/index.html

reverse_proxy /api/* localhost:4444

With my proposed change, the docs pages still function as expected (canonicalization redirects are NOT dispatched, since I explicitly rewrote the filename to be what I want it to be).

Test case 3

A simple file server with directory listings, that does not have an index file, where we use handle_path to serve the root folder within the path of /foo/*:

localhost

root * /any/folder/that's/not/a/website

handle_path /foo/* {
	file_server browse
}

With my proposed change, this continues to issue canonicalization redirects because only the path prefix is rewritten, not the filename; and the redirects correctly preserve the /foo/ prefix and the relative hrefs continue to work because of the redirects.

I’m going to commit and push this soon, then probably release v2.4.3 later today.

I am experiencing the same issue and downgrading to 2.4.1 worked for me. Adding repro details:

1. Environment

1a. Operating system and version

Ubuntu 18.04

1b. Caddy version (run caddy version or paste commit SHA)

v2.4.2 h1:chB106RlsIaY4mVEyq9OQM5g/9lHYVputo/LAX2ndFg=

2. Description

2a. What happens (briefly explain what is wrong)

Using file_server to serve the output of a webpack build, where we need all paths not belonging to a static file to rewrite to an index.html outside the directory of the Caddyfile. A GET to Caddy at http://localhost:3030 responds with a 308 Redirect with the header Location: /. This causes a redirect loop for any request to Caddy.

2b. Why it’s a bug (if it’s not obvious)

On Caddy 2.4.1 this redirect would not happen and the expected html file is served.

2c. Log output

01:28 # caddy run --config=caddy/Caddyfile
2021/06/15 01:28:56.577 INFO    using provided configuration    {"config_file": "caddy/Caddyfile", "config_adapter": ""}
2021/06/15 01:28:56.578 WARN    input is not formatted with 'caddy fmt' {"adapter": "caddyfile", "file": "caddy/Caddyfile", "line": 4}
2021/06/15 01:28:56.579 INFO    admin   admin endpoint started  {"address": "tcp/localhost:2019", "enforce_origin": false, "origins": ["127.0.0.1:2019", "localhost:2019", "[::1]:2019"]}
2021/06/15 01:28:56.579 INFO    tls.cache.maintenance   started background certificate maintenance      {"cache": "0xc00015f110"}
2021/06/15 01:28:56.580 INFO    tls     cleaning storage unit   {"description": "FileStorage:/root/.local/share/caddy"}
2021/06/15 01:28:56.580 INFO    tls     finished cleaning storage units
2021/06/15 01:28:56.581 INFO    autosaved config (load with --resume flag)      {"file": "/root/.config/caddy/autosave.json"}
2021/06/15 01:28:56.581 INFO    serving initial configuration
 2021/06/15 01:13:05.598 INFO    http.log.access.log0    handled request {"request": {"remote_addr": "[::1]:40218", "proto": "HTTP/1.1", "method": "GET", "host": "localhost:3030", "uri": "/", "headers": {"User-Agent": ["curl/7.68.0"], "Accept": ["*/*"]}}, "common_log": "::1 - - [15/Jun/2021:01:13:05 +0000] \"GET / HTTP/1.1\" 308 37", "duration": 0.000205737, "size": 37, "status": 308, "resp_headers": {"Content-Type": ["text/html; charset=utf-8"], "Server": ["Caddy"], "Location": ["/"]}}

2d. Workaround(s)

Downgrade to Caddy 2.4.1

3. Tutorial (minimal steps to reproduce the bug)

1. The bug was first encountered in a CI environment where our repo is cloned to /root, so add a /root/index.html that we want to serve with Caddy.

2. In a different directory, add minimum Caddyfile:

:3030 {
    root * /root
    try_files {path} /index.html
    file_server
}

1. caddy run
2. curl localhost:3030 => 308 Redirect to /

The trailing slash creates a path matcher which only matches exactly / and nothing else.

I agree this is confusing behaviour, and I plan to deprecate path matchers in site addresses soon, because it’s clearer to use a handle block explicitly instead, if you’re actually intending to use a path matcher.

Your issue is not related to the one that was originally reported in this issue. It’s a different problem altogether.

For the record, I tried using a solution that Francis suggested where we don’t canonicalize (redirect) if the rewritten path is explicitly an index file. This worked for one of the use cases I tried, but not the Caddy website for example (the docs part of the site), in part due to how templates use the rewritten path (it expects the index page to have “index” in the rewritten path).

I think for now the best thing to do is keep the revert for v2.4.3. Yes, it can be problematic for URL canonicalization when used inside handle_path, but we’ll need a solution that doesn’t break so many sites in such complex ways.

Point taken. I agree the redirect is not necessarily wrong, but it is not obvious. I wouldn’t expect that being more explicit with /index.html would cause an issue. (edit: while the redirect is not wrong, I also don’t think /index.html is wrong either?)

Perhaps worth noting, my solution was guided by this community forum and your comment where the recommended solution is try_files {path} /index.html. This is the top search result for “caddy serve single page app”.

Thanks for the repro instructions. That exhibits it for me. The commit I linked earlier is definitely a regression, because that shouldn’t yield a redirect loop (edit: actually, maybe it should, see below).

(I still want to know @inoblue’s exact use case & config though, since how to solve both problems without a regression is still an open question.)

Are there any debug logs available with the broken commit?

Edit: I think it has to do with the changes in modules/caddyhttp/fileserver/staticfiles.go. I wasn’t too sure what the changes there had to do with my issue, and I only experienced an issue with modules/caddyhttp/fileserver/browse.go, but the original fix was for staticfiles.go.

I think I can make a commit with just the fix for browse.go and see how that goes.

The issue was solved after building from source.

Thanks for your help.