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

path matcher does not behave as expected #5613

Closed
gucki opened this issue Jul 2, 2023 · 8 comments
Closed

path matcher does not behave as expected #5613

gucki opened this issue Jul 2, 2023 · 8 comments
Labels
bug 🐞 Something isn't working
Milestone

Comments

@gucki
Copy link
Contributor

gucki commented Jul 2, 2023

Using caddy 2.6.4 and the following matcher:

@test {
  path /api/v1/servers/localhost/zones/test.com.
}

However, requests showing "uri": "/api/v1/servers/localhost/zones/test.com." in the caddy logs are not matched. When the path matcher doesn't have the "." at the end, it matches:

@test {
  path /api/v1/servers/localhost/zones/test.com
}
@mholt
Copy link
Member

mholt commented Jul 3, 2023

Ah, this is because of #2917:

// See #2917; Windows ignores trailing dots and spaces
// when accessing files (sigh), potentially causing a
// security risk (cry) if PHP files end up being served
// as static files, exposing the source code, instead of
// being matched by *.php to be treated as PHP scripts.
reqPath = strings.TrimRight(reqPath, ". ")

This is a security feature, so a fix will need to be sensitive to that.

@mholt mholt added bug 🐞 Something isn't working and removed bug 🐞 Something isn't working labels Jul 3, 2023
@gucki
Copy link
Contributor Author

gucki commented Jul 4, 2023

Hi @mholt, it seems the referenced issue is only for FastCGI (see https://github.com/caddyserver/caddy/pull/2917/files)? Or did the implementation change over time?

@mholt
Copy link
Member

mholt commented Jul 4, 2023

Serving php relies on path matchers.

I'll see if there's a better way to do this.

@mholt
Copy link
Member

mholt commented Jul 7, 2023

I wonder if I limit this trimming behavior to only happen on Windows servers, would that be sufficient for you?

@gucki
Copy link
Contributor Author

gucki commented Jul 7, 2023

@mholt Yes, I'm not using Windows. However, I still don't really get why code in the FastCGI module affects my requests, as I'm not using FastCGI, PHP, ...

@mholt
Copy link
Member

mholt commented Jul 7, 2023

I might try that then.

However, I still don't really get why code in the FastCGI module affects my requests, as I'm not using FastCGI, PHP, ...

The only "fastcgi module" in Caddy is the transport that turns an HTTP Request into a Response. However, for PHP apps, that alone is not very useful, since PHP apps often rely on special rewrites and static file serving. The php_fastcgi directive of the Caddyfile bundles these up for users as a one-liner.

I may try a patch that only trims the dot and space on Windows; but unfortunately there is no known absolute fix unless Windows changes its exploitable behavior.

I'm open to suggestions!

@mohammed90
Copy link
Member

Correction for the referenced code, it isn't in the FastCGI module but in the MatchPath module; but the reasoning is duplicated:

https://github.com/caddyserver/caddy/blob/master/modules/caddyhttp/matchers.go#L393-398

@mholt
Copy link
Member

mholt commented Jul 8, 2023

@mohammed90 Yes, thank you. I was linking to the fastcgi module to demonstrate its limited scope of HTTP Request -> Response functionality. I realize now I hadn't linked to the code I quoted above, which is indeed in the path matcher. Thank you for linking to that and clearing that up :)

@mholt mholt closed this as completed in 66114cb Jul 8, 2023
@mholt mholt added this to the v2.7.0 milestone Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants