Caddy: Rewrite directive in Caddyfile change in 0.10.01

[Cinerar]

(Are you asking for help with using Caddy? Please use our forum instead: https://caddy.community. If you are filing a bug report, please take a few minutes to carefully answer the following questions. If your issue is not a bug report, you do not need to use this template. Thanks!)

1. What version of Caddy are you using (caddy -version)?

Caddy 0.10.1

2. What are you trying to do?

Rewrite url

3. What is your entire Caddyfile?

example from docs

localhost:80
rewrite {
    if {file} not favicon.ico
    to {path} {path}/ /maintenance.html /index.php
}

4. How did you run Caddy (give the full command and describe the execution environment)?

./caddy

7. What did you see instead (give full error messages and/or log)?

2017/05/02 09:17:21 Caddyfile:4 - Parse error: Caddyfile:4 - Syntax error: Rewrite path must begin with '/'. Provided: '/index.php'

slashes.... something wrong with them again?

[tobya]

hmmm. That would be me, I didn't test with a placeholder. I'll take a look

[tobya]

As a workaround what happens if you try

localhost:80
rewrite {
if {file} not favicon.ico
to /{path} /{path}/ /maintenance.html /index.php
}

[Cinerar]

with

 /{path} /{path}/ /maintenance.html /index.php 

it works

thanks for workaround

[francislavoie]

I hope this can get fixed, I'd consider this a breaking change for a lot of people. The recommended rewrite has been without the slash. Most of everyone who uses fastcgi with a router will probably run into this problem.

It's listed as an example here https://caddyserver.com/docs/rewrite and has been recommended many times in the forums for users getting started with PHP and Caddy.

[mholt]

Heh, oops. This change is because of #1610. How do we fix both now?

Edit: @tobya Can you make it work with placeholders? I can roll out a release today with a fix.

[mholt]

@tobya One thought I had was to just have rewrite always tack on a leading slash if missing.

But, looking more at #1610, is it not operator error, rather than a Caddy bug?

I see some issues on Stack Exchange sites where site owners want to strip the leading slash for their backend apps. So maybe we shouldn't always tack on a leading slash in rewrite OR require one at startup. I think this is awful, but...

Other than the risk of displaying the unparsed contents of a PHP file, I'm not sure how real the risk is. Users who don't want a leading slash can match their fastcgi directive on "" instead of / and it'll get literally every request. But rewriting to a path without a slash does seem like a bad idea IMO.

[tobya]

@mholt We really really can't server php content without it being interpreted as php this is really really bad.

Any user has the expectation that if they do

fastcgi / 127.0.0.1:9000 php

that all requests, every single request for a file with a .php extension will be served via the php interpreter. It would be a huge security hole to ever do anything else.

I don't like the idea of telling users to match "" to match everything, when that is really what / is doing.

I was originally in favour of simply either

• make / match all paths, not just paths starting with / which is basically what / means anyway. It means everything.
• ensure a '/' is placed in front of every rewrite path whether it is in the Caddyfile or not.

It would be great if we could get the opinions of a few more PHP fastcgi users here.

Basically to fix this, I can either do 1 of the 2 things above, or add code to not check for / if the token starts with {

I will look at a fix later today.

[mholt]

@tobya Since not all placeholders end up with values that start with a forward slash, my personal vote is for rewrite to always implicitly add the leading slash if not present after performing the rewrite. I think we should remove the prefix check at setup that was added in #1629.

EDIT: Wait, I misread your first suggestion/preference. That's a good idea too. Actually. That's a really good idea. I don't know of anyone using "" as a "base path" anyway. Let's try that?

But I will give this issue a few more hours to see if anyone will chime in. Feel free to submit a PR in the meantime or, if you've got things going on, I will just implement the fix myself and release it today.

[tobya]

@mholt In reality I won't get to this until tonight. About 10 pm Irish Time, so if it suits you to do the fix that would be great. We should probably just revert the previous fix.

[tobya]

Another option @mholt is to revert the change for #1610 and rerelease without it, that would give us more time to decide on and test a comprehensive fix for this issue.

[mholt]

@tobya Thanks -- I can take care of it, no problem. I'm leaning toward this change in the Path.Matches method:

if other == "/" {
    return true
}

How's that sound?

[tobya]

@mholt That should do the trick I think.

[mholt]

@Cinerar Please tell me https://github.com/mholt/caddy/commit/5e467883b882780ff0334cc9a5487cd6abd26dc9 fixes this. :smile: If so I will push out the release immediately.

[tobya]

Is this fixed for non root paths? E.g.

Rewrite /blog

Also does this fix without reverting the original change?

[mholt]

@tobya If the base path is "/", then any other path will be considered to match. If the base path is "/blog" then it works the same.

Also does this fix without reverting the original change?

No, because "{path}" wouldn't be an allowed destination rewrite, which it should be.

[mholt]

@tobya I've also just added logic to fastcgi that will match the path even if the request path doesn't have the leading slash, by prepending the leading slash if it is missing; this modified path is only used for matching and nothing else with fastcgi, so it shouldn't break anything there. 😅 And it gives the extra safety you're looking for, in case rewrites are sloppily configured. :wink:

So can you take a look at 59a5afa? I'm ready to push the big red button if it's good to go.

[mholt]

@Cinerar If you have a moment to build Caddy at 59a5afa and try it, that'd be great too.

[Cinerar]

@mholt i believe i have some problems with build env, couse compiled 10.01 that i downloaded today fixed #1582 exactly as you told. So i can't check https://github.com/mholt/caddy/commit/59a5afab292046b67526b6f38582d3a0efbfcc50 right now until i fugure out what is the problem with my build env.
Are there any official instructions how to build from source?

[mholt]

@Cinerar Yes, the readme has it, but it's easy; in your case, add the -u flag to update: go get -u github.com/mholt/caddy/caddy

Then just $GOPATH/bin/caddy to run Caddy.

[Cinerar]

@mholt ok looks like
Caddy 0.10.1 (+59a5afa Tue May 02 17:41:33 UTC 2017)
is ok at least with this bug
the problem is that i'm building inside docker container, so looks like i was missing some caching that happened.

[mholt]

@Cinerar Excellent, I will pull the trigger then and release this fix. Thanks for trying it out!