Fix #2259 open redirect vulnerability in echo.StaticDirectoryHandler (used by e.Static, e.StaticFs etc)

remove pre Go1.16 and after differences

binder_go1.15_test.go

@@ -1,265 +0,0 @@
-// +build go1.15
-
-package echo
-
-/**
-	Since version 1.15 time.Time and time.Duration error message pattern has changed (values are wrapped now in \"\")
-	So pre 1.15 these tests fail with similar error:
-
-  expected: "code=400, message=failed to bind field value to Duration, internal=time: invalid duration \"nope\", field=param"
-  actual  : "code=400, message=failed to bind field value to Duration, internal=time: invalid duration nope, field=param"
-*/
-
-import (
-	"errors"
-	"github.com/stretchr/testify/assert"
-	"io"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-	"time"
-)
-
-func createTestContext15(URL string, body io.Reader, pathParams map[string]string) Context {
-	e := New()
-	req := httptest.NewRequest(http.MethodGet, URL, body)
-	if body != nil {
-		req.Header.Set(HeaderContentType, MIMEApplicationJSON)
-	}
-	rec := httptest.NewRecorder()
-	c := e.NewContext(req, rec)
-
-	if len(pathParams) > 0 {
-		names := make([]string, 0)
-		values := make([]string, 0)
-		for name, value := range pathParams {
-			names = append(names, name)
-			values = append(values, value)
-		}
-		c.SetParamNames(names...)
-		c.SetParamValues(values...)
-	}
-
-	return c
-}
-
-func TestValueBinder_TimeError(t *testing.T) {
-	var testCases = []struct {
-		name            string
-		givenFailFast   bool
-		givenBindErrors []error
-		whenURL         string
-		whenMust        bool
-		whenLayout      string
-		expectValue     time.Time
-		expectError     string
-	}{
-		{
-			name:        "nok, conversion fails, value is not changed",
-			whenURL:     "/search?param=nope&param=100",
-			expectValue: time.Time{},
-			expectError: "code=400, message=failed to bind field value to Time, internal=parsing time \"nope\": extra text: \"nope\", field=param",
-		},
-		{
-			name:        "nok (must), conversion fails, value is not changed",
-			whenMust:    true,
-			whenURL:     "/search?param=nope&param=100",
-			expectValue: time.Time{},
-			expectError: "code=400, message=failed to bind field value to Time, internal=parsing time \"nope\": extra text: \"nope\", field=param",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			c := createTestContext15(tc.whenURL, nil, nil)
-			b := QueryParamsBinder(c).FailFast(tc.givenFailFast)
-			if tc.givenFailFast {
-				b.errors = []error{errors.New("previous error")}
-			}
-
-			dest := time.Time{}
-			var err error
-			if tc.whenMust {
-				err = b.MustTime("param", &dest, tc.whenLayout).BindError()
-			} else {
-				err = b.Time("param", &dest, tc.whenLayout).BindError()
-			}
-
-			assert.Equal(t, tc.expectValue, dest)
-			if tc.expectError != "" {
-				assert.EqualError(t, err, tc.expectError)
-			} else {
-				assert.NoError(t, err)
-			}
-		})
-	}
-}
-
-func TestValueBinder_TimesError(t *testing.T) {
-	var testCases = []struct {
-		name            string
-		givenFailFast   bool
-		givenBindErrors []error
-		whenURL         string
-		whenMust        bool
-		whenLayout      string
-		expectValue     []time.Time
-		expectError     string
-	}{
-		{
-			name:          "nok, fail fast without binding value",
-			givenFailFast: true,
-			whenURL:       "/search?param=1&param=100",
-			expectValue:   []time.Time(nil),
-			expectError:   "code=400, message=failed to bind field value to Time, internal=parsing time \"1\" as \"2006-01-02T15:04:05Z07:00\": cannot parse \"1\" as \"2006\", field=param",
-		},
-		{
-			name:        "nok, conversion fails, value is not changed",
-			whenURL:     "/search?param=nope&param=100",
-			expectValue: []time.Time(nil),
-			expectError: "code=400, message=failed to bind field value to Time, internal=parsing time \"nope\" as \"2006-01-02T15:04:05Z07:00\": cannot parse \"nope\" as \"2006\", field=param",
-		},
-		{
-			name:        "nok (must), conversion fails, value is not changed",
-			whenMust:    true,
-			whenURL:     "/search?param=nope&param=100",
-			expectValue: []time.Time(nil),
-			expectError: "code=400, message=failed to bind field value to Time, internal=parsing time \"nope\" as \"2006-01-02T15:04:05Z07:00\": cannot parse \"nope\" as \"2006\", field=param",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			c := createTestContext15(tc.whenURL, nil, nil)
-			b := QueryParamsBinder(c).FailFast(tc.givenFailFast)
-			b.errors = tc.givenBindErrors
-
-			layout := time.RFC3339
-			if tc.whenLayout != "" {
-				layout = tc.whenLayout
-			}
-
-			var dest []time.Time
-			var err error
-			if tc.whenMust {
-				err = b.MustTimes("param", &dest, layout).BindError()
-			} else {
-				err = b.Times("param", &dest, layout).BindError()
-			}
-
-			assert.Equal(t, tc.expectValue, dest)
-			if tc.expectError != "" {
-				assert.EqualError(t, err, tc.expectError)
-			} else {
-				assert.NoError(t, err)
-			}
-		})
-	}
-}
-
-func TestValueBinder_DurationError(t *testing.T) {
-	var testCases = []struct {
-		name            string
-		givenFailFast   bool
-		givenBindErrors []error
-		whenURL         string
-		whenMust        bool
-		expectValue     time.Duration
-		expectError     string
-	}{
-		{
-			name:        "nok, conversion fails, value is not changed",
-			whenURL:     "/search?param=nope&param=100",
-			expectValue: 0,
-			expectError: "code=400, message=failed to bind field value to Duration, internal=time: invalid duration \"nope\", field=param",
-		},
-		{
-			name:        "nok (must), conversion fails, value is not changed",
-			whenMust:    true,
-			whenURL:     "/search?param=nope&param=100",
-			expectValue: 0,
-			expectError: "code=400, message=failed to bind field value to Duration, internal=time: invalid duration \"nope\", field=param",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			c := createTestContext15(tc.whenURL, nil, nil)
-			b := QueryParamsBinder(c).FailFast(tc.givenFailFast)
-			if tc.givenFailFast {
-				b.errors = []error{errors.New("previous error")}
-			}
-
-			var dest time.Duration
-			var err error
-			if tc.whenMust {
-				err = b.MustDuration("param", &dest).BindError()
-			} else {
-				err = b.Duration("param", &dest).BindError()
-			}
-
-			assert.Equal(t, tc.expectValue, dest)
-			if tc.expectError != "" {
-				assert.EqualError(t, err, tc.expectError)
-			} else {
-				assert.NoError(t, err)
-			}
-		})
-	}
-}
-
-func TestValueBinder_DurationsError(t *testing.T) {
-	var testCases = []struct {
-		name            string
-		givenFailFast   bool
-		givenBindErrors []error
-		whenURL         string
-		whenMust        bool
-		expectValue     []time.Duration
-		expectError     string
-	}{
-		{
-			name:          "nok, fail fast without binding value",
-			givenFailFast: true,
-			whenURL:       "/search?param=1&param=100",
-			expectValue:   []time.Duration(nil),
-			expectError:   "code=400, message=failed to bind field value to Duration, internal=time: missing unit in duration \"1\", field=param",
-		},
-		{
-			name:        "nok, conversion fails, value is not changed",
-			whenURL:     "/search?param=nope&param=100",
-			expectValue: []time.Duration(nil),
-			expectError: "code=400, message=failed to bind field value to Duration, internal=time: invalid duration \"nope\", field=param",
-		},
-		{
-			name:        "nok (must), conversion fails, value is not changed",
-			whenMust:    true,
-			whenURL:     "/search?param=nope&param=100",
-			expectValue: []time.Duration(nil),
-			expectError: "code=400, message=failed to bind field value to Duration, internal=time: invalid duration \"nope\", field=param",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			c := createTestContext15(tc.whenURL, nil, nil)
-			b := QueryParamsBinder(c).FailFast(tc.givenFailFast)
-			b.errors = tc.givenBindErrors
-
-			var dest []time.Duration
-			var err error
-			if tc.whenMust {
-				err = b.MustDurations("param", &dest).BindError()
-			} else {
-				err = b.Durations("param", &dest).BindError()
-			}
-
-			assert.Equal(t, tc.expectValue, dest)
-			if tc.expectError != "" {
-				assert.EqualError(t, err, tc.expectError)
-			} else {
-				assert.NoError(t, err)
-			}
-		})
-	}
-}

binder_test.go

@@ -1,4 +1,3 @@
-// run tests as external package to get real feel for API
 package echo
 
 import (
@@ -3029,3 +3028,224 @@ func BenchmarkValueBinder_BindInt64_10_fields(b *testing.B) {
 		}
 	}
 }
+
+func TestValueBinder_TimeError(t *testing.T) {
+	var testCases = []struct {
+		name            string
+		givenFailFast   bool
+		givenBindErrors []error
+		whenURL         string
+		whenMust        bool
+		whenLayout      string
+		expectValue     time.Time
+		expectError     string
+	}{
+		{
+			name:        "nok, conversion fails, value is not changed",
+			whenURL:     "/search?param=nope&param=100",
+			expectValue: time.Time{},
+			expectError: "code=400, message=failed to bind field value to Time, internal=parsing time \"nope\": extra text: \"nope\", field=param",
+		},
+		{
+			name:        "nok (must), conversion fails, value is not changed",
+			whenMust:    true,
+			whenURL:     "/search?param=nope&param=100",
+			expectValue: time.Time{},
+			expectError: "code=400, message=failed to bind field value to Time, internal=parsing time \"nope\": extra text: \"nope\", field=param",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			c := createTestContext(tc.whenURL, nil, nil)
+			b := QueryParamsBinder(c).FailFast(tc.givenFailFast)
+			if tc.givenFailFast {
+				b.errors = []error{errors.New("previous error")}
+			}
+
+			dest := time.Time{}
+			var err error
+			if tc.whenMust {
+				err = b.MustTime("param", &dest, tc.whenLayout).BindError()
+			} else {
+				err = b.Time("param", &dest, tc.whenLayout).BindError()
+			}
+
+			assert.Equal(t, tc.expectValue, dest)
+			if tc.expectError != "" {
+				assert.EqualError(t, err, tc.expectError)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestValueBinder_TimesError(t *testing.T) {
+	var testCases = []struct {
+		name            string
+		givenFailFast   bool
+		givenBindErrors []error
+		whenURL         string
+		whenMust        bool
+		whenLayout      string
+		expectValue     []time.Time
+		expectError     string
+	}{
+		{
+			name:          "nok, fail fast without binding value",
+			givenFailFast: true,
+			whenURL:       "/search?param=1&param=100",
+			expectValue:   []time.Time(nil),
+			expectError:   "code=400, message=failed to bind field value to Time, internal=parsing time \"1\" as \"2006-01-02T15:04:05Z07:00\": cannot parse \"1\" as \"2006\", field=param",
+		},
+		{
+			name:        "nok, conversion fails, value is not changed",
+			whenURL:     "/search?param=nope&param=100",
+			expectValue: []time.Time(nil),
+			expectError: "code=400, message=failed to bind field value to Time, internal=parsing time \"nope\" as \"2006-01-02T15:04:05Z07:00\": cannot parse \"nope\" as \"2006\", field=param",
+		},
+		{
+			name:        "nok (must), conversion fails, value is not changed",
+			whenMust:    true,
+			whenURL:     "/search?param=nope&param=100",
+			expectValue: []time.Time(nil),
+			expectError: "code=400, message=failed to bind field value to Time, internal=parsing time \"nope\" as \"2006-01-02T15:04:05Z07:00\": cannot parse \"nope\" as \"2006\", field=param",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			c := createTestContext(tc.whenURL, nil, nil)
+			b := QueryParamsBinder(c).FailFast(tc.givenFailFast)
+			b.errors = tc.givenBindErrors
+
+			layout := time.RFC3339
+			if tc.whenLayout != "" {
+				layout = tc.whenLayout
+			}
+
+			var dest []time.Time
+			var err error
+			if tc.whenMust {
+				err = b.MustTimes("param", &dest, layout).BindError()
+			} else {
+				err = b.Times("param", &dest, layout).BindError()
+			}
+
+			assert.Equal(t, tc.expectValue, dest)
+			if tc.expectError != "" {
+				assert.EqualError(t, err, tc.expectError)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestValueBinder_DurationError(t *testing.T) {
+	var testCases = []struct {
+		name            string
+		givenFailFast   bool
+		givenBindErrors []error
+		whenURL         string
+		whenMust        bool
+		expectValue     time.Duration
+		expectError     string
+	}{
+		{
+			name:        "nok, conversion fails, value is not changed",
+			whenURL:     "/search?param=nope&param=100",
+			expectValue: 0,
+			expectError: "code=400, message=failed to bind field value to Duration, internal=time: invalid duration \"nope\", field=param",
+		},
+		{
+			name:        "nok (must), conversion fails, value is not changed",
+			whenMust:    true,
+			whenURL:     "/search?param=nope&param=100",
+			expectValue: 0,
+			expectError: "code=400, message=failed to bind field value to Duration, internal=time: invalid duration \"nope\", field=param",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			c := createTestContext(tc.whenURL, nil, nil)
+			b := QueryParamsBinder(c).FailFast(tc.givenFailFast)
+			if tc.givenFailFast {
+				b.errors = []error{errors.New("previous error")}
+			}
+
+			var dest time.Duration
+			var err error
+			if tc.whenMust {
+				err = b.MustDuration("param", &dest).BindError()
+			} else {
+				err = b.Duration("param", &dest).BindError()
+			}
+
+			assert.Equal(t, tc.expectValue, dest)
+			if tc.expectError != "" {
+				assert.EqualError(t, err, tc.expectError)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestValueBinder_DurationsError(t *testing.T) {
+	var testCases = []struct {
+		name            string
+		givenFailFast   bool
+		givenBindErrors []error
+		whenURL         string
+		whenMust        bool
+		expectValue     []time.Duration
+		expectError     string
+	}{
+		{
+			name:          "nok, fail fast without binding value",
+			givenFailFast: true,
+			whenURL:       "/search?param=1&param=100",
+			expectValue:   []time.Duration(nil),
+			expectError:   "code=400, message=failed to bind field value to Duration, internal=time: missing unit in duration \"1\", field=param",
+		},
+		{
+			name:        "nok, conversion fails, value is not changed",
+			whenURL:     "/search?param=nope&param=100",
+			expectValue: []time.Duration(nil),
+			expectError: "code=400, message=failed to bind field value to Duration, internal=time: invalid duration \"nope\", field=param",
+		},
+		{
+			name:        "nok (must), conversion fails, value is not changed",
+			whenMust:    true,
+			whenURL:     "/search?param=nope&param=100",
+			expectValue: []time.Duration(nil),
+			expectError: "code=400, message=failed to bind field value to Duration, internal=time: invalid duration \"nope\", field=param",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			c := createTestContext(tc.whenURL, nil, nil)
+			b := QueryParamsBinder(c).FailFast(tc.givenFailFast)
+			b.errors = tc.givenBindErrors
+
+			var dest []time.Duration
+			var err error
+			if tc.whenMust {
+				err = b.MustDurations("param", &dest).BindError()
+			} else {
+				err = b.Durations("param", &dest).BindError()
+			}
+
+			assert.Equal(t, tc.expectValue, dest)
+			if tc.expectError != "" {
+				assert.EqualError(t, err, tc.expectError)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}

context_fs.go

@@ -1,33 +1,49 @@
-//go:build !go1.16
-// +build !go1.16
-
 package echo
 
 import (
+	"errors"
+	"io"
+	"io/fs"
 	"net/http"
-	"os"
 	"path/filepath"
 )
 
-func (c *context) File(file string) (err error) {
+func (c *context) File(file string) error {
-	f, err := os.Open(file)
+	return fsFile(c, file, c.echo.Filesystem)
+}
+
+// FileFS serves file from given file system.
+//
+// When dealing with `embed.FS` use `fs := echo.MustSubFS(fs, "rootDirectory") to create sub fs which uses necessary
+// prefix for directory path. This is necessary as `//go:embed assets/images` embeds files with paths
+// including `assets/images` as their prefix.
+func (c *context) FileFS(file string, filesystem fs.FS) error {
+	return fsFile(c, file, filesystem)
+}
+
+func fsFile(c Context, file string, filesystem fs.FS) error {
+	f, err := filesystem.Open(file)
 	if err != nil {
-		return NotFoundHandler(c)
+		return ErrNotFound
 	}
 	defer f.Close()
 
 	fi, _ := f.Stat()
 	if fi.IsDir() {
-		file = filepath.Join(file, indexPage)
+		file = filepath.ToSlash(filepath.Join(file, indexPage)) // ToSlash is necessary for Windows. fs.Open and os.Open are different in that aspect.
-		f, err = os.Open(file)
+		f, err = filesystem.Open(file)
 		if err != nil {
-			return NotFoundHandler(c)
+			return ErrNotFound
 		}
 		defer f.Close()
 		if fi, err = f.Stat(); err != nil {
-			return
+			return err
 		}
 	}
-	http.ServeContent(c.Response(), c.Request(), fi.Name(), fi.ModTime(), f)
+	ff, ok := f.(io.ReadSeeker)
-	return
+	if !ok {
+		return errors.New("file does not implement io.ReadSeeker")
+	}
+	http.ServeContent(c.Response(), c.Request(), fi.Name(), fi.ModTime(), ff)
+	return nil
 }

context_fs_go1.16.go

@@ -1,52 +0,0 @@
-//go:build go1.16
-// +build go1.16
-
-package echo
-
-import (
-	"errors"
-	"io"
-	"io/fs"
-	"net/http"
-	"path/filepath"
-)
-
-func (c *context) File(file string) error {
-	return fsFile(c, file, c.echo.Filesystem)
-}
-
-// FileFS serves file from given file system.
-//
-// When dealing with `embed.FS` use `fs := echo.MustSubFS(fs, "rootDirectory") to create sub fs which uses necessary
-// prefix for directory path. This is necessary as `//go:embed assets/images` embeds files with paths
-// including `assets/images` as their prefix.
-func (c *context) FileFS(file string, filesystem fs.FS) error {
-	return fsFile(c, file, filesystem)
-}
-
-func fsFile(c Context, file string, filesystem fs.FS) error {
-	f, err := filesystem.Open(file)
-	if err != nil {
-		return ErrNotFound
-	}
-	defer f.Close()
-
-	fi, _ := f.Stat()
-	if fi.IsDir() {
-		file = filepath.ToSlash(filepath.Join(file, indexPage)) // ToSlash is necessary for Windows. fs.Open and os.Open are different in that aspect.
-		f, err = filesystem.Open(file)
-		if err != nil {
-			return ErrNotFound
-		}
-		defer f.Close()
-		if fi, err = f.Stat(); err != nil {
-			return err
-		}
-	}
-	ff, ok := f.(io.ReadSeeker)
-	if !ok {
-		return errors.New("file does not implement io.ReadSeeker")
-	}
-	http.ServeContent(c.Response(), c.Request(), fi.Name(), fi.ModTime(), ff)
-	return nil
-}

context_fs_test.go

@@ -1,6 +1,3 @@
-//go:build go1.16
-// +build go1.16
-
 package echo
 
 import (

echo_fs.go

@@ -1,62 +1,175 @@
-//go:build !go1.16
-// +build !go1.16
-
 package echo
 
 import (
+	"fmt"
+	"io/fs"
 	"net/http"
 	"net/url"
 	"os"
 	"path/filepath"
+	"runtime"
+	"strings"
 )
 
 type filesystem struct {
+	// Filesystem is file system used by Static and File handlers to access files.
+	// Defaults to os.DirFS(".")
+	//
+	// When dealing with `embed.FS` use `fs := echo.MustSubFS(fs, "rootDirectory") to create sub fs which uses necessary
+	// prefix for directory path. This is necessary as `//go:embed assets/images` embeds files with paths
+	// including `assets/images` as their prefix.
+	Filesystem fs.FS
 }
 
 func createFilesystem() filesystem {
-	return filesystem{}
+	return filesystem{
-}
+		Filesystem: newDefaultFS(),
-
-// Static registers a new route with path prefix to serve static files from the
-// provided root directory.
-func (e *Echo) Static(prefix, root string) *Route {
-	if root == "" {
-		root = "." // For security we want to restrict to CWD.
 	}
-	return e.static(prefix, root, e.GET)
 }
 
-func (common) static(prefix, root string, get func(string, HandlerFunc, ...MiddlewareFunc) *Route) *Route {
+// Static registers a new route with path prefix to serve static files from the provided root directory.
-	h := func(c Context) error {
+func (e *Echo) Static(pathPrefix, fsRoot string) *Route {
-		p, err := url.PathUnescape(c.Param("*"))
+	subFs := MustSubFS(e.Filesystem, fsRoot)
-		if err != nil {
+	return e.Add(
-			return err
+		http.MethodGet,
+		pathPrefix+"*",
+		StaticDirectoryHandler(subFs, false),
+	)
+}
+
+// StaticFS registers a new route with path prefix to serve static files from the provided file system.
+//
+// When dealing with `embed.FS` use `fs := echo.MustSubFS(fs, "rootDirectory") to create sub fs which uses necessary
+// prefix for directory path. This is necessary as `//go:embed assets/images` embeds files with paths
+// including `assets/images` as their prefix.
+func (e *Echo) StaticFS(pathPrefix string, filesystem fs.FS) *Route {
+	return e.Add(
+		http.MethodGet,
+		pathPrefix+"*",
+		StaticDirectoryHandler(filesystem, false),
+	)
+}
+
+// StaticDirectoryHandler creates handler function to serve files from provided file system
+// When disablePathUnescaping is set then file name from path is not unescaped and is served as is.
+func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) HandlerFunc {
+	return func(c Context) error {
+		p := c.Param("*")
+		if !disablePathUnescaping { // when router is already unescaping we do not want to do is twice
+			tmpPath, err := url.PathUnescape(p)
+			if err != nil {
+				return fmt.Errorf("failed to unescape path variable: %w", err)
+			}
+			p = tmpPath
 		}
 
-		name := filepath.Join(root, filepath.Clean("/"+p)) // "/"+ for security
+		// fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/` as invalid
-		fi, err := os.Stat(name)
+		name := filepath.ToSlash(filepath.Clean(strings.TrimPrefix(p, "/")))
+		fi, err := fs.Stat(fileSystem, name)
 		if err != nil {
-			// The access path does not exist
+			return ErrNotFound
-			return NotFoundHandler(c)
 		}
 
 		// If the request is for a directory and does not end with "/"
 		p = c.Request().URL.Path // path must not be empty.
-		if fi.IsDir() && p[len(p)-1] != '/' {
+		if fi.IsDir() && len(p) > 0 && p[len(p)-1] != '/' {
 			// Redirect to ends with "/"
-			return c.Redirect(http.StatusMovedPermanently, p+"/")
+			return c.Redirect(http.StatusMovedPermanently, sanitizeURI(p+"/"))
 		}
-		return c.File(name)
+		return fsFile(c, name, fileSystem)
 	}
-	// Handle added routes based on trailing slash:
+}
-	// 	/prefix  => exact route "/prefix" + any route "/prefix/*"
+
-	// 	/prefix/ => only any route "/prefix/*"
+// FileFS registers a new route with path to serve file from the provided file system.
-	if prefix != "" {
+func (e *Echo) FileFS(path, file string, filesystem fs.FS, m ...MiddlewareFunc) *Route {
-		if prefix[len(prefix)-1] == '/' {
+	return e.GET(path, StaticFileHandler(file, filesystem), m...)
-			// Only add any route for intentional trailing slash
+}
-			return get(prefix+"*", h)
+
-		}
+// StaticFileHandler creates handler function to serve file from provided file system
-		get(prefix, h)
+func StaticFileHandler(file string, filesystem fs.FS) HandlerFunc {
-	}
+	return func(c Context) error {
-	return get(prefix+"/*", h)
+		return fsFile(c, file, filesystem)
+	}
+}
+
+// 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
+	fs     fs.FS
+}
+
+func newDefaultFS() *defaultFS {
+	dir, _ := os.Getwd()
+	return &defaultFS{
+		prefix: 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.
+		// 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),
+		}, nil
+	}
+	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.
+//
+// MustSubFS is helpful when dealing with `embed.FS` because for example `//go:embed assets/images` embeds files with
+// paths including `assets/images` as their prefix. In that case use `fs := echo.MustSubFS(fs, "rootDirectory") to
+// create sub fs which uses necessary prefix for directory path.
+func MustSubFS(currentFs fs.FS, fsRoot string) fs.FS {
+	subFs, err := subFS(currentFs, fsRoot)
+	if err != nil {
+		panic(fmt.Errorf("can not create sub FS, invalid root given, err: %w", err))
+	}
+	return subFs
+}
+
+func sanitizeURI(uri string) string {
+	// double slash `\\`, `//` or even `\/` is absolute uri for browsers and by redirecting request to that uri
+	// we are vulnerable to open redirect attack. so replace all slashes from the beginning with single slash
+	if len(uri) > 1 && (uri[0] == '\\' || uri[0] == '/') && (uri[1] == '\\' || uri[1] == '/') {
+		uri = "/" + strings.TrimLeft(uri, `/\`)
+	}
+	return uri
 }

echo_fs_go1.16.go

@@ -1,169 +0,0 @@
-//go:build go1.16
-// +build go1.16
-
-package echo
-
-import (
-	"fmt"
-	"io/fs"
-	"net/http"
-	"net/url"
-	"os"
-	"path/filepath"
-	"runtime"
-	"strings"
-)
-
-type filesystem struct {
-	// Filesystem is file system used by Static and File handlers to access files.
-	// Defaults to os.DirFS(".")
-	//
-	// When dealing with `embed.FS` use `fs := echo.MustSubFS(fs, "rootDirectory") to create sub fs which uses necessary
-	// prefix for directory path. This is necessary as `//go:embed assets/images` embeds files with paths
-	// including `assets/images` as their prefix.
-	Filesystem fs.FS
-}
-
-func createFilesystem() filesystem {
-	return filesystem{
-		Filesystem: newDefaultFS(),
-	}
-}
-
-// Static registers a new route with path prefix to serve static files from the provided root directory.
-func (e *Echo) Static(pathPrefix, fsRoot string) *Route {
-	subFs := MustSubFS(e.Filesystem, fsRoot)
-	return e.Add(
-		http.MethodGet,
-		pathPrefix+"*",
-		StaticDirectoryHandler(subFs, false),
-	)
-}
-
-// StaticFS registers a new route with path prefix to serve static files from the provided file system.
-//
-// When dealing with `embed.FS` use `fs := echo.MustSubFS(fs, "rootDirectory") to create sub fs which uses necessary
-// prefix for directory path. This is necessary as `//go:embed assets/images` embeds files with paths
-// including `assets/images` as their prefix.
-func (e *Echo) StaticFS(pathPrefix string, filesystem fs.FS) *Route {
-	return e.Add(
-		http.MethodGet,
-		pathPrefix+"*",
-		StaticDirectoryHandler(filesystem, false),
-	)
-}
-
-// StaticDirectoryHandler creates handler function to serve files from provided file system
-// When disablePathUnescaping is set then file name from path is not unescaped and is served as is.
-func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) HandlerFunc {
-	return func(c Context) error {
-		p := c.Param("*")
-		if !disablePathUnescaping { // when router is already unescaping we do not want to do is twice
-			tmpPath, err := url.PathUnescape(p)
-			if err != nil {
-				return fmt.Errorf("failed to unescape path variable: %w", err)
-			}
-			p = tmpPath
-		}
-
-		// fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/` as invalid
-		name := filepath.ToSlash(filepath.Clean(strings.TrimPrefix(p, "/")))
-		fi, err := fs.Stat(fileSystem, name)
-		if err != nil {
-			return ErrNotFound
-		}
-
-		// If the request is for a directory and does not end with "/"
-		p = c.Request().URL.Path // path must not be empty.
-		if fi.IsDir() && len(p) > 0 && p[len(p)-1] != '/' {
-			// Redirect to ends with "/"
-			return c.Redirect(http.StatusMovedPermanently, p+"/")
-		}
-		return fsFile(c, name, fileSystem)
-	}
-}
-
-// FileFS registers a new route with path to serve file from the provided file system.
-func (e *Echo) FileFS(path, file string, filesystem fs.FS, m ...MiddlewareFunc) *Route {
-	return e.GET(path, StaticFileHandler(file, filesystem), m...)
-}
-
-// StaticFileHandler creates handler function to serve file from provided file system
-func StaticFileHandler(file string, filesystem fs.FS) HandlerFunc {
-	return func(c Context) error {
-		return fsFile(c, file, filesystem)
-	}
-}
-
-// 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
-	fs     fs.FS
-}
-
-func newDefaultFS() *defaultFS {
-	dir, _ := os.Getwd()
-	return &defaultFS{
-		prefix: 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.
-		// 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),
-		}, nil
-	}
-	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.
-//
-// MustSubFS is helpful when dealing with `embed.FS` because for example `//go:embed assets/images` embeds files with
-// paths including `assets/images` as their prefix. In that case use `fs := echo.MustSubFS(fs, "rootDirectory") to
-// create sub fs which uses necessary prefix for directory path.
-func MustSubFS(currentFs fs.FS, fsRoot string) fs.FS {
-	subFs, err := subFS(currentFs, fsRoot)
-	if err != nil {
-		panic(fmt.Errorf("can not create sub FS, invalid root given, err: %w", err))
-	}
-	return subFs
-}

echo_fs_test.go

@@ -1,6 +1,3 @@
-//go:build go1.16
-// +build go1.16
-
 package echo
 
 import (
@@ -139,6 +136,15 @@ func TestEcho_StaticFS(t *testing.T) {
 			expectStatus:         http.StatusNotFound,
 			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
 		},
+		{
+			name:                 "open redirect vulnerability",
+			givenPrefix:          "/",
+			givenFs:              os.DirFS("_fixture/"),
+			whenURL:              "/open.redirect.hackercom%2f..",
+			expectStatus:         http.StatusMovedPermanently,
+			expectHeaderLocation: "/open.redirect.hackercom/../", // location starting with `//open` would be very bad
+			expectBodyStartsWith: "",
+		},
 	}
 
 	for _, tc := range testCases {

group_fs.go

@@ -1,9 +1,30 @@
-//go:build !go1.16
-// +build !go1.16
-
 package echo
 
+import (
+	"io/fs"
+	"net/http"
+)
+
 // Static implements `Echo#Static()` for sub-routes within the Group.
-func (g *Group) Static(prefix, root string) {
+func (g *Group) Static(pathPrefix, fsRoot string) {
-	g.static(prefix, root, g.GET)
+	subFs := MustSubFS(g.echo.Filesystem, fsRoot)
+	g.StaticFS(pathPrefix, subFs)
+}
+
+// StaticFS implements `Echo#StaticFS()` for sub-routes within the Group.
+//
+// When dealing with `embed.FS` use `fs := echo.MustSubFS(fs, "rootDirectory") to create sub fs which uses necessary
+// prefix for directory path. This is necessary as `//go:embed assets/images` embeds files with paths
+// including `assets/images` as their prefix.
+func (g *Group) StaticFS(pathPrefix string, filesystem fs.FS) {
+	g.Add(
+		http.MethodGet,
+		pathPrefix+"*",
+		StaticDirectoryHandler(filesystem, false),
+	)
+}
+
+// FileFS implements `Echo#FileFS()` for sub-routes within the Group.
+func (g *Group) FileFS(path, file string, filesystem fs.FS, m ...MiddlewareFunc) *Route {
+	return g.GET(path, StaticFileHandler(file, filesystem), m...)
 }

group_fs_go1.16.go

@@ -1,33 +0,0 @@
-//go:build go1.16
-// +build go1.16
-
-package echo
-
-import (
-	"io/fs"
-	"net/http"
-)
-
-// Static implements `Echo#Static()` for sub-routes within the Group.
-func (g *Group) Static(pathPrefix, fsRoot string) {
-	subFs := MustSubFS(g.echo.Filesystem, fsRoot)
-	g.StaticFS(pathPrefix, subFs)
-}
-
-// StaticFS implements `Echo#StaticFS()` for sub-routes within the Group.
-//
-// When dealing with `embed.FS` use `fs := echo.MustSubFS(fs, "rootDirectory") to create sub fs which uses necessary
-// prefix for directory path. This is necessary as `//go:embed assets/images` embeds files with paths
-// including `assets/images` as their prefix.
-func (g *Group) StaticFS(pathPrefix string, filesystem fs.FS) {
-	g.Add(
-		http.MethodGet,
-		pathPrefix+"*",
-		StaticDirectoryHandler(filesystem, false),
-	)
-}
-
-// FileFS implements `Echo#FileFS()` for sub-routes within the Group.
-func (g *Group) FileFS(path, file string, filesystem fs.FS, m ...MiddlewareFunc) *Route {
-	return g.GET(path, StaticFileHandler(file, filesystem), m...)
-}

group_fs_test.go

@@ -1,6 +1,3 @@
-//go:build go1.16
-// +build go1.16
-
 package echo
 
 import (

middleware/static_1_16_test.go

@@ -1,106 +0,0 @@
-// +build go1.16
-
-package middleware
-
-import (
-	"io/fs"
-	"net/http"
-	"net/http/httptest"
-	"os"
-	"testing"
-	"testing/fstest"
-
-	"github.com/labstack/echo/v4"
-	"github.com/stretchr/testify/assert"
-)
-
-func TestStatic_CustomFS(t *testing.T) {
-	var testCases = []struct {
-		name           string
-		filesystem     fs.FS
-		root           string
-		whenURL        string
-		expectContains string
-		expectCode     int
-	}{
-		{
-			name:           "ok, serve index with Echo message",
-			whenURL:        "/",
-			filesystem:     os.DirFS("../_fixture"),
-			expectCode:     http.StatusOK,
-			expectContains: "<title>Echo</title>",
-		},
-
-		{
-			name:           "ok, serve index with Echo message",
-			whenURL:        "/_fixture/",
-			filesystem:     os.DirFS(".."),
-			expectCode:     http.StatusOK,
-			expectContains: "<title>Echo</title>",
-		},
-		{
-			name:    "ok, serve file from map fs",
-			whenURL: "/file.txt",
-			filesystem: fstest.MapFS{
-				"file.txt": &fstest.MapFile{Data: []byte("file.txt is ok")},
-			},
-			expectCode:     http.StatusOK,
-			expectContains: "file.txt is ok",
-		},
-		{
-			name:       "nok, missing file in map fs",
-			whenURL:    "/file.txt",
-			expectCode: http.StatusNotFound,
-			filesystem: fstest.MapFS{
-				"file2.txt": &fstest.MapFile{Data: []byte("file2.txt is ok")},
-			},
-		},
-		{
-			name:    "nok, file is not a subpath of root",
-			whenURL: `/../../secret.txt`,
-			root:    "/nested/folder",
-			filesystem: fstest.MapFS{
-				"secret.txt": &fstest.MapFile{Data: []byte("this is a secret")},
-			},
-			expectCode: http.StatusNotFound,
-		},
-		{
-			name:       "nok, backslash is forbidden",
-			whenURL:    `/..\..\secret.txt`,
-			expectCode: http.StatusNotFound,
-			root:       "/nested/folder",
-			filesystem: fstest.MapFS{
-				"secret.txt": &fstest.MapFile{Data: []byte("this is a secret")},
-			},
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			e := echo.New()
-
-			config := StaticConfig{
-				Root:       ".",
-				Filesystem: http.FS(tc.filesystem),
-			}
-
-			if tc.root != "" {
-				config.Root = tc.root
-			}
-
-			middlewareFunc := StaticWithConfig(config)
-			e.Use(middlewareFunc)
-
-			req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil)
-			rec := httptest.NewRecorder()
-
-			e.ServeHTTP(rec, req)
-
-			assert.Equal(t, tc.expectCode, rec.Code)
-			if tc.expectContains != "" {
-				responseBody := rec.Body.String()
-				assert.Contains(t, responseBody, tc.expectContains)
-			}
-		})
-	}
-}

middleware/static_test.go

@@ -1,10 +1,13 @@
 package middleware
 
 import (
+	"io/fs"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"strings"
 	"testing"
+	"testing/fstest"
 
 	"github.com/labstack/echo/v4"
 	"github.com/stretchr/testify/assert"
@@ -207,6 +210,15 @@ func TestStatic_GroupWithStatic(t *testing.T) {
 			expectHeaderLocation: "/group/folder/",
 			expectBodyStartsWith: "",
 		},
+		{
+			name:                 "Directory redirect",
+			givenPrefix:          "/",
+			givenRoot:            "../_fixture",
+			whenURL:              "/group/folder%2f..",
+			expectStatus:         http.StatusMovedPermanently,
+			expectHeaderLocation: "/group/folder/../",
+			expectBodyStartsWith: "",
+		},
 		{
 			name:                 "Prefixed directory 404 (request URL without slash)",
 			givenGroup:           "_fixture",
@@ -306,3 +318,94 @@ func TestStatic_GroupWithStatic(t *testing.T) {
 		})
 	}
 }
+
+func TestStatic_CustomFS(t *testing.T) {
+	var testCases = []struct {
+		name           string
+		filesystem     fs.FS
+		root           string
+		whenURL        string
+		expectContains string
+		expectCode     int
+	}{
+		{
+			name:           "ok, serve index with Echo message",
+			whenURL:        "/",
+			filesystem:     os.DirFS("../_fixture"),
+			expectCode:     http.StatusOK,
+			expectContains: "<title>Echo</title>",
+		},
+
+		{
+			name:           "ok, serve index with Echo message",
+			whenURL:        "/_fixture/",
+			filesystem:     os.DirFS(".."),
+			expectCode:     http.StatusOK,
+			expectContains: "<title>Echo</title>",
+		},
+		{
+			name:    "ok, serve file from map fs",
+			whenURL: "/file.txt",
+			filesystem: fstest.MapFS{
+				"file.txt": &fstest.MapFile{Data: []byte("file.txt is ok")},
+			},
+			expectCode:     http.StatusOK,
+			expectContains: "file.txt is ok",
+		},
+		{
+			name:       "nok, missing file in map fs",
+			whenURL:    "/file.txt",
+			expectCode: http.StatusNotFound,
+			filesystem: fstest.MapFS{
+				"file2.txt": &fstest.MapFile{Data: []byte("file2.txt is ok")},
+			},
+		},
+		{
+			name:    "nok, file is not a subpath of root",
+			whenURL: `/../../secret.txt`,
+			root:    "/nested/folder",
+			filesystem: fstest.MapFS{
+				"secret.txt": &fstest.MapFile{Data: []byte("this is a secret")},
+			},
+			expectCode: http.StatusNotFound,
+		},
+		{
+			name:       "nok, backslash is forbidden",
+			whenURL:    `/..\..\secret.txt`,
+			expectCode: http.StatusNotFound,
+			root:       "/nested/folder",
+			filesystem: fstest.MapFS{
+				"secret.txt": &fstest.MapFile{Data: []byte("this is a secret")},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			e := echo.New()
+
+			config := StaticConfig{
+				Root:       ".",
+				Filesystem: http.FS(tc.filesystem),
+			}
+
+			if tc.root != "" {
+				config.Root = tc.root
+			}
+
+			middlewareFunc := StaticWithConfig(config)
+			e.Use(middlewareFunc)
+
+			req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil)
+			rec := httptest.NewRecorder()
+
+			e.ServeHTTP(rec, req)
+
+			assert.Equal(t, tc.expectCode, rec.Code)
+			if tc.expectContains != "" {
+				responseBody := rec.Body.String()
+				assert.Contains(t, responseBody, tc.expectContains)
+			}
+		})
+	}
+}