code review 1742057: http: obscure passwords in return value of URL.String

http: obscure passwords in return value of URL.String

Fixes issue 974.

[bytbox, 2010-08-06 05:24:09]

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

I'd like you to review this change.

[rsc, 2010-08-06 18:59:52]

I'm not sure about the design here.

I don't believe HidePassword should be public.
I also don't believe that the error string function
should be in the business of putting URLs back
together.

Instead, I think http.URL.String should just
hide the password when preparing the string,
no matter what the context is.

Russ

[bytbox, 2010-08-06 22:19:37]

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

Please take another look.

[rsc, 2010-08-06 23:41:19]

On Fri, Aug 6, 2010 at 15:19,  <bytbox@gmail.com> wrote:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.

I suggested that http.URL.String elide the password.
It should be easy to do this, and without package regexp.
Then in Get you'd just set url = req.URL.String() after
the successful ParseURL call.

Russ

[bytbox, 2010-08-07 01:35:42]

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

Please take another look.

[rsc1, 2010-08-12 03:59:18]

looks pretty good.  thanks for fixing this

http://codereview.appspot.com/1742057/diff/11001/12002
File src/pkg/http/url.go (right):

http://codereview.appspot.com/1742057/diff/11001/12002#newcode393
src/pkg/http/url.go:393: userinfoParts := strings.Split(url.Userinfo, ":", 2)
Please use strings.Index and a string slice.  strings.Split is overkill for
splitting a string in half.  Also avoid duplication of code by using a
temporary variable.

info := url.Userinfo
if i := strings.Index(info, ":"); i >= 0 {
    info = info[0:i] + ":******"
}
result += urlEscape(info, false) + "@"

http://codereview.appspot.com/1742057/diff/11001/12003
File src/pkg/http/url_test.go (right):

http://codereview.appspot.com/1742057/diff/11001/12003#newcode449
src/pkg/http/url_test.go:449: type HidePasswordTest struct {
I don't think this needs a separate test struct and logic.
You should be able to add a test or two to the urltests
slice at the top of the file and let the usual code do the
testing automatically.

[bytbox, 2010-08-12 08:48:30]

http://codereview.appspot.com/1742057/diff/11001/12003
File src/pkg/http/url_test.go (right):

http://codereview.appspot.com/1742057/diff/11001/12003#newcode449
src/pkg/http/url_test.go:449: type HidePasswordTest struct {
On 2010/08/12 03:59:18, rsc1 wrote:
> I don't think this needs a separate test struct and logic.
> You should be able to add a test or two to the urltests
> slice at the top of the file and let the usual code do the
> testing automatically.
Unless I'm misreading something, the testing code above assumes the URL input to
Parse will be the same as the URL output from String, and thus cannot be used
where this does not hold.

[bytbox, 2010-08-12 08:49:09]

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

Please take another look.

[rsc, 2010-08-12 15:56:35]

> Unless I'm misreading something, the testing code above assumes the URL
> input to Parse will be the same as the URL output from String, and thus
> cannot be used where this does not hold.

It's entirely possible that I am misremembering something
but the comment on the roundtrip field suggests to me
that if you put the expected output URL in that field
instead of "" then the test will just work.
I'm looking at the computation around lnie 276 too.

Russ

[bytbox, 2010-08-12 20:45:49]

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

Please take another look.

[rsc1, 2010-08-26 16:38:43]

LGTM

[rsc, 2010-08-26 17:32:21]

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

http: obscure passwords in return value of URL.String

Fixes issue 974.

R=rsc
CC=golang-dev
http://codereview.appspot.com/1742057

Committer: Russ Cox <rsc@golang.org>