net/http: http.FileServer returns 500 on "not a directory" error

[jba]

go version devel +b7a85e000

linux/amd64

What did you do?

Tried to serve shared/icon/favicon.ico/static/shared/icon/favicon.ico with http.FileServer(http.FS(MY_STATIC_PATH)).

What did you expect to see?

404

What did you see instead?

500

Diagnosis

The toHTTPError function (in net/http/fs.go) treats anything other than ErrNotExist and ErrPermission as a 500. But here we have an invalid path—a path that doesn't exist—that happens to go through the existing file shared/icon/favicon.ico. It should treat that the same as ErrNotExist.

[odeke-em]

Thank you for filing this issue @jba and great to catch you here!

I have some code for you that if you minimally tweak can perhaps make your issue even more actionable and it can also serve as a test when/if the fix is issued. Mine correctly issues a 404. Please see

package issue49552

import (
	"net/http"
	"net/http/httptest"
	"net/http/httputil"
	"os"
	"path/filepath"
	"testing"
)

func TestIssue49552(t *testing.T) {
	tmpDir := t.TempDir()
	// 1. Setup the scaffolding mentioned in the issue.
	staticDirPath := filepath.Join(tmpDir, "shared")
	if err := os.MkdirAll(staticDirPath, 0755); err != nil {
		t.Fatal(err)
	}
	srv := http.FileServer(http.Dir("./shared"))
	cst := httptest.NewServer(srv)
	defer cst.Close()

	res, err := cst.Client().Get(cst.URL + "/icon/favicon.ico/static/shared/icon/favicon.ico")
	if err != nil {
		t.Fatalf("Failed to make the request: %v", err)
	}
	defer res.Body.Close()

	blob, err := httputil.DumpResponse(res, true)
	if err != nil {
		t.Fatal(err)
	}
	println(string(blob))
	if g, w := res.StatusCode, 404; g != w {
		t.Fatalf("Error code mismatch: got %d, want %d", g, w)
	}
}

which produces

$ go test
HTTP/1.1 404 Not Found
Content-Length: 19
Content-Type: text/plain; charset=utf-8
Date: Sat, 13 Nov 2021 23:51:47 GMT
X-Content-Type-Options: nosniff

404 page not found

PASS
ok  	github.com/odeke-em/bugs/golang/49552	0.186s

[neild]

If you use http.FileServer(http.Dir("dir")), you get a 404 error if a component of the path prefix is not a directory.

If you use http.FileServer(http.FS(os.DirFS("dir"))), you get a 500.

Perhaps ENOTDIR should compare as equivalent to fs.ErrNotExist, since it is a more specific case of a file not existing.

[gopherbot]

Change https://golang.org/cl/364074 mentions this issue: syscall: make ENOTDIR match fs.ErrNotExist

[neild]

If you use http.FileServer(http.FS(os.DirFS("dir"))), you get a 500.

I cannot reproduce this. I probably made a mistake in my original attempt at reproducing the original problem. You might get a 500 for os.DirFS("file_not_a_directory"), but that seems like reasonable behavior.

[jba]

A hermetic repro is below. Thanks, @odeke-em, for the starting point.
This fails on both go 1.17 and go1.18beta1.
http.Dir succeeds while http.FS fails, which suggests that the bug is in the net/http package.

package issue49552

import (
    "net/http"
    "net/http/httptest"
    "os"
    "path/filepath"
    "testing"
)

func TestIssue49552(t *testing.T) {
    tmpDir := t.TempDir()
    staticDirPath := filepath.Join(tmpDir, "static")
    if err := os.MkdirAll(staticDirPath, 0755); err != nil {
        t.Fatal(err)
    }
    if err := os.WriteFile(filepath.Join(staticDirPath, "file.txt"), []byte("contents"), 0644); err != nil {
        t.Fatal(err)
    }
    t.Run("http.Dir", func(t *testing.T) { testFileSystem(t, http.Dir(staticDirPath)) })
    t.Run("http.FS", func(t *testing.T) { testFileSystem(t, http.FS(os.DirFS(staticDirPath))) })
}

func testFileSystem(t *testing.T, hfs http.FileSystem) {
    cst := httptest.NewServer(http.FileServer(hfs))
    defer cst.Close()

    checkStatus := func(path string, want int) {
        t.Helper()
        res, err := cst.Client().Get(cst.URL + "/" + path)
        if err != nil {
            t.Fatal(err)
        }
        defer res.Body.Close()
        if got := res.StatusCode; got != want {
            t.Errorf("status: got %d, want %d", got, want)
        }
    }

    // Serve an existing file.                                                                                                                                                                                      
    checkStatus("file.txt", 200)
    // Serve a non-existent file.                                                                                                                                                                                   
    checkStatus("static/notexist", 404)
    // Serve a non-existent file with an existing file in the path.                                                                                                                                                 
    checkStatus("file.txt/file.txt", 404)
}

[jba]

Indeed, http.Dir runs the code below, which handles our problematic case.
The issue mentioned, #18984, is the twin of this issue.

// mapDirOpenError maps the provided non-nil error from opening name                                                                                                                                                
// to a possibly better non-nil error. In particular, it turns OS-specific errors                                                                                                                                   
// about opening files in non-directories into fs.ErrNotExist. See Issue 18984.                                                                                                                                     
func mapDirOpenError(originalErr error, name string) error {
    if errors.Is(originalErr, fs.ErrNotExist) || errors.Is(originalErr, fs.ErrPermission) {
        return originalErr
    }

    parts := strings.Split(name, string(filepath.Separator))
    for i := range parts {
        if parts[i] == "" {
            continue
        }
        fi, err := os.Stat(strings.Join(parts[:i+1], string(filepath.Separator)))
        if err != nil {
            return originalErr
        }
        if !fi.IsDir() {
            return fs.ErrNotExist
        }
    }
    return originalErr
}

[gopherbot]

Change https://golang.org/cl/376874 mentions this issue: net/http: map FS Open errors just like Dir