diff --git a/server/message_cache_test.go b/server/message_cache_test.go index ea9580a..c72debc 100644 --- a/server/message_cache_test.go +++ b/server/message_cache_test.go @@ -12,6 +12,10 @@ import ( "github.com/stretchr/testify/require" ) +var ( + exampleIP1234 = netip.MustParseAddr("1.2.3.4") +) + func TestSqliteCache_Messages(t *testing.T) { testCacheMessages(t, newSqliteTestCache(t)) } @@ -283,7 +287,7 @@ func testCacheAttachments(t *testing.T, c *messageCache) { expires1 := time.Now().Add(-4 * time.Hour).Unix() m := newDefaultMessage("mytopic", "flower for you") m.ID = "m1" - m.Sender = netip.MustParseAddr("1.2.3.4") + m.Sender = exampleIP1234 m.Attachment = &attachment{ Name: "flower.jpg", Type: "image/jpeg", @@ -296,7 +300,7 @@ func testCacheAttachments(t *testing.T, c *messageCache) { expires2 := time.Now().Add(2 * time.Hour).Unix() // Future m = newDefaultMessage("mytopic", "sending you a car") m.ID = "m2" - m.Sender = netip.MustParseAddr("1.2.3.4") + m.Sender = exampleIP1234 m.Attachment = &attachment{ Name: "car.jpg", Type: "image/jpeg", @@ -309,7 +313,7 @@ func testCacheAttachments(t *testing.T, c *messageCache) { expires3 := time.Now().Add(1 * time.Hour).Unix() // Future m = newDefaultMessage("another-topic", "sending you another car") m.ID = "m3" - m.Sender = netip.MustParseAddr("1.2.3.4") + m.Sender = exampleIP1234 m.Attachment = &attachment{ Name: "another-car.jpg", Type: "image/jpeg", @@ -329,7 +333,7 @@ func testCacheAttachments(t *testing.T, c *messageCache) { require.Equal(t, int64(5000), messages[0].Attachment.Size) require.Equal(t, expires1, messages[0].Attachment.Expires) require.Equal(t, "https://ntfy.sh/file/AbDeFgJhal.jpg", messages[0].Attachment.URL) - require.Equal(t, "1.2.3.4", messages[0].Sender) + require.Equal(t, "1.2.3.4", messages[0].Sender.String()) require.Equal(t, "sending you a car", messages[1].Message) require.Equal(t, "car.jpg", messages[1].Attachment.Name) @@ -337,7 +341,7 @@ func testCacheAttachments(t *testing.T, c *messageCache) { require.Equal(t, int64(10000), messages[1].Attachment.Size) require.Equal(t, expires2, messages[1].Attachment.Expires) require.Equal(t, "https://ntfy.sh/file/aCaRURL.jpg", messages[1].Attachment.URL) - require.Equal(t, "1.2.3.4", messages[1].Sender) + require.Equal(t, "1.2.3.4", messages[1].Sender.String()) size, err := c.AttachmentBytesUsed("1.2.3.4") require.Nil(t, err) diff --git a/server/server.go b/server/server.go index 6b80173..a4ef193 100644 --- a/server/server.go +++ b/server/server.go @@ -1440,7 +1440,12 @@ func (s *Server) visitor(r *http.Request) *visitor { ipport, err := netip.ParseAddrPort(remoteAddr) ip := ipport.Addr() if err != nil { - ip = netip.MustParseAddr(remoteAddr) // This should not happen in real life; only in tests. So, using MustParse, which panics on error. + // This should not happen in real life; only in tests. So, using falling back to 0.0.0.0 if address unspecified + ip, err = netip.ParseAddr(remoteAddr) + if err != nil { + ip = netip.IPv4Unspecified() + log.Error("Unable to parse IP (%s), new visitor with unspecified IP (0.0.0.0) created %s", remoteAddr, err) + } } 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, diff --git a/server/server_test.go b/server/server_test.go index 5a3dcc8..a9c6ba6 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -294,13 +294,13 @@ func TestServer_PublishAt(t *testing.T) { messages = toMessages(t, response.Body.String()) require.Equal(t, 1, len(messages)) require.Equal(t, "a message", messages[0].Message) - require.Equal(t, "", messages[0].Sender) // Never return the sender! + require.Equal(t, netip.Addr{}, messages[0].Sender) // Never return the sender! messages, err := s.messageCache.Messages("mytopic", sinceAllMessages, true) require.Nil(t, err) require.Equal(t, 1, len(messages)) require.Equal(t, "a message", messages[0].Message) - require.Equal(t, "9.9.9.9", messages[0].Sender) // It's stored in the DB though! + require.Equal(t, "9.9.9.9", messages[0].Sender.String()) // It's stored in the DB though! } func TestServer_PublishAtWithCacheError(t *testing.T) { @@ -1134,7 +1134,7 @@ func TestServer_PublishAttachment(t *testing.T) { require.Equal(t, int64(5000), msg.Attachment.Size) require.GreaterOrEqual(t, msg.Attachment.Expires, time.Now().Add(179*time.Minute).Unix()) // Almost 3 hours require.Contains(t, msg.Attachment.URL, "http://127.0.0.1:12345/file/") - require.Equal(t, "", msg.Sender) // Should never be returned + require.Equal(t, netip.Addr{}, msg.Sender) // Should never be returned require.FileExists(t, filepath.Join(s.config.AttachmentCacheDir, msg.ID)) // GET @@ -1170,7 +1170,7 @@ func TestServer_PublishAttachmentShortWithFilename(t *testing.T) { require.Equal(t, int64(21), msg.Attachment.Size) require.GreaterOrEqual(t, msg.Attachment.Expires, time.Now().Add(3*time.Hour).Unix()) require.Contains(t, msg.Attachment.URL, "http://127.0.0.1:12345/file/") - require.Equal(t, "", msg.Sender) // Should never be returned + require.Equal(t, netip.Addr{}, msg.Sender) // Should never be returned require.FileExists(t, filepath.Join(s.config.AttachmentCacheDir, msg.ID)) path := strings.TrimPrefix(msg.Attachment.URL, "http://127.0.0.1:12345") @@ -1197,7 +1197,7 @@ func TestServer_PublishAttachmentExternalWithoutFilename(t *testing.T) { require.Equal(t, "", msg.Attachment.Type) require.Equal(t, int64(0), msg.Attachment.Size) require.Equal(t, int64(0), msg.Attachment.Expires) - require.Equal(t, "", msg.Sender) + require.Equal(t, netip.Addr{}, msg.Sender) // Slightly unrelated cross-test: make sure we don't add an owner for external attachments size, err := s.messageCache.AttachmentBytesUsed("127.0.0.1") @@ -1218,7 +1218,7 @@ func TestServer_PublishAttachmentExternalWithFilename(t *testing.T) { require.Equal(t, "", msg.Attachment.Type) require.Equal(t, int64(0), msg.Attachment.Size) require.Equal(t, int64(0), msg.Attachment.Expires) - require.Equal(t, "", msg.Sender) + require.Equal(t, netip.Addr{}, msg.Sender) } func TestServer_PublishAttachmentBadURL(t *testing.T) { @@ -1393,7 +1393,7 @@ func TestServer_Visitor_XForwardedFor_None(t *testing.T) { 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) + require.Equal(t, "8.9.10.11", v.ip.String()) } func TestServer_Visitor_XForwardedFor_Single(t *testing.T) { @@ -1404,7 +1404,7 @@ func TestServer_Visitor_XForwardedFor_Single(t *testing.T) { 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) + require.Equal(t, "1.1.1.1", v.ip.String()) } func TestServer_Visitor_XForwardedFor_Multiple(t *testing.T) { @@ -1415,7 +1415,7 @@ func TestServer_Visitor_XForwardedFor_Multiple(t *testing.T) { 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) + require.Equal(t, "234.5.2.1", v.ip.String()) } func TestServer_PublishWhileUpdatingStatsWithLotsOfMessages(t *testing.T) {