Static: fix directory bounds

In the static file serving, - Reject paths containing "\" or "//" - Reject non-empty paths not starting with a slash - Reject paths containing a ".", ".." or "" segment - For the Content-Disposition header, use the actual path to get the name if the file instead of the raw input
go-goyave · Nov 25, 2024 · 5836bff · 5836bff
1 parent fc57b4c
commit 5836bff
Show file tree

Hide file tree

Showing 4 changed files with 150 additions and 4 deletions.
diff --git a/resources/index.html b/resources/index.html
@@ -0,0 +1 @@
+<html></html>
diff --git a/router.go b/router.go
@@ -445,6 +445,10 @@ func (r *Router) Options(uri string, handler Handler) *Route {
 //
 // If no file is given in the url, or if the given file is a directory, the handler will
 // send the "index.html" file if it exists.
+//
+// As a precaution, all requests with a path containing a path segment in the form of ".", ".." are rejected with
+// `http.StatusNotFound`. This ensures clients cannot access files outside of the given filesystem base directory.
+// Paths containing  "\" or "//" are also rejected.
 func (r *Router) Static(fs fs.StatFS, uri string, download bool) *Route {
 	return r.registerRoute([]string{http.MethodGet}, uri+"{resource:.*}", staticHandler(fs, download))
 }

diff --git a/static.go b/static.go
@@ -2,18 +2,25 @@ package goyave
 
 import (
 	"io/fs"
+	"net/http"
+	"path/filepath"
 	"strings"
 
+	"github.com/samber/lo"
 	"goyave.dev/goyave/v5/util/fsutil"
 )
 
 func staticHandler(fs fs.StatFS, download bool) Handler {
 	return func(response *Response, r *Request) {
 		file := r.RouteParams["resource"]
+		if !checkStaticPath(file) {
+			response.Status(http.StatusNotFound)
+			return
+		}
 		path := cleanStaticPath(fs, file)
 
 		if download {
-			response.Download(fs, path, file[strings.LastIndex(file, "/")+1:])
+			response.Download(fs, path, path[lo.Clamp(strings.LastIndex(file, "/"), 0, len(path)):])
 			return
 		}
 		response.File(fs, path)
@@ -32,5 +39,24 @@ func cleanStaticPath(fs fs.StatFS, file string) string {
 		}
 		path += "index.html"
 	}
-	return path
+	return filepath.Clean(path)
+}
+
+func checkStaticPath(path string) bool {
+	if !strings.HasPrefix(path, "/") && len(path) > 0 { // Force leading slash if the path is not empty
+		return false
+	}
+	if strings.Contains(path, "\\") || strings.Contains(path, "//") {
+		return false
+	}
+	for _, ent := range strings.FieldsFunc(path, isSlash) {
+		if ent == "." || ent == ".." || ent == "" {
+			return false
+		}
+	}
+	return true
+}
+
+func isSlash(r rune) bool {
+	return r == '/'
 }
diff --git a/static_test.go b/static_test.go
@@ -5,6 +5,7 @@ import (
 	"io/fs"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -70,6 +71,74 @@ func TestStaticHandler(t *testing.T) {
 				assert.Equal(t, http.StatusNotFound, response.GetStatus())
 			},
 		},
+		{
+			uri:       "custom_config.json", // Force leading slash if the path is not empty
+			directory: "resources",
+			download:  false,
+			expected: func(t *testing.T, response *Response, _ *http.Response, _ []byte) {
+				assert.Equal(t, http.StatusNotFound, response.GetStatus())
+			},
+		},
+		{
+			uri:       "index.html", // Force leading slash if the path is not empty
+			directory: "resources",
+			download:  false,
+			expected: func(t *testing.T, response *Response, _ *http.Response, _ []byte) {
+				assert.Equal(t, http.StatusNotFound, response.GetStatus())
+			},
+		},
+		{
+			uri:       "",
+			directory: "resources",
+			download:  false,
+			expected: func(t *testing.T, response *Response, result *http.Response, body []byte) {
+				assert.Equal(t, http.StatusOK, response.GetStatus())
+				assert.Equal(t, "text/html; charset=utf-8", result.Header.Get("Content-Type"))
+				assert.Equal(t, "<html></html>", string(body))
+			},
+		},
+		{
+			uri:       "", // Download index.html
+			directory: "resources",
+			download:  true,
+			expected: func(t *testing.T, response *Response, result *http.Response, body []byte) {
+				assert.Equal(t, http.StatusOK, response.GetStatus())
+				assert.Equal(t, "text/html; charset=utf-8", result.Header.Get("Content-Type"))
+				assert.Equal(t, "attachment; filename=\"index.html\"", result.Header.Get("Content-Disposition"))
+				assert.Equal(t, "<html></html>", string(body))
+			},
+		},
+		{
+			uri:       "/",
+			directory: "resources",
+			download:  false,
+			expected: func(t *testing.T, response *Response, result *http.Response, body []byte) {
+				assert.Equal(t, http.StatusOK, response.GetStatus())
+				assert.Equal(t, "text/html; charset=utf-8", result.Header.Get("Content-Type"))
+				assert.Equal(t, "<html></html>", string(body))
+			},
+		},
+		{
+			uri:       "/index.html",
+			directory: "resources",
+			download:  false,
+			expected: func(t *testing.T, response *Response, result *http.Response, body []byte) {
+				assert.Equal(t, http.StatusOK, response.GetStatus())
+				assert.Equal(t, "text/html; charset=utf-8", result.Header.Get("Content-Type"))
+				assert.Equal(t, "<html></html>", string(body))
+			},
+		},
+		{
+			uri:       "/", // Download index.html
+			directory: "resources",
+			download:  true,
+			expected: func(t *testing.T, response *Response, result *http.Response, body []byte) {
+				assert.Equal(t, http.StatusOK, response.GetStatus())
+				assert.Equal(t, "text/html; charset=utf-8", result.Header.Get("Content-Type"))
+				assert.Equal(t, "attachment; filename=\"index.html\"", result.Header.Get("Content-Disposition"))
+				assert.Equal(t, "<html></html>", string(body))
+			},
+		},
 		{
 			uri:       "/custom_config.json",
 			directory: "resources",
@@ -95,12 +164,12 @@ func TestStaticHandler(t *testing.T) {
 	}
 
 	for _, c := range cases {
-		t.Run(c.uri, func(t *testing.T) {
+		t.Run(strings.ReplaceAll(c.uri, "/", "_"), func(t *testing.T) {
 			cfg := config.LoadDefault()
 			srv, err := New(Options{Config: cfg})
 			require.NoError(t, err)
 
-			request := NewRequest(httptest.NewRequest(http.MethodGet, c.uri, nil))
+			request := NewRequest(httptest.NewRequest(http.MethodGet, "/static"+c.uri, nil))
 			request.RouteParams = map[string]string{"resource": c.uri}
 
 			recorder := httptest.NewRecorder()
@@ -119,3 +188,49 @@ func TestStaticHandler(t *testing.T) {
 		})
 	}
 }
+
+func TestStaticHandlerBounds(t *testing.T) {
+	// This test ensures a client cannot request a file outside of
+	// the bounds of the set directory (using . or ..)
+	cases := []string{
+		"/resources/.",
+		"/resources/./",
+		"/resources/..",
+		"/resources/../",
+		"/resources/img/..",
+		"/resources/img/../custom_config.json",
+		"/resources/img/./logo/goyave_16.png",
+		"/resources/img//logo/goyave_16.png",
+		"/resources/\\",
+		"/resources/img\\logo\\goyave_16.png",
+		"/resources/../README.md",
+		"/resources//custom_config.json",
+		"/.",
+		"/..",
+		".",
+		"..",
+	}
+
+	for _, c := range cases {
+		t.Run(strings.ReplaceAll(c, "/", "_"), func(t *testing.T) {
+			cfg := config.LoadDefault()
+			srv, err := New(Options{Config: cfg})
+			require.NoError(t, err)
+
+			request := NewRequest(httptest.NewRequest(http.MethodGet, "/static"+c, nil))
+			request.RouteParams = map[string]string{"resource": c}
+
+			recorder := httptest.NewRecorder()
+			response := NewResponse(srv, request, recorder)
+
+			require.NoError(t, err)
+			handler := staticHandler(&osfs.FS{}, false)
+			handler(response, request)
+
+			result := recorder.Result()
+			assert.NoError(t, result.Body.Close())
+			require.NoError(t, err)
+			assert.Equal(t, http.StatusNotFound, response.GetStatus())
+		})
+	}
+}