diff --git a/context.go b/context.go index a661e47f..c7bc61fe 100644 --- a/context.go +++ b/context.go @@ -988,18 +988,32 @@ func (c *Context) ClientIP() string { } } - // It also checks if the remoteIP is a trusted proxy or not. - // In order to perform this validation, it will see if the IP is contained within at least one of the CIDR blocks - // defined by Engine.SetTrustedProxies() - remoteIP := net.ParseIP(c.RemoteIP()) - if remoteIP == nil { - return "" + var ( + trusted bool + remoteIP net.IP + ) + // If gin is listening a unix socket, always trust it. + localAddr, ok := c.Request.Context().Value(http.LocalAddrContextKey).(net.Addr) + if ok && strings.HasPrefix(localAddr.Network(), "unix") { + trusted = true + } + + // Fallback + if !trusted { + // It also checks if the remoteIP is a trusted proxy or not. + // In order to perform this validation, it will see if the IP is contained within at least one of the CIDR blocks + // defined by Engine.SetTrustedProxies() + remoteIP = net.ParseIP(c.RemoteIP()) + if remoteIP == nil { + return "" + } + trusted = c.engine.isTrustedProxy(remoteIP) } - trusted := c.engine.isTrustedProxy(remoteIP) if trusted && c.engine.ForwardedByClientIP && c.engine.RemoteIPHeaders != nil { for _, headerName := range c.engine.RemoteIPHeaders { - ip, valid := c.engine.validateHeader(c.requestHeader(headerName)) + headerValue := strings.Join(c.Request.Header.Values(headerName), ",") + ip, valid := c.engine.validateHeader(headerValue) if valid { return ip } diff --git a/context_test.go b/context_test.go index a0805e9d..f3b46432 100644 --- a/context_test.go +++ b/context_test.go @@ -1165,6 +1165,37 @@ func TestContextRenderNoContentIndentedJSON(t *testing.T) { assert.Equal(t, "application/json; charset=utf-8", w.Header().Get("Content-Type")) } +func TestContextClientIPWithMultipleHeaders(t *testing.T) { + c, _ := CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest(http.MethodGet, "/test", nil) + + // Multiple X-Forwarded-For headers + c.Request.Header.Add("X-Forwarded-For", "1.2.3.4, "+localhostIP) + c.Request.Header.Add("X-Forwarded-For", "5.6.7.8") + c.Request.RemoteAddr = localhostIP + ":1234" + + c.engine.ForwardedByClientIP = true + c.engine.RemoteIPHeaders = []string{"X-Forwarded-For"} + _ = c.engine.SetTrustedProxies([]string{localhostIP}) + + // Should return 5.6.7.8 (last non-trusted IP) + assert.Equal(t, "5.6.7.8", c.ClientIP()) +} + +func TestContextClientIPWithSingleHeader(t *testing.T) { + c, _ := CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest(http.MethodGet, "/test", nil) + c.Request.Header.Set("X-Forwarded-For", "1.2.3.4, "+localhostIP) + c.Request.RemoteAddr = localhostIP + ":1234" + + c.engine.ForwardedByClientIP = true + c.engine.RemoteIPHeaders = []string{"X-Forwarded-For"} + _ = c.engine.SetTrustedProxies([]string{localhostIP}) + + // Should return 1.2.3.4 + assert.Equal(t, "1.2.3.4", c.ClientIP()) +} + // Tests that the response is serialized as Secure JSON // and Content-Type is set to application/json func TestContextRenderSecureJSON(t *testing.T) { @@ -1906,6 +1937,16 @@ func TestContextClientIP(t *testing.T) { c.engine.trustedCIDRs, _ = c.engine.prepareTrustedCIDRs() resetContextForClientIPTests(c) + // unix address + addr := &net.UnixAddr{Net: "unix", Name: "@"} + c.Request = c.Request.WithContext(context.WithValue(c.Request.Context(), http.LocalAddrContextKey, addr)) + c.Request.RemoteAddr = addr.String() + assert.Equal(t, "20.20.20.20", c.ClientIP()) + + // reset + c.Request = c.Request.WithContext(context.Background()) + resetContextForClientIPTests(c) + // Legacy tests (validating that the defaults don't break the // (insecure!) old behaviour) assert.Equal(t, "20.20.20.20", c.ClientIP()) diff --git a/recovery.go b/recovery.go index 6d4b4b2b..bbf1d565 100644 --- a/recovery.go +++ b/recovery.go @@ -12,12 +12,12 @@ import ( "fmt" "io" "log" - "net" "net/http" "net/http/httputil" "os" "runtime" "strings" + "syscall" "time" "github.com/gin-gonic/gin/internal/bytesconv" @@ -57,40 +57,33 @@ func CustomRecoveryWithWriter(out io.Writer, handle RecoveryFunc) HandlerFunc { } return func(c *Context) { defer func() { - if err := recover(); err != nil { + if rec := recover(); rec != nil { // Check for a broken connection, as it is not really a // condition that warrants a panic stack trace. - var brokenPipe bool - if ne, ok := err.(*net.OpError); ok { - var se *os.SyscallError - if errors.As(ne, &se) { - seStr := strings.ToLower(se.Error()) - if strings.Contains(seStr, "broken pipe") || - strings.Contains(seStr, "connection reset by peer") { - brokenPipe = true - } - } - } - if e, ok := err.(error); ok && errors.Is(e, http.ErrAbortHandler) { - brokenPipe = true + var isBrokenPipe bool + err, ok := rec.(error) + if ok { + isBrokenPipe = errors.Is(err, syscall.EPIPE) || + errors.Is(err, syscall.ECONNRESET) || + errors.Is(err, http.ErrAbortHandler) } if logger != nil { - if brokenPipe { - logger.Printf("%s\n%s%s", err, secureRequestDump(c.Request), reset) + if isBrokenPipe { + logger.Printf("%s\n%s%s", rec, secureRequestDump(c.Request), reset) } else if IsDebugging() { logger.Printf("[Recovery] %s panic recovered:\n%s\n%s\n%s%s", - timeFormat(time.Now()), secureRequestDump(c.Request), err, stack(stackSkip), reset) + timeFormat(time.Now()), secureRequestDump(c.Request), rec, stack(stackSkip), reset) } else { logger.Printf("[Recovery] %s panic recovered:\n%s\n%s%s", - timeFormat(time.Now()), err, stack(stackSkip), reset) + timeFormat(time.Now()), rec, stack(stackSkip), reset) } } - if brokenPipe { + if isBrokenPipe { // If the connection is dead, we can't write a status to it. - c.Error(err.(error)) //nolint: errcheck + c.Error(err) //nolint: errcheck c.Abort() } else { - handle(c, err) + handle(c, rec) } } }() diff --git a/recovery_test.go b/recovery_test.go index 912ab601..0faa3280 100644 --- a/recovery_test.go +++ b/recovery_test.go @@ -98,13 +98,13 @@ func TestFunction(t *testing.T) { func TestPanicWithBrokenPipe(t *testing.T) { const expectCode = 204 - expectMsgs := map[syscall.Errno]string{ - syscall.EPIPE: "broken pipe", - syscall.ECONNRESET: "connection reset by peer", + expectErrnos := []syscall.Errno{ + syscall.EPIPE, + syscall.ECONNRESET, } - for errno, expectMsg := range expectMsgs { - t.Run(expectMsg, func(t *testing.T) { + for _, errno := range expectErrnos { + t.Run("Recovery from "+errno.Error(), func(t *testing.T) { var buf strings.Builder router := New() @@ -122,7 +122,8 @@ func TestPanicWithBrokenPipe(t *testing.T) { w := PerformRequest(router, http.MethodGet, "/recovery") // TEST assert.Equal(t, expectCode, w.Code) - assert.Contains(t, strings.ToLower(buf.String()), expectMsg) + assert.Contains(t, strings.ToLower(buf.String()), errno.Error()) + assert.NotContains(t, strings.ToLower(buf.String()), "[Recovery]") }) } }