Skip to content

Commit

Permalink
Restore original behaviou
Browse files Browse the repository at this point in the history
This makes for bad default security settings, but we avoid breaking the
API.
  • Loading branch information
sorenisanerd committed Aug 21, 2020
1 parent 33aac7f commit 0ca3c51
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 18 deletions.
13 changes: 9 additions & 4 deletions context.go
Original file line number Diff line number Diff line change
Expand Up @@ -713,10 +713,12 @@ func (c *Context) ShouldBindBodyWith(obj interface{}, bb binding.BindingBody) (e
// X-Real-IP and X-Forwarded-For in order to work properly with reverse-proxies such us: nginx or haproxy.
// Use X-Forwarded-For before X-Real-Ip as nginx uses X-Real-Ip with the proxy's IP.
func (c *Context) ClientIP() string {
if c.engine.RemoteIPHeader != "" {
ipChain := filterIPsFromUntrustedProxies(c.requestHeader(c.engine.RemoteIPHeader), c.Request, c.engine)
if len(ipChain) > 0 {
return ipChain[0]
if c.engine.RemoteIPHeaders != nil {
for _, header := range c.engine.RemoteIPHeaders {
ipChain := filterIPsFromUntrustedProxies(c.requestHeader(header), c.Request, c.engine)
if len(ipChain) > 0 {
return ipChain[0]
}
}
}

Expand All @@ -735,6 +737,8 @@ func filterIPsFromUntrustedProxies(XForwardedForHeader string, req *http.Request
var items, out []string
if XForwardedForHeader != "" {
items = strings.Split(XForwardedForHeader, ",")
} else {
return []string{}
}
if peerIP, err := getTransportPeerIPForRequest(req); err == nil {
items = append(items, peerIP)
Expand All @@ -751,6 +755,7 @@ func filterIPsFromUntrustedProxies(XForwardedForHeader string, req *http.Request
if !isTrustedProxy(ip, e) {
return out
}
// out = prependString(ip.String(), out)
}
return out
}
Expand Down
68 changes: 61 additions & 7 deletions context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1387,40 +1387,86 @@ func TestContextClientIP(t *testing.T) {
if host == "bar" {
return nil, errors.New("hostname lookup failed")
}
if host == "baz" {
return []string{"thisshouldneverhappen"}, nil
}
return []string{}, nil
}

c.Request.Header.Set("X-Real-IP", " 10.10.10.10 ")
c.Request.Header.Set("X-Forwarded-For", " 20.20.20.20, 30.30.30.30")
c.Request.Header.Set("X-Appengine-Remote-Addr", "50.50.50.50")
c.Request.RemoteAddr = " 40.40.40.40:42123 "
resetContextForClientIPTests(c)

// Legacy tests (validating that the defaults don't break the
// (insecure!) old behaviour)
assert.Equal(t, "20.20.20.20", c.ClientIP())

c.Request.Header.Del("X-Forwarded-For")
assert.Equal(t, "10.10.10.10", c.ClientIP())

c.engine.RemoteIPHeader = "X-Forwarded-For"
c.Request.Header.Set("X-Forwarded-For", "30.30.30.30 ")
assert.Equal(t, "30.30.30.30", c.ClientIP())

c.Request.Header.Del("X-Forwarded-For")
c.Request.Header.Del("X-Real-IP")
c.engine.AppEngine = true
assert.Equal(t, "50.50.50.50", c.ClientIP())

c.Request.Header.Del("X-Appengine-Remote-Addr")
assert.Equal(t, "40.40.40.40", c.ClientIP())

// no port
c.Request.RemoteAddr = "50.50.50.50"
assert.Empty(t, c.ClientIP())

// Tests exercising the TrustedProxies functionality
resetContextForClientIPTests(c)

// No trusted proxies
c.engine.TrustedProxies = []string{}
c.engine.RemoteIPHeaders = []string{"X-Forwarded-For"}
assert.Equal(t, "40.40.40.40", c.ClientIP())

// Last proxy is trusted, but the RemoteAddr is not
c.engine.TrustedProxies = []string{"30.30.30.30"}
assert.Equal(t, "40.40.40.40", c.ClientIP())

// Only trust RemoteAddr
c.engine.TrustedProxies = []string{"40.40.40.40"}
assert.Equal(t, "30.30.30.30", c.ClientIP())

// All steps are trusted
c.engine.TrustedProxies = []string{"40.40.40.40", "30.30.30.30", "20.20.20.20"}
assert.Equal(t, "20.20.20.20", c.ClientIP())

// Use CIDR
c.engine.TrustedProxies = []string{"40.40.25.25/16", "30.30.30.30"}
assert.Equal(t, "20.20.20.20", c.ClientIP())

// Use hostname that resolves to all the proxies
c.engine.TrustedProxies = []string{"foo"}
assert.Equal(t, "20.20.20.20", c.ClientIP())

// Use hostname that returns an error
c.engine.TrustedProxies = []string{"bar"}
assert.Equal(t, "40.40.40.40", c.ClientIP())

// X-Forwarded-For has a non-IP element
c.engine.TrustedProxies = []string{"40.40.40.40"}
c.Request.Header.Set("X-Forwarded-For", " blah ")
assert.Equal(t, "40.40.40.40", c.ClientIP())

// Result from LookupHost has non-IP element. This should never
// happen, but we should test it to make sure we handle it
// gracefully.
c.engine.TrustedProxies = []string{"baz"}
c.Request.Header.Set("X-Forwarded-For", " 30.30.30.30 ")
assert.Equal(t, "40.40.40.40", c.ClientIP())

c.engine.TrustedProxies = []string{"40.40.40.40"}
c.engine.RemoteIPHeader = "X-Real-IP"
c.Request.Header.Del("X-Forwarded-For")
c.engine.RemoteIPHeaders = []string{"X-Forwarded-For", "X-Real-IP"}
assert.Equal(t, "10.10.10.10", c.ClientIP())

c.engine.RemoteIPHeader = ""
c.engine.RemoteIPHeaders = []string{}
c.engine.AppEngine = true
assert.Equal(t, "50.50.50.50", c.ClientIP())

Expand All @@ -1432,6 +1478,14 @@ func TestContextClientIP(t *testing.T) {
assert.Empty(t, c.ClientIP())
}

func resetContextForClientIPTests(c *Context) {
c.Request.Header.Set("X-Real-IP", " 10.10.10.10 ")
c.Request.Header.Set("X-Forwarded-For", " 20.20.20.20, 30.30.30.30")
c.Request.Header.Set("X-Appengine-Remote-Addr", "50.50.50.50")
c.Request.RemoteAddr = " 40.40.40.40:42123 "
c.engine.AppEngine = false
}

func TestContextContentType(t *testing.T) {
c, _ := CreateTestContext(httptest.NewRecorder())
c.Request, _ = http.NewRequest("POST", "/", nil)
Expand Down
10 changes: 5 additions & 5 deletions gin.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ type Engine struct {
// If no other Method is allowed, the request is delegated to the NotFound
// handler.
HandleMethodNotAllowed bool
RemoteIPHeader string

TrustedProxies []string
ForwardedByClientIP bool
RemoteIPHeaders []string
TrustedProxies []string

// #726 #755 If enabled, it will thrust some headers starting with
// 'X-AppEngine...' for better integration with that PaaS.
Expand Down Expand Up @@ -142,8 +142,8 @@ func New() *Engine {
RedirectTrailingSlash: true,
RedirectFixedPath: false,
HandleMethodNotAllowed: false,
RemoteIPHeader: "",
TrustedProxies: []string{},
RemoteIPHeaders: []string{"X-Forwarded-For", "X-Real-IP"},
TrustedProxies: []string{"0.0.0.0/0"},
AppEngine: defaultAppEngine,
UseRawPath: false,
RemoveExtraSlash: false,
Expand Down
2 changes: 0 additions & 2 deletions logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,6 @@ func TestLoggerWithConfigFormatting(t *testing.T) {
buffer := new(bytes.Buffer)

router := New()
router.TrustedProxies = []string{"192.0.2.1"}
router.RemoteIPHeader = "X-Forwarded-For"
router.Use(LoggerWithConfig(LoggerConfig{
Output: buffer,
Formatter: func(param LogFormatterParams) string {
Expand Down

0 comments on commit 0ca3c51

Please sign in to comment.