Revised fileserver Accept-Encoding and ETag (#1435)

* Revised fileserver Accept-Encoding and ETag

* calculateEtag improved following microbenchmarking

caddyhttp/staticfiles/fileserver.go

 package staticfiles
 
 import (
-	"fmt"
 	"math/rand"
 	"net/http"
 	"os"
@@ -40,6 +39,14 @@ func (fs FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, err
 	return fs.serveFile(w, r, r.URL.Path)
 }
 
+// calculateEtag produces a strong etag by default. Prefix the result with "W/" to convert this into a weak one.
+// see https://tools.ietf.org/html/rfc7232#section-2.3
+func calculateEtag(d os.FileInfo) string {
+	t := strconv.FormatInt(d.ModTime().Unix(), 36)
+	s := strconv.FormatInt(d.Size(), 36)
+	return `"` + t + s + `"`
+}
+
 // serveFile writes the specified file to the HTTP response.
 // name is '/'-separated, not filepath.Separator.
 func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name string) (int, error) {
@@ -138,9 +145,19 @@ func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name stri
 	}
 
 	filename := d.Name()
+	etag := calculateEtag(d) // strong
 
 	for _, encoding := range staticEncodingPriority {
-		if !strings.Contains(r.Header.Get("Accept-Encoding"), encoding) {
+		acceptEncoding := strings.Split(r.Header.Get("Accept-Encoding"), ",")
+
+		accepted := false
+		for _, acc := range acceptEncoding {
+			if accepted || strings.TrimSpace(acc) == encoding {
+				accepted = true
+			}
+		}
+
+		if !accepted {
 			continue
 		}
 
@@ -158,8 +175,7 @@ func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name stri
 		// Close previous file - release fd
 		f.Close()
 
-		// Stat is needed for generating valid ETag
-		d = encodedFileInfo
+		etag = calculateEtag(encodedFileInfo)
 
 		// Encoded file will be served
 		f = encodedFile
@@ -169,12 +185,11 @@ func (fs FileServer) serveFile(w http.ResponseWriter, r *http.Request, name stri
 
 		defer f.Close()
 		break
-
 	}
 
-	// Experimental ETag header
-	e := fmt.Sprintf(`W/"%x-%x"`, d.ModTime().Unix(), d.Size())
-	w.Header().Set("ETag", e)
+	// Set the ETag returned to the user-agent. Note that a conditional If-None-Match
+	// request is handled in http.ServeContent below, which checks against this ETag value.
+	w.Header().Set("ETag", etag)
 
 	// Note: Errors generated by ServeContent are written immediately
 	// to the response. This usually only happens if seeking fails (rare).

caddyhttp/staticfiles/fileserver_test.go

@@ -19,6 +19,19 @@ var (
 	testWebRoot = filepath.Join(testDir, "webroot")
 )
 
+var (
+	webrootFile1Html            = filepath.Join("webroot", "file1.html")
+	webrootDirFile2Html         = filepath.Join("webroot", "dir", "file2.html")
+	webrootDirHiddenHtml        = filepath.Join("webroot", "dir", "hidden.html")
+	webrootDirwithindexIndeHtml = filepath.Join("webroot", "dirwithindex", "index.html")
+	webrootSubGzippedHtml       = filepath.Join("webroot", "sub", "gzipped.html")
+	webrootSubGzippedHtmlGz     = filepath.Join("webroot", "sub", "gzipped.html.gz")
+	webrootSubGzippedHtmlBr     = filepath.Join("webroot", "sub", "gzipped.html.br")
+	webrootSubBrotliHtml        = filepath.Join("webroot", "sub", "brotli.html")
+	webrootSubBrotliHtmlGz      = filepath.Join("webroot", "sub", "brotli.html.gz")
+	webrootSubBrotliHtmlBr      = filepath.Join("webroot", "sub", "brotli.html.br")
+)
+
 // testFiles is a map with relative paths to test files as keys and file content as values.
 // The map represents the following structure:
 // - $TEMP/caddy_testdir/
@@ -32,16 +45,16 @@ var (
 // '------ hidden.html
 var testFiles = map[string]string{
 	"unreachable.html":          "<h1>must not leak</h1>",
-	filepath.Join("webroot", "file1.html"):                 "<h1>file1.html</h1>",
-	filepath.Join("webroot", "sub", "gzipped.html"):        "<h1>gzipped.html</h1>",
-	filepath.Join("webroot", "sub", "gzipped.html.gz"):     "gzipped.html.gz",
-	filepath.Join("webroot", "sub", "gzipped.html.gz"):     "gzipped.html.gz",
-	filepath.Join("webroot", "sub", "brotli.html"):         "brotli.html",
-	filepath.Join("webroot", "sub", "brotli.html.gz"):      "brotli.html.gz",
-	filepath.Join("webroot", "sub", "brotli.html.br"):      "brotli.html.br",
-	filepath.Join("webroot", "dirwithindex", "index.html"): "<h1>dirwithindex/index.html</h1>",
-	filepath.Join("webroot", "dir", "file2.html"):          "<h1>dir/file2.html</h1>",
-	filepath.Join("webroot", "dir", "hidden.html"):         "<h1>dir/hidden.html</h1>",
+	webrootFile1Html:            "<h1>file1.html</h1>",
+	webrootDirFile2Html:         "<h1>dir/file2.html</h1>",
+	webrootDirwithindexIndeHtml: "<h1>dirwithindex/index.html</h1>",
+	webrootDirHiddenHtml:        "<h1>dir/hidden.html</h1>",
+	webrootSubGzippedHtml:       "<h1>gzipped.html</h1>",
+	webrootSubGzippedHtmlGz:     "1.gzipped.html.gz",
+	webrootSubGzippedHtmlBr:     "2.gzipped.html.br",
+	webrootSubBrotliHtml:        "3.brotli.html",
+	webrootSubBrotliHtmlGz:      "4.brotli.html.gz",
+	webrootSubBrotliHtmlBr:      "5.brotli.html.br",
 }
 
 // TestServeHTTP covers positive scenarios when serving files.
@@ -59,10 +72,13 @@ func TestServeHTTP(t *testing.T) {
 
 	tests := []struct {
 		url            string
+		acceptEncoding string
 
 		expectedStatus      int
 		expectedBodyContent string
 		expectedEtag        string
+		expectedVary        string
+		expectedEncoding    string
 	}{
 		// Test 0 - access without any path
 		{
@@ -78,15 +94,15 @@ func TestServeHTTP(t *testing.T) {
 		{
 			url:                 "https://foo/file1.html",
 			expectedStatus:      http.StatusOK,
-			expectedBodyContent: testFiles[filepath.Join("webroot", "file1.html")],
-			expectedEtag:        `W/"1e240-13"`,
+			expectedBodyContent: testFiles[webrootFile1Html],
+			expectedEtag:        `"2n9cj"`,
 		},
 		// Test 3 - access folder with index file with trailing slash
 		{
 			url:                 "https://foo/dirwithindex/",
 			expectedStatus:      http.StatusOK,
-			expectedBodyContent: testFiles[filepath.Join("webroot", "dirwithindex", "index.html")],
-			expectedEtag:        `W/"1e240-20"`,
+			expectedBodyContent: testFiles[webrootDirwithindexIndeHtml],
+			expectedEtag:        `"2n9cw"`,
 		},
 		// Test 4 - access folder with index file without trailing slash
 		{
@@ -125,8 +141,8 @@ func TestServeHTTP(t *testing.T) {
 		{
 			url:                 "https://foo/dirwithindex/index.html",
 			expectedStatus:      http.StatusOK,
-			expectedBodyContent: testFiles[filepath.Join("webroot", "dirwithindex", "index.html")],
-			expectedEtag:        `W/"1e240-20"`,
+			expectedBodyContent: testFiles[webrootDirwithindexIndeHtml],
+			expectedEtag:        `"2n9cw"`,
 		},
 		// Test 11 - send a request with query params
 		{
@@ -152,6 +168,7 @@ func TestServeHTTP(t *testing.T) {
 		// Test 15 - attempt to bypass hidden file
 		{
 			url:            "https://foo/dir/hidden.html%20.",
+			acceptEncoding: "br, gzip",
 			expectedStatus: http.StatusNotFound,
 		},
 		// Test 16 - serve another file with same name as hidden file.
@@ -167,16 +184,32 @@ func TestServeHTTP(t *testing.T) {
 		// Test 18 - try to get pre-gzipped file.
 		{
 			url:                 "https://foo/sub/gzipped.html",
+			acceptEncoding:      "gzip",
 			expectedStatus:      http.StatusOK,
-			expectedBodyContent: testFiles[filepath.Join("webroot", "sub", "gzipped.html.gz")],
-			expectedEtag:        `W/"1e240-f"`,
+			expectedBodyContent: testFiles[webrootSubGzippedHtmlGz],
+			expectedEtag:        `"2n9ch"`,
+			expectedVary:        "Accept-Encoding",
+			expectedEncoding:    "gzip",
 		},
 		// Test 19 - try to get pre-brotli encoded file.
 		{
 			url:                 "https://foo/sub/brotli.html",
+			acceptEncoding:      "br,gzip",
 			expectedStatus:      http.StatusOK,
-			expectedBodyContent: testFiles[filepath.Join("webroot", "sub", "brotli.html.br")],
-			expectedEtag:        `W/"1e240-e"`,
+			expectedBodyContent: testFiles[webrootSubBrotliHtmlBr],
+			expectedEtag:        `"2n9cg"`,
+			expectedVary:        "Accept-Encoding",
+			expectedEncoding:    "br",
+		},
+		// Test 20 - not allowed to get pre-brotli encoded file.
+		{
+			url:                 "https://foo/sub/brotli.html",
+			acceptEncoding:      "nicebrew", // contains "br" substring but not "br"
+			expectedStatus:      http.StatusOK,
+			expectedBodyContent: testFiles[webrootSubBrotliHtml],
+			expectedEtag:        `"2n9cd"`,
+			expectedVary:        "",
+			expectedEncoding:    "",
 		},
 		// Test 20 - treat existing file as a directory.
 		{
@@ -189,7 +222,7 @@ func TestServeHTTP(t *testing.T) {
 		responseRecorder := httptest.NewRecorder()
 		request, err := http.NewRequest("GET", test.url, nil)
 
-		request.Header.Add("Accept-Encoding", "br,gzip")
+		request.Header.Add("Accept-Encoding", test.acceptEncoding)
 
 		if err != nil {
 			t.Errorf("Test %d: Error making request: %v", i, err)
@@ -200,6 +233,9 @@ func TestServeHTTP(t *testing.T) {
 		}
 		status, err := fileserver.ServeHTTP(responseRecorder, request)
 		etag := responseRecorder.Header().Get("Etag")
+		body := responseRecorder.Body.String()
+		vary := responseRecorder.Header().Get("Vary")
+		encoding := responseRecorder.Header().Get("Content-Encoding")
 
 		// check if error matches expectations
 		if err != nil {
@@ -216,9 +252,19 @@ func TestServeHTTP(t *testing.T) {
 			t.Errorf("Test %d: Expected Etag header %s, found %s", i, test.expectedEtag, etag)
 		}
 
+		// check vary
+		if test.expectedVary != vary {
+			t.Errorf("Test %d: Expected Vary header %s, found %s", i, test.expectedVary, vary)
+		}
+
+		// check content-encoding
+		if test.expectedEncoding != encoding {
+			t.Errorf("Test %d: Expected Content-Encoding header %s, found %s", i, test.expectedEncoding, encoding)
+		}
+
 		// check body content
-		if !strings.Contains(responseRecorder.Body.String(), test.expectedBodyContent) {
-			t.Errorf("Test %d: Expected body to contain %q, found %q", i, test.expectedBodyContent, responseRecorder.Body.String())
+		if !strings.Contains(body, test.expectedBodyContent) {
+			t.Errorf("Test %d: Expected body to contain %q, found %q", i, test.expectedBodyContent, body)
 		}
 	}
 
@@ -418,3 +464,53 @@ func TestServeHTTPFailingStat(t *testing.T) {
 		}
 	}
 }
+
+//-------------------------------------------------------------------------------------------------
+
+type fileInfo struct {
+	name    string
+	size    int64
+	mode    os.FileMode
+	modTime time.Time
+	isDir   bool
+}
+
+func (fi fileInfo) Name() string {
+	return fi.name
+}
+
+func (fi fileInfo) Size() int64 {
+	return fi.size
+}
+
+func (fi fileInfo) Mode() os.FileMode {
+	return fi.mode
+}
+
+func (fi fileInfo) ModTime() time.Time {
+	return fi.modTime
+}
+
+func (fi fileInfo) IsDir() bool {
+	return fi.isDir
+}
+
+func (fi fileInfo) Sys() interface{} {
+	return nil
+}
+
+var _ os.FileInfo = fileInfo{}
+
+//-------------------------------------------------------------------------------------------------
+
+func BenchmarkEtag(b *testing.B) {
+	d := fileInfo{
+		size:    1234567890,
+		modTime: time.Now(),
+	}
+
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		calculateEtag(d)
+	}
+}