From cd3e7f2b185bc245def7a4995c82371801e86167 Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Sat, 12 Mar 2022 18:28:53 +0200 Subject: [PATCH 1/4] Fix e.File() being picky with relative paths after 4.7.0 introduced echo.Fs support (Go1.16+) --- echo_fs_go1.16.go | 2 +- echo_fs_go1.16_test.go | 45 +++++++++++++++++++++++++++++++ echo_test.go | 61 ++++++++++++++++++++++++++++++++++++++---- 3 files changed, 102 insertions(+), 6 deletions(-) diff --git a/echo_fs_go1.16.go b/echo_fs_go1.16.go index 435459de2..601b003f7 100644 --- a/echo_fs_go1.16.go +++ b/echo_fs_go1.16.go @@ -113,7 +113,7 @@ func newDefaultFS() *defaultFS { } func (fs defaultFS) Open(name string) (fs.File, error) { - return fs.fs.Open(name) + return fs.fs.Open(filepath.ToSlash(filepath.Clean(name))) } func subFS(currentFs fs.FS, root string) (fs.FS, error) { diff --git a/echo_fs_go1.16_test.go b/echo_fs_go1.16_test.go index 07e516555..b57608415 100644 --- a/echo_fs_go1.16_test.go +++ b/echo_fs_go1.16_test.go @@ -263,3 +263,48 @@ func TestEcho_StaticPanic(t *testing.T) { }) } } + +func TestEchoStatic16(t *testing.T) { // when we drop Go1.15 support merge these testcases with `TestEchoStatic()` + var testCases = []struct { + name string + givenPrefix string + givenRoot string + whenURL string + expectStatus int + expectHeaderLocation string + expectBodyStartsWith string + }{ + { // `e.Static` is not meant to work by pointing `root` to file. This would be insecure. + name: "nok, should not work when relative path for root points to file", + givenPrefix: "/images", + givenRoot: "./_fixture/images/walle.png", + whenURL: "/images", + expectStatus: http.StatusNotFound, + expectBodyStartsWith: "{\"message\":\"Not Found\"}\n", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := New() + e.Static(tc.givenPrefix, tc.givenRoot) + req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, tc.expectStatus, rec.Code) + body := rec.Body.String() + if tc.expectBodyStartsWith != "" { + assert.True(t, strings.HasPrefix(body, tc.expectBodyStartsWith)) + } else { + assert.Equal(t, "", body) + } + + if tc.expectHeaderLocation != "" { + assert.Equal(t, tc.expectHeaderLocation, rec.Result().Header["Location"][0]) + } else { + _, ok := rec.Result().Header["Location"] + assert.False(t, ok) + } + }) + } +} diff --git a/echo_test.go b/echo_test.go index d31e7b604..0e1e42be0 100644 --- a/echo_test.go +++ b/echo_test.go @@ -84,6 +84,14 @@ func TestEchoStatic(t *testing.T) { expectStatus: http.StatusOK, expectBodyStartsWith: string([]byte{0x89, 0x50, 0x4e, 0x47}), }, + { + name: "ok with relative path for root points to directory", + givenPrefix: "/images", + givenRoot: "./_fixture/images", + whenURL: "/images/walle.png", + expectStatus: http.StatusOK, + expectBodyStartsWith: string([]byte{0x89, 0x50, 0x4e, 0x47}), + }, { name: "No file", givenPrefix: "/images", @@ -246,11 +254,54 @@ func TestEchoStaticRedirectIndex(t *testing.T) { } func TestEchoFile(t *testing.T) { - e := New() - e.File("/walle", "_fixture/images/walle.png") - c, b := request(http.MethodGet, "/walle", e) - assert.Equal(t, http.StatusOK, c) - assert.NotEmpty(t, b) + var testCases = []struct { + name string + givenPath string + givenFile string + whenPath string + expectCode int + expectStartsWith string + }{ + { + name: "ok", + givenPath: "/walle", + givenFile: "_fixture/images/walle.png", + whenPath: "/walle", + expectCode: http.StatusOK, + expectStartsWith: string([]byte{0x89, 0x50, 0x4e}), + }, + { + name: "ok with relative path", + givenPath: "/", + givenFile: "./go.mod", + whenPath: "/", + expectCode: http.StatusOK, + expectStartsWith: "module github.com/labstack/echo/v", + }, + { + name: "nok file does not exist", + givenPath: "/", + givenFile: "./this-file-does-not-exist", + whenPath: "/", + expectCode: http.StatusNotFound, + expectStartsWith: "{\"message\":\"Not Found\"}\n", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := New() // we are using echo.defaultFS instance + e.File(tc.givenPath, tc.givenFile) + + c, b := request(http.MethodGet, tc.whenPath, e) + assert.Equal(t, tc.expectCode, c) + + if len(b) > len(tc.expectStartsWith) { + b = b[:len(tc.expectStartsWith)] + } + assert.Equal(t, tc.expectStartsWith, b) + }) + } } func TestEchoMiddleware(t *testing.T) { From 37e77f2e834c719327c0bec6d9d17d15737056ed Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Sat, 12 Mar 2022 21:56:06 +0200 Subject: [PATCH 2/4] Drop relative path test - this works differently on Linux vs Windows --- echo_fs_go1.16_test.go | 45 ------------------------------------------ 1 file changed, 45 deletions(-) diff --git a/echo_fs_go1.16_test.go b/echo_fs_go1.16_test.go index b57608415..07e516555 100644 --- a/echo_fs_go1.16_test.go +++ b/echo_fs_go1.16_test.go @@ -263,48 +263,3 @@ func TestEcho_StaticPanic(t *testing.T) { }) } } - -func TestEchoStatic16(t *testing.T) { // when we drop Go1.15 support merge these testcases with `TestEchoStatic()` - var testCases = []struct { - name string - givenPrefix string - givenRoot string - whenURL string - expectStatus int - expectHeaderLocation string - expectBodyStartsWith string - }{ - { // `e.Static` is not meant to work by pointing `root` to file. This would be insecure. - name: "nok, should not work when relative path for root points to file", - givenPrefix: "/images", - givenRoot: "./_fixture/images/walle.png", - whenURL: "/images", - expectStatus: http.StatusNotFound, - expectBodyStartsWith: "{\"message\":\"Not Found\"}\n", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - e := New() - e.Static(tc.givenPrefix, tc.givenRoot) - req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil) - rec := httptest.NewRecorder() - e.ServeHTTP(rec, req) - assert.Equal(t, tc.expectStatus, rec.Code) - body := rec.Body.String() - if tc.expectBodyStartsWith != "" { - assert.True(t, strings.HasPrefix(body, tc.expectBodyStartsWith)) - } else { - assert.Equal(t, "", body) - } - - if tc.expectHeaderLocation != "" { - assert.Equal(t, tc.expectHeaderLocation, rec.Result().Header["Location"][0]) - } else { - _, ok := rec.Result().Header["Location"] - assert.False(t, ok) - } - }) - } -} From da5f798ccbeb930bddbde8c4a721a55f4355954c Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Sun, 13 Mar 2022 13:31:02 +0200 Subject: [PATCH 3/4] Fix e.File(), c.Attachment(), being picky with relative paths after 4.7.0 introduced echo.Fs support --- echo_fs_go1.16.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/echo_fs_go1.16.go b/echo_fs_go1.16.go index 601b003f7..93b221f99 100644 --- a/echo_fs_go1.16.go +++ b/echo_fs_go1.16.go @@ -94,10 +94,12 @@ func StaticFileHandler(file string, filesystem fs.FS) HandlerFunc { } } -// defaultFS emulates os.Open behaviour with filesystem opened by `os.DirFs`. Difference between `os.Open` and `fs.Open` -// is that FS does not allow to open path that start with `..` or `/` etc. For example previously you could have `../images` -// in your application but `fs := os.DirFS("./")` would not allow you to use `fs.Open("../images")` and this would break -// all old applications that rely on being able to traverse up from current executable run path. +// defaultFS exists to preserve pre v4.7.0 behaviour where files were open by `os.Open`. +// v4.7 introduced `echo.Filesystem` field which is Go1.16+ `fs.Fs` interface. +// Difference between `os.Open` and `fs.Open` is that FS does not allow opening path that start with `.`, `..` or `/` +// etc. For example previously you could have `../images` in your application but `fs := os.DirFS("./")` would not +// allow you to use `fs.Open("../images")` and this would break all old applications that rely on being able to +// traverse up from current executable run path. // NB: private because you really should use fs.FS implementation instances type defaultFS struct { prefix string @@ -108,20 +110,26 @@ func newDefaultFS() *defaultFS { dir, _ := os.Getwd() return &defaultFS{ prefix: dir, - fs: os.DirFS(dir), + fs: nil, } } func (fs defaultFS) Open(name string) (fs.File, error) { - return fs.fs.Open(filepath.ToSlash(filepath.Clean(name))) + if fs.fs == nil { + return os.Open(name) + } + return fs.fs.Open(name) } func subFS(currentFs fs.FS, root string) (fs.FS, error) { root = filepath.ToSlash(filepath.Clean(root)) // note: fs.FS operates only with slashes. `ToSlash` is necessary for Windows if dFS, ok := currentFs.(*defaultFS); ok { - // we need to make exception for `defaultFS` instances as it interprets root prefix differently from fs.FS to - // allow cases when root is given as `../somepath` which is not valid for fs.FS - root = filepath.Join(dFS.prefix, root) + // we need to make exception for `defaultFS` instances as it interprets root prefix differently from fs.FS. + // fs.Fs.Open does not like relative paths ("./", "../") and absolute paths at all but prior echo.Filesystem we + // were able to use paths like `./myfile.log`, `/etc/hosts` and these would work fine with `os.Open` but not with fs.Fs + if len(root) > 0 && root[0] != '/' { + root = filepath.Join(dFS.prefix, root) + } return &defaultFS{ prefix: root, fs: os.DirFS(root), From 8b9b584cf4aef941ac81f04a5a8fe3418bdf1a8c Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Sun, 13 Mar 2022 14:43:25 +0200 Subject: [PATCH 4/4] Fix defaultFs relative path check for windows --- echo_fs_go1.16.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/echo_fs_go1.16.go b/echo_fs_go1.16.go index 93b221f99..eb17768ab 100644 --- a/echo_fs_go1.16.go +++ b/echo_fs_go1.16.go @@ -10,6 +10,7 @@ import ( "net/url" "os" "path/filepath" + "runtime" "strings" ) @@ -127,7 +128,7 @@ func subFS(currentFs fs.FS, root string) (fs.FS, error) { // we need to make exception for `defaultFS` instances as it interprets root prefix differently from fs.FS. // fs.Fs.Open does not like relative paths ("./", "../") and absolute paths at all but prior echo.Filesystem we // were able to use paths like `./myfile.log`, `/etc/hosts` and these would work fine with `os.Open` but not with fs.Fs - if len(root) > 0 && root[0] != '/' { + if isRelativePath(root) { root = filepath.Join(dFS.prefix, root) } return &defaultFS{ @@ -138,6 +139,21 @@ func subFS(currentFs fs.FS, root string) (fs.FS, error) { return fs.Sub(currentFs, root) } +func isRelativePath(path string) bool { + if path == "" { + return true + } + if path[0] == '/' { + return false + } + if runtime.GOOS == "windows" && strings.IndexByte(path, ':') != -1 { + // https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#file_and_directory_names + // https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats + return false + } + return true +} + // MustSubFS creates sub FS from current filesystem or panic on failure. // Panic happens when `fsRoot` contains invalid path according to `fs.ValidPath` rules. //