code review 4789042: http: make serveFile redirects relative to work with St...

http: make serveFile redirects relative to work with StripPrefix

serveFile was using absolute redirects, which didn't work under StripPrefix.
Now it uses relative redirects.

[andybalholm, 2011-07-19 20:09:45]

Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org,
golang-dev@googlegroups.com, kevlar@google.com),

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

[bradfitz, 2011-07-19 20:13:26]

I'm afraid the next CL involves parsing the response HTML and rewriting
links.

StripPrefix does one small simple thing currently.  I'd rather keep it like
that.


On Tue, Jul 19, 2011 at 1:09 PM, <andybalholm@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org,
> golang-dev@googlegroups.com, kevlar@google.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http: redirect with StripPrefix
>
> When a handler running under StripPrefix issues a redirect,
> it doesn't include the prefix. So now StripPrefix adds the
> prefix to redirects.
>
> Please review this at
http://codereview.appspot.com/**4789042/<http://codereview.appspot.com/4789042/>
>
> 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
> @@ -852,6 +852,9 @@
>  func TestStripPrefix(t *testing.T) {
>        h := HandlerFunc(func(w ResponseWriter, r *Request) {
>                w.Header().Set("X-Path", r.URL.Path)
> +               if r.URL.Path == "/baz" {
> +                       Redirect(w, r, "/bar", StatusMovedPermanently)
> +               }
>        })
>        ts := httptest.NewServer(**StripPrefix("/foo", h))
>        defer ts.Close()
> @@ -871,6 +874,14 @@
>        if g, e := res.StatusCode, 404; g != e {
>                t.Errorf("test 2: got status %v, want %v", g, e)
>        }
> +
> +       res, err = Get(ts.URL + "/foo/baz")
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       if g, e := res.Header.Get("X-Path"), "/bar"; g != e {
> +               t.Errorf("test 3: got %s, want %s", g, e)
> +       }
>  }
>
>  type errorListener struct {
> Index: src/pkg/http/server.go
> ==============================**==============================**=======
> --- a/src/pkg/http/server.go
> +++ b/src/pkg/http/server.go
> @@ -650,6 +650,11 @@
>  // and invoking the handler h. StripPrefix handles a
>  // request for a path that doesn't begin with prefix by
>  // replying with an HTTP 404 not found error.
> +//
> +// If h issues a redirect to a local path
> +// (one that does not include the scheme and host),
> +// StripPrefix will attempt to make it work properly by
> +// adding prefix back on to the new path.
>  func StripPrefix(prefix string, h Handler) Handler {
>        return HandlerFunc(func(w ResponseWriter, r *Request) {
>                if !strings.HasPrefix(r.URL.Path, prefix) {
> @@ -657,10 +662,35 @@
>                        return
>                }
>                r.URL.Path = r.URL.Path[len(prefix):]
> -               h.ServeHTTP(w, r)
> +               h.ServeHTTP(&**subDirResponseWriter{w, prefix}, r)
>        })
>  }
>
> +type subDirResponseWriter struct {
> +       ResponseWriter
> +       path string
> +}
> +
> +func (w *subDirResponseWriter) WriteHeader(code int) {
> +       if code == StatusMovedPermanently || code ==
> StatusTemporaryRedirect {
> +               loc := w.Header().Get("Location")
> +               if strings.HasPrefix(loc, "/") {
> +                       url, err := ParseURLReference(loc)
> +                       if err == nil {
> +                               p := w.path + "/" + url.Path
> +                               trailing := p[len(p)-1] == '/'
> +                               p = path.Clean(p)
> +                               if trailing && p[len(p)-1] != '/' {
> +                                       p += "/"
> +                               }
> +                               url.Path = p
> +                               w.Header().Set("Location", url.String())
> +                       }
> +               }
> +       }
> +       w.ResponseWriter.WriteHeader(**code)
> +}
> +
>  // Redirect replies to the request with a redirect to url,
>  // which may be a path relative to the request path.
>  func Redirect(w ResponseWriter, r *Request, url string, code int) {
>
>
>

[rsc, 2011-07-19 20:32:16]

No thanks.

StripPrefix is about stripping a prefix, which it does admirably.
That's *all* it does.

[andybalholm, 2011-07-19 20:42:34]

On Jul 19, 2011, at 1:32 PM, Russ Cox wrote:

> No thanks.
> 
> StripPrefix is about stripping a prefix, which it does admirably.
> That's *all* it does.

Something needs to be done to make redirects work under StripPrefix. What type
of approach to you suggest?

Andy

[bradfitz, 2011-07-19 20:45:11]

On Tue, Jul 19, 2011 at 1:42 PM, Andy Balholm <andybalholm@gmail.com> wrote:

> On Jul 19, 2011, at 1:32 PM, Russ Cox wrote:
>
> > No thanks.
> >
> > StripPrefix is about stripping a prefix, which it does admirably.
> > That's *all* it does.
>
> Something needs to be done to make redirects work under StripPrefix. What
> type of approach to you suggest?


Why not just a relative redirect?

fs.go's <someBase>/index.html -> <someBase>/ redirect can just be "./"

[rsc, 2011-07-19 20:48:10]

On Tue, Jul 19, 2011 at 16:42, Andy Balholm <andybalholm@gmail.com> wrote:
> On Jul 19, 2011, at 1:32 PM, Russ Cox wrote:
>
>> No thanks.
>>
>> StripPrefix is about stripping a prefix, which it does admirably.
>> That's *all* it does.
>
> Something needs to be done to make redirects work under StripPrefix. What type
of approach to you suggest?

i suggest that the user of StripPrefix write working redirects.
StripPrefix is not an http jail.

[andybalholm, 2011-07-19 21:43:49]

Hello golang-dev@googlegroups.com, rsc@golang.org (cc: bradfitz@golang.org,
golang-dev@googlegroups.com, kevlar@google.com),

Please take another look.

[andybalholm, 2011-07-19 21:47:02]

On Jul 19, 2011, at 1:45 PM, Brad Fitzpatrick wrote:

> Why not just a relative redirect?
> 
> fs.go's <someBase>/index.html -> <someBase>/ redirect can just be "./"


I tried it with Safari and Firefox, and it works with both of them.

I've redone the CL to use relative redirects instead of having StripPrefix
rewrite them.

Andy

[andybalholm, 2011-07-19 22:01:21]

On Jul 19, 2011, at 1:48 PM, Russ Cox wrote:

> i suggest that the user of StripPrefix write working redirects.
> StripPrefix is not an http jail.

The most common "user" of StripPrefix is FileServer. I have rewritten my
changelist to fix the redirects that FileServer issues instead of making
StripPrefix try to patch them up.

Andy

[bradfitz, 2011-07-19 22:44:20]

I'm in favor of making the redirect "./", but can this be done with less
repetition?  And perhaps a test?

[andybalholm, 2011-07-19 23:45:42]

Hello golang-dev@googlegroups.com, rsc@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com, kevlar@google.com),

Please take another look.

[andybalholm, 2011-07-23 15:13:35]

Hello golang-dev@googlegroups.com, rsc@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com, kevlar@google.com),

Please take another look.

[andybalholm, 2011-07-28 14:33:28]

On 2011/07/19 22:44:20, bradfitz wrote:
> I'm in favor of making the redirect "./", but can this be done with less
> repetition?  And perhaps a test?

I've moved the redirect code into a new function, and written a test. Does
anyone want to review the changes?

[bradfitz, 2011-07-28 18:44:17]

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

http: make serveFile redirects relative to work with StripPrefix

serveFile was using absolute redirects, which didn't work under StripPrefix.
Now it uses relative redirects.

R=golang-dev, rsc, bradfitz
CC=golang-dev, kevlar
http://codereview.appspot.com/4789042

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[bradfitz, 2011-07-28 19:12:39]

LGTM

On Thu, Jul 28, 2011 at 7:33 AM, <andybalholm@gmail.com> wrote:

> On 2011/07/19 22:44:20, bradfitz wrote:
>
>> I'm in favor of making the redirect "./", but can this be done with
>>
> less
>
>> repetition?  And perhaps a test?
>>
>
> I've moved the redirect code into a new function, and written a test.
> Does anyone want to review the changes?
>
>
>
http://codereview.appspot.com/**4789042/<http://codereview.appspot.com/4789042/>
>