Being able to edit `@phpFiles` matcher in `php_server` directive

[DenuxPlays]

Describe your feature request

Is your feature request related to a problem? Please describe.

We had some security issues where a user can upload a php file and execute it using our webserver.
In our old setups (with apache & php-fpm) we used to deny those executions using .htaccess files which ofcourse do not work in caddy.

Describe the solution you'd like

In the docs there is a Section (Using the php_server directive is equivalent to this configuration:) that describes how the php_server directive works.
In there you can find a @phpFiles matcher.

Would it be possible to define this matcher via an option in the php_server directive that defaults to *.php.
I imagined something like this:

php_server {
    try_files {path} index.php
    php_files "/index.php"
}

Describe alternatives you've considered

1. Disable the upload from php files:

• would not really achieve the same goal but block the security thread (for now)

2. Create a matcher that serves a 403 page for every other php file

• would work but again Frankenphp itself would still be able to execute any php files that does not match the matcher (if configured wrong)

3. don't use the php_server directive and copy the snippet from the docs and edit it

• would work but other users would not benefit from this approach

[henderkes]

Silly question... can't you just "rewrite /index.php" before your php_server block? Or disable try_files?

I don't think we should create a setting for that. It's perfectly doable in many ways with Caddy itself.

[DenuxPlays]

No not really.
The problem is we don't just have one php files we have multiple.
What do you mean by disabling try_files?

The problem is that if a user uploads a .php file under /public/ugc.php and you visit /ugc.php it's getting executed by frankenphp instead of being served as plain text. (Or just don't execute it)
Our solution:

    # Match any other PHP file requests (but exclude magento)
    @phpServeStatic {
        path *.php
        not path /index.php /get.php /static.php /errors/report.php /errors/404.php /errors/503.php /health_check.php
    }

    # For all matched php files that are NOT exceptions, serve the source (do not execute)
    handle @phpServeStatic {
        # Send as plain text to make intent clear and avoid accidental execution
        header Content-Type "text/plain; charset=utf-8"
        file_server
    }

Our specific issue: magento/magento2#40583

I understand that this can be done in caddy but I just don't want to copy chunks of code every time.

I hope this is understandable. This is maybe more a UX/DX improvement.

[henderkes]

I still vote against this because it's an administrative workaround for an inherently flawed design. User input should never live directly accessible in a public folder, especially not with its original filename.

Replacing a few lines of Caddy logic, for something that shouldn't be needed in the first place, is not frankenphps responsibility.

[DenuxPlays]

Yes I agree but still if you have to deploy something it needs to work regardless if the design is suboptimal.

But it can also be seen as a security flaw inside frankenphp itself.
Not being able to restrict the files that frankenphp is allowed to execute is in it self a flawed design imo.

Maybe this thread should be moved into a discussion to see if others agree or disagree with me.

[henderkes]

But it can also be seen as a security flaw inside frankenphp itself.
Not being able to restrict the files that frankenphp is allowed to execute is in it self a flawed design imo.

Calling that a security flaw in FrankenPHP is ridiculous. The immediate security flaw lies in the code, the worksaround for it exists in Caddy directives and shouldn't be duplicated in FrankenPHP. This would have us maintain logic specifically to solve an edge case in projects with terrible design, 99.9% of users would never need this.

Does FastCGI have security issues because there's no directive to do that? Obviously not.

[DenuxPlays]

No not a security flaw more like bad practice

There would not be much to maintain. All I want is that *.php which is currently hardcoded can be configured (just like it can be in with fast_cgi & apache php_fpm for example).
This solution would be much leaner & nicer imo

But again if you disagree that's fine. If you are completely against it this issue can be closed.

[henderkes]

It can be configured exactly the same way as for fastcgi. You can use Caddy directives in FrankenPHP.

The php_fastcgi notably does not have such a setting either: https://caddyserver.com/docs/caddyfile/directives/php_fastcgi#expanded-form

And I very strongly disagree that it's bad practise not to have this extra setting. It would require duplicating existing functionality and doesn't have a clear definition of what happens if a php file is not matched. Is it a 403? 503? 400? Served as a file? Should the directive just not handle it?

Better to be explicit.

[DenuxPlays]

Oh you are right sorry for that.
I even looked it up and thought I have seen such setting.
Sorry that was my mistake.

I don't ask for more logic directly if a file does not match the phpFiles matcher it simply gets served.
That should also apply here if you want more logic then I agree this should not be in frankenphp.

[henderkes]

I don't ask for more logic directly if a file does not match the phpFiles matcher it simply gets served. That should also apply here if you want more logic then I agree this should not be in frankenphp.

That would go against the php_fastcgi directives implementation of completely ignoring the file if it doesn't match the *.php handler. So we either defy php_server expectations, or we break php_fastcgi compatibility. Neither are good.

[DenuxPlays]

Yes I agree.
Haven't noticed that php_server is compatible with fast_cgi

Then yes breaking compatibility would not be great.

I think we should move this into a discussion to see if others agree with me.

[dunglas]

Maybe this can mentioned in the "migrating to FrankenPHP" page?