Repository: labstack/echo
Pull request #2123 (2022-03-12 16:31:28)

Fix e.File() being picky with relative paths after 4.7.0 introduced echo.Fs support
Fix e.File() being picky with relative paths after 4.7.0 introduced echo.Fs support (Go 1.16+).

See https://github.com/labstack/echo/issues/2117#issuecomment-1063804825

TLDR: `fs.Fs` and to be specific `os.DirFs.Open` does not like path to files starting with `./`, `../` and `/` but `os.Open` did not care about that and now old code has problems.

```go
func main() {
	e := echo.New()
	e.Use(middleware.Logger())
	e.Use(middleware.Recover())

	e.File("", "./index.html")

	e.Logger.Fatal(e.Start(":8080"))
}
```

```bash
./
./main.go
./index.html
```

codecov[bot]: # [Codecov](https://codecov.io/gh/labstack/echo/pull/2123?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) Report
> Merging [#2123](https://codecov.io/gh/labstack/echo/pull/2123?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) (ac5f414) into [master](https://codecov.io/gh/labstack/echo/commit/5ebed440aeec1abf7f08cca41cb02f6aaf0d7f6a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) (5ebed44) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/labstack/echo/pull/2123/graphs/tree.svg?width=650&height=150&src=pr&token=vqnT76l79i&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack)](https://codecov.io/gh/labstack/echo/pull/2123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack)

```diff
@@           Coverage Diff           @@
##           master    #2123   +/-   ##
=======================================
  Coverage   92.21%   92.21%           
=======================================
  Files          37       37           
  Lines        3018     3018           
=======================================
  Hits         2783     2783           
  Misses        148      148           
  Partials       87       87           
```


| [Impacted Files](https://codecov.io/gh/labstack/echo/pull/2123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack) | Coverage Δ | |
|---|---|---|
| [echo\_fs\_go1.16.go](https://codecov.io/gh/labstack/echo/pull/2123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack#diff-ZWNob19mc19nbzEuMTYuZ28=) | `95.83% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/labstack/echo/pull/2123?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/labstack/echo/pull/2123?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack). Last update [5ebed44...ac5f414](https://codecov.io/gh/labstack/echo/pull/2123?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=labstack).

aldas: Absolute paths have similar problem with `echo.defaultFs.Open` method

```go
func main() {
	e := echo.New()
	e.Use(middleware.Logger())
	e.Use(middleware.Recover())

	e.GET("/", func(c echo.Context) error {
		return c.Attachment("/etc/hosts", "test")
	})

	e.Logger.Fatal(e.Start(":8080"))
}
```

Patch:
diff --git a/echo_fs_go1.16.go b/echo_fs_go1.16.go
index 435459d..eb17768 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"
 )
 
@@ -94,10 +95,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 +111,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) {
+	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 isRelativePath(root) {
+			root = filepath.Join(dFS.prefix, root)
+		}
 		return &defaultFS{
 			prefix: root,
 			fs:     os.DirFS(root),
@@ -130,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.
 //


Test patch:
diff --git a/echo_test.go b/echo_test.go
index d31e7b6..0e1e42b 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) {