net/http: FileServer responds with 500 status when syscall.ENOTDIR is returned from fs.Open

[mastercactapus]

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

1.7.5 and 1.8rc3

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/nathan/projects"
GORACE=""
GOROOT="/Users/nathan/go"
GOTOOLDIR="/Users/nathan/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/mg/sm3pzxbx1n93_2jzksr2clwm0000gp/T/go-build206309359=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

example: https://play.golang.org/p/y_wAm3nZpq

What did you expect to see?

I expected to see a 404 for a non-existing file

What did you see instead?

The server returned a 500 error

---

Opening a separate issue specifically to discuss/track the behavior of the http.FileServer.

It seems trivial to cause FileServer to emit 500 errors with paths that don't exist by making a request to something like /index.html/6am-pager-call. It's also not feasible to work around this issue without re-implementing the FileServer entirely, as the 500 is ambiguous after the handler and code beyond that is private.

As discussed in #18974, os.IsNotExist (used by FileServer) can only return true if the error itself would always mean the file does not exist. ENOTDIR does not satisfy that requirement (e.g. Readdirnames on a file). However, in a fileserver application, a call to os.Open with ENOTDIR would most accurately be described by StatusNotFound, rather than StatusInternalServerError, given the context of opening a file.

There may be other places where os.IsNotExist should not be used by itself in stdlib too, but FileServer is used commonly, as well as used as an example for other implementations, in the ecosystem.

[bradfitz]

Sounds good to me. Should be an easy fix. Want to do it?

[mastercactapus]

Yeah, I'd be happy to! I'll take a look tonight

…

On Tue, Feb 7, 2017, 4:20 PM Brad Fitzpatrick ***@***.***> wrote: Sounds good to me. Should be an easy fix. Want to do it? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#18984 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkUQterTf3jNxT1Vihd8YnDzFf8hzFqks5raO4RgaJpZM4L6HEa> .

[mastercactapus]

@bradfitz I got a test case and working implementation and started down the contributor path (CLA, gerrit, codereview, aliases etc..)

• test mastercactapus/go@6b0395e
• impl mastercactapus/go@0c27892

However, in testing I realized this is actually a unix-specific issue. As such, I think I should wait for a response to my comment on #18974 before moving forward with this route.

In either case, I'll gladly submit the change.

[bradfitz]

I'll wait until you upload the code to Gerrit to review.

Here's a plan that's portable:

In fs.go, in Dir.Open, in this code:

        f, err := os.Open(filepath.Join(dir, filepath.FromSlash(path.Clean("/"+name))))
        if err != nil {
                return nil, err
        }

Instead of just returning the error immediately, first try to os.Stat each path component down the tree to the root and see if any an os.IsNotExist(err) first. If you get one, return that error instead (which will then 404). Otherwise return the original err.

That accomplishes your goal on Unix, without OS-specific files or type assertions, and without slowing down the normal case.

[gopherbot]

CL https://golang.org/cl/36635 mentions this issue.

[gopherbot]

CL https://golang.org/cl/36804 mentions this issue.