diff --git a/docs/releases.md b/docs/releases.md index 9150251a..a5961207 100644 --- a/docs/releases.md +++ b/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** diff --git a/server/server.go b/server/server.go index 21a6c584..37744fec 100644 --- a/server/server.go +++ b/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") != "" { - ip = r.Header.Get("X-Forwarded-For") + if s.config.BehindProxy && strings.TrimSpace(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) } diff --git a/server/server_test.go b/server/server_test.go index 55125c3e..6c58a871 100644 --- a/server/server_test.go +++ b/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" diff --git a/util/util.go b/util/util.go index c8a42347..e9e23ff9 100644 --- a/util/util.go +++ b/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?! diff --git a/util/util_test.go b/util/util_test.go index 508e96bf..836cf4b5 100644 --- a/util/util_test.go +++ b/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")) +}