diff --git a/CHANGELOG.md b/CHANGELOG.md index 576b69a5..2b617664 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/apis/file.go b/apis/file.go index 25756f6d..8c1cad03 100644 --- a/apis/file.go +++ b/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() diff --git a/tools/filesystem/filesystem.go b/tools/filesystem/filesystem.go index ad00344e..69c6e775 100644 --- a/tools/filesystem/filesystem.go +++ b/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, - // so it safe to unset the global X-Frame-Options to allow files embedding - // (see https://github.com/pocketbase/pocketbase/issues/677) - 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")) - } + setHeaderIfMissing(res, "Content-Disposition", disposition+"; filename="+name) + setHeaderIfMissing(res, "Content-Type", extContentType) + setHeaderIfMissing(res, "Content-Security-Policy", "default-src 'none'; media-src 'self'; style-src 'unsafe-inline'; sandbox") // 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") == "" { - res.Header().Set("Cache-Control", "max-age=2592000, stale-while-revalidate=86400") - } + setHeaderIfMissing(res, "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. diff --git a/tools/filesystem/filesystem_test.go b/tools/filesystem/filesystem_test.go index 256d01fb..84666f5a 100644 --- a/tools/filesystem/filesystem_test.go +++ b/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 { - t.Errorf("(%s) Expected hasError %v, got %v (%v)", scenario.path, scenario.expectError, hasErr, err) + if hasErr != s.expectError { + 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) - } } }