net/http: FileServer no longer serves content for POST

[beaubrueggemann]

What version of Go are you using (go version)?

$ go version 1.20.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/beaubrueggemann/Library/Caches/go-build"
GOENV="/Users/beaubrueggemann/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/beaubrueggemann/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/beaubrueggemann/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/47/c0lsmsxn3_z86w657l519d6m0000gn/T/go-build1135191649=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I updated Go from version 1.19.4 to version 1.20.2 and ran my server.

Here's an abbreviated version of the source, that shows the problem:

package main

import (
        "log"
        "net/http"
)

const (
        PostPath = "/postpage.html"
        PostName = "name"
)

var FileServer = http.FileServer(http.Dir("/site"))

func MyHandler(w http.ResponseWriter, r *http.Request) {
        if r.URL.Path == PostPath {
                // Handle post data.
                value := r.PostFormValue(PostName)
                // Server does something with value here...
                log.Println(value)
        }

        // Serve page from filesystem
        FileServer.ServeHTTP(w, r)
}

func main() {
        log.Fatal(http.ListenAndServe(":8080", http.HandlerFunc(MyHandler)))
}

Then I tried a POST request to the "localhost:8080/postpage.html" URL.

What did you expect to see?

I expected to receive a status 200, served with a body matching the file on my filesystem at /site/postpage.html

What did you see instead?

I received a 405 status (method not allowed), with a body consisting of "read-only"

I dug into the problem, and it results from commit 413554.

This change breaks the compatibility promise. I was surprised there was no discussion of compatibility in the commit review or the corresponding issue (53501).

It is useful to be able to wrap an http.FileServer with an http.Handler that handles POST data, etc., then forwards to http.FileServer to serve a static response body. But now this doesn't work anymore.

[seankhliao]

if you're wrapping the handler, then you can modify the request before you pass it to fileserver
this also sounds like a situation where you'd use servecontent rather than an entire fileserver

cc @neild

[beaubrueggemann]

The documentation for type http.Handler says not to modify the request:

Except for reading the body, handlers should not modify the provided Request.

Also, really the point is that compatibility was broken. Go has a compatibility promise—is this still taken seriously? This promise was a major factor in my choosing to work with Go.

For many years http.FileServer has ignored the HTTP method (POST, DELETE, etc.), and just served files regardless. Suddenly this behavior has changed, and it serves errors for some methods.

This broke my server and wasted some of my time. I'm bothered that the change was made with absolutely no discussion about the compatibility breaking. I thought the compatibility promise would prevent that.

While you could argue that the new method-filtering behavior is a better design for http.FileServer, I think you could also make a good argument for the original design. But it shouldn't matter—the ship sailed long ago, and the original design was chosen.

[ianlancetaylor]

CC @neild

[rsc]

The CL was adding support for OPTIONS, so it had to do something with methods generally.

There was no discussion of compatibility because I don't think any of us realized there was a compatibility problem. That is, I don't think we realized people would actually expect 'DELETE /foo' to serve the content of foo, nor that code would be sending DELETE /foo to get the content. For DELETE I still think it's probably more correct on balance to reject.

POST is trickier: the verb POST by itself really doesn't imply mutation the way DELETE does. POST is in a grey area between GET and DELETE, on the fence if you will pardon the pun.

I think it would be reasonable for compatibility to restore support for POST in the next point release, although perhaps others disagree.

[neild]

If we were designing FileServer today, I think we might want it to only respond to GET requests. But I agree that this ship has sailed, and we should restore the original behavior.

[gopherbot]

Change https://go.dev/cl/482635 mentions this issue: Revert "net/http: FileServer method check + minimal OPTIONS implementation"

[pascaldekloe]

The "compatibility promise" is about documented behaviour @beaubrueggemann. Otherwise you'd never be able to fix a bug, as any correction breaks the "original behaviour".

Protecting creative use can not outweight all websites broken without the method check in place.

Static content can only be served by read operations. When POST-ing a body, then the status code must match the processing of such request body. For example, an HTTP 404 means that the request was not executed, while the hack in this issue will execute, regardless of the response.

[beaubrueggemann]

Thanks for all the discussion around this.

@pascaldekloe, I don't think this is a bug, but rather an API design choice.

The original design of http.FileServer was simple: It mapped the request path to the filesystem, and served either a file or a 404. That's it. In particular, http.FileServer ignored the HTTP method. I think this is the better design.

Being so simple makes http.FileServer a flexible component that can be composed into servers in different ways. Dictating how the overall server must interpret HTTP methods limits FileServer's use cases.

For example, you can get your CL's behavior from the original design:

func MethodFileServer(root http.FileSystem) http.Handler {
    fs := http.FileServer(root)
    return HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        switch {
        case http.MethodGet, http.MethodHead:
            fs.ServeHTTP(w, r)
        default:
            http.Error(w, "read-only", http.StatusMethodNotAllowed)
        }
    })
}

But you cannot get the original behavior from your CL's design. This eliminates use cases like mine above, where a POST is handled, then forwarded to http.FileServer to serve the request path.

This means http.FileServer has become strictly less useful with the new design.

[rsc]

@gopherbot please backport go1.20

[gopherbot]

Backport issue(s) opened: #59469 (for 1.20).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[pascaldekloe]

Go started so nicely with the "if its broken, we fix it". After the statement was dropped, we got the v1 compatibility, and now even malicious API use enjoys better protection than correct operation. 😢

[rsc]

@pascaldekloe I don't think that's entirely fair, and thank you for filing #59470, which I've turned into a proposal. The original CL did not take compatibility into account, so it is entirely fair to roll it back in Go 1.20 and revisit with more consideration for Go 1.21.

@beaubrueggemann You may wish to follow or contribute to #59470 as well.

[dmitshur]

@neild Is this issue fixed by CL 482635 and can be closed, or is there more to do here?
If that's it, we can proceed with making the backport CL for #59469, otherwise it'll wait.

[neild]

This is fixed by CL 482635 and we can backport.

[gopherbot]

Change https://go.dev/cl/488635 mentions this issue: [release-branch.go1.20] Revert "net/http: FileServer method check + minimal OPTIONS implementation"