Skip to content

fileserver: map invalid path errors to fs.ErrInvalid, and return 400 …#7017

Merged
mholt merged 1 commit intocaddyserver:masterfrom
Compy:bugfix-7008-fs-handle-invalid-paths
May 13, 2025
Merged

fileserver: map invalid path errors to fs.ErrInvalid, and return 400 …#7017
mholt merged 1 commit intocaddyserver:masterfrom
Compy:bugfix-7008-fs-handle-invalid-paths

Conversation

@Compy
Copy link
Copy Markdown
Contributor

@Compy Compy commented May 13, 2025 •
edited
Loading

…for any invalid path errors. (close #7008 #7007)

Background
Currently when using the static file server, when injecting a null byte into the request path, the server returns a 500. Edge infrastructure such as nginx, Apache, AWS ELB and Cloudflare treat these types of errors with a 400 Bad Request, which is more indicative of a bad input compared to an application-level 500 response.

This change adds error handling for fs.PathError to the mapDirOpenError() function by mapping it to fs.ErrInvalid, and modifies the caller (ServeHTTP()) to explicitly return http.StatusBadRequest for fs.ErrInvalid errors.

Local tests are positive. Unit tests are also passing. Tested on both MacOS and Linux/Ubuntu 24.

Before the change:

curl http://localhost:8080/foo.txt%00 -vvvv
* Host localhost:8080 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:8080...
* Connected to localhost (::1) port 8080
> GET /foo.txt%00 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.7.1
> Accept: */*
> 
* Request completely sent off
< HTTP/1.1 500 Internal Server Error
< Server: Caddy
< Date: Tue, 13 May 2025 02:52:15 GMT
< Content-Length: 0
< 
* Connection #0 to host localhost left intact

After the change:

curl http://localhost:8080/foo.txt%00 -vvvv
* Host localhost:8080 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:8080...
* Connected to localhost (::1) port 8080
> GET /foo.txt%00 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.7.1
> Accept: */*
> 
* Request completely sent off
< HTTP/1.1 400 Bad Request
< Server: Caddy
< Date: Tue, 13 May 2025 02:54:16 GMT
< Content-Length: 0
< 
* Connection #0 to host localhost left intact

@Compy Compy mentioned this pull request May 13, 2025
Closed
mholt
mholt approved these changes May 13, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks, this is pretty simple. Looks like a good patch.

@mholt mholt merged commit 94147ca into caddyserver:master May 13, 2025
20 checks passed
@mholt mholt added this to the v2.10.1 milestone May 13, 2025
@BrewTestBot BrewTestBot mentioned this pull request Aug 22, 2025
Merged
1 task
mohammed90 pushed a commit to cedricziel/caddy that referenced this pull request Aug 29, 2025
@Compy @mohammed90
fileserver: map invalid path errors to fs.ErrInvalid, and return 400 …
c66a517 
…for any invalid path errors. (close caddyserver#7008) (caddyserver#7017)
@mholt
Copy link
Copy Markdown
Member

mholt commented Nov 10, 2025

@Compy Could you take a look at this change relative to this report: https://caddy.community/t/nextcloud-setup-broken-after-update-to-2-10-2/33140

It might be that we are applying this error a bit too generously... (or, maybe we are just now returning an error where before a bug was being covered up?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mholt mholt mholt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.10.1

Development

Successfully merging this pull request may close these issues.

500 Internal Server Error for URL Containing Null Byte (%00)

2 participants

@Compy @mholt