Only use last X-Forwarded-For address as visitor address, closes #328

docs/releases.md

@@ -23,6 +23,7 @@ and the [ntfy Android app](https://github.com/binwiederhier/ntfy-android/release
 **Bugs:**
 
 * Web app: Show "notifications not supported" alert on HTTP ([#323](https://github.com/binwiederhier/ntfy/issues/323), thanks to [@milksteakjellybeans](https://github.com/milksteakjellybeans) for reporting)
+* Use last address in `X-Forwarded-For` header as visitor address ([#328](https://github.com/binwiederhier/ntfy/issues/328))
 
 **Documentation**
 

server/server.go

@@ -1382,8 +1382,12 @@ func (s *Server) visitor(r *http.Request) *visitor {
 	if err != nil {
 		ip = remoteAddr // This should not happen in real life; only in tests.
 	}
-	if s.config.BehindProxy && r.Header.Get("X-Forwarded-For") != "" {
+	if s.config.BehindProxy && strings.TrimSpace(r.Header.Get("X-Forwarded-For")) != "" {
-		ip = r.Header.Get("X-Forwarded-For")
+		// X-Forwarded-For can contain multiple addresses (see #328). If we are behind a proxy,
+		// only the right-most address can be trusted (as this is the one added by our proxy server).
+		// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For for details.
+		ips := util.SplitNoEmpty(r.Header.Get("X-Forwarded-For"), ",")
+		ip = strings.TrimSpace(util.LastString(ips, remoteAddr))
 	}
 	return s.visitorFromIP(ip)
 }

server/server_test.go

@@ -1319,6 +1319,39 @@ func TestServer_PublishAttachmentUserStats(t *testing.T) {
 	require.Equal(t, int64(1001), stats.VisitorAttachmentBytesRemaining)
 }
 
+func TestServer_Visitor_XForwardedFor_None(t *testing.T) {
+	c := newTestConfig(t)
+	c.BehindProxy = true
+	s := newTestServer(t, c)
+	r, _ := http.NewRequest("GET", "/bla", nil)
+	r.RemoteAddr = "8.9.10.11"
+	r.Header.Set("X-Forwarded-For", "  ") // Spaces, not empty!
+	v := s.visitor(r)
+	require.Equal(t, "8.9.10.11", v.ip)
+}
+
+func TestServer_Visitor_XForwardedFor_Single(t *testing.T) {
+	c := newTestConfig(t)
+	c.BehindProxy = true
+	s := newTestServer(t, c)
+	r, _ := http.NewRequest("GET", "/bla", nil)
+	r.RemoteAddr = "8.9.10.11"
+	r.Header.Set("X-Forwarded-For", "1.1.1.1")
+	v := s.visitor(r)
+	require.Equal(t, "1.1.1.1", v.ip)
+}
+
+func TestServer_Visitor_XForwardedFor_Multiple(t *testing.T) {
+	c := newTestConfig(t)
+	c.BehindProxy = true
+	s := newTestServer(t, c)
+	r, _ := http.NewRequest("GET", "/bla", nil)
+	r.RemoteAddr = "8.9.10.11"
+	r.Header.Set("X-Forwarded-For", "1.2.3.4 , 2.4.4.2,234.5.2.1 ")
+	v := s.visitor(r)
+	require.Equal(t, "234.5.2.1", v.ip)
+}
+
 func newTestConfig(t *testing.T) *Config {
 	conf := NewConfig()
 	conf.BaseURL = "http://127.0.0.1:12345"

util/util.go

@@ -88,6 +88,14 @@ func SplitKV(s string, sep string) (key string, value string) {
 	return "", strings.TrimSpace(kv[0])
 }
 
+// LastString returns the last string in a slice, or def if s is empty
+func LastString(s []string, def string) string {
+	if len(s) == 0 {
+		return def
+	}
+	return s[len(s)-1]
+}
+
 // RandomString returns a random string with a given length
 func RandomString(length int) string {
 	randomMutex.Lock() // Who would have thought that random.Intn() is not thread-safe?!

util/util_test.go

@@ -157,3 +157,8 @@ func TestSplitKV(t *testing.T) {
 	require.Equal(t, "mykey", key)
 	require.Equal(t, "value=with=separator", value)
 }
+
+func TestLastString(t *testing.T) {
+	require.Equal(t, "last", LastString([]string{"first", "second", "last"}, "default"))
+	require.Equal(t, "default", LastString([]string{}, "default"))
+}