From 3b39f0883c60b6978c6c6df2c0029bd4c96613fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Guillot?= Date: Fri, 1 Jun 2018 07:22:18 -0700 Subject: [PATCH] Rewrite RealIP() to avoid returning an empty string --- Gopkg.lock | 8 +- Gopkg.toml | 4 - http/request/request.go | 29 ++++++ http/request/request_test.go | 82 ++++++++++++++++ middleware/logging.go | 5 +- ui/login_check.go | 4 +- ui/oauth2_callback.go | 4 +- vendor/github.com/tomasen/realip/.travis.yml | 19 ---- vendor/github.com/tomasen/realip/README.md | 33 ------- vendor/github.com/tomasen/realip/realip.go | 89 ----------------- .../github.com/tomasen/realip/realip_test.go | 95 ------------------- 11 files changed, 117 insertions(+), 255 deletions(-) create mode 100644 http/request/request_test.go delete mode 100644 vendor/github.com/tomasen/realip/.travis.yml delete mode 100644 vendor/github.com/tomasen/realip/README.md delete mode 100644 vendor/github.com/tomasen/realip/realip.go delete mode 100644 vendor/github.com/tomasen/realip/realip_test.go diff --git a/Gopkg.lock b/Gopkg.lock index 8cfabcfe..398956e9 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -69,12 +69,6 @@ revision = "639f6272aec6b52094db77b9ec488214b0b4b1a1" version = "v2.3.2" -[[projects]] - branch = "master" - name = "github.com/tomasen/realip" - packages = ["."] - revision = "b5850897b7b539a1c9f22cdaa3b547d1bd453db8" - [[projects]] branch = "master" name = "golang.org/x/crypto" @@ -158,6 +152,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "0cb64ea4a8054f26b80103c60380d800d74bdd2faec4ef53453874922e7de748" + inputs-digest = "f030af166f90ac96d133b7ddf9f1f80785508462cac8cae4c65abbdb7c20474b" solver-name = "gps-cdcl" solver-version = 1 diff --git a/Gopkg.toml b/Gopkg.toml index b8ae7d74..42d25673 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -45,10 +45,6 @@ name = "github.com/tdewolff/minify" version = "2.3.3" -[[constraint]] - branch = "master" - name = "github.com/tomasen/realip" - [[constraint]] branch = "master" name = "golang.org/x/crypto" diff --git a/http/request/request.go b/http/request/request.go index da79a3ff..1de6f736 100644 --- a/http/request/request.go +++ b/http/request/request.go @@ -6,8 +6,10 @@ package request import ( "fmt" + "net" "net/http" "strconv" + "strings" "github.com/gorilla/mux" ) @@ -88,3 +90,30 @@ func HasQueryParam(r *http.Request, param string) bool { _, ok := values[param] return ok } + +// RealIP returns client's real IP address. +func RealIP(r *http.Request) string { + headers := []string{"X-Forwarded-For", "X-Real-Ip"} + for _, header := range headers { + value := r.Header.Get(header) + + if value != "" { + addresses := strings.Split(value, ",") + address := strings.TrimSpace(addresses[0]) + + if net.ParseIP(address) != nil { + return address + } + } + } + + // Fallback to TCP/IP source IP address. + var remoteIP string + if strings.ContainsRune(r.RemoteAddr, ':') { + remoteIP, _, _ = net.SplitHostPort(r.RemoteAddr) + } else { + remoteIP = r.RemoteAddr + } + + return remoteIP +} diff --git a/http/request/request_test.go b/http/request/request_test.go new file mode 100644 index 00000000..a4aaabf6 --- /dev/null +++ b/http/request/request_test.go @@ -0,0 +1,82 @@ +// Copyright 2018 Frédéric Guillot. All rights reserved. +// Use of this source code is governed by the Apache 2.0 +// license that can be found in the LICENSE file. + +package request + +import ( + "net/http" + "testing" +) + +func TestRealIPWithoutHeaders(t *testing.T) { + r := &http.Request{RemoteAddr: "192.168.0.1:4242"} + if ip := RealIP(r); ip != "192.168.0.1" { + t.Fatalf(`Unexpected result, got: %q`, ip) + } + + r = &http.Request{RemoteAddr: "192.168.0.1"} + if ip := RealIP(r); ip != "192.168.0.1" { + t.Fatalf(`Unexpected result, got: %q`, ip) + } +} + +func TestRealIPWithXFFHeader(t *testing.T) { + // Test with multiple IPv4 addresses. + headers := http.Header{} + headers.Set("X-Forwarded-For", "203.0.113.195, 70.41.3.18, 150.172.238.178") + r := &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers} + + if ip := RealIP(r); ip != "203.0.113.195" { + t.Fatalf(`Unexpected result, got: %q`, ip) + } + + // Test with single IPv6 address. + headers = http.Header{} + headers.Set("X-Forwarded-For", "2001:db8:85a3:8d3:1319:8a2e:370:7348") + r = &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers} + + if ip := RealIP(r); ip != "2001:db8:85a3:8d3:1319:8a2e:370:7348" { + t.Fatalf(`Unexpected result, got: %q`, ip) + } + + // Test with single IPv4 address. + headers = http.Header{} + headers.Set("X-Forwarded-For", "70.41.3.18") + r = &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers} + + if ip := RealIP(r); ip != "70.41.3.18" { + t.Fatalf(`Unexpected result, got: %q`, ip) + } + + // Test with invalid IP address. + headers = http.Header{} + headers.Set("X-Forwarded-For", "fake IP") + r = &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers} + + if ip := RealIP(r); ip != "192.168.0.1" { + t.Fatalf(`Unexpected result, got: %q`, ip) + } +} + +func TestRealIPWithXRealIPHeader(t *testing.T) { + headers := http.Header{} + headers.Set("X-Real-Ip", "192.168.122.1") + r := &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers} + + if ip := RealIP(r); ip != "192.168.122.1" { + t.Fatalf(`Unexpected result, got: %q`, ip) + } +} + +func TestRealIPWithBothHeaders(t *testing.T) { + headers := http.Header{} + headers.Set("X-Forwarded-For", "203.0.113.195, 70.41.3.18, 150.172.238.178") + headers.Set("X-Real-Ip", "192.168.122.1") + + r := &http.Request{RemoteAddr: "192.168.0.1:4242", Header: headers} + + if ip := RealIP(r); ip != "203.0.113.195" { + t.Fatalf(`Unexpected result, got: %q`, ip) + } +} diff --git a/middleware/logging.go b/middleware/logging.go index a6c141b5..48158b43 100644 --- a/middleware/logging.go +++ b/middleware/logging.go @@ -7,15 +7,14 @@ package middleware import ( "net/http" + "github.com/miniflux/miniflux/http/request" "github.com/miniflux/miniflux/logger" - - "github.com/tomasen/realip" ) // Logging logs the HTTP request. func (m *Middleware) Logging(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - logger.Debug("[HTTP] %s %s %s", realip.RealIP(r), r.Method, r.RequestURI) + logger.Debug("[HTTP] %s %s %s", request.RealIP(r), r.Method, r.RequestURI) next.ServeHTTP(w, r) }) } diff --git a/ui/login_check.go b/ui/login_check.go index bc49c8d1..7cd63b7e 100644 --- a/ui/login_check.go +++ b/ui/login_check.go @@ -5,6 +5,7 @@ import ( "github.com/miniflux/miniflux/http/context" "github.com/miniflux/miniflux/http/cookie" + "github.com/miniflux/miniflux/http/request" "github.com/miniflux/miniflux/http/response" "github.com/miniflux/miniflux/http/response/html" "github.com/miniflux/miniflux/http/route" @@ -12,7 +13,6 @@ import ( "github.com/miniflux/miniflux/ui/form" "github.com/miniflux/miniflux/ui/session" "github.com/miniflux/miniflux/ui/view" - "github.com/tomasen/realip" ) // CheckLogin validates the username/password and redirects the user to the unread page. @@ -38,7 +38,7 @@ func (c *Controller) CheckLogin(w http.ResponseWriter, r *http.Request) { return } - sessionToken, userID, err := c.store.CreateUserSession(authForm.Username, r.UserAgent(), realip.RealIP(r)) + sessionToken, userID, err := c.store.CreateUserSession(authForm.Username, r.UserAgent(), request.RealIP(r)) if err != nil { html.ServerError(w, err) return diff --git a/ui/oauth2_callback.go b/ui/oauth2_callback.go index f564c7ef..23e379b2 100644 --- a/ui/oauth2_callback.go +++ b/ui/oauth2_callback.go @@ -16,8 +16,6 @@ import ( "github.com/miniflux/miniflux/logger" "github.com/miniflux/miniflux/model" "github.com/miniflux/miniflux/ui/session" - - "github.com/tomasen/realip" ) // OAuth2Callback receives the authorization code and create a new session. @@ -107,7 +105,7 @@ func (c *Controller) OAuth2Callback(w http.ResponseWriter, r *http.Request) { } } - sessionToken, _, err := c.store.CreateUserSession(user.Username, r.UserAgent(), realip.RealIP(r)) + sessionToken, _, err := c.store.CreateUserSession(user.Username, r.UserAgent(), request.RealIP(r)) if err != nil { html.ServerError(w, err) return diff --git a/vendor/github.com/tomasen/realip/.travis.yml b/vendor/github.com/tomasen/realip/.travis.yml deleted file mode 100644 index fdfbf875..00000000 --- a/vendor/github.com/tomasen/realip/.travis.yml +++ /dev/null @@ -1,19 +0,0 @@ -language: go - -go: - - tip - -before_install: - # lint - - go get github.com/golang/lint/golint - - # code complexity - - go get github.com/on99/gocyclo - - # test - - go get github.com/smartystreets/goconvey/convey - -script: - - golint ./... - - go vet ./... - - gocyclo -skip-godeps=true -top 10 -over 10 . # over 10 is bad code diff --git a/vendor/github.com/tomasen/realip/README.md b/vendor/github.com/tomasen/realip/README.md deleted file mode 100644 index 085f1829..00000000 --- a/vendor/github.com/tomasen/realip/README.md +++ /dev/null @@ -1,33 +0,0 @@ -# RealIP - -[![GoDoc](https://godoc.org/github.com/Tomasen/realip?status.svg)](http://godoc.org/github.com/Tomasen/realip) - -Go package that can be used to get client's real public IP, which usually useful for logging HTTP server. - -### Feature - -* Follows the rule of X-Real-IP -* Follows the rule of X-Forwarded-For -* Exclude local or private address - -## Example - -```go -package main - -import "github.com/Tomasen/realip" - -func (h *Handler) ServeIndexPage(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - clientIP := realip.FromRequest(r) - log.Println("GET / from", clientIP) -} -``` - -## Developing - -Commited code must pass: - -* [golint](https://github.com/golang/lint) -* [go vet](https://godoc.org/golang.org/x/tools/cmd/vet) -* [gofmt](https://golang.org/cmd/gofmt) -* [go test](https://golang.org/cmd/go/#hdr-Test_packages): diff --git a/vendor/github.com/tomasen/realip/realip.go b/vendor/github.com/tomasen/realip/realip.go deleted file mode 100644 index e2803a2b..00000000 --- a/vendor/github.com/tomasen/realip/realip.go +++ /dev/null @@ -1,89 +0,0 @@ -package realip - -import ( - "errors" - "net" - "net/http" - "strings" -) - -var cidrs []*net.IPNet - -func init() { - maxCidrBlocks := []string{ - "127.0.0.1/8", // localhost - "10.0.0.0/8", // 24-bit block - "172.16.0.0/12", // 20-bit block - "192.168.0.0/16", // 16-bit block - "169.254.0.0/16", // link local address - "::1/128", // localhost IPv6 - "fc00::/7", // unique local address IPv6 - "fe80::/10", // link local address IPv6 - } - - cidrs = make([]*net.IPNet, len(maxCidrBlocks)) - for i, maxCidrBlock := range maxCidrBlocks { - _, cidr, _ := net.ParseCIDR(maxCidrBlock) - cidrs[i] = cidr - } -} - -// isLocalAddress works by checking if the address is under private CIDR blocks. -// List of private CIDR blocks can be seen on : -// -// https://en.wikipedia.org/wiki/Private_network -// -// https://en.wikipedia.org/wiki/Link-local_address -func isPrivateAddress(address string) (bool, error) { - ipAddress := net.ParseIP(address) - if ipAddress == nil { - return false, errors.New("address is not valid") - } - - for i := range cidrs { - if cidrs[i].Contains(ipAddress) { - return true, nil - } - } - - return false, nil -} - -// FromRequest return client's real public IP address from http request headers. -func FromRequest(r *http.Request) string { - // Fetch header value - xRealIP := r.Header.Get("X-Real-Ip") - xForwardedFor := r.Header.Get("X-Forwarded-For") - - // If both empty, return IP from remote address - if xRealIP == "" && xForwardedFor == "" { - var remoteIP string - - // If there are colon in remote address, remove the port number - // otherwise, return remote address as is - if strings.ContainsRune(r.RemoteAddr, ':') { - remoteIP, _, _ = net.SplitHostPort(r.RemoteAddr) - } else { - remoteIP = r.RemoteAddr - } - - return remoteIP - } - - // Check list of IP in X-Forwarded-For and return the first global address - for _, address := range strings.Split(xForwardedFor, ",") { - address = strings.TrimSpace(address) - isPrivate, err := isPrivateAddress(address) - if !isPrivate && err == nil { - return address - } - } - - // If nothing succeed, return X-Real-IP - return xRealIP -} - -// RealIP is depreciated, use FromRequest instead -func RealIP(r *http.Request) string { - return FromRequest(r) -} diff --git a/vendor/github.com/tomasen/realip/realip_test.go b/vendor/github.com/tomasen/realip/realip_test.go deleted file mode 100644 index e80efe0b..00000000 --- a/vendor/github.com/tomasen/realip/realip_test.go +++ /dev/null @@ -1,95 +0,0 @@ -package realip - -import ( - "net/http" - "testing" -) - -func TestIsPrivateAddr(t *testing.T) { - testData := map[string]bool{ - "127.0.0.0": true, - "10.0.0.0": true, - "169.254.0.0": true, - "192.168.0.0": true, - "::1": true, - "fc00::": true, - - "172.15.0.0": false, - "172.16.0.0": true, - "172.31.0.0": true, - "172.32.0.0": false, - - "147.12.56.11": false, - } - - for addr, isLocal := range testData { - isPrivate, err := isPrivateAddress(addr) - if err != nil { - t.Errorf("fail processing %s: %v", addr, err) - } - - if isPrivate != isLocal { - format := "%s should " - if !isLocal { - format += "not " - } - format += "be local address" - - t.Errorf(format, addr) - } - } -} - -func TestRealIP(t *testing.T) { - // Create type and function for testing - type testIP struct { - name string - request *http.Request - expected string - } - - newRequest := func(remoteAddr, xRealIP string, xForwardedFor ...string) *http.Request { - h := http.Header{} - h.Set("X-Real-IP", xRealIP) - for _, address := range xForwardedFor { - h.Set("X-Forwarded-For", address) - } - - return &http.Request{ - RemoteAddr: remoteAddr, - Header: h, - } - } - - // Create test data - publicAddr1 := "144.12.54.87" - publicAddr2 := "119.14.55.11" - localAddr := "127.0.0.0" - - testData := []testIP{ - { - name: "No header", - request: newRequest(publicAddr1, ""), - expected: publicAddr1, - }, { - name: "Has X-Forwarded-For", - request: newRequest("", "", publicAddr1), - expected: publicAddr1, - }, { - name: "Has multiple X-Forwarded-For", - request: newRequest("", "", localAddr, publicAddr1, publicAddr2), - expected: publicAddr2, - }, { - name: "Has X-Real-IP", - request: newRequest("", publicAddr1), - expected: publicAddr1, - }, - } - - // Run test - for _, v := range testData { - if actual := FromRequest(v.request); v.expected != actual { - t.Errorf("%s: expected %s but get %s", v.name, v.expected, actual) - } - } -}