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

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

Closed
mastercactapus opened this issue Feb 7, 2017 · 6 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@mastercactapus
Copy link
Contributor

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 bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 7, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Feb 7, 2017

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

@bradfitz bradfitz added this to the Go1.9 milestone Feb 7, 2017
@bradfitz bradfitz added help wanted Suggested Issues that may be good for new contributors looking for work to do. labels Feb 7, 2017
@mastercactapus
Copy link
Contributor Author

mastercactapus commented Feb 7, 2017 via email

@mastercactapus
Copy link
Contributor Author

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

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
Copy link
Contributor

bradfitz commented Feb 8, 2017

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
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Feb 14, 2017
The current implementation does not account for Dir being
initialized with an absolute path on systems that start
paths with filepath.Separator. In this scenario, the
original error is returned, and not checked for file
segments.

This change adds a test for this case, and corrects the
behavior by ignoring blank path segments in the loop.

Refs #18984

Change-Id: I9b79fd0a73a46976c8e2feda0283ef0bb2b62ea1
Reviewed-on: https://go-review.googlesource.com/36804
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
at15 added a commit to dyweb/gommon that referenced this issue Feb 11, 2018
- using `os.Open` would work as well but it would have many 500 if user
has url like `index.html/is-not-a-folder`
  - it is fixed in golang/go#18984 there is
a function called `mapDirOpenError`
- [ ] TODO: serveFile would call `dirList` and it can't be disabled ...
@golang golang locked and limited conversation to collaborators Feb 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests

3 participants