Skip to content
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

net/http: ServeMux redirect loses query info #17841

Closed
sdwarwick opened this issue Nov 8, 2016 · 16 comments
Closed

net/http: ServeMux redirect loses query info #17841

sdwarwick opened this issue Nov 8, 2016 · 16 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@sdwarwick
Copy link

sdwarwick commented Nov 8, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.7.1 windows/amd64

What operating system and processor architecture are you using (go env)?

go version go1.7.1 windows/amd64

What did you do?

this error cannot be seen on play as it requires http.

package main

import (
	"fmt"
	"net/http"
)

func main() {

	http.HandleFunc("/testOne", testOneHandler)  // no slash at end or you lose the query string
	http.HandleFunc("/testTwo/", testTwoHandler) // no slash at end or you lose the query string

	http.ListenAndServe(":8008", nil)

}

func testOneHandler(w http.ResponseWriter, r *http.Request) {

	textOut := r.URL.Query()
	fmt.Fprintln(w, textOut)

	return
}

func testTwoHandler(w http.ResponseWriter, r *http.Request) {

	textOut := r.URL.Query()
	fmt.Fprintln(w, textOut)
	return
}

What did you expect to see?

This version works:
input URL: http://localhost:8008/testOne?this=that
result: URL stays the same, and the output is: map[this:[that]]

This doesn't:

input URL: http://localhost:8008/testTwo?this=that

result: URL is redirected to http://localhost:8008/testTwo/
there is no output.

conclusion:
query strings are lost when the input URL and the http router have a "/" at the end of the selector.

this error was initially reported in #5382,

I hope that this is sufficient information.

@bradfitz
Copy link
Contributor

bradfitz commented Nov 8, 2016

In summary:

bradfitz@gdev:~$ curl 'http://localhost:8080/testOne?foo=bar' 
/testOne?foo=bar
bradfitz@gdev:~$ curl 'http://localhost:8080/testOne/?foo=bar'
404 page not found
bradfitz@gdev:~$ curl 'http://localhost:8080/testTwo?foo=bar'
<a href="/testTwo/">Moved Permanently</a>.

bradfitz@gdev:~$ curl 'http://localhost:8080/testTwo/?foo=bar'
/testTwo/?foo=bar

I assume your question is about my third case? (http://localhost:8080/testTwo?foo=bar)

But you said http://localhost:8080/testTwo/?foo=bar in your bug report, which I assume is a mistake.

What is your actual bug report?

@bradfitz bradfitz changed the title apparent return of problem #5382 ( repost with more info ) net/http: ServeMux redirect loses query info Nov 8, 2016
@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 8, 2016
@sdwarwick
Copy link
Author

correct, I edited the issue to reflect the typo. The error is in case #3 where a re-direct occurs and doesn't forward the query string. Thanks for looking at this.

@bradfitz bradfitz removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 9, 2016
@bradfitz bradfitz added this to the Go1.9 milestone Nov 9, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Nov 9, 2016

Not a regression from Go 1.7, so targetting Go 1.9.

@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 9, 2016
@odeke-em
Copy link
Member

And here is a playground runnable repro https://play.golang.org/p/t0sLqU5tIS for anyone who wants to test or fix the bug.

@gopherbot
Copy link

CL https://golang.org/cl/43779 mentions this issue.

@odeke-em
Copy link
Member

I've mailed https://go-review.googlesource.com/43779.

@odeke-em
Copy link
Member

I was just whetting my appetite for Kubernetes code and testing it out, then noticed that this bug fix breaks their two of their tests(which seemed to rely on the old buggy behavior)
screen shot 2017-05-23 at 4 14 28 am
with the test failing when they encounter:

server_test.go:1327: 7: response location: expected http://localhost:12345/exec, got http://localhost:12345/exec?ignore=1&command=ls&command=-a&output=1

Just a note as am also going to file a bug on their project for them too, to keep vigilant when they update to Go1.9.

@bradfitz
Copy link
Contributor

@odeke-em, nice find! Yeah, if you could send them a PR to make their test work with either Go 1.8 or Go 1.9, that'd save everybody a lot of hassle later. Thanks!

@odeke-em
Copy link
Member

Aye aye, done sent in kubernetes/kubernetes#46306.

odeke-em added a commit to orijtech/kubernetes that referenced this issue May 24, 2017
Fixes kubernetes#46282.

After golang/go#17841 was fixed as part
of Go1.9, Go's http.ServeMux now forwards along the query string
during redirects. The tests pre-Go1.9 relied on the old/buggy
behavior.

This PR ensures both forward and backward compability so that
if the new redirect Location is received with either pre-Go1.9 behavior
or Go1.9+ behavior, we can still properly test for both.
@odeke-em odeke-em reopened this May 25, 2017
@odeke-em
Copy link
Member

From offline discussions with @bradfitz, the closing CL broke the builds at tip and didn't account for the case where Redirect was invoked directly with a full URL that itself had its own query string. Reopening this issue.

@gopherbot
Copy link

CL https://golang.org/cl/44100 mentions this issue.

gopherbot pushed a commit that referenced this issue May 25, 2017
CL 43779/commit 6a6c792
broke the builds at tip, and that CL doesn't account for
cases where Redirect is directly invoked with a full URL
that itself has a query string.

Updates #17841

Change-Id: Idb0486bae8625e1f9e033ca4cfcd87de95bc835c
Reviewed-on: https://go-review.googlesource.com/44100
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bradfitz
Copy link
Contributor

bradfitz commented Jun 5, 2017

We should only change ServeMux, not func Redirect.

@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 5, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jun 5, 2017
@bradfitz bradfitz removed this from the Go1.9 milestone Jun 5, 2017
@gopherbot
Copy link

Change https://golang.org/cl/61210 mentions this issue: net/http: make ServeMux redirect mux at the runtime

@namusyaka
Copy link
Member

@odeke-em Could you take a look at the CL?

@odeke-em
Copy link
Member

odeke-em commented Sep 7, 2017

Thank you for the ping @namusyaka, and thank you for the CL, I am going to take a look at it in a few.

@namusyaka
Copy link
Member

@sdwarwick The issue has been fixed by this commit. Please let me know if you still have the issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants