Clean on go1.20 (#2406)

* Fix tests failing on Go 1.20 on Windows. Clean works differently on 1.20. Use path.Clean instead with some workaround related to errors.

Author: aldas

middleware/context_timeout_test.go

@@ -148,7 +148,7 @@ func TestContextTimeoutWithDefaultErrorMessage(t *testing.T) {
 	c := e.NewContext(req, rec)
 
 	err := m(func(c echo.Context) error {
-		if err := sleepWithContext(c.Request().Context(), time.Duration(20*time.Millisecond)); err != nil {
+		if err := sleepWithContext(c.Request().Context(), time.Duration(80*time.Millisecond)); err != nil {
 			return err
 		}
 		return c.String(http.StatusOK, "Hello, World!")
@@ -176,7 +176,7 @@ func TestContextTimeoutCanHandleContextDeadlineOnNextHandler(t *testing.T) {
 		return nil
 	}
 
-	timeout := 10 * time.Millisecond
+	timeout := 50 * time.Millisecond
 	m := ContextTimeoutWithConfig(ContextTimeoutConfig{
 		Timeout:      timeout,
 		ErrorHandler: timeoutErrorHandler,
@@ -189,11 +189,10 @@ func TestContextTimeoutCanHandleContextDeadlineOnNextHandler(t *testing.T) {
 	c := e.NewContext(req, rec)
 
 	err := m(func(c echo.Context) error {
-		// NOTE: when difference between timeout duration and handler execution time is almost the same (in range of 100microseconds)
-		// the result of timeout does not seem to be reliable - could respond timeout, could respond handler output
-		// difference over 500microseconds (0.5millisecond) response seems to be reliable
+		// NOTE: Very short periods are not reliable for tests due to Go routine scheduling and the unpredictable order
+		// for 1) request and 2) time goroutine. For most OS this works as expected, but MacOS seems most flaky.
 
-		if err := sleepWithContext(c.Request().Context(), time.Duration(20*time.Millisecond)); err != nil {
+		if err := sleepWithContext(c.Request().Context(), 100*time.Millisecond); err != nil {
 			return err
 		}
 

middleware/static.go

@@ -8,7 +8,6 @@ import (
 	"net/url"
 	"os"
 	"path"
-	"path/filepath"
 	"strings"
 
 	"github.com/labstack/echo/v4"
@@ -157,9 +156,9 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc {
 	}
 
 	// Index template
-	t, err := template.New("index").Parse(html)
-	if err != nil {
-		panic(fmt.Sprintf("echo: %v", err))
+	t, tErr := template.New("index").Parse(html)
+	if tErr != nil {
+		panic(fmt.Errorf("echo: %w", tErr))
 	}
 
 	return func(next echo.HandlerFunc) echo.HandlerFunc {
@@ -176,7 +175,7 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc {
 			if err != nil {
 				return
 			}
-			name := filepath.Join(config.Root, filepath.Clean("/"+p)) // "/"+ for security
+			name := path.Join(config.Root, path.Clean("/"+p)) // "/"+ for security
 
 			if config.IgnoreBase {
 				routePath := path.Base(strings.TrimRight(c.Path(), "/*"))
@@ -187,12 +186,14 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc {
 				}
 			}
 
-			file, err := openFile(config.Filesystem, name)
+			file, err := config.Filesystem.Open(name)
 			if err != nil {
-				if !os.IsNotExist(err) {
+				if !isIgnorableOpenFileError(err) {
 					return err
 				}
 
+				// file with that path did not exist, so we continue down in middleware/handler chain, hoping that we end up in
+				// handler that is meant to handle this request
 				if err = next(c); err == nil {
 					return err
 				}
@@ -202,7 +203,7 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc {
 					return err
 				}
 
-				file, err = openFile(config.Filesystem, filepath.Join(config.Root, config.Index))
+				file, err = config.Filesystem.Open(path.Join(config.Root, config.Index))
 				if err != nil {
 					return err
 				}
@@ -216,15 +217,13 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc {
 			}
 
 			if info.IsDir() {
-				index, err := openFile(config.Filesystem, filepath.Join(name, config.Index))
+				index, err := config.Filesystem.Open(path.Join(name, config.Index))
 				if err != nil {
 					if config.Browse {
 						return listDir(t, name, file, c.Response())
 					}
 
-					if os.IsNotExist(err) {
-						return next(c)
-					}
+					return next(c)
 				}
 
 				defer index.Close()
@@ -242,11 +241,6 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc {
 	}
 }
 
-func openFile(fs http.FileSystem, name string) (http.File, error) {
-	pathWithSlashes := filepath.ToSlash(name)
-	return fs.Open(pathWithSlashes)
-}
-
 func serveFile(c echo.Context, file http.File, info os.FileInfo) error {
 	http.ServeContent(c.Response(), c.Request(), info.Name(), info.ModTime(), file)
 	return nil

middleware/static_other.go

@@ -0,0 +1,12 @@
+//go:build !windows
+
+package middleware
+
+import (
+	"os"
+)
+
+// We ignore these errors as there could be handler that matches request path.
+func isIgnorableOpenFileError(err error) bool {
+	return os.IsNotExist(err)
+}

middleware/static_windows.go

@@ -0,0 +1,23 @@
+package middleware
+
+import (
+	"os"
+)
+
+// We ignore these errors as there could be handler that matches request path.
+//
+// As of Go 1.20 filepath.Clean has different behaviour on OS related filesystems so we need to use path.Clean
+// on Windows which has some caveats. The Open methods might return different errors than earlier versions and
+// as of 1.20 path checks are more strict on the provided path and considers [UNC](https://en.wikipedia.org/wiki/Path_(computing)#UNC)
+// paths with missing host etc parts as invalid. Previously it would result you `fs.ErrNotExist`.
+//
+// For 1.20@Windows we need to treat those errors the same as `fs.ErrNotExists` so we can continue handling
+// errors in the middleware/handler chain. Otherwise we might end up with status 500 instead of finding a route
+// or return 404 not found.
+func isIgnorableOpenFileError(err error) bool {
+	if os.IsNotExist(err) {
+		return true
+	}
+	errTxt := err.Error()
+	return errTxt == "http: invalid or unsafe file path" || errTxt == "invalid path"
+}