Unusual path parameter values make mux to choose incorrect handler

[sergey-seleznev]

Hi!

I would like to define a number of handlers, that have:

• same URL with a file path as the rightmost parameter,
• different HTTP request methods.

Here's a simplified example:

func main() {
	router := mux.NewRouter()
	router.HandleFunc("/files/{path:.*}", getHandler).Methods("GET")
	router.HandleFunc("/files/{path:.*}", postHandler).Methods("POST")
	log.Fatal(http.ListenAndServe(":8080", router))
}

func getHandler(w http.ResponseWriter, r *http.Request) {
	w.Write([]byte("[GET] " + mux.Vars(r)["path"]))
}

func postHandler(w http.ResponseWriter, r *http.Request) {
	w.Write([]byte("[POST] " + mux.Vars(r)["path"]))
}

It seems that mux tends to ignore methods and select improper handler in some specific cases:

• GET http://localhost:8080/files/file.txt => [GET] file.txt (OK)
• POST http://localhost:8080/files/file.txt => [POST] file.txt (OK)
• GET http://localhost:8080/files/path/to/file.txt => [GET] path/to/file.txt (OK)
• POST http://localhost:8080/files/path/to/file.txt => [POST] path/to/file.txt (OK)
• GET http://localhost:8080/files/path/to//file.txt => [GET] path/to/file.txt (OK)
• POST http://localhost:8080/files/path/to//file.txt => [GET] path/to/file.txt (unexpected)
• GET http://localhost:8080/files/path/to/./file.txt => [GET] path/to/file.txt (OK)
• POST http://localhost:8080/files/path/to/./file.txt => [GET] path/to/file.txt (unexpected)
• GET http://localhost:8080/files/path/to/../file.txt => [GET] path/file.txt (OK)
• POST http://localhost:8080/files/path/to/../file.txt => [GET] path/file.txt (unexpected)

Handler declaration order doesn't change the tests results mentioned.

[elithrar]

This may be related to #300

[kisielk]

Can you check if the latest version fixes the problem for you?

[sergey-seleznev]

Seems not to fix that. I having cleared out the previous package traces from src and pkg, and updated to commit 65ec7248c53f499f6b480655e019e0b9d7a6ce11 of the package. Still incorrect methods are called in unexpected scenarios listed above. It's quite easy to verify: all the sample code I run is in the 1st message. Does it execute correctly on your side?

[elithrar]

Normal paths from the tests in the OP:

for i in `seq 20`; do curl http://localhost:8080/files/path/to/file.txt; done
[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt[GET] path/to/file.txt% 

 for i in `seq 20`; do curl -XPOST http://localhost:8080/files/path/to/file.txt; done
[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt% 

Paths noted as "unexpected" in the OP:

for i in `seq 20`; do curl -XPOST "http://localhost:8080/files/path/to/./file.txt"; done
[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt% 

# re-directs at first due to the double slash.
for i in `seq 20`; do curl -L -XPOST "http://localhost:8080/files/path/to//file.txt"; done
[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt[POST] path/to/file.txt% 

I don't see the bug on the latest HEAD (65ec724).

[sergey-seleznev]

Thank you for the details!
I've tried with CURL now as you did - it worked fine for me as well.
The incorrect result seems to be specific to Postman:

[kisielk]

Can you provide a log of the full request sent by Postman? I imagine it must print it somewhere

[sergey-seleznev]

I have this one, but it is more like request-response details:

[elithrar]

Very odd. Can you just have the handler log directly, hit it with Postman, and see what the server itself says? I suspect Postman has buggy output given that curl has no issue.

…

On Wed, Nov 29, 2017 at 11:23 PM Sergey Seleznev ***@***.***> wrote: I have this one, but it is more like request-response details: [image: image] <https://user-images.githubusercontent.com/18219010/33418740-02848548-d5b0-11e7-96ce-f7e26f984251.png> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#308 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcDXBnn1Vgg23dpgr0X5A4L9U4OZnks5s7lgGgaJpZM4QFqAe> .

[sergey-seleznev]

@elithrar it outputs the same that Postman receives.

func main() {
	router := mux.NewRouter()
	router.HandleFunc("/files/{path:.*}", getHandler).Methods("GET")
	router.HandleFunc("/files/{path:.*}", postHandler).Methods("POST")
	log.Fatal(http.ListenAndServe(":8080", router))
}

func getHandler(w http.ResponseWriter, r *http.Request) {
	log := "[GET] " + mux.Vars(r)["path"]
	fmt.Println(log)
	w.Write([]byte(log))
}

func postHandler(w http.ResponseWriter, r *http.Request) {
	log := "[POST] " + mux.Vars(r)["path"]
	fmt.Println(log)
	w.Write([]byte(log))
}

I would blame Postman, but it's actually getting expected response calling a sample ASP.NET Core WebAPI server containing just a couple lines of custom code:

[HttpGet("/files/{*path}")]
public String GetHandler(String path) => "[GET] " + path;

[HttpPost("/files/{*path}")]
public String PostHandler(String path) => "[POST] " + path;

So I guess it's some request specifics of Postman (that could come from other client as well) that gets mux to mix up the handlers.

[kisielk]

Here's a reproducer in Go, using the server code from the post above:

package main

import (
	"io/ioutil"
	"log"
	"net/http"
)

func main() {
	urls := []string{
		"http://localhost:8080/files/path/to/file.txt",
		"http://localhost:8080/files/path/to/./file.txt",
	}
	for _, url := range urls {
		req, err := http.NewRequest("POST", url, nil)
		if err != nil {
			log.Fatal(err)
		}
		client := &http.Client{}
		resp, err := client.Do(req)
		if err != nil {
			log.Fatal(err)
		} else {
			out, _ := ioutil.ReadAll(resp.Body)
			log.Println(string(out))
		}
	}
}

The first one results in [POST] but the second one in [GET]

[kisielk]

With some further testing and debugging I narrowed the problem down to here: https://github.com/gorilla/mux/blob/master/mux.go#L122

When the client receives the redirect, it retries the request with a GET instead of a POST. I guess this must be what's happening with postman too.

[kisielk]

It behaves correctly when using http.StatusPermanentRedirect instead of http.StatusMovedPermanently. So maybe the code should switch to that?

[elithrar]

@kisielk Correct, a 308 allows a retry w/ the same method, and a 301/302 must change to a GET on the subsequent (re-directed) request.

308 isn't as widely supported on all clients, so this is a little tricky. This isn't so much "broken" as implicit. I also loathe "more dials" to configure things.

Perhaps default to 308 (newer) and allow a hook to modify that if need be? RedirectHandler?

[kisielk]

I'm inclined to leave it as it is. At least 301 is understood by all clients, and nothing bad will happen. If you have a client that's making POST requests to a URL that results in a redirect, and they turn into GETs then you could either:

a) Fix your request URL
b) Configure your client to try the redirected URL with POSTs instead of switching to GETs (it seems curl already does this).

If we switch it to 308, the only advantage is that you don't need to do a) or b), but the potential consequence is that some clients won't understand the status code at all, and it could break some existing uses.

[elithrar]

SGTM. We should add this to the documentation for StrictSlash to make the behavior clear to users.

…

On Fri, Dec 1, 2017 at 3:46 PM Kamil Kisiel ***@***.***> wrote: I'm inclined to leave it as it is. At least 301 is understood by all clients, and nothing bad will happen. If you have a client that's making POST requests to a URL that results in a redirect, and they turn into GETs then you could either: a) Fix your request URL b) Configure your client to try the redirected URL with POSTs instead of switching to GETs (it seems curl already does this). If we switch it to 308, the only advantage is that you don't need to do a) or b), but the potential consequence is that some clients won't understand the status code at all, and it could break some existing uses. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#308 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcPLGjg8RrMNpv_pjgGa0T_IiG4kBks5s8I_bgaJpZM4QFqAe> .

[elithrar]

See PR #321

Closing this out, but let me know if there are outstanding concerns.