Repository: caddyserver/caddy (language: go)

Issue:
Send 501 responses as a fallback to unknown HTTP methods
Caddy's default behavior appears to be treating unhandled HTTP request methods as if they were GET. This is a very bad idea: it seriously impedes the ability to add new HTTP methods, which is terrible for the Internet as a whole.

Caddy SHOULD be producing 405 responses to any method it recognizes but does not allow. This requires populating the "Allow" header, which could be difficult to do automatically, so I can understand reluctance on that front.

501 responses (which are explicitly meant for unknown methods) have no such requirement, and so are trivial to implement. For now, I'm using something along the lines[^1] of this to approximate that:
```
@unsupported {
        not method GET HEAD POST PUT DELETE CONNECT OPTIONS TRACE
}

error @unsupported 501
```

This is obviously rather messy. It would be much better if Caddy did this on its own, especially since I doubt many users actually read RFC 9110.

[^1]: What I'm actually doing is matching methods I allow separately, so I can configure 405 responses for the methods Caddy knows about but disallows.


Discussion / hints:
If the issue is that a 501 status code applies to every single resource, and Caddy can't be certain a resource doesn't support the request method, a generic 500 status would still be preferable.
Thanks for opening an issue!

We don't make assumptions about people's http applications, other than that TLS should be used. We don't know what methods are understood by the application, and doing that for every config will break a lot of sites.

You can remove the braces around your matcher and make it a one-liner. That will simplify it a bit. 

Anyway, that config looks good to me other than the mentioned points.
Why the 200 status code in a response, though?
Well, why not?

It's basically a "no op". Nothing broke. Everything is working as configured. It's just that nothing was configured for that request. (Think of it like a null/nil value.) Hence 200 OK. It's the mark of a well-behaved server.
But 200 has a specific meaning that depends on the method, which you aren't handling.
You'll need to be more specific about how you configured Caddy. What handler directive are you using?

By default, Caddy doesn't do anything at all, and will just respond with a empty 200 response because "it worked as configured", i.e. you didn't configure Caddy to do anything, so... it worked.

It's your responsibility to tell Caddy what to do with requests.
https://www.rfc-editor.org/rfc/rfc9110.html#section-15.3.1

> The 200 (OK) status code indicates that the request has succeeded. The content sent in a 200 response depends on the request method.

I would rather Caddy crashed than say something succeeded without even knowing what that means.
Well, it _did_ succeed. That's the thing. Caddy did exactly what it was configured to do (i.e. nothing).
Whether the server "succeeded" is irrelevant. A status of 200 means the **request** succeeded.
It's not our job to decide how to handle every method. If that's important to your application, then you configure the server to handle it.
Again, our view is that the _request did succeed_.

It's fine that you disagree, but we avoid adding implicit behaviour so that you have a clean slate to build up from. Add config to do what you need, and constrain what Caddy does.
Wow GitHub mobile is not showing me all the comments. Sorry if I missed some, I didn't see half of OP's or any of Francis'.
There is [literally](https://www.rfc-editor.org/rfc/rfc9110.html#section-15.6.2-1) a status code for saying you don't know how to handle a method.

> This is the appropriate response when the server does not recognize the request method and is not capable of supporting it for any resource.

---

If Caddy is configured to do nothing, why not just do literally nothing? Don't respond.
Let me give a very specific example of why this is problematic. Let's say you set up a file server with minimal configuration changes.

Now you try sending a DELETE request to a resource. RFC 9110 [says](https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.5-5.3) that a 200 status in the response means the action is successful. Caddy obviously didn't delete the file described by the request, but it's going to explicitly say it did anyway.
Okay, that's more useful feedback.

The `file_server` handler passes the request to Go's stdlib `http.ServeContent` which checks for `GET` or `HEAD` and does nothing for other methods; it doesn't reject other methods. We could probably do that (unless `pass_thru` is configured in which case no error would be emitted).
If the bug is in Go's standard library, it should be fixed there as well.

At any rate, the heart of the issue is that Caddy is returning a 200 status without being configured to do so and without any awareness of what that means. Caddy's default behavior needs to avoid violating standards like RFC 9110.

If it was reasonable to rely on users to configure Caddy properly, I wouldn't get a 200 response from this command:
```sh
curl --request DELETE 'https://caddyserver.com/docs/caddyfile'
```

---

Anyway, if this issue does in fact have some merit, could you reopen it?
That would leak resources.
> That would leak resources.

What would?
Why is the current behavior a problem?
It's flat-out violating the HTTP specification in numerous ways, even though Caddy is ostensibly an HTTP server.

---

I can give more specific examples of how this is dangerous, if you prefer.
So first, I need to apologize for being incoherent: apparently the GitHub Mobile app continues to show me only part of the conversation, lagging way behind in what's happening now, even after refreshing the thread. Only closing and restarting the app brought the content up-to-date. Now I'm at my computer so hopefully what I'm seeing is faithful.

I'm also really frustrated with the app right now. Sorry if I come off a bit terse.

I've just had to re-read the entire thread from the beginning to fill in many replies that didn't show up for me. I'll try to catch up here and reply to everything in-line.

@Saklad5 

> I would rather Caddy crashed than say something succeeded without even knowing what that means.

We can't allow Caddy to crash; that's out of the question. But we can't say that there was an error either, without knowing what the error is :thinking: (Because there was no error.)

Hopefully our explanations of "what that means" ("something succeeded") makes sense, about the request succeeding. 200 OK is the appropriate response for that.

> Whether the server "succeeded" is irrelevant. A status of 200 means the request succeeded.

Exactly. The request succeeded. That's what 200 means, and that's why we return that.

> If Caddy is configured to do nothing, why not just do literally nothing? Don't respond.

Okay, now that I can see the full thread, I see the confusion with my later reply, which I thought was immediately on the heels of your comment here. I meant that not responding would leak resources. If we simply ignore the request, the client will continue to wait and hold the connection open until it gets a response or we close the connection. If we close the connection we also interrupt other requests on it, and the experience for the client is very jarring, tells you nothing, and makes you think the server crashed or is broken rather than telling you the server is at least responding. So closing the connection is also out of the question, as it's akin with crashing.

> Let me give a very specific example of why this is problematic. Let's say you set up a file server with minimal configuration changes.
>
> Now you try sending a DELETE request to a resource. RFC 9110 [says](https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.5-5.3) that a 200 status in the response means the action is successful. Caddy obviously didn't delete the file described by the request, but it's going to explicitly say it did anyway.

Okay, now we're getting somewhere. Thanks for the specifics.

Please recall that I said we cannot assume what the application is doing. In this case Caddy cannot know that the application is a static, read-only file server, and that DELETEs aren't allowed. It doesn't know that unless you tell it that or the application enforces that.

So maybe what you're _actually_ asking for is our file_server handler module to return 405 for methods that aren't GET or HEAD? Since it can't mutate the files at all.

Please differentiate between "Caddy" and "a Caddy module", as they are distinct. All modules are plugins to Caddy, even the HTTP server as a whole is a plugin. All its handlers are plugins. It would be more productive to talk about the behavior of the specific plugin you're having trouble with, than suggesting that Caddy should reject all the methods you specified by default, which as I said would break a ton of servers and we aren't going to do that.

> I can give more specific examples of how this is dangerous, if you prefer.

If you can demonstrate a security breach with the latest commits, I would be definitely be interested in that.

Anyway, it sounds like all we need in the file server is to reject methods that aren't GET or HEAD. Yeah?
> Anyway, it sounds like all we need in the file server is to reject methods that aren't GET or HEAD. Yeah?

I didn't intend for this to be specific to file servers: I believe Caddy should only send a 200 response if a handler tells it to.

Failing that, fixing this for file servers would be a step in the right direction.

Fix patch:
diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go
index 04728ce4d86..7d38b734e15 100644
--- a/modules/caddyhttp/fileserver/staticfiles.go
+++ b/modules/caddyhttp/fileserver/staticfiles.go
@@ -396,6 +396,14 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
 		etag = calculateEtag(info)
 	}
 
+	// at this point, we're serving a file; Go std lib supports only
+	// GET and HEAD, which is sensible for a static file server - reject
+	// any other methods (see issue #5166)
+	if r.Method != http.MethodGet && r.Method != http.MethodHead {
+		w.Header().Add("Allow", "GET, HEAD")
+		return caddyhttp.Error(http.StatusMethodNotAllowed, nil)
+	}
+
 	// set the Etag - note that a conditional If-None-Match request is handled
 	// by http.ServeContent below, which checks against this Etag value
 	w.Header().Set("Etag", etag)


Test patch: