code review 4476045: http: don't Clean query string in relative redirects

http: don't Clean query string in relative redirects

[bradfitz, 2011-05-06 01:25:28]

Hello adg, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[kevlar, 2011-05-06 01:49:11]

On 2011/05/06 01:25:28, bradfitz wrote:
> Hello adg, rsc (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

LGTM

[r, 2011-05-06 19:48:08]

LGTM

[bradfitz, 2011-05-06 20:18:41]

Russ,

Low priority, but do you remember the motivation for the path.Clean here?
 Was it just for doing the path resolution of /foo/bar + ../baz etc?

Particularly I'm wondering whether the double slash // -> / conversion was
intentional or a side effect.

If the redirect url starts with a "/", couldn't it just be passed through
unmodified, since in that case we don't need to do any path resolution?
 Apache has no problem with two slashes:

http://bradfitz.com/cgi-bin/dump.cgi/foo//bar

Then perhaps we just keep path.Clean for the case where a Location header is
a path-relative ../foo but not when it's a host-relative /foo?


On Thu, May 5, 2011 at 6:25 PM, <bradfitz@golang.org> wrote:

> Reviewers: adg, rsc,
>
> Message:
> Hello adg, rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http: don't Clean query string in relative redirects
>
> Please review this at http://codereview.appspot.com/4476045/
>
> Affected files:
>  M src/pkg/http/serve_test.go
>  M src/pkg/http/server.go
>
>
> Index: src/pkg/http/serve_test.go
> ===================================================================
> --- a/src/pkg/http/serve_test.go
> +++ b/src/pkg/http/serve_test.go
> @@ -693,3 +693,12 @@
>                t.Errorf("expected Write error of %v; got %v", e, g)
>        }
>  }
> +
> +func TestRelativeRedirect(t *testing.T) {
> +       req, _ := NewRequest("GET", "http://example.com/", nil)
> +       resp := httptest.NewRecorder()
> +       Redirect(resp, req, "/foo?next=http://bar.com/", 302)
> +       if g, e := resp.Header().Get("Location"), "/foo?next=
> http://bar.com/"; g != e {
> +               t.Errorf("Location header was %q; want %q", g, e)
> +       }
> +}
> Index: src/pkg/http/server.go
> ===================================================================
> --- a/src/pkg/http/server.go
> +++ b/src/pkg/http/server.go
> @@ -581,12 +581,19 @@
>                                url = olddir + url
>                        }
>
> +                       var query string
> +                       if qmark := strings.Index(url, "?"); qmark != -1 {
> +                               query = url[qmark:]
> +                               url = url[:qmark]
> +                       }
> +
>                        // clean up but preserve trailing slash
>                        trailing := url[len(url)-1] == '/'
>                        url = path.Clean(url)
>                        if trailing && url[len(url)-1] != '/' {
>                                url += "/"
>                        }
> +                       url += query
>                }
>        }
>
>
>
>

[rsc, 2011-05-06 20:46:47]

LGTM

http://codereview.appspot.com/4476045/diff/1001/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4476045/diff/1001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:586: query = url[qmark:]
if i := strings.Index(url, "?"); i >= 0 {
    url, query = url[:i], url[i:]
}

[rsc, 2011-05-06 20:46:57]

I was just trying to fix up unnecessary .. to
short circuit some future redirections.

[bradfitz, 2011-05-11 11:30:34]

*** Submitted as http://code.google.com/p/go/source/detail?r=3556fc4657a3 ***

http: don't Clean query string in relative redirects

R=adg, rsc, kevlar, r
CC=golang-dev
http://codereview.appspot.com/4476045