Skip to content

Fix middleware.CleanPath panicking on a non-chi handler - #1193

Open
januththedev wants to merge 1 commit into
go-chi:masterfrom
januththedev:fix/cleanpath-nil-context
Open

januththedev wants to merge 1 commit into
go-chi:masterfrom
januththedev:fix/cleanpath-nil-context

Conversation

@januththedev

Copy link
Copy Markdown

middleware.CleanPath panics on a non-chi handler

Description

CleanPath dereferences the chi routing context unconditionally:

rctx := chi.RouteContext(r.Context())
routePath := rctx.RoutePath   // nil deref when rctx == nil

chi.RouteContext returns nil whenever the handler is not a chi.Mux.

Why it's wrong

CleanPath is exported as a plain func CleanPath(next http.Handler) http.Handler with no chi-only restriction, so composing it onto an http.ServeMux, an http.FileServer, or any other non-chi handler panics with a nil-pointer dereference on the very first request.

It has no chi-specific work to do — it only needs to normalise a path and write it back. And its sibling in the same package already handles this exact case:

// middleware/strip.go:15-27
if rctx == nil { r.URL.Path = newPath } else { rctx.RoutePath = newPath }

with a dedicated regression test, TestStripSlashesWithNilContext (middleware/strip_test.go:203), commented "This tests a http.Handler that is not chi.Router / In these cases, the routeContext is nil". CleanPath was simply never given the same guard. I probed the rest of the package: StripSlashes, RedirectSlashes, URLFormat, Heartbeat and PageRoute all survive a nil context — only CleanPath and GetHead panic, and GetHead is fundamentally chi-dependent so a panic there is more defensible.

Issue #787 "middleware.CleanPath doesn't work with other routers" is still open (its PR #786 closed unmerged).

The fix

rctx := chi.RouteContext(r.Context())

routePath := ""
if rctx != nil {
    routePath = rctx.RoutePath
}
if routePath == "" {
    if r.URL.RawPath != "" {
        routePath = r.URL.RawPath
    } else {
        routePath = r.URL.Path
    }
    if rctx == nil {
        r.URL.Path = path.Clean(routePath)
    } else {
        rctx.RoutePath = path.Clean(routePath)
    }
}

Tests

TestCleanPathWithNilContext in middleware/clean_path_test.go, mirroring the existing nil-context test's style.

  • Before: panic: runtime error: invalid memory address or nil pointer dereference at clean_path.go:16. After: passes, and the pre-existing TestCleanPath still passes.
  • Full suite go test -count=1 ./... (uncached): chi and chi/v5/middleware both ok, exit 0, 0 failures, 0 skips — 129 top-level PASS. Identical to the baseline on unmodified HEAD, which had no pre-existing failures. go vet ./... clean.

Upstream status

I checked 60 open issues and 55 open PRs and mapped each open PR to the files it touches. middleware/clean_path.go is touched only by open PR #1162 (CONNECT authority-form, unrelated — it wraps the path.Clean call in if routePath != "" and does not address the nil context). No open issue or PR fixes the nil-context panic.

I steered clear of the heavily-contended files: tree.go/mux.go (11+ PRs), middleware/get_head.go (4), middleware/compress.go (10), plus strip.go, supress_notfound.go, route_headers.go, nocache.go, recoverer.go, content_encoding.go, context.go.

gofmt -l flags these files, but it flags 65 non-_examples Go files including untouched ones (strip.go, tree.go, context.go) — this machine has core.autocrlf=true, so the whole checkout is CRLF. Pre-existing checkout condition, not introduced here; git diff shows no line-ending churn.

Real neighbouring defects I found but left alone

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant