caddyserver / caddy

Fast and extensible multi-platform HTTP/1-2-3 web server with automatic HTTPS
https://caddyserver.com
Apache License 2.0
57.34k stars 4k forks source link

vars not expanded in path directives #6387

Open CRCinAU opened 3 months ago

CRCinAU commented 3 months ago

I'm trying to simplify a bit of Caddyfile - and using a variable as part of the path name.

Part of the Caddyfile I'm using is as follows:

handle /xplane/charts/* {
        vars {
                cycle "2024-MAR-21"
        }
        @daps {
                not file
                not path /xplane/charts/DAPS-{vars.cycle}/
                path_regexp ^/xplane/charts/DAPS.+/(.*)$
        }
        @ersa {
                not file
                not path /xplane/charts/ERSA-{vars.cycle}/
                path_regexp ^/xplane/charts/ERSA.+/(.*)$
        }

        redir @daps /xplane/charts/DAPS-{vars.cycle}/{re.1} permanent
        redir @ersa /xplane/charts/ERSA-{vars.cycle}/{re.1} permanent

        file_server {
                browse
        }
}

It seems like {vars.cycle} does not get expanded in the not path lines, but does work correctly in the redir lines.

Currently, with {vars.cycle} present, this results in an endless loop of 301 redirects.

If I change the line to: not path /xplane/charts/DAPS-2024-MAR-21/, then everything works as expected.

Caddy running via caddy:latest - currently:

docker exec -ti caddy /bin/sh
/srv # caddy --version
v2.8.4 h1:q3pe0wpBj1OcHFZ3n/1nl4V4bxBrYoSoab7rL9BMYNk=
/srv # 
francislavoie commented 3 months ago

For ref, context from the forums https://caddy.community/t/rewriting-of-changing-url-paths/24426

The path matcher matches paths exactly, not as a prefix. Do you mean to match /xplane/charts/DAPS-{vars.cycle}/* instead (note the * at the end)?

CRCinAU commented 3 months ago

Hi @francislavoie - the aim is to:

1) Redirect a path, say: /xplane/charts/DAPS-2000-DEC-22/filename.pdf to /xplane/charts/DAPS-2024-MAR-21/filename.pdf

2) The directory is browseable, but without the not path matcher to the directory listing page, a loop of 301's is entered.

3) If I hard-set the not path to the value of vars.cycle instead of using the variable, then it works as expected.

For the sake of example, with the not path hard coded instead of using variables, the following redirects happen:

/xplane/charts/DAPS-2000-DEC-01/  -> /xplane/charts/DAPS-2024-MAR-21/ (can browse)
/xplane/charts/DAPS-2018-JAN-23/Melbourne (YMML).pdf  ->  /xplane/charts/DAPS-2024-MAR-21/Melbourne (YMML).pdf

With the not path set to include {vars.cycle}, the following redirects happen:

/xplane/charts/DAPS-2000-DEC-01/  -> /xplane/charts/DAPS-2024-MAR-21/ (Redirect loop to this URL)
/xplane/charts/DAPS-2018-JAN-23/Melbourne (YMML).pdf  ->  /xplane/charts/DAPS-2024-MAR-21/Melbourne (YMML).pdf

I don't believe that using a /* on the end would negate the infinite redirect loop for a directory listing served by the browse option to file_server.

The not path is there to prevent the infinite 301 redirects to the same page.

francislavoie commented 3 months ago

Oh right. The other details is the path matcher matches case-insensitively by converting the request path to lowercase, which means you need to write your matcher with lowercase characters as well to match it (including the variable contents). So if you change your variable to 2024-mar-21 it should work.

The tricky part is we lowercase the path during the provisioning phase (at server/config start) to avoid needing to do it on every request (saving a few nanoseconds at most on each request). This means that any dynamic parts of the path don't get lowercased since it happens before replacing the placeholders.

This also means that the placeholder itself must also be lowercase :grimacing: so you can't have uppercase characters in the placeholder/variable name :man_facepalming: I'd call this part a bug. Obviously the only fix is to move the lowercasing to after replacing any placeholders, which would be a slight performance hit for everyone even if they don't use placeholders. Or in provisioning we could do a smarter lowercase to only lowercase parts not within placeholders, but I dunno.

/cc @mholt a decision needs to be made here

CRCinAU commented 3 months ago

The other details is the path matcher matches case-insensitively by converting the request path to lowercase, which means you need to write your matcher with lowercase characters as well to match it (including the variable contents). So if you change your variable to 2024-mar-21 it should work.

Hmmmm - this is interesting - as I'm guessing that would break the redir line using lower case as well - as it'll do filesystem matching on a lower case name - where the actual directory on disk is upper case?

EDIT: This does seem to work - although obviously not ideal:

handle /xplane/charts/* {
        vars {
                cycle "2024-mar-21"
                cycle2 "2024-MAR-21"
        }
        @daps {
                not file
                not path /xplane/charts/DAPS-{vars.cycle}/
                path_regexp ^/xplane/charts/DAPS.+/(.*)$
        }
        @ersa {
                not file
                not path /xplane/charts/ERSA-{vars.cycle}/
                path_regexp ^/xplane/charts/ERSA.+/(.*)$
        }

        redir @daps /xplane/charts/DAPS-{vars.cycle2}/{re.1} permanent
        redir @ersa /xplane/charts/ERSA-{vars.cycle2}/{re.1} permanent

        file_server {
                browse
        }
}
francislavoie commented 3 months ago

Yeah you'd probably have to use 2 different variables in that case I guess. Or you could change to using an expression matcher and do the string comparison yourself, something like this:

    vars cycle "2024-MAR-21"
    @daps `!file() && {path} != ('/xplane/charts/DAPS-' + {vars.cycle} + '/') && path_regexp('^/xplane/charts/DAPS.+/(.*)$')`
    redir @daps ...

Placeholders inside a CEL expression act like a function call so they can't be inlined inside of a string, so concat needs to be used to form the path there. Simple enough though.

CRCinAU commented 3 months ago

Or you could change to using an expression matcher and do the string comparison yourself, something like this:

This actually works perfectly. Thanks for this workaround! Gets me one step closer to moving my static site to Caddy :)

Will leave this issue open though, as I believe the original config should actually work.