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

http: Make StripPrefix leave a "/" at the start of the stripped path, even if the prefix ends with a "/".

It was using the empty string when the path was equal to the prefix, which caused a panic in ServeFile.

[andybalholm, 2011-07-16 04:43:22]

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

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

[rsc, 2011-07-18 13:59:57]

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

Russ

[andybalholm, 2011-07-18 14:22:58]

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

[rsc, 2011-07-18 14:24:29]

> 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.

[andybalholm, 2011-07-18 14:40:23]

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

[rsc, 2011-07-18 14:49:01]

>> 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

[andybalholm, 2011-07-18 15:22:18]

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

[andybalholm, 2011-07-18 15:31:35]

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

Please take another look.

[rsc, 2011-07-18 15:35:14]

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#newcod...
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.

[andybalholm, 2011-07-18 17:23:31]

On Jul 18, 2011, at 8:35 AM, rsc@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

[andybalholm, 2011-07-18 17:30:14]

*** Abandoned ***