Skip to content

fix: handle urls with multiple slashes #16527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 12, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions coderd/coderd.go
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,7 @@ func New(options *Options) *API {
httpmw.AttachRequestID,
httpmw.ExtractRealIP(api.RealIPConfig),
httpmw.Logger(api.Logger),
singleSlashMW,
rolestore.CustomRoleMW,
prometheusMW,
// Build-Version is helpful for debugging.
Expand Down Expand Up @@ -1731,3 +1732,31 @@ func ReadExperiments(log slog.Logger, raw []string) codersdk.Experiments {
}
return exps
}

var multipleSlashesRe = regexp.MustCompile(`/+`)

func singleSlashMW(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, "/")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, non-blocking: I wonder what the cost of a regex replace is versus iterating over the string once?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had that thought to fiddle with chars, but blindly assumed that regexp is safer and easier for devs to comprehend. Thanks for the comment anyway!


// 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
}

next.ServeHTTP(w, r)
}
return http.HandlerFunc(fn)
}
69 changes: 69 additions & 0 deletions coderd/coderd_internal_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
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) {
t.Parallel()

tests := []struct {
name string
inputPath string
wantPath string
}{
{"No changes", "/api/v1/buildinfo", "/api/v1/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("chi/"+tt.name, func(t *testing.T) {
t.Parallel()
req := httptest.NewRequest("GET", tt.inputPath, nil)
rec := httptest.NewRecorder()

// given
rctx := chi.NewRouteContext()
rctx.RoutePath = tt.inputPath
req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx))

// when
singleSlashMW(handler).ServeHTTP(rec, req)
updatedCtx := chi.RouteContext(req.Context())

// 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()))
})
}
}
6 changes: 6 additions & 0 deletions site/vite.config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading