net/http: ServeFile bug when directory address ends without "/"

[cirfi]

go version 1.5.3

Suppose a directory Documents/ has sub-directories A/, B/, C/, and files a, b, c

URL in address bar: http://localhost/Documents/

Everything is okay

URL in address bar: http://localhost/Documents

Sub-directory's href is "http://localhost/A/", NOT "http://localhost/Documents/A/".

So do other sub-directories and files.

https://github.com/golang/go/blob/master/src/net/http/fs.go#L458-L461

Why is the redirect flag in serveFile(w, r, Dir(dir), file, false) set to be false?

What's the difference between ServeFile and FileServer except that FileServer will add "/" for directories and remove "/" for files?

As suggestion, I hope that ServeFile won't show directory structure. It only serves files.

Edited

Code: http://130.211.241.67:3000/test.go

The following 2 URLs has the same html output. But as two URLs have different endings, broswer generates different links.
http://130.211.241.67:3000/test

http://130.211.241.67:3000/test/

Just click "testFile"

The ServeFile function doesn't redirect directory URLs ends without "/" to right place.

If ServeFile redirects, there are no difference between ServeFile and FileServer.

@bradfitz tested

#13996 (comment)

So I wonder how to modify it.

Edited Jan 20, 2016, 13:40:00 UTC+8

@bradfitz 's test

If I change ServeFile's false to true and re-run the net/http tests, there are failures

#13996 (comment)

[ianlancetaylor]

CC @bradfitz

[bradfitz]

I don't really understand.

What is "mydomain"?
What Go code is being used?
What files & directories are on disk?
What HTML is generated, and what is expected, for which URLs? (I think part of this was answered?)

[cirfi]

@bradfitz

I'm sorry. The "mydomain" is exactly "http://localhost". I've edited it.

I was confused before. It's just because the broswer don't know whether it's a directory or file. Broswer distinguish them by the "/" at the end of the URL.

So a directory ended without "/" will be treated as a file by broswer.

Here an example:

view-source:localhost:3000/.vim

<pre>
<a href=".netrwhist">.netrwhist</a>
<a href="autoload/">autoload/</a>
<a href="bundle/">bundle/</a>
<a href="vimrc">vimrc</a>
</pre>

There is nothing wrong in href. But since "localhost:3000/.vim" is a file. The link generated by broswer is localhost:3000/autoload/, localhost:3000/bundle/ and localhost:3000/vimrc

https://github.com/golang/go/blob/master/src/net/http/fs.go#L460

Expected to be serveFile(w, r, Dir(dir), file, true) here

[bradfitz]

I'm asking about your expectation of the output, not your expectations of the code. What HTML is wrong, from your perspective?

Browser distinguish what things are by the response Content-Type. There is no concept of a "directory" as far as browsers are concerned, except for the behavior of relative links. So perhaps this bug is about relative links not working?

Sorry, I still don't understand. Please show me your server code, test files, what URL you hit, what HTML is produced, and what HTML you expect to be produced.

[cirfi]

@bradfitz

There is nothing wrong in html output.

http://cirfi.com:3000/test

http://cirfi.com:3000/test/

They have same html output.

But the links generated by broswer are different.

Click the "testFile"

[cirfi]

@bradfitz

https://github.com/golang/go/blob/master/src/net/http/fs.go#L353

The redirect flag here seems to be useless. A directory index flag will be better, I think.

Directory index is sometimes annoying. To avoid it, I have to write about 20 extra lines:

func Static(next http.Handler, staticPath string) http.Handler {
    fn := func(w http.ResponseWriter, r *http.Request) {
        const indexPage = "/index.html"
        path := staticPath + r.URL.Path[1:]
        f, err := os.Stat(path)
        if err == nil {
            if f.IsDir() {
                _, err := os.Stat(pathFix(path) + indexPage)
                if err == nil {
                    http.ServeFile(w, r, path)
                } else {
                    if next != nil {
                        next.ServeHTTP(w, r)
                    }
                }
            } else {
                http.ServeFile(w, r, path)
            }
        } else {
            if next != nil {
                next.ServeHTTP(w, r)
            }
        }
    }
    return http.HandlerFunc(fn)
}

[mattn]

@cirfi please show me your code and your expected behavior.

it seems working good to me.

[cirfi]

@mattn

Code: http://130.211.241.67:3000/test.go

The following 2 URLs has the same html output. But as they have different endings, broswer generates different links.

http://130.211.241.67:3000/test

http://130.211.241.67:3000/test/

Just click "testFile"

The ServeFile function doesn't redirect directory URLs ends without "/" to right place.

If ServeFile redirects, there are no difference between ServeFile and FileServer.

So I wonder how to modify it.

[mattn]

@cirfi Ah, you mean "accessing directory which is not ended with last slash, it should return Location header". Right?

[mattn]

@bradfitz it seems different behavior between:

package main

import (
    "net/http"
)

func main() {
    http.Handle("/", http.FileServer(http.Dir(".")))
    http.ListenAndServe(":3000", nil)
}

and

package main

import (
    "net/http"
    "path/filepath"
)

func serveFile(w http.ResponseWriter, r *http.Request) {
    http.ServeFile(w, r, filepath.Join(".", r.URL.Path))
}

func main() {
    http.HandleFunc("/", serveFile)
    http.ListenAndServe(":3000", nil)
}

first one return Location: /Document/ when accessing http://localhost:3000/Document. But second one doesn't. it just return html of directory index. Is this your expected?

[cirfi]

@mattn
Yep.
They both call serveFile funcion.
ServeFile:

func ServeFile(w ResponseWriter, r *Request, name string) {
    dir, file := filepath.Split(name)
    serveFile(w, r, Dir(dir), file, false)   // <- redirect flag is set to be false
}

FileServer:

func FileServer(root FileSystem) Handler {
    return &fileHandler{root}
}
func (f *fileHandler) ServeHTTP(w ResponseWriter, r *Request) {
    upath := r.URL.Path
    if !strings.HasPrefix(upath, "/") {
        upath = "/" + upath
        r.URL.Path = upath
    }
    serveFile(w, r, f.root, path.Clean(upath), true)  // <- redirect flag's set to be true
}

Redirect part in serveFile

if redirect {
    // redirect to canonical path: / at end of directory url
    // r.URL.Path always begins with /
    url := r.URL.Path
    if d.IsDir() {
        if url[len(url)-1] != '/' {
            localRedirect(w, r, path.Base(url)+"/")
            return
        }
    } else {
        if url[len(url)-1] == '/' {
            localRedirect(w, r, "../"+path.Base(url))
            return
        }
    }
}

[mattn]

@cirfi I'm just thinking if @bradfitz says this is expected behavior, this is not an issue and you need to put following into your code.

if !strings.HasSuffix(r.URL.Path, "/") && fileExists(filepath.Join(".", r.URL.Path)) {
    w.Header.Set("Location", r.URL.Path + "/")
    return
}

[cirfi]

@mattn

If I write a framework, it's ok for me to do such thing.
If I'm a general developper, and I hate frameworks, the best way for me is to drop ServeFile, to use FileServer.
But they all show entire directory structure, that's sometimes disgusting.

Thankfully, I have nginx.

[bradfitz]

It definitely does seem like a bug.

Looking at the history, it seems like it's been like this since the day I joined the project May 5, 2010 (git rev 954ea53):

Here's the code at that time:

https://github.com/golang/go/blob/954ea53582eaff84a560b12ce880d084d7ea428f/src/pkg/http/fs.go

// ServeFile replies to the request with the contents of the named file or directory.
func ServeFile(c *Conn, r *Request, name string) {
    serveFileInternal(c, r, name, false)
}

func serveFileInternal(c *Conn, r *Request, name string, redirect bool) {
    const indexPage = "/index.html"
...
}

It has always passed false there. I don't know why.

I remember I didn't like ServeFile at the time, but not because of how it handled directories, but how it assumed the local disk (os.Open) was the source of all content. I worked to get ServeContent added in git rev 4539d1f (#2039), but I don't think I've ever looked at ServeFile.

If I change ServeFile's false to true and re-run the net/http tests, there are failures:

--- FAIL: TestServeFile (0.00s)
        fs_test.go:976: straight get: for URL "http://127.0.0.1:55813", send error: Get ..//: stopped after 10 redirects
--- FAIL: TestServeFileContentType (0.00s)
        fs_test.go:435: Get ..//?override=0: stopped after 10 redirects
--- FAIL: TestServeFileMimeType (0.00s)
        fs_test.go:455: Get ..//: stopped after 10 redirects
--- FAIL: TestServeFileFromCWD (0.25s)
        fs_test.go:472: Get ..//: stopped after 10 redirects
--- FAIL: TestServeFileWithContentEncoding_h1 (0.00s)
        fs_test.go:502: Get ..//: stopped after 10 redirects
--- FAIL: TestServeFileWithContentEncoding_h2 (0.01s)
        fs_test.go:502: Get ..//: stopped after 10 redirects

I haven't looked into why. We can investigate in the Go 1.7 dev cycle.

For my future self, to reproduce the original report:

$ mkdir /tmp/foo
$ mkdir /tmp/foo/dir
$ echo A > /tmp/foo/A
$ echo B > /tmp/foo/B
$ echo DA > /tmp/foo/dir/DA
$ echo DB > /tmp/foo/dir/DB

func main() {
        http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
                http.ServeFile(w, r, filepath.Join("/tmp/foo", r.URL.Path[1:]))
        })
        log.Fatal(http.ListenAndServe("0.0.0.0:8080", nil))
}

[cirfi]

@bradfitz
Thank you. Never considered that setting ServeFile's false to true will cause test failures.

[gopherbot]

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

[tbroyer]

FWIW, the fix introduced code duplication. As I was cleaning it up (as a first contribution), I looked up the commit history (well, git blame) and found that issue.

I too was wondering what the redirect flag was about and simplifying the code reveals that it will redirect testdata/file/ to testdata/file (when file is a file). BTW it turns out the flag has always been there: fa60226, and I suppose ServeFile was not meant to be used to serve a directory and was expected to be called after you checked for the trailing slash in cases where you wanted to serve an index.html; or maybe that was the bug: that this “strip index.html” redirect should have been conditioned to the redirect flag or moved into fileHandler.ServeHTTP.

I would've liked to suggest that the index.html-suffix-stripping be removed (removed from ServeFile, moved to FileServer only) instead of the fix as proposed here, leaving it to the caller to check the URL and file system when in doubt, but I suppose it's too late now.

So let's dig into that redirect flag and why removing it breaks the tests: it just happens that the failing tests use ServeFile at the root. When serving a file while r.URL.Path is "/", serveFile would redirect to "../", which the user agent will normalize back to "/", leading to the redirect loop.

Fixing the redirect loop would be as easy as checking whether r.URL.Path is "/" before redirecting (I verified), but I think it would break many people who possibly use ServeFile to serve "arbitrary" files when the path ends in a slash (e.g. http.HandleFunc("/foo/", func(w http.ResponseWriter, r *http.Request) { http.ServeFile(w, r, baseDir+"foo.txt" }) because they'd now redirect to remove the trailing slash. TL;DR: we cannot remove the redirect flag, we do need two different behaviors between ServeFile and FileServer.