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

proposal: net/http: decide and document accepted HTTP methods in FileServer #59470

Open
pascaldekloe opened this issue Apr 6, 2023 · 8 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@pascaldekloe
Copy link
Contributor

Now that the fix got reversed, the documentation should at least inform/warn about it's broken by default.

@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 6, 2023
@mknyszek mknyszek modified the milestones: Backlog, Go1.21 Apr 6, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Apr 6, 2023

CC @neild

@mknyszek mknyszek added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Apr 6, 2023
@rsc rsc changed the title net/http: FileServer breaks HTTP documentation proposal: net/http: decide and document accepted HTTP methods in FileServer Apr 6, 2023
@rsc rsc modified the milestones: Go1.21, Proposal Apr 6, 2023
@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

I would still support a change to reject methods other than GET, HEAD, POST. This is the kind of protocol compliance change that would be good to use a GODEBUG to control and roll out.

@neild
Copy link
Contributor

neild commented Apr 6, 2023

It's surprising that http.Handle("/", http.FileServer(http.Dir("/tmp")) will respond to DELETE requests with the contents of a file. Given that this is the example usage from the FileServer documentation, there's also a good argument to be made that FileServer's behavior here is wrong.

FileServer does examine the request method and handles HEAD requests differently than GET. So it's not entirely agnostic to the request method.

On the other hand, the negative impact of responding to a DELETE request with the contents of a file seems low. I certainly agree that this is surprising, but "broken" may be a bit much.

Perhaps this is a good motivation to consider whether we should have an easier way to register method-specific handlers on a ServeMux. If we did, the fix might just be to update the FileServer example to register a GET-only handler:

http.Handle("GET /", http.FileServer(http.Dir("/tmp"))) // The FileServer never sees DELETE or POST requests.

@pascaldekloe
Copy link
Contributor Author

Method handling would be a more thorough solution indeed. Even the examples from the http package documentation, and the code listed at Writing Web Applications is broken, in that it will serve DELETE and friends unintentionally.

@neild
Copy link
Contributor

neild commented Apr 6, 2023

Whatever we do, we should consider ServeFile and ServeContent as well as FileServer.

@gopherbot
Copy link

Change https://go.dev/cl/482876 mentions this issue: net/http: add tests covering non-GET methods for file serving

gopherbot pushed a commit that referenced this issue Apr 6, 2023
ServeFile and FileServer will respond to methods such as DELETE by
serving the file contents. This is surprising, but we don't want to
change it without some consideration.

Add tests covering the current behavior.

For #59470

Change-Id: Ib6a2594c5b2b7f380149fc1628f7204b308161e1
Reviewed-on: https://go-review.googlesource.com/c/go/+/482876
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@beaubrueggemann
Copy link

I don't think ServeFile or ServeContent should be changed. I see these as lower-level functions, used to compose handlers (they are not themselves handlers).

For example, as part of a handler that processes HTTP DELETE requests, you may want to return some static response from a file: http.ServeFile(w, r, "delete-success-message.html")

FileServer is different. It returns a handler, and it might make sense to return a high-level handler that rejects methods such as DELETE. Or it could continue to be treated as a low-level primitive (more like ServeFile), that doesn't decide HTTP method policies itself.

The documentation example for FileServer, http.Handle("/", http.FileServer(http.Dir("/tmp"))), does seem to imply a higher-level handler that takes care of everything.

Similar to @neild's ServeMux idea above, another option would be a wrapper handler (similar to http.TimeoutHandler, etc.), that is something like:

func AllowMethods(h Handler, methods ...string)

Then the FileServer example would become: http.Handle("/", http.AllowMethods(http.FileServer(http.Dir("/tmp")), http.MethodGet, http.MethodHead, http.MethodPost)). But that's a bit wordy and heavy.

And the ServeMux idea might be more flexible, because you could add different handlers for different methods (one for http.Handle("GET /", ...) and one for http.Handle("POST /", ...), etc.).

But if most people use FileServer as a high-level handler, it might be more convenient for them if it were changed to reject DELETEs, etc. (Though I don't see much harm in FileServer just ignoring the method, and serving files regardless. It follows the "be liberal in what you accept" robustness principle).

@bcmills
Copy link
Member

bcmills commented Apr 6, 2023

Though I don't see much harm in FileServer just ignoring the method, and serving files regardless. It follows the "be liberal in what you accept" robustness principle

That principle is not universally held.
(https://datatracker.ietf.org/doc/html/draft-thomson-postel-was-wrong-03)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: Incoming
Development

No branches or pull requests

7 participants