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

Comments

Projects
None yet
3 participants
@mastercactapus
Copy link
Contributor

commented Feb 7, 2017

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 label Feb 7, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

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

@mastercactapus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2017

@mastercactapus

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2017

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Feb 9, 2017

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

@gopherbot gopherbot closed this in ee60d39 Feb 10, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Feb 11, 2017

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

gopherbot pushed a commit that referenced this issue Feb 14, 2017

net/http: handle absolute paths in mapDirOpenError
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

[noodle] Init local fs wraps http.Dir
- 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.
You can’t perform that action at this time.