code review 6448053: httputil: accumulate X-Forwarded-For header info

httputil: accumulate X-Forwarded-For header info

If the X-Forwarded-For header already exists on a request, we
should append our client's IP to it after a comma+space instead
of overwriting it.

Fixes issue 3846.

[bpowers, 2012-07-26 14:39:58]

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

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

[bradfitz, 2012-07-29 23:01:47]

http://codereview.appspot.com/6448053/diff/4001/src/pkg/net/http/httputil/rev...
File src/pkg/net/http/httputil/reverseproxy.go (right):

http://codereview.appspot.com/6448053/diff/4001/src/pkg/net/http/httputil/rev...
src/pkg/net/http/httputil/reverseproxy.go:113: if prior :=
outreq.Header.Get("X-Forwarded-For"); prior != "" {
This isn't correct if there are multiple X-Forwarded-For headers.  You only look
at the first one.

Do something like:

if prior := outreq.Header["X-Forwarded-For"]; len(prior) > 0 {
    clientIP = strings.Join(prior, ", ") + ", " + clientIP
}

Also, rename clientIp to clientIP while you're here.  Go style says you
shouldn't mix case in abbreviations.

http://codereview.appspot.com/6448053/diff/4001/src/pkg/net/http/httputil/rev...
File src/pkg/net/http/httputil/reverseproxy_test.go (right):

http://codereview.appspot.com/6448053/diff/4001/src/pkg/net/http/httputil/rev...
src/pkg/net/http/httputil/reverseproxy_test.go:101:
getReq.Header.Set("X-Forwarded-For", prevForwardedFor)
also test multiple X-Forwarded-For lines

[bpowers, 2012-07-30 15:58:04]

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

Please take another look.

[bpowers, 2012-07-30 16:00:21]

On Sun, Jul 29, 2012 at 7:01 PM,  <bradfitz@golang.org> wrote:
>
>
http://codereview.appspot.com/6448053/diff/4001/src/pkg/net/http/httputil/rev...
> File src/pkg/net/http/httputil/reverseproxy.go (right):
>
>
http://codereview.appspot.com/6448053/diff/4001/src/pkg/net/http/httputil/rev...
> src/pkg/net/http/httputil/reverseproxy.go:113: if prior :=
>
> outreq.Header.Get("X-Forwarded-For"); prior != "" {
> This isn't correct if there are multiple X-Forwarded-For headers.  You
> only look at the first one.
>
> Do something like:
>
> if prior := outreq.Header["X-Forwarded-For"]; len(prior) > 0 {
>     clientIP = strings.Join(prior, ", ") + ", " + clientIP
> }

I used the prior, ok := ...; ok idiom.  Are there cases when the
Header would return a 0-length array for a known header?

> Also, rename clientIp to clientIP while you're here.  Go style says you
> shouldn't mix case in abbreviations.
>
>
http://codereview.appspot.com/6448053/diff/4001/src/pkg/net/http/httputil/rev...
> File src/pkg/net/http/httputil/reverseproxy_test.go (right):
>
>
http://codereview.appspot.com/6448053/diff/4001/src/pkg/net/http/httputil/rev...
> src/pkg/net/http/httputil/reverseproxy_test.go:101:
> getReq.Header.Set("X-Forwarded-For", prevForwardedFor)
> also test multiple X-Forwarded-For lines
>
> http://codereview.appspot.com/6448053/

[bradfitz, 2012-07-30 22:37:56]

LGTM

http://codereview.appspot.com/6448053/diff/8002/src/pkg/net/http/httputil/rev...
File src/pkg/net/http/httputil/reverseproxy.go (right):

http://codereview.appspot.com/6448053/diff/8002/src/pkg/net/http/httputil/rev...
src/pkg/net/http/httputil/reverseproxy.go:113: if prior, ok :=
outreq.Header["X-Forwarded-For"]; ok {
the way I suggested with len is slightly more robust: it handles the case of an
empty list appearing in X-Forwarded-For and then won't have a leading ", ",
although that's probably harmless.  And len(prior) works even if prior is nil,
so it's not really any more code.

But either way.

[bradfitz, 2012-07-30 22:39:15]

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

httputil: accumulate X-Forwarded-For header info

If the X-Forwarded-For header already exists on a request, we
should append our client's IP to it after a comma+space instead
of overwriting it.

Fixes issue 3846.

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/6448053

Committer: Brad Fitzpatrick <bradfitz@golang.org>