From 7d598e4935ac691e5a821e235c7ab4df72e9a792 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 11 Feb 2025 15:57:32 +0100 Subject: [PATCH 1/6] fix: rewrite urls with multiple slashes --- coderd/coderd.go | 32 ++++++++++++++++++++++++++++++++ site/vite.config.mts | 6 ++++++ 2 files changed, 38 insertions(+) diff --git a/coderd/coderd.go b/coderd/coderd.go index 4603f78acc0d9..dae27d88c3dbb 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -788,6 +788,7 @@ func New(options *Options) *API { httpmw.AttachRequestID, httpmw.ExtractRealIP(api.RealIPConfig), httpmw.Logger(api.Logger), + stripSlashesMW, rolestore.CustomRoleMW, prometheusMW, // Build-Version is helpful for debugging. @@ -1731,3 +1732,34 @@ func ReadExperiments(log slog.Logger, raw []string) codersdk.Experiments { } return exps } + +var multipleSlashesRe = regexp.MustCompile(`/+`) + +func stripSlashesMW(next http.Handler) http.Handler { + fn := func(w http.ResponseWriter, r *http.Request) { + var path string + rctx := chi.RouteContext(r.Context()) + if rctx != nil && rctx.RoutePath != "" { + path = rctx.RoutePath + } else { + path = r.URL.Path + } + + // Normalize multiple slashes to a single slash + newPath := multipleSlashesRe.ReplaceAllString(path, "/") + + // Ensure it doesn't strip the root `/` + if len(newPath) > 1 && newPath[len(newPath)-1] == '/' { + newPath = strings.TrimSuffix(newPath, "/") + } + + // Apply the cleaned path + if rctx != nil { + rctx.RoutePath = newPath + } + r.URL.Path = newPath + + next.ServeHTTP(w, r) + } + return http.HandlerFunc(fn) +} diff --git a/site/vite.config.mts b/site/vite.config.mts index 4deaac0dd5365..aab894ce0599e 100644 --- a/site/vite.config.mts +++ b/site/vite.config.mts @@ -52,6 +52,12 @@ export default defineConfig({ "csrf_token=JXm9hOUdZctWt0ZZGAy9xiS/gxMKYOThdxjjMnMUyn4=; Path=/; HttpOnly; SameSite=Lax", }, proxy: { + "//": { + changeOrigin: true, + target: process.env.CODER_HOST || "http://localhost:3000", + secure: process.env.NODE_ENV === "production", + rewrite: (path) => path.replace(/\/+/g, "/"), + }, "/api": { ws: true, changeOrigin: true, From 2aef54d9a2e128e145b08b92ba7c7fd986356ef6 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 11 Feb 2025 16:13:44 +0100 Subject: [PATCH 2/6] unit tests --- coderd/coderd_internal_test.go | 47 ++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 coderd/coderd_internal_test.go diff --git a/coderd/coderd_internal_test.go b/coderd/coderd_internal_test.go new file mode 100644 index 0000000000000..8e4510af420cf --- /dev/null +++ b/coderd/coderd_internal_test.go @@ -0,0 +1,47 @@ +package coderd + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +func TestStripSlashesMW(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + inputPath string + wantPath string + }{ + {"No changes", "/api/v1/buildinfo", "/api/v1/buildinfo"}, + {"Single trailing slash", "/api/v2/buildinfo/", "/api/v2/buildinfo"}, + {"Double slashes", "/api//v2//buildinfo", "/api/v2/buildinfo"}, + {"Triple slashes", "/api///v2///buildinfo///", "/api/v2/buildinfo"}, + {"Leading slashes", "///api/v2/buildinfo", "/api/v2/buildinfo"}, + {"Root path", "/", "/"}, + {"Double slashes root", "//", "/"}, + {"Only slashes", "/////", "/"}, + } + + handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest("GET", tt.inputPath, nil) + rec := httptest.NewRecorder() + + stripSlashesMW(handler).ServeHTTP(rec, req) + + if req.URL.Path != tt.wantPath { + t.Errorf("stripSlashesMW got path = %q, want %q", req.URL.Path, tt.wantPath) + } + }) + } +} From 3d43a72de32a08304ebb68c71a84b3a6f3b4a495 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 11 Feb 2025 16:22:03 +0100 Subject: [PATCH 3/6] chi --- coderd/coderd_internal_test.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/coderd/coderd_internal_test.go b/coderd/coderd_internal_test.go index 8e4510af420cf..ca91b8c583df5 100644 --- a/coderd/coderd_internal_test.go +++ b/coderd/coderd_internal_test.go @@ -1,9 +1,13 @@ package coderd import ( + "context" "net/http" "net/http/httptest" "testing" + + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/assert" ) func TestStripSlashesMW(t *testing.T) { @@ -30,18 +34,26 @@ func TestStripSlashesMW(t *testing.T) { for _, tt := range tests { tt := tt - t.Run(tt.name, func(t *testing.T) { t.Parallel() req := httptest.NewRequest("GET", tt.inputPath, nil) rec := httptest.NewRecorder() + // Create a chi RouteContext and attach it to the request + rctx := chi.NewRouteContext() + rctx.RoutePath = tt.inputPath // Simulate chi route path + req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) + + // Pass the request through the middleware stripSlashesMW(handler).ServeHTTP(rec, req) - if req.URL.Path != tt.wantPath { - t.Errorf("stripSlashesMW got path = %q, want %q", req.URL.Path, tt.wantPath) - } + // Get the updated chi RouteContext after middleware processing + updatedCtx := chi.RouteContext(req.Context()) + + // Validate URL path + assert.Equal(t, tt.wantPath, req.URL.Path) + assert.Equal(t, tt.wantPath, updatedCtx.RoutePath) }) } } From efd0965236824ee1e692a3aa1fe1a1ce03326db2 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 11 Feb 2025 16:54:43 +0100 Subject: [PATCH 4/6] just single slash --- coderd/coderd.go | 14 +++++++------- coderd/coderd_internal_test.go | 5 ++--- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index dae27d88c3dbb..00a36f8a21c0e 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -10,6 +10,7 @@ import ( "flag" "fmt" "io" + "log" "net/http" "net/url" "path/filepath" @@ -788,7 +789,7 @@ func New(options *Options) *API { httpmw.AttachRequestID, httpmw.ExtractRealIP(api.RealIPConfig), httpmw.Logger(api.Logger), - stripSlashesMW, + singleSlashMW, rolestore.CustomRoleMW, prometheusMW, // Build-Version is helpful for debugging. @@ -1735,8 +1736,12 @@ func ReadExperiments(log slog.Logger, raw []string) codersdk.Experiments { var multipleSlashesRe = regexp.MustCompile(`/+`) -func stripSlashesMW(next http.Handler) http.Handler { +func singleSlashMW(next http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "test-app-owner") { + log.Println(r.URL.Path) + } + var path string rctx := chi.RouteContext(r.Context()) if rctx != nil && rctx.RoutePath != "" { @@ -1748,11 +1753,6 @@ func stripSlashesMW(next http.Handler) http.Handler { // Normalize multiple slashes to a single slash newPath := multipleSlashesRe.ReplaceAllString(path, "/") - // Ensure it doesn't strip the root `/` - if len(newPath) > 1 && newPath[len(newPath)-1] == '/' { - newPath = strings.TrimSuffix(newPath, "/") - } - // Apply the cleaned path if rctx != nil { rctx.RoutePath = newPath diff --git a/coderd/coderd_internal_test.go b/coderd/coderd_internal_test.go index ca91b8c583df5..edded3007b146 100644 --- a/coderd/coderd_internal_test.go +++ b/coderd/coderd_internal_test.go @@ -19,9 +19,8 @@ func TestStripSlashesMW(t *testing.T) { wantPath string }{ {"No changes", "/api/v1/buildinfo", "/api/v1/buildinfo"}, - {"Single trailing slash", "/api/v2/buildinfo/", "/api/v2/buildinfo"}, {"Double slashes", "/api//v2//buildinfo", "/api/v2/buildinfo"}, - {"Triple slashes", "/api///v2///buildinfo///", "/api/v2/buildinfo"}, + {"Triple slashes", "/api///v2///buildinfo", "/api/v2/buildinfo"}, {"Leading slashes", "///api/v2/buildinfo", "/api/v2/buildinfo"}, {"Root path", "/", "/"}, {"Double slashes root", "//", "/"}, @@ -46,7 +45,7 @@ func TestStripSlashesMW(t *testing.T) { req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) // Pass the request through the middleware - stripSlashesMW(handler).ServeHTTP(rec, req) + singleSlashMW(handler).ServeHTTP(rec, req) // Get the updated chi RouteContext after middleware processing updatedCtx := chi.RouteContext(req.Context()) From 7340fb947ad2ddc927a6693b5fe1617764d664e3 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 11 Feb 2025 16:55:34 +0100 Subject: [PATCH 5/6] tidy --- coderd/coderd.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 00a36f8a21c0e..13e70f00e9fa2 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -10,7 +10,6 @@ import ( "flag" "fmt" "io" - "log" "net/http" "net/url" "path/filepath" @@ -1738,10 +1737,6 @@ var multipleSlashesRe = regexp.MustCompile(`/+`) func singleSlashMW(next http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "test-app-owner") { - log.Println(r.URL.Path) - } - var path string rctx := chi.RouteContext(r.Context()) if rctx != nil && rctx.RoutePath != "" { From cdfef4ddec91620a93d1b36a3da8f2052cdfa7c2 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Wed, 12 Feb 2025 08:47:56 +0100 Subject: [PATCH 6/6] fix: correct approach --- coderd/coderd.go | 4 +++- coderd/coderd_internal_test.go | 29 ++++++++++++++++++++--------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 13e70f00e9fa2..d11535f58022d 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1749,10 +1749,12 @@ func singleSlashMW(next http.Handler) http.Handler { newPath := multipleSlashesRe.ReplaceAllString(path, "/") // Apply the cleaned path + // The approach is consistent with: https://github.com/go-chi/chi/blob/e846b8304c769c4f1a51c9de06bebfaa4576bd88/middleware/strip.go#L24-L28 if rctx != nil { rctx.RoutePath = newPath + } else { + r.URL.Path = newPath } - r.URL.Path = newPath next.ServeHTTP(w, r) } diff --git a/coderd/coderd_internal_test.go b/coderd/coderd_internal_test.go index edded3007b146..34f5738bf90a0 100644 --- a/coderd/coderd_internal_test.go +++ b/coderd/coderd_internal_test.go @@ -33,26 +33,37 @@ func TestStripSlashesMW(t *testing.T) { for _, tt := range tests { tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() + t.Run("chi/"+tt.name, func(t *testing.T) { + t.Parallel() req := httptest.NewRequest("GET", tt.inputPath, nil) rec := httptest.NewRecorder() - // Create a chi RouteContext and attach it to the request + // given rctx := chi.NewRouteContext() - rctx.RoutePath = tt.inputPath // Simulate chi route path + rctx.RoutePath = tt.inputPath req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) - // Pass the request through the middleware + // when singleSlashMW(handler).ServeHTTP(rec, req) - - // Get the updated chi RouteContext after middleware processing updatedCtx := chi.RouteContext(req.Context()) - // Validate URL path - assert.Equal(t, tt.wantPath, req.URL.Path) + // then + assert.Equal(t, tt.inputPath, req.URL.Path) assert.Equal(t, tt.wantPath, updatedCtx.RoutePath) }) + + t.Run("stdlib/"+tt.name, func(t *testing.T) { + t.Parallel() + req := httptest.NewRequest("GET", tt.inputPath, nil) + rec := httptest.NewRecorder() + + // when + singleSlashMW(handler).ServeHTTP(rec, req) + + // then + assert.Equal(t, tt.wantPath, req.URL.Path) + assert.Nil(t, chi.RouteContext(req.Context())) + }) } }