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: ServeFile bug when directory address ends without "/" #13996

Closed
cirfi opened this issue Jan 18, 2016 · 17 comments
Closed

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

cirfi opened this issue Jan 18, 2016 · 17 comments
Milestone

Comments

@cirfi
Copy link

cirfi commented Jan 18, 2016

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 ianlancetaylor changed the title net/http/fs.go: ServeFile bug when directory address ends without "/" net/http: ServeFile bug when directory address ends without "/" Jan 18, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Jan 18, 2016
@ianlancetaylor
Copy link
Contributor

CC @bradfitz

@bradfitz
Copy link
Contributor

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

cirfi commented Jan 19, 2016

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

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

cirfi commented Jan 19, 2016

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

cirfi commented Jan 19, 2016

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

mattn commented Jan 19, 2016

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

it seems working good to me.

@cirfi
Copy link
Author

cirfi commented Jan 19, 2016

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

mattn commented Jan 19, 2016

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

@mattn
Copy link
Member

mattn commented Jan 19, 2016

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

cirfi commented Jan 19, 2016

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

mattn commented Jan 19, 2016

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

cirfi commented Jan 19, 2016

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

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

cirfi commented Jan 20, 2016

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

@gopherbot
Copy link

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

@tbroyer
Copy link

tbroyer commented Jan 4, 2017

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.

@golang golang locked and limited conversation to collaborators Jan 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants