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

Comments

Projects
None yet
5 participants
@sdwarwick
Copy link

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

This comment has been minimized.

Copy link
Member

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

@sdwarwick

This comment has been minimized.

Copy link
Author

commented Nov 8, 2016

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 label Nov 9, 2016

@bradfitz bradfitz added this to the Go1.9 milestone Nov 9, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 9, 2016

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

@odeke-em

This comment has been minimized.

Copy link
Member

commented May 20, 2017

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

@gopherbot

This comment has been minimized.

Copy link

commented May 21, 2017

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

@odeke-em

This comment has been minimized.

Copy link
Member

commented May 21, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented May 23, 2017

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

This comment has been minimized.

Copy link
Member

commented May 23, 2017

@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 added a commit to orijtech/kubernetes that referenced this issue May 23, 2017

pkg/kubelet/server: ensure tests compatible with net/http.ServeMux
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 added a commit to orijtech/kubernetes that referenced this issue May 23, 2017

pkg/kubelet/server: ensure tests compatible with net/http.ServeMux
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 added a commit to orijtech/kubernetes that referenced this issue May 23, 2017

pkg/kubelet/server: ensure tests compatible with net/http.ServeMux
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

This comment has been minimized.

Copy link
Member

commented May 23, 2017

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

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

pkg/kubelet/server: ensure tests compatible with net/http.ServeMux
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

This comment has been minimized.

Copy link
Member

commented May 25, 2017

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

This comment has been minimized.

Copy link

commented May 25, 2017

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

gopherbot pushed a commit that referenced this issue May 25, 2017

net/http: revert CL 43779
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

This comment has been minimized.

Copy link
Member

commented Jun 5, 2017

We should only change ServeMux, not func Redirect.

@bradfitz bradfitz added NeedsFix and removed NeedsDecision 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

This comment has been minimized.

Copy link

commented Sep 2, 2017

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

@namusyaka

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

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

@odeke-em

This comment has been minimized.

Copy link
Member

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.

@gopherbot gopherbot closed this in ab40107 Sep 8, 2017

@namusyaka

This comment has been minimized.

Copy link
Member

commented Sep 11, 2017

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

@golang golang locked and limited conversation to collaborators Sep 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.