code review 4754045: http: Make StripPrefix use a path of "/" when the input... (issue4754045)

[andyb...@gmail.com]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com (cc: golan...@googlegroups.com),

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

Description:
http: Make StripPrefix use a path of "/" when the input path equals the
prefix.

It was using the empty string, which caused a panic in ServeFile.

Please review this at http://codereview.appspot.com/4754045/

Affected files:
M src/pkg/http/server.go

Index: src/pkg/http/server.go
===================================================================
--- a/src/pkg/http/server.go
+++ b/src/pkg/http/server.go
@@ -657,6 +657,9 @@
return
}
r.URL.Path = r.URL.Path[len(prefix):]
+ if r.URL.Path == "" {
+ r.URL.Path = "/"
+ }
h.ServeHTTP(w, r)
})
}

[Russ Cox]

I am not convinced.
If you want it not to strip the final slash in /x/, pass in /x.

Russ

[Andy Balholm]

On Jul 18, 2011, at 6:59 AM, Russ Cox wrote:

> I am not convinced.
> If you want it not to strip the final slash in /x/, pass in /x.

That makes sense for new code.

But I took a working program and updated it to work with the new http file system with gofix. Then it started crashing.

Maybe we should change gofix instead and make it remove the final slash from the path to strip.

Andy

[Russ Cox]

> That makes sense for new code.
>
> But I took a working program and updated it to work with the new http file system with gofix. Then it started crashing.
>
> Maybe we should change gofix instead and make it remove the final slash from the path to strip.

Aha. Yes, gofix is wrong.

[Andy Balholm]

On Jul 18, 2011, at 7:24 AM, Russ Cox wrote:

> Aha. Yes, gofix is wrong.

What should we do to gofix when the prefix is a variable?

Andy

[Russ Cox]

>> Aha.  Yes, gofix is wrong.
>
> What should we do to gofix when the prefix is a variable?

I am pretty sure that StripPrefix is correct.
It's just a string manipulation function.

An alternate fix for this situation would be to
let the FileServer not assume the path to
be absolute in ServeHTTP.

Russ

[andyb...@gmail.com]

Hello golan...@googlegroups.com, r...@golang.org (cc:
golan...@googlegroups.com),

Please take another look.

http://codereview.appspot.com/4754045/

[r...@golang.org]

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

http://codereview.appspot.com/4754045/diff/7002/src/pkg/http/server.go#newcode654
src/pkg/http/server.go:654: if prefix != "" && prefix[len(prefix)-1] ==
'/' {
Modifying StripPrefix is wrong.

This is a string manipulation function, not a
file system path manipulation function.

strings.HasPrefix does not test for a following /
before returning true.

http://codereview.appspot.com/4754045/

[andyb...@gmail.com]

*** Abandoned ***

http://codereview.appspot.com/4754045/

[Andy Balholm]

On Jul 18, 2011, at 7:48 AM, Russ Cox wrote:

> I am pretty sure that StripPrefix is correct.
> It's just a string manipulation function.
>
> An alternate fix for this situation would be to
> let the FileServer not assume the path to
> be absolute in ServeHTTP.

In fs.go, line 126, there is a comment that says,

// r.URL.Path always begins with /

The crash occurs because that assumption is violated. (An empty string does not begin with "/".)

We can change that assumption, or we can change other functions to ensure that it is not violated.

The change I had proposed is working in the second direction, but it does not go far enough to protect the assumption in other cases (where it doesn't cause a crash, but it might cause other unforeseen problems). I'll update it to cover the other cases, and then we can see if that is what we want to do.

Andy

[Andy Balholm]

On Jul 18, 2011, at 8:35 AM, r...@golang.org wrote:

> This is a string manipulation function, not a
> file system path manipulation function.
>
> strings.HasPrefix does not test for a following /
> before returning true.

Ok, it looks like Brad has taken care of it.

Andy