allowed overwriting the default file serve headers if an explicit response header is set

CHANGELOG.md

@@ -1,3 +1,10 @@
+## (WIP) v0.13.0
+
+- Allowed overwriting the default file serve headers if an explicit response header is set.
+
+- Changed `System.GetFile()` to return directly `*blob.Reader` instead of the `io.ReadCloser` interface.
+
+
 ## v0.12.1
 
 - Fixed js error on empty relation save.

apis/file.go

@@ -92,6 +92,11 @@ func (api *fileApi) download(c echo.Context) error {
 	event.ServedPath = servedPath
 	event.ServedName = servedName
 
+	// clickjacking shouldn't be a concern when serving uploaded files,
+	// so it safe to unset the global X-Frame-Options to allow files embedding
+	// (note: it is out of the hook to allow users to customize the behavior)
+	c.Response().Header().Del("X-Frame-Options")
+
 	return api.app.OnFileDownloadRequest().Trigger(event, func(e *core.FileDownloadEvent) error {
 		res := e.HttpContext.Response()
 		req := e.HttpContext.Request()

tools/filesystem/filesystem.go

@@ -13,7 +13,6 @@ import (
 	"sort"
 	"strconv"
 	"strings"
-	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/credentials"
@@ -101,7 +100,7 @@ func (s *System) Attributes(fileKey string) (*blob.Attributes, error) {
 // GetFile returns a file content reader for the given fileKey.
 //
 // NB! Make sure to call `Close()` after you are done working with it.
-func (s *System) GetFile(fileKey string) (io.ReadCloser, error) {
+func (s *System) GetFile(fileKey string) (*blob.Reader, error) {
 	br, err := s.bucket.NewReader(s.ctx, fileKey, nil)
 	if err != nil {
 		return nil, err
@@ -316,37 +315,27 @@ func (s *System) Serve(res http.ResponseWriter, req *http.Request, fileKey strin
 		extContentType = ct
 	}
 
-	// clickjacking shouldn't be a concern when serving uploaded files,
+	setHeaderIfMissing(res, "Content-Disposition", disposition+"; filename="+name)
-	// so it safe to unset the global X-Frame-Options to allow files embedding
+	setHeaderIfMissing(res, "Content-Type", extContentType)
-	// (see https://github.com/pocketbase/pocketbase/issues/677)
+	setHeaderIfMissing(res, "Content-Security-Policy", "default-src 'none'; media-src 'self'; style-src 'unsafe-inline'; sandbox")
-	res.Header().Del("X-Frame-Options")
-
-	res.Header().Set("Content-Disposition", disposition+"; filename="+name)
-	res.Header().Set("Content-Type", extContentType)
-	res.Header().Set("Content-Length", strconv.FormatInt(br.Size(), 10))
-	res.Header().Set("Content-Security-Policy", "default-src 'none'; media-src 'self'; style-src 'unsafe-inline'; sandbox")
-
-	// all HTTP date/time stamps MUST be represented in Greenwich Mean Time (GMT)
-	// (see https://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.3.1)
-	//
-	// NB! time.LoadLocation may fail on non-Unix systems (see https://github.com/pocketbase/pocketbase/issues/45)
-	location, locationErr := time.LoadLocation("GMT")
-	if locationErr == nil {
-		res.Header().Set("Last-Modified", br.ModTime().In(location).Format("Mon, 02 Jan 06 15:04:05 MST"))
-	}
 
 	// set a default cache-control header
 	// (valid for 30 days but the cache is allowed to reuse the file for any requests
 	// that are made in the last day while revalidating the res in the background)
-	if res.Header().Get("Cache-Control") == "" {
+	setHeaderIfMissing(res, "Cache-Control", "max-age=2592000, stale-while-revalidate=86400")
-		res.Header().Set("Cache-Control", "max-age=2592000, stale-while-revalidate=86400")
-	}
 
 	http.ServeContent(res, req, name, br.ModTime(), br)
 
 	return nil
 }
 
+// note: expects key to be in a canonical form (eg. "accept-encoding" should be "Accept-Encoding").
+func setHeaderIfMissing(res http.ResponseWriter, key string, value string) {
+	if _, ok := res.Header()[key]; !ok {
+		res.Header().Set(key, value)
+	}
+}
+
 var ThumbSizeRegex = regexp.MustCompile(`^(\d+)x(\d+)(t|b|f)?$`)
 
 // CreateThumb creates a new thumb image for the file at originalKey location.

tools/filesystem/filesystem_test.go

@@ -251,9 +251,13 @@ func TestFileSystemServe(t *testing.T) {
 	}
 	defer fs.Close()
 
+	csp := "default-src 'none'; media-src 'self'; style-src 'unsafe-inline'; sandbox"
+	cacheControl := "max-age=2592000, stale-while-revalidate=86400"
+
 	scenarios := []struct {
 		path          string
 		name          string
+		customHeaders map[string]string
 		expectError   bool
 		expectHeaders map[string]string
 	}{
@@ -261,6 +265,7 @@ func TestFileSystemServe(t *testing.T) {
 			// missing
 			"missing.txt",
 			"test_name.txt",
+			nil,
 			true,
 			nil,
 		},
@@ -268,84 +273,110 @@ func TestFileSystemServe(t *testing.T) {
 			// existing regular file
 			"test/sub1.txt",
 			"test_name.txt",
+			nil,
 			false,
 			map[string]string{
 				"Content-Disposition":     "attachment; filename=test_name.txt",
 				"Content-Type":            "application/octet-stream",
 				"Content-Length":          "0",
-				"Content-Security-Policy": "default-src 'none'; media-src 'self'; style-src 'unsafe-inline'; sandbox",
+				"Content-Security-Policy": csp,
+				"Cache-Control":           cacheControl,
 			},
 		},
 		{
 			// png inline
 			"image.png",
 			"test_name.png",
+			nil,
 			false,
 			map[string]string{
 				"Content-Disposition":     "inline; filename=test_name.png",
 				"Content-Type":            "image/png",
 				"Content-Length":          "73",
-				"Content-Security-Policy": "default-src 'none'; media-src 'self'; style-src 'unsafe-inline'; sandbox",
+				"Content-Security-Policy": csp,
+				"Cache-Control":           cacheControl,
 			},
 		},
 		{
 			// svg exception
 			"image.svg",
 			"test_name.svg",
+			nil,
 			false,
 			map[string]string{
 				"Content-Disposition":     "attachment; filename=test_name.svg",
 				"Content-Type":            "image/svg+xml",
 				"Content-Length":          "0",
-				"Content-Security-Policy": "default-src 'none'; media-src 'self'; style-src 'unsafe-inline'; sandbox",
+				"Content-Security-Policy": csp,
+				"Cache-Control":           cacheControl,
 			},
 		},
 		{
 			// css exception
 			"style.css",
 			"test_name.css",
+			nil,
 			false,
 			map[string]string{
 				"Content-Disposition":     "attachment; filename=test_name.css",
 				"Content-Type":            "text/css",
 				"Content-Length":          "0",
-				"Content-Security-Policy": "default-src 'none'; media-src 'self'; style-src 'unsafe-inline'; sandbox",
+				"Content-Security-Policy": csp,
+				"Cache-Control":           cacheControl,
+			},
+		},
+		{
+			// custom header
+			"test/sub2.txt",
+			"test_name.txt",
+			map[string]string{
+				"Content-Disposition":     "1",
+				"Content-Type":            "2",
+				"Content-Length":          "3",
+				"Content-Security-Policy": "4",
+				"Cache-Control":           "5",
+				"X-Custom":                "6",
+			},
+			false,
+			map[string]string{
+				"Content-Disposition":     "1",
+				"Content-Type":            "2",
+				"Content-Length":          "0", // overwriten by http.ServeContent
+				"Content-Security-Policy": "4",
+				"Cache-Control":           "5",
+				"X-Custom":                "6",
 			},
 		},
 	}
 
-	for _, scenario := range scenarios {
+	for _, s := range scenarios {
 		res := httptest.NewRecorder()
 		req := httptest.NewRequest("GET", "/", nil)
 
-		err := fs.Serve(res, req, scenario.path, scenario.name)
+		for k, v := range s.customHeaders {
+			res.Header().Set(k, v)
+		}
+
+		err := fs.Serve(res, req, s.path, s.name)
 		hasErr := err != nil
 
-		if hasErr != scenario.expectError {
+		if hasErr != s.expectError {
-			t.Errorf("(%s) Expected hasError %v, got %v (%v)", scenario.path, scenario.expectError, hasErr, err)
+			t.Errorf("(%s) Expected hasError %v, got %v (%v)", s.path, s.expectError, hasErr, err)
 			continue
 		}
 
-		if scenario.expectError {
+		if s.expectError {
 			continue
 		}
 
 		result := res.Result()
 
-		for hName, hValue := range scenario.expectHeaders {
+		for hName, hValue := range s.expectHeaders {
 			v := result.Header.Get(hName)
 			if v != hValue {
-				t.Errorf("(%s) Expected value %q for header %q, got %q", scenario.path, hValue, hName, v)
+				t.Errorf("(%s) Expected value %q for header %q, got %q", s.path, hValue, hName, v)
 			}
 		}
-
-		if v := result.Header.Get("X-Frame-Options"); v != "" {
-			t.Errorf("(%s) Expected the X-Frame-Options header to be unset, got %v", scenario.path, v)
-		}
-
-		if v := result.Header.Get("Cache-Control"); v == "" {
-			t.Errorf("(%s) Expected Cache-Control header to be set, got empty string", scenario.path)
-		}
 	}
 }