diff --git a/_fixture/dist/public/admin/private.txt b/_fixture/dist/public/admin/private.txt new file mode 100644 index 000000000..f84d12eb8 --- /dev/null +++ b/_fixture/dist/public/admin/private.txt @@ -0,0 +1 @@ +public/admin/private.txt - private file diff --git a/_fixture/dist/public/hello world.txt b/_fixture/dist/public/hello world.txt new file mode 100644 index 000000000..10afc37de --- /dev/null +++ b/_fixture/dist/public/hello world.txt @@ -0,0 +1 @@ +hello world file \ No newline at end of file diff --git a/echo.go b/echo.go index ee3a53cd0..a5133ecdb 100644 --- a/echo.go +++ b/echo.go @@ -59,8 +59,6 @@ import ( "sync" "sync/atomic" "syscall" - - "github.com/labstack/echo/v5/internal/pathutil" ) // Echo is the top-level framework instance. @@ -111,6 +109,8 @@ type Echo struct { // formParseMaxMemory is passed to Context for multipart form parsing (See http.Request.ParseMultipartForm) formParseMaxMemory int64 + + enablePathUnescapingStaticFiles bool } // JSONSerializer is the interface that encodes and decodes JSON to and from interfaces. @@ -299,6 +299,31 @@ type Config struct { // FormParseMaxMemory is default value for memory limit that is used // when parsing multipart forms (See (*http.Request).ParseMultipartForm) FormParseMaxMemory int64 + + // EnablePathUnescapingStaticFiles enables path parameter (param: *) unescaping for static file methods. + // Default false (safe): encoded slashes (%2f) in the wildcard param are NOT decoded, + // preventing ACL bypass where /admin%2fprivate.txt bypasses a /admin/* route guard by + // not matching that route but having its wildcard param decoded to admin/private.txt. + // Set to true only when serving files whose names contain URL-encoded characters + // (e.g. "hello world.txt" via /hello%20world.txt) and you are not relying on + // route-based ACL guards to restrict access. + // If you are enabling this option, make sure you understand the security implications. + // See: https://github.com/labstack/echo/security/advisories/GHSA-vfp3-v2gw-7wfq + // + // Enabling RouterConfig.UseEscapedPathForMatching makes this field irrelevant and can lead to security issues when + // using different Routes to exclude some of the files from being served. + // e.g. if you serve files from directory as such and use different route to exclude some of the files from being served. + // 0. given folder structure: + // public/ + // public/index.html + // public/admin/private.txt + // 1. share `public/` folder contents from the server root with `e.Static("/", "public")` + // 2. naively assume that everything under /admin folder is now forbidden + // e.GET("/admin/*", func(c *Context) error { return echo.ErrForbidden }) + // Then request to `/assets/../admin%2fprivate.txt` will be served as router does not match it to guarded route. + // + // Applies to methods: Echo.Static, Echo.StaticFS, Group.Static, Group.StaticFS. + EnablePathUnescapingStaticFiles bool } // NewWithConfig creates an instance of Echo with given configuration. @@ -337,6 +362,8 @@ func NewWithConfig(config Config) *Echo { if config.FormParseMaxMemory > 0 { e.formParseMaxMemory = config.FormParseMaxMemory } + e.enablePathUnescapingStaticFiles = config.EnablePathUnescapingStaticFiles + return e } @@ -567,7 +594,7 @@ func (e *Echo) Static(pathPrefix, fsRoot string, middleware ...MiddlewareFunc) R return e.Add( http.MethodGet, pathPrefix+"*", - StaticDirectoryHandler(subFs, false), + StaticDirectoryHandler(subFs, !e.enablePathUnescapingStaticFiles), middleware..., ) } @@ -581,27 +608,26 @@ func (e *Echo) StaticFS(pathPrefix string, filesystem fs.FS, middleware ...Middl return e.Add( http.MethodGet, pathPrefix+"*", - StaticDirectoryHandler(filesystem, false), + StaticDirectoryHandler(filesystem, !e.enablePathUnescapingStaticFiles), middleware..., ) } // 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. +// +// Note: when disablePathUnescaping=false, the handler decodes the wildcard param before serving. +// If route guards (e.g. e.GET("/admin/*", forbidden)) are used to restrict parts of the +// filesystem, an encoded separator (%2F) or encoded dot-dot (%2E%2E) in the URL can resolve to +// a path that the router never matched against the guard route. Enabling +// RouterConfig.UseEscapedPathForMatching does NOT fix this — it changes which path the router +// uses for matching but still lets path.Clean resolve ".." segments into a guarded directory. +// Do not rely on route guards alone to restrict a filesystem served by this handler. +// See https://github.com/labstack/echo/security/advisories/GHSA-vfp3-v2gw-7wfq 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 - // By default the router matches routes against the raw, still-encoded request path - // (unless UseEscapedPathForMatching is enabled), so an encoded path separator is not - // treated as a segment boundary during routing. Unescaping it here would let it act as - // a separator and resolve a file outside the path the router authorized, bypassing - // route-level middleware (e.g. auth on a sibling route). No real filename contains a - // separator, so reject it as not found, carrying the reason internally for operators. - if pathutil.HasEncodedPathSeparator(p) { - return NewHTTPError(http.StatusNotFound, http.StatusText(http.StatusNotFound)). - Wrap(fmt.Errorf("rejected encoded path separator in static path %q", p)) - } tmpPath, err := url.PathUnescape(p) if err != nil { return fmt.Errorf("failed to unescape path variable: %w", err) @@ -609,7 +635,8 @@ func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) Handle p = tmpPath } - // fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/` as invalid. + // fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/` + // as invalid // Use path.Clean (not filepath.Clean): fs.FS paths are always forward-slash, so a backslash must stay a literal // character rather than being interpreted as a separator on Windows (which would resolve a file across a boundary // the router never matched on, the same Windows backslash traversal class as GHSA-pgvm-wxw2-hrv9). @@ -619,10 +646,9 @@ func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) Handle 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 the request is for a directory and does not end with "/" redirect to path which ends with "/" + p = c.Request().URL.Path if fi.IsDir() && len(p) > 0 && p[len(p)-1] != '/' { - // Redirect to ends with "/" return c.Redirect(http.StatusMovedPermanently, sanitizeURI(p+"/")) } return fsFile(c, name, fileSystem) diff --git a/echo_test.go b/echo_test.go index 0af0b96ec..73d6ba830 100644 --- a/echo_test.go +++ b/echo_test.go @@ -20,6 +20,7 @@ import ( "runtime" "strings" "testing" + "testing/fstest" "time" "github.com/stretchr/testify/assert" @@ -133,15 +134,21 @@ func TestNewDefaultFS(t *testing.T) { } func TestEcho_StaticFS(t *testing.T) { + dotsInFilenameFS := fstest.MapFS{ + // On Windows filename `%2f..` can not be created but in Linux it is possible. + "%2f..": {Data: []byte("This filename is escaped to `/..` in URL.Path")}, + } + var testCases = []struct { - givenFs fs.FS - name string - givenPrefix string - givenFsRoot string - whenURL string - expectHeaderLocation string - expectBodyStartsWith string - expectStatus int + givenFs fs.FS + name string + givenPrefix string + givenFsRoot string + givenEnablePathUnescapingStaticFiles bool + whenURL string + expectHeaderLocation string + expectBodyStartsWith string + expectStatus int }{ { name: "ok", @@ -250,6 +257,33 @@ func TestEcho_StaticFS(t *testing.T) { expectStatus: http.StatusNotFound, expectBodyStartsWith: "{\"message\":\"Not Found\"}\n", }, + { + name: "do not accept encoded dots in path (%2E%2E is `..`) to traverse within filesystem boundary", + givenPrefix: "/", + givenFs: os.DirFS("_fixture/"), + givenEnablePathUnescapingStaticFiles: false, + whenURL: `/dist/public/%2E%2E/private.txt`, // `/dist/public/../private.txt` + expectStatus: http.StatusNotFound, + expectBodyStartsWith: "{\"message\":\"Not Found\"}\n", + }, + { + name: "allow encoded dots in path (%2E%2E is `..`) to traverse within filesystem", + givenPrefix: "/", + givenFs: os.DirFS("_fixture/"), + givenEnablePathUnescapingStaticFiles: true, + whenURL: `/dist/public/%2E%2E/private.txt`, // `/dist/public/../private.txt` + expectStatus: http.StatusOK, + expectBodyStartsWith: "private file", + }, + { + name: "ok, file with space in name is served when path unescaping is enabled", + givenPrefix: "/", + givenFs: os.DirFS("_fixture/dist/public"), + givenEnablePathUnescapingStaticFiles: true, + whenURL: "/hello%20world.txt", + expectStatus: http.StatusOK, + expectBodyStartsWith: "hello world file", + }, { name: "do not allow directory traversal (slash - unix separator)", givenPrefix: "/", @@ -259,22 +293,42 @@ func TestEcho_StaticFS(t *testing.T) { expectBodyStartsWith: "{\"message\":\"Not Found\"}\n", }, { - // An encoded slash (%2f) is rejected outright (GHSA-vfp3-v2gw-7wfq): by default the - // router matches on the raw path so %2f is not a separator, and unescaping it here - // would let it act as one. No redirect is emitted, closing the open-redirect vector. - name: "encoded slash is rejected, not redirected", + name: "do not unescape path variables by default", givenPrefix: "/", givenFs: os.DirFS("_fixture/"), whenURL: "/open.redirect.hackercom%2f..", expectStatus: http.StatusNotFound, - expectHeaderLocation: "", expectBodyStartsWith: "{\"message\":\"Not Found\"}\n", }, + { + name: "possible open redirect vulnerability when unescaping path variables in static handler", + givenPrefix: "/", + givenFs: os.DirFS("_fixture/"), + givenEnablePathUnescapingStaticFiles: true, + // `//open.redirect.hackercom/..` resolves to directory but does not end with `/` so redirect is done but this + // redirect can not be to path starting with `//` or `\\` (open redirect) + whenURL: "/%2fopen.redirect.hackercom%2f..", + expectStatus: http.StatusMovedPermanently, + expectHeaderLocation: "/open.redirect.hackercom/../", // location starting with `//open` would be very bad + expectBodyStartsWith: "", + }, + { + name: "possible open redirect vulnerability when not unescaping path variables in static handler", + givenPrefix: "/", + givenFs: dotsInFilenameFS, + givenEnablePathUnescapingStaticFiles: false, + whenURL: "/%2f..", + expectStatus: http.StatusOK, + expectHeaderLocation: "", + expectBodyStartsWith: "This filename is escaped to `/..` in URL.Path", + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - e := New() + e := NewWithConfig(Config{ + EnablePathUnescapingStaticFiles: tc.givenEnablePathUnescapingStaticFiles, + }) tmpFs := tc.givenFs if tc.givenFsRoot != "" { @@ -305,6 +359,122 @@ func TestEcho_StaticFS(t *testing.T) { } } +func TestStaticDirectoryHandlerAndRouterInconsistentEscaping(t *testing.T) { + var testCases = []struct { + name string + givenEnablePathUnescapingStaticFiles bool + givenRouterUnescapePathParamValues bool + givenRouterUseEscapedPathForMatching bool + whenURL string + expectBody string + expectStatus int + }{ + { + name: "ok, file is served from not-forbidden path", + givenEnablePathUnescapingStaticFiles: false, + whenURL: "/test.txt", + expectBody: "test.txt contents", + expectStatus: http.StatusOK, + }, + { + name: "ok, forbidden path is matched by route wildcard and forbidden by that", + givenEnablePathUnescapingStaticFiles: false, + whenURL: "/admin/private.txt", + expectBody: "{\"message\":\"Forbidden\"}", + expectStatus: http.StatusForbidden, + }, + { + name: "ok, escaped filename from forbidden path is routed to guarded route", + givenEnablePathUnescapingStaticFiles: false, + givenRouterUnescapePathParamValues: false, + givenRouterUseEscapedPathForMatching: true, // Router uses escaped path (req.URL.RawPath) for matching + whenURL: "/admin%2fprivate.txt", + expectBody: "{\"message\":\"Forbidden\"}", + expectStatus: http.StatusForbidden, + }, + { + name: "ok, escaped filename from forbidden path is not unescaped and results 404", + givenEnablePathUnescapingStaticFiles: false, // router path escaping and StaticDirectoryHandler is consistent + whenURL: "/admin%2fprivate.txt", + expectBody: "{\"message\":\"Not Found\"}", + expectStatus: http.StatusNotFound, + }, + { + name: "nok, escaped filename from forbidden path is unescaped and returns file contents (handler unescapes)", + givenEnablePathUnescapingStaticFiles: true, // router path escaping and StaticDirectoryHandler is NOT consistent + givenRouterUnescapePathParamValues: false, + whenURL: "/admin%2fprivate.txt", + expectBody: "public/admin/private.txt - private file", + expectStatus: http.StatusOK, + }, + { + name: "nok, escaped filename from forbidden path is unescaped and returns file contents (router unescapes)", + givenEnablePathUnescapingStaticFiles: false, + givenRouterUnescapePathParamValues: true, // router path escaping and StaticDirectoryHandler is NOT consistent + whenURL: "/admin%2fprivate.txt", + expectBody: "public/admin/private.txt - private file", + expectStatus: http.StatusOK, + }, + { + name: "nok, unescaped filename from forbidden path is escaped and returns file contents (router unescapes and method unescapes)", + givenEnablePathUnescapingStaticFiles: true, + givenRouterUnescapePathParamValues: true, // consistent path unescaping - makes no difference + whenURL: "/admin%2fprivate.txt", + expectBody: "public/admin/private.txt - private file", + expectStatus: http.StatusOK, + }, + { + name: "nok, escaped filename, resolves to from forbidden path is not routed to guarded route and includes guarded file", + givenEnablePathUnescapingStaticFiles: false, + givenRouterUnescapePathParamValues: false, + // Router uses escaped path (req.URL.RawPath) for matching, but that file resolves to `admin/private.txt` after path.Clean() + givenRouterUseEscapedPathForMatching: true, + whenURL: "/assets/../admin%2fprivate.txt", + expectBody: "public/admin/private.txt - private file", + expectStatus: http.StatusOK, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r := NewRouter(RouterConfig{ + UnescapePathParamValues: tc.givenRouterUnescapePathParamValues, + UseEscapedPathForMatching: tc.givenRouterUseEscapedPathForMatching, + }) + e := NewWithConfig(Config{ + EnablePathUnescapingStaticFiles: tc.givenEnablePathUnescapingStaticFiles, + Router: r, + Filesystem: os.DirFS("./_fixture/dist"), + }) + + // 0. + // given folder structure: + // private.txt + // public/ + // public/index.html + // public/text.txt + // public/admin/private.txt + + // 1. share `public/` folder contents from the server root. This folder actually contains subfolder `admin` which + // contents we want to forbid from downloading + e.Static("/", "public") + + // 2. naively assume that everything under /admin folder is now forbidden + e.GET("/admin/*", func(c *Context) error { + return ErrForbidden + }) + + req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil) + rec := httptest.NewRecorder() + + e.ServeHTTP(rec, req) + + assert.Equal(t, tc.expectStatus, rec.Code) + body := strings.TrimRight(rec.Body.String(), "\r\n") + assert.Equal(t, tc.expectBody, body) + }) + } +} + func TestEcho_FileFS(t *testing.T) { var testCases = []struct { whenFS fs.FS diff --git a/group.go b/group.go index 8092bc904..03289fac3 100644 --- a/group.go +++ b/group.go @@ -123,7 +123,7 @@ func (g *Group) StaticFS(pathPrefix string, filesystem fs.FS, middleware ...Midd return g.Add( http.MethodGet, pathPrefix+"*", - StaticDirectoryHandler(filesystem, false), + StaticDirectoryHandler(filesystem, !g.echo.enablePathUnescapingStaticFiles), middleware..., ) } diff --git a/internal/pathutil/pathutil.go b/internal/pathutil/pathutil.go deleted file mode 100644 index e9171752e..000000000 --- a/internal/pathutil/pathutil.go +++ /dev/null @@ -1,30 +0,0 @@ -// SPDX-License-Identifier: MIT -// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors - -// Package pathutil holds internal helpers for safely handling request and file -// paths. It is internal so it can be shared between the echo package and the -// middleware package without becoming part of the public API. -package pathutil - -// HasEncodedPathSeparator reports whether s contains a percent-encoded path -// separator, case-insensitively: %2F/%2f (forward slash) or %5C/%5c (backslash). -// Backslash is included as defense-in-depth against Windows-style separators even -// though fs.FS itself only uses forward slashes. -// -// Such sequences let an attacker smuggle a separator past the router, which by -// default matches on the raw encoded path, so they must be rejected before -// unescaping when resolving static files. -func HasEncodedPathSeparator(s string) bool { - for i := 0; i+2 < len(s); i++ { - if s[i] != '%' { - continue - } - switch { - case s[i+1] == '2' && (s[i+2] == 'f' || s[i+2] == 'F'): // %2F - return true - case s[i+1] == '5' && (s[i+2] == 'c' || s[i+2] == 'C'): // %5C - return true - } - } - return false -} diff --git a/internal/pathutil/pathutil_test.go b/internal/pathutil/pathutil_test.go deleted file mode 100644 index fd9a9d90d..000000000 --- a/internal/pathutil/pathutil_test.go +++ /dev/null @@ -1,27 +0,0 @@ -// SPDX-License-Identifier: MIT -// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors - -package pathutil - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestHasEncodedPathSeparator(t *testing.T) { - for s, want := range map[string]bool{ - "foo/bar.txt": false, - "100%25.txt": false, // encoded percent, not a separator - "a%2Fb": true, - "a%2fb": true, - "a%5Cb": true, - "a%5cb": true, - "trailing%2F": true, - "%2F": true, - "%2": false, // truncated, not a full sequence - "": false, - } { - assert.Equal(t, want, HasEncodedPathSeparator(s), "input=%q", s) - } -} diff --git a/middleware/static.go b/middleware/static.go index 5169ec80d..62cf04e34 100644 --- a/middleware/static.go +++ b/middleware/static.go @@ -18,7 +18,6 @@ import ( "sync" "github.com/labstack/echo/v5" - "github.com/labstack/echo/v5/internal/pathutil" ) // StaticConfig defines the config for Static middleware. @@ -54,10 +53,31 @@ type StaticConfig struct { // Optional. Default value false. IgnoreBase bool - // DisablePathUnescaping disables path parameter (param: *) unescaping. This is useful when router is set to unescape - // all parameter and doing it again in this middleware would corrupt filename that is requested. + // Deprecated: this field is ignored, use EnablePathUnescaping instead. DisablePathUnescaping will be removed in a future version. + // Note: previously the zero value (false) enabled unescaping, which was the unsafe default. DisablePathUnescaping bool + // EnablePathUnescaping enables path parameter (param: *) unescaping. + // Default false (safe): encoded slashes (%2f) in the wildcard param are NOT decoded, + // preventing ACL bypass where /admin%2fprivate.txt bypasses a /admin/* route guard by + // not matching that route but having its wildcard param decoded to admin/private.txt. + // Set to true only when serving files whose names contain URL-encoded characters + // (e.g. "hello world.txt" via /hello%20world.txt) and you are not relying on + // route-based ACL guards to restrict access. + // + // Enabling echo.RouterConfig.UseEscapedPathForMatching makes this field irrelevant and can lead to security issues when + // using different Routes to exclude some of the files from being served. + // e.g. if you serve files from directory as such and use different route to exclude some of the files from being served. + // 0. given folder structure: + // public/ + // public/index.html + // public/admin/private.txt + // 1. share `public/` folder contents from the server root with `e.Static("/", "public")` + // 2. naively assume that everything under /admin folder is now forbidden + // e.GET("/admin/*", func(c *Context) error { return echo.ErrForbidden }) + // Then request to `/assets/../admin%2fprivate.txt` will be served as router does not match it to guarded route. + EnablePathUnescaping bool + // DirectoryListTemplate is template to list directory contents // Optional. Default to `directoryListHTMLTemplate` constant below. DirectoryListTemplate string @@ -197,23 +217,10 @@ func (config StaticConfig) ToMiddleware() (echo.MiddlewareFunc, error) { } p := c.Request().URL.Path - // URL.Path is already decoded by net/http, so it must not be unescaped - // again (doing so breaks file names containing '%', see #2599). Only the - // wildcard param from a group route (set below) may still be escaped. - pathUnescape := false if strings.HasSuffix(c.Path(), "*") { // When serving from a group, e.g. `/static*`. p = c.Param("*") - pathUnescape = !config.DisablePathUnescaping // because router could already do PathUnescape } - if pathUnescape { - // The router matched on the raw, still-encoded path (by default), so an encoded - // path separator in the wildcard would only now become a real separator and - // resolve a file the matched route never authorized, bypassing route-level - // middleware. Reject it before unescaping (see echo.StaticDirectoryHandler). - if pathutil.HasEncodedPathSeparator(p) { - return echo.NewHTTPError(http.StatusNotFound, http.StatusText(http.StatusNotFound)). - Wrap(fmt.Errorf("rejected encoded path separator in static path %q", p)) - } + if config.EnablePathUnescaping { p, err = url.PathUnescape(p) if err != nil { return err diff --git a/middleware/static_test.go b/middleware/static_test.go index 0390c9e93..3567c0a55 100644 --- a/middleware/static_test.go +++ b/middleware/static_test.go @@ -280,6 +280,78 @@ func TestStatic(t *testing.T) { } } +func TestStaticMiddlewareAndRouterInconsistentEscaping(t *testing.T) { + var testCases = []struct { + name string + givenConfig StaticConfig + whenURL string + expectCode int + expectBodyContains string + expectBodyNotContains string + }{ + { + name: "ok, normal file is served", + givenConfig: StaticConfig{Root: "testdata/dist/public"}, + whenURL: "/test.txt", + expectCode: http.StatusOK, + expectBodyContains: "test", + }, + { + name: "ok, direct request to restricted path is blocked by ACL route", + givenConfig: StaticConfig{Root: "testdata/dist/public"}, + whenURL: "/admin/private.txt", + expectCode: http.StatusForbidden, + }, + { + // With EnablePathUnescaping=false (default/safe), the wildcard param "admin%2fprivate.txt" + // is NOT decoded, so the FS lookup is for literal "admin%2fprivate.txt" which does + // not exist → falls through to the /* handler → 404. ACL is not bypassed. + name: "ok, encoded slash returns 404 with default safe config (EnablePathUnescaping=false)", + givenConfig: StaticConfig{Root: "testdata/dist/public"}, + whenURL: "/admin%2fprivate.txt", + expectCode: http.StatusNotFound, + expectBodyNotContains: "private file", + }, + { + // With EnablePathUnescaping=true, the wildcard param "admin%2fprivate.txt" IS decoded + // to "admin/private.txt". The router already routed to /* (encoded %2f prevented matching + // /admin/*), so the ACL guard never ran. The file is served — ACL bypass. + // Only use EnablePathUnescaping: true when not relying on route-based ACL guards. + name: "nok, encoded slash bypasses ACL when EnablePathUnescaping=true", + givenConfig: StaticConfig{Root: "testdata/dist/public", EnablePathUnescaping: true}, + whenURL: "/admin%2fprivate.txt", + expectCode: http.StatusOK, + expectBodyContains: "dist/public/admin/private.txt - private file", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := echo.New() + + // Global middleware runs for all matched routes. The /* wildcard route ensures + // the middleware takes the c.Param("*") branch (c.Path() ends with "*"). + e.Use(StaticWithConfig(tc.givenConfig)) + e.GET("/*", func(c *echo.Context) error { return echo.ErrNotFound }) + // ACL guard: requests with a literal /admin/ prefix are forbidden. + e.GET("/admin/*", func(c *echo.Context) error { return echo.ErrForbidden }) + + req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, tc.expectCode, rec.Code) + body := rec.Body.String() + if tc.expectBodyContains != "" { + assert.Contains(t, body, tc.expectBodyContains) + } + if tc.expectBodyNotContains != "" { + assert.NotContains(t, body, tc.expectBodyNotContains) + } + }) + } +} + // Regression for GHSA-vfp3-v2gw-7wfq: the static middleware mounted on a group // must not let an encoded separator in the wildcard bypass route-level middleware // and disclose a file the matched route never authorized. diff --git a/middleware/testdata/dist/private.txt b/middleware/testdata/dist/private.txt index 0f9d2435b..e517f5d41 100644 --- a/middleware/testdata/dist/private.txt +++ b/middleware/testdata/dist/private.txt @@ -1 +1 @@ -private file +dist/private.txt - private file diff --git a/middleware/testdata/dist/public/admin/private.txt b/middleware/testdata/dist/public/admin/private.txt new file mode 100644 index 000000000..e7aa64249 --- /dev/null +++ b/middleware/testdata/dist/public/admin/private.txt @@ -0,0 +1 @@ +dist/public/admin/private.txt - private file diff --git a/middleware/testdata/private.txt b/middleware/testdata/private.txt index 0f9d2435b..a0ae304e4 100644 --- a/middleware/testdata/private.txt +++ b/middleware/testdata/private.txt @@ -1 +1 @@ -private file +private.txt - private file diff --git a/static_encoded_separator_test.go b/static_encoded_separator_test.go index a10eafabc..4fbc71a18 100644 --- a/static_encoded_separator_test.go +++ b/static_encoded_separator_test.go @@ -12,65 +12,113 @@ import ( // Regression for GHSA-vfp3-v2gw-7wfq: an encoded slash (%2F) must not let a static // file request resolve across a path separator and bypass route-level middleware. func TestStaticDirectoryHandler_EncodedSeparatorDoesNotBypassRoute(t *testing.T) { - fsys := fstest.MapFS{ - "admin/secret.txt": {Data: []byte("TOP-SECRET")}, - "index.html": {Data: []byte("public")}, - } - e := New() - g := e.Group("/admin", func(next HandlerFunc) HandlerFunc { - return func(c *Context) error { return c.String(http.StatusForbidden, "denied") } - }) - g.GET("/*", func(c *Context) error { return c.String(http.StatusOK, "reached-protected-handler") }) - e.StaticFS("/", fsys) - - cases := []struct { + var testCases = []struct { + name string target string wantCode int wantBody string }{ - {"/admin/secret.txt", http.StatusForbidden, "denied"}, // protected route fires - {"/admin%2Fsecret.txt", http.StatusNotFound, ""}, // encoded slash rejected, no disclosure - {"/admin%2fsecret.txt", http.StatusNotFound, ""}, // lower-case hex variant - {"/admin%5Csecret.txt", http.StatusNotFound, ""}, // encoded backslash variant - {"/admin%252Fsecret.txt", http.StatusNotFound, ""}, // double-encoded: single unescape -> literal filename, not a separator - {"/index.html", http.StatusOK, "public"}, // legitimate static file still served + { + name: "protected route fires", + target: "/admin/secret.txt", + wantCode: http.StatusForbidden, + wantBody: "denied", + }, + { + name: "encoded slash rejected, no disclosure", + target: "/admin%2Fsecret.txt", + wantCode: http.StatusNotFound, + wantBody: "", + }, + { + name: "lower-case hex variant", + target: "/admin%2fsecret.txt", + wantCode: http.StatusNotFound, + wantBody: "", + }, + { + name: "encoded backslash variant - Windows specific related", + target: "/admin%5Csecret.txt", + wantCode: http.StatusNotFound, + wantBody: "", + }, + { + name: "double-encoded: single unescape -> literal filename, not a separator", + target: "/admin%252Fsecret.txt", + wantCode: http.StatusNotFound, + wantBody: "", + }, + { + name: "legitimate static file still served", + target: "/index.html", + wantCode: http.StatusOK, + wantBody: "public", + }, } - for _, tc := range cases { - req := httptest.NewRequest(http.MethodGet, tc.target, nil) - rec := httptest.NewRecorder() - e.ServeHTTP(rec, req) - assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target) - if tc.wantBody != "" { - assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target) - } - assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fsys := fstest.MapFS{ + "admin/secret.txt": {Data: []byte("TOP-SECRET")}, + "index.html": {Data: []byte("public")}, + } + e := New() + g := e.Group("/admin", func(next HandlerFunc) HandlerFunc { + return func(c *Context) error { return c.String(http.StatusForbidden, "denied") } + }) + g.GET("/*", func(c *Context) error { return c.String(http.StatusOK, "reached-protected-handler") }) + e.StaticFS("/", fsys) + + req := httptest.NewRequest(http.MethodGet, tc.target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target) + if tc.wantBody != "" { + assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target) + } + assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target) + }) } } // A Group-mounted StaticFS shares StaticDirectoryHandler, so it must reject the // same encoded separators when served under a non-root prefix. func TestGroupStaticFS_EncodedSeparatorDoesNotBypassRoute(t *testing.T) { - fsys := fstest.MapFS{ - "admin/secret.txt": {Data: []byte("TOP-SECRET")}, - "index.html": {Data: []byte("public")}, - } - e := New() - g := e.Group("/files") - g.StaticFS("/", fsys) - - cases := []struct { + var testCases = []struct { + name string target string wantCode int }{ - {"/files/index.html", http.StatusOK}, - {"/files/admin%2Fsecret.txt", http.StatusNotFound}, - {"/files/admin%5Csecret.txt", http.StatusNotFound}, + { + name: "ok", + target: "/files/index.html", + wantCode: http.StatusOK, + }, + { + name: "nok, encoded slash", + target: "/files/admin%2Fsecret.txt", + wantCode: http.StatusNotFound, + }, + { + name: "nok encoded backslash", + target: "/files/admin%5Csecret.txt", + wantCode: http.StatusNotFound, + }, } - for _, tc := range cases { - req := httptest.NewRequest(http.MethodGet, tc.target, nil) - rec := httptest.NewRecorder() - e.ServeHTTP(rec, req) - assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target) - assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fsys := fstest.MapFS{ + "admin/secret.txt": {Data: []byte("TOP-SECRET")}, + "index.html": {Data: []byte("public")}, + } + e := New() + g := e.Group("/files") + g.StaticFS("/", fsys) + + req := httptest.NewRequest(http.MethodGet, tc.target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target) + assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target) + }) } }