net/http: prevent redirect loop in serveFile if "/" is a normal file

When FileServer(Dir("file")) is used where "file" is a normal file and
not a directory, the server enters a redirect loop.

The usage of a file inplace of a directory path is not documented in
http.Dir and it could be considered undefined behavior.

This CL updates serveFile to check if we are trying to traverse a normal
file instead of a directory and return an error, preventing the redirect
loop.

Fixes #63769

Change-Id: I81e289444e7d0bd72189c2e7b763f5540333e2d0
GitHub-Last-Rev: 754c9a1167916b5a8c3c827391d7e4a2ff3bc44d
GitHub-Pull-Request: golang/go#63860
Reviewed-on: https://go-review.googlesource.com/c/go/+/538719
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Commit-Queue: Bryan Mills <bcmills@google.com>

diff --git a/src/net/http/fs.go b/src/net/http/fs.go
index 977c3a766ebb01cf3e9b9255cfa45860a59db90c..45cf16eed11a0c1991334267f515369736859975 100644 (file)
--- a/src/net/http/fs.go
+++ b/src/net/http/fs.go
@@ -28,7 +28,7 @@ import (
 // specific directory tree.
 //
 // While the [FileSystem.Open] method takes '/'-separated paths, a Dir's string
-// value is a filename on the native file system, not a URL, so it is separated
+// value is a directory path on the native file system, not a URL, so it is separated
 // by [filepath.Separator], which isn't necessarily '/'.
 //
 // Note that Dir could expose sensitive files and directories. Dir will follow
@@ -665,11 +665,16 @@ func serveFile(w ResponseWriter, r *Request, fs FileSystem, name string, redirec
                                localRedirect(w, r, path.Base(url)+"/")
                                return
                        }
-               } else {
-                       if url[len(url)-1] == '/' {
-                               localRedirect(w, r, "../"+path.Base(url))
+               } else if url[len(url)-1] == '/' {
+                       base := path.Base(url)
+                       if base == "/" || base == "." {
+                               // The FileSystem maps a path like "/" or "/./" to a file instead of a directory.
+                               msg := "http: attempting to traverse a non-directory"
+                               Error(w, msg, StatusInternalServerError)
                                return
                        }
+                       localRedirect(w, r, "../"+base)
+                       return
                }
        }
 

diff --git a/src/net/http/fs_test.go b/src/net/http/fs_test.go
index 383d27df9b1aba31f72c492cfe7da620e9d3f2e8..70a4b8982f4fe454461521c30e3ef3ddcbfbaa36 100644 (file)
--- a/src/net/http/fs_test.go
+++ b/src/net/http/fs_test.go
@@ -1668,3 +1668,29 @@ func (grw gzipResponseWriter) Flush() {
                fw.Flush()
        }
 }
+
+// Issue 63769
+func TestFileServerDirWithRootFile(t *testing.T) { run(t, testFileServerDirWithRootFile) }
+func testFileServerDirWithRootFile(t *testing.T, mode testMode) {
+       testDirFile := func(t *testing.T, h Handler) {
+               ts := newClientServerTest(t, mode, h).ts
+               defer ts.Close()
+
+               res, err := ts.Client().Get(ts.URL)
+               if err != nil {
+                       t.Fatal(err)
+               }
+               if g, w := res.StatusCode, StatusInternalServerError; g != w {
+                       t.Errorf("StatusCode mismatch: got %d, want: %d", g, w)
+               }
+               res.Body.Close()
+       }
+
+       t.Run("FileServer", func(t *testing.T) {
+               testDirFile(t, FileServer(Dir("testdata/index.html")))
+       })
+
+       t.Run("FileServerFS", func(t *testing.T) {
+               testDirFile(t, FileServerFS(os.DirFS("testdata/index.html")))
+       })
+}