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: serveContent() via FS() fails when the fs.File does not implement io.Seeker and content-type cannot be determined from filename #44553

Open
pdmccormick opened this issue Feb 23, 2021 · 3 comments

Comments

@pdmccormick
Copy link

@pdmccormick pdmccormick commented Feb 23, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
...
GOARCH="amd64"
GOOS="linux"
...

What did you do?

Using http.FS with an fs.FS that does not return io.Seeker files will fail to serve if the content-type cannot be guessed from the filename.

Here is a testcase bug_test.go:

package main

import (
	"io"
	"io/fs"
	"net/http"
	"net/http/httptest"
	"strings"
	"testing"
	"time"
)

const FilenameWithoutExtension = "my-filename-without-extension"

type myFs struct{ fs fs.FS }

type myFile struct{ r *strings.Reader }

func (my *myFs) Open(name string) (fs.File, error) {
	if name != FilenameWithoutExtension {
		return nil, &fs.PathError{"Open", name, fs.ErrNotExist}
	}

	var content = `Go!`

	return &myFile{strings.NewReader(content)}, nil
}

func (my *myFile) Close() error               { return nil }
func (my *myFile) Read(p []byte) (int, error) { return my.r.Read(p) }
func (my *myFile) Stat() (fs.FileInfo, error) { return &fileInfo{my.r}, nil }

type fileInfo struct{ r *strings.Reader }

func (fi *fileInfo) Name() string       { return FilenameWithoutExtension }
func (fi *fileInfo) Size() int64        { return int64(fi.r.Len()) }
func (fi *fileInfo) Mode() fs.FileMode  { return 0444 }
func (fi *fileInfo) ModTime() time.Time { var t time.Time; return t }
func (fi *fileInfo) IsDir() bool        { return false }
func (fi *fileInfo) Sys() interface{}   { return nil }

func TestDefaultContentTypeForUnseekableContent(t *testing.T) {
	var (
		fs     = &myFs{}
		httpfs = http.FS(fs)
		fsrv   = http.FileServer(httpfs)
		req, _ = http.NewRequest("GET", FilenameWithoutExtension, nil)
		rr     = httptest.NewRecorder()
	)

	f, _ := fs.Open(FilenameWithoutExtension)
	if _, ok := f.(io.Seeker); ok {
		t.Fatalf("%T must not implement `io.Seeker`", f)
	}

	fsrv.ServeHTTP(rr, req)

	t.Logf("rr %+v", rr)

	if rr.Body.String() == "seeker can't seek\n" {
		t.Logf("An `fs.File` not implementing `io.Seeker` fails when Content-Type cannot be guessed from filename")
	}

	// PDM: Suggested fix
	if rr.HeaderMap.Get("Content-Type") != "application/octet-stream" {
		t.Fatalf("BUG: Should return `application/octet-stream` when file is not seekable")
	}
}

(play.golang.org is currently running Go 1.14, but this testcase relies on io/fs from 1.16)

What did you expect to see?

When the content-type cannot be guessed from the filename, and when the content itself is not seekable, I would suggest that returning application/octet-stream would be a reasonable default.

What did you see instead?

HTTP response code 500 with body seeker can't seek (coming from https://github.com/golang/go/blob/master/src/net/http/fs.go#L243 ).

Suggested fix

The following patch to src/net/http/fs.go works around the case where *http.ioFile is being used to wrap an fs.File (via http.FS) and add a Seek method, even when a given fs.File doesn't implement it (see https://github.com/golang/go/blob/master/src/net/http/fs.go#L774-L780):

diff --git a/src/net/http/fs.go b/src/net/http/fs.go
index a28ae85..e6df3ce 100644
--- a/src/net/http/fs.go
+++ b/src/net/http/fs.go
@@ -229,10 +229,18 @@ func serveContent(w ResponseWriter, r *Request, name string, modtime time.Time,
 
        // If Content-Type isn't set, use the file's extension to find it, but
        // if the Content-Type is unset explicitly, do not sniff the type.
+       // Finally, if the underlying content doesn't support seeking, use `application/octet-stream`.
        ctypes, haveType := w.Header()["Content-Type"]
        var ctype string
        if !haveType {
                ctype = mime.TypeByExtension(filepath.Ext(name))
+
+               if ctype == "" {
+                       if f, ok := content.(*ioFile); ok {
+                               if _, ok := f.file.(io.Seeker); !ok {
+                                       ctype = "application/octet-stream"
+                               }
+                       }
+               }
+
                if ctype == "" {
                        // read a chunk to decide between utf-8 text and binary
                        var buf [sniffLen]byte
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 7, 2021

Thank you for filing this issue @pdmccormick and welcome to the Go project! Thank you @seankhliao for the triage!

@pdmccormick the contract for http.ServeContent firmly requests that an io.Seeker be passed in, as has been for the past 9 years
Screen Shot 2021-03-07 at 2 27 28 PM

Breaking this contract might be forward compatible, but it'll introduce unknown behavior and it renders http.ServeContent useful defeating its make up. The seek method is also later used to determine the size. For common usage, we recommend using http.Dir(DIRECTORY_ROOT") https://golang.org/pkg/net/http/#Dir

In what cases in the wild would it be possible to have a non-artificial fs.FS that doesn't implement io.Seeker?

@pdmccormick
Copy link
Author

@pdmccormick pdmccormick commented Mar 7, 2021

Thanks @odeke-em for the review and feedback. I suppose my problem is not so much with ServeContent itself but with the fact that serveFile and ServeContent both use serveContent, and that adapting a fs.FS to a http.FileSystem quietly imposes the extra requirement that the returned fs.Files must implement io.Seeker iff the content type cannot be guessed by the filename extension.

I came across this issue when creating a gzip decompressing filesystem wrapper: when asked to open path/to/filename, first check if path/to/filename.gz exists in the underlying filesytem, and a return a compress/gzip reader for it. My first attempt at this didn't implement any kind of seeking, and my filename happen to not have an extension. This has come in handy since embed currently embeds file data as-is.

@pdmccormick pdmccormick changed the title net/http: ServeContent() via FS() fails when content doesn't implement io.Seeker and content-type cannot be determined from filename net/http: serveContent() via FS() fails when the fs.File does not implement io.Seeker and content-type cannot be determined from filename Mar 8, 2021
@pdmccormick
Copy link
Author

@pdmccormick pdmccormick commented Mar 8, 2021

Edited title to clarify that it is serveContent (not ServeContent) invoked via (*fileHandler).ServeHTTP to serveFile to serveContent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants