SkipClean does not work on Subrouters or any subroutes

[gavbaa]

What version of Go are you running? (Paste the output of go version)
go version go1.11.5 darwin/amd64

What version of gorilla/mux are you at? (Paste the output of git rev-parse HEAD inside $GOPATH/src/github.com/gorilla/mux)
15a353a

Describe your problem (and what you have tried so far)
SkipClean when applied only on subrouters does not check for cleaned URLs on subroutes. It only applies to the entire routing event, on or off.

What we expect to happen: On routes that are declared with SkipClean(false), those routes are checked against the cleaned URL. On routes that are declared with SkipClean(true), those routes are checked against the original URL.

Or else, SkipClean should only be allowed on the parent Router, and not on any subrouters, since it's currently ineffective.

(This probably opens the door for also processing those routes as an internal redirect instead of a public 302 as an option, but I think that should be a separate ticket and isn't required for this behavior).

Paste a minimal, runnable, reproduction of your issue below (use backticks to format it)

func TestSkipCleanSubrouter(t *testing.T) {
	func1 := func(w http.ResponseWriter, r *http.Request) {}
	func2 := func(w http.ResponseWriter, r *http.Request) {}

	r := NewRouter()
	r.HandleFunc("/api/", func1).Name("func2")
	r.SkipClean(true)
	subRouter := r.PathPrefix("/v0").Subrouter().SkipClean(false)
	subRouter.HandleFunc("/action/do/", func2).Name("func2")

	req, _ := http.NewRequest("GET", "http://localhost/api/?abc=def", nil)
	res := NewRecorder()
	r.ServeHTTP(res, req)

	if len(res.HeaderMap["Location"]) != 0 {
		t.Errorf("Req 1: Shouldn't redirect since route is already clean")
	}

	req, _ = http.NewRequest("GET", "http://localhost//api/?abc=def", nil)
	res = NewRecorder()
	r.ServeHTTP(res, req)

	if len(res.HeaderMap["Location"]) != 0 {
		t.Errorf("Req 2: Shouldn't redirect since skip clean is disabled")
	}

	req, _ = http.NewRequest("GET", "http://localhost/v0/action//do/?ghi=jkl", nil)
	res = NewRecorder()
	r.ServeHTTP(res, req)

	if len(res.HeaderMap["Location"]) == 0 {
		t.Errorf("Req 3: Should redirect since skip clean is enabled for subroute")
	}
}

Expected:
Tests pass.

Actual:

--- FAIL: TestSkipCleanSubrouter (0.00s)
    mux_test.go:2029: Req 3: Should redirect since skip clean is enabled for subroute

I'm happy to provide a patch that would address this in whichever direction should be supported. I'm of the opinion that subroutes should be able to determine whether or not they should be tested against the original or the cleaned URL.

[elithrar]

Agree that subroutes should be able to define this. (Preventing applying this to subrouters is both harder technically & less obvious). Would welcome a PR.

…

On Wed, Mar 13, 2019 at 11:01 AM George Vilches ***@***.***> wrote: *What version of Go are you running?* (Paste the output of go version) go version go1.11.5 darwin/amd64 *What version of gorilla/mux are you at?* (Paste the output of git rev-parse HEAD inside $GOPATH/src/github.com/gorilla/mux) 15a353a <15a353a> *Describe your problem* (and what you have tried so far) SkipClean when applied only on subrouters does not check for cleaned URLs on subroutes. It only applies to the entire routing event, on or off. What we expect to happen: On routes that are declared with SkipClean(false), those routes are checked against the cleaned URL. On routes that are declared with SkipClean(true), those routes are checked against the original URL. Or else, SkipClean should only be allowed on the parent Router, and not on any subrouters, since it's currently ineffective. (This probably opens the door for also processing those routes as an internal redirect instead of a public 302 as an option, but I think that should be a separate ticket and isn't required for this behavior). *Paste a minimal, runnable, reproduction of your issue below* (use backticks to format it) func TestSkipCleanSubrouter(t *testing.T) { func1 := func(w http.ResponseWriter, r *http.Request) {} func2 := func(w http.ResponseWriter, r *http.Request) {} r := NewRouter() r.HandleFunc("/api/", func1).Name("func2") r.SkipClean(true) subRouter := r.PathPrefix("/v0").Subrouter().SkipClean(false) subRouter.HandleFunc("/action/do/", func2).Name("func2") req, _ := http.NewRequest("GET", "http://localhost/api/?abc=def", nil) res := NewRecorder() r.ServeHTTP(res, req) if len(res.HeaderMap["Location"]) != 0 { t.Errorf("Req 1: Shouldn't redirect since route is already clean") } req, _ = http.NewRequest("GET", "http://localhost//api/?abc=def", nil) res = NewRecorder() r.ServeHTTP(res, req) if len(res.HeaderMap["Location"]) != 0 { t.Errorf("Req 2: Shouldn't redirect since skip clean is disabled") } req, _ = http.NewRequest("GET", "http://localhost/v0/action//do/?ghi=jkl", nil) res = NewRecorder() r.ServeHTTP(res, req) if len(res.HeaderMap["Location"]) == 0 { t.Errorf("Req 3: Should redirect since skip clean is enabled for subroute") } } Expected: Tests pass. Actual: --- FAIL: TestSkipCleanSubrouter (0.00s) mux_test.go:2029: Req 3: Should redirect since skip clean is enabled for subroute I'm happy to provide a patch that would address this in whichever direction should be supported. I'm of the opinion that subroutes should be able to determine whether or not they should be tested against the original or the cleaned URL. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#460>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcDuu3JGptUQ2vWN99ewlyhfEEFx2ks5vWTz8gaJpZM4buYXR> .

[gavbaa]

PR provided in #463 . New tests added, all existing tests pass. Would welcome a review.

[stale]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.