From d5052d79e60269d470de24e1f8e780304c3d45b3 Mon Sep 17 00:00:00 2001 From: binwiederhier Date: Fri, 24 Feb 2023 21:10:41 -0500 Subject: [PATCH] Add up* length requirement --- server/errors.go | 3 +++ server/server.go | 13 +++++++---- server/server_test.go | 54 +++++++++++++++++++++++++++---------------- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/server/errors.go b/server/errors.go index a00105c3..5b0797b5 100644 --- a/server/errors.go +++ b/server/errors.go @@ -93,3 +93,6 @@ var ( errHTTPInternalErrorInvalidPath = &errHTTP{50002, http.StatusInternalServerError, "internal server error: invalid path", ""} errHTTPInternalErrorMissingBaseURL = &errHTTP{50003, http.StatusInternalServerError, "internal server error: base-url must be be configured for this feature", "https://ntfy.sh/docs/config/"} ) + +// errHTTPConflictCannotPublishWithoutRateVisitor = &errHTTP{40904, http.StatusConflict, "conflict: cannot publish to UnifiedPush topic without active subscriber", ""} + diff --git a/server/server.go b/server/server.go index 87a7093b..eb1ea91e 100644 --- a/server/server.go +++ b/server/server.go @@ -112,6 +112,7 @@ const ( encodingBase64 = "base64" // Used mainly for binary UnifiedPush messages jsonBodyBytesLimit = 16384 unifiedPushTopicPrefix = "up" // Temporarily, we rate limit all "up*" topics based on the subscriber + unifiedPushTopicLength = 14 ) // WebSocket constants @@ -571,9 +572,6 @@ func (s *Server) handleMatrixDiscovery(w http.ResponseWriter) error { func (s *Server) handlePublishWithoutResponse(r *http.Request, v *visitor) (*message, error) { t := fromContext[topic](r, contextTopic) vrate := fromContext[visitor](r, contextRateVisitor) - if !vrate.MessageAllowed() { - return nil, errHTTPTooManyRequestsLimitMessages - } body, err := util.Peek(r.Body, s.config.MessageLimit) if err != nil { return nil, err @@ -583,7 +581,12 @@ func (s *Server) handlePublishWithoutResponse(r *http.Request, v *visitor) (*mes if err != nil { return nil, err } - if email != "" && !vrate.EmailAllowed() { + /*if unifiedpush && t.RateVisitor() == nil { + return nil, errHTTPConflictCannotPublishWithoutRateVisitor + } else*/ + if !util.ContainsIP(s.config.VisitorRequestExemptIPAddrs, v.ip) && !vrate.MessageAllowed() { + return nil, errHTTPTooManyRequestsLimitMessages + } else if email != "" && !vrate.EmailAllowed() { return nil, errHTTPTooManyRequestsLimitEmails } if m.PollID != "" { @@ -1173,7 +1176,7 @@ func (s *Server) maybeSetRateVisitors(r *http.Request, v *visitor, topics []*top // Make a list of topics that we'll actually set the RateVisitor on eligibleRateTopics := make([]*topic, 0) for _, t := range topics { - if strings.HasPrefix(t.ID, unifiedPushTopicPrefix) || util.Contains(rateTopics, t.ID) { + if (strings.HasPrefix(t.ID, unifiedPushTopicPrefix) && len(t.ID) == unifiedPushTopicLength) || util.Contains(rateTopics, t.ID) { eligibleRateTopics = append(eligibleRateTopics, t) } } diff --git a/server/server_test.go b/server/server_test.go index 24525849..87034252 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -1028,14 +1028,27 @@ func TestServer_PublishTooRequests_Defaults(t *testing.T) { func TestServer_PublishTooRequests_Defaults_ExemptHosts(t *testing.T) { c := newTestConfig(t) + c.VisitorRequestLimitBurst = 3 c.VisitorRequestExemptIPAddrs = []netip.Prefix{netip.MustParsePrefix("9.9.9.9/32")} // see request() s := newTestServer(t, c) - for i := 0; i < 65; i++ { // > 60 + for i := 0; i < 5; i++ { // > 3 response := request(t, s, "PUT", "/mytopic", fmt.Sprintf("message %d", i), nil) require.Equal(t, 200, response.Code) } } +func TestServer_PublishTooRequests_Defaults_ExemptHosts_MessageDailyLimit(t *testing.T) { + c := newTestConfig(t) + c.VisitorRequestLimitBurst = 10 + c.VisitorMessageDailyLimit = 4 + c.VisitorRequestExemptIPAddrs = []netip.Prefix{netip.MustParsePrefix("9.9.9.9/32")} // see request() + s := newTestServer(t, c) + for i := 0; i < 8; i++ { // 4 + response := request(t, s, "PUT", "/mytopic", "message", nil) + require.Equal(t, 200, response.Code) + } +} + func TestServer_PublishTooRequests_ShortReplenish(t *testing.T) { c := newTestConfig(t) c.VisitorRequestLimitBurst = 60 @@ -1127,7 +1140,8 @@ func TestServer_PublishUnifiedPushBinary_AndPoll(t *testing.T) { require.Nil(t, err) s := newTestServer(t, newTestConfig(t)) - response := request(t, s, "PUT", "/mytopic?up=1", string(b), nil) + + response := request(t, s, "PUT", "/up123456789012?up=1", string(b), nil) require.Equal(t, 200, response.Code) m := toMessage(t, response.Body.String()) @@ -1136,7 +1150,7 @@ func TestServer_PublishUnifiedPushBinary_AndPoll(t *testing.T) { require.Nil(t, err) require.Equal(t, b, b2) - response = request(t, s, "GET", "/mytopic/json?poll=1", string(b), nil) + response = request(t, s, "GET", "/up123456789012/json?poll=1", string(b), nil) require.Equal(t, 200, response.Code) m = toMessage(t, response.Body.String()) require.Equal(t, "base64", m.Encoding) @@ -1164,6 +1178,7 @@ func TestServer_PublishUnifiedPushBinary_Truncated(t *testing.T) { func TestServer_PublishUnifiedPushText(t *testing.T) { s := newTestServer(t, newTestConfig(t)) + response := request(t, s, "PUT", "/mytopic?up=1", "this is a unifiedpush text message", nil) require.Equal(t, 200, response.Code) @@ -1281,7 +1296,7 @@ func TestServer_PublishAsJSON(t *testing.T) { require.True(t, m.Time < time.Now().Unix()+31*60) } -func TestServer_PublishAsJSON_RateLimit(t *testing.T) { +func TestServer_PublishAsJSON_RateLimit_MessageDailyLimit(t *testing.T) { // Publishing as JSON follows a different path. This ensures that rate // limiting works for this endpoint as well c := newTestConfig(t) @@ -1926,7 +1941,7 @@ func TestServer_AnonymousUser_And_NonTierUser_Are_Same_Visitor(t *testing.T) { require.Equal(t, int64(2), account.Stats.Messages) } -func TestServer_SubscriberRateLimiting(t *testing.T) { +func TestServer_SubscriberRateLimiting_Success(t *testing.T) { c := newTestConfigWithAuthFile(t) c.VisitorRequestLimitBurst = 3 s := newTestServer(t, c) @@ -1941,11 +1956,11 @@ func TestServer_SubscriberRateLimiting(t *testing.T) { require.Equal(t, 200, rr.Code) require.Equal(t, "", rr.Body.String()) - // "Register" visitor 8.7.7.1 to topic "upSUB2topic" as a rate limit visitor (implicitly via topic name) + // "Register" visitor 8.7.7.1 to topic "up012345678912" as a rate limit visitor (implicitly via topic name) subscriber2Fn := func(r *http.Request) { r.RemoteAddr = "8.7.7.1" } - rr = request(t, s, "GET", "/upSUB2topic/json?poll=1", "", nil, subscriber2Fn) + rr = request(t, s, "GET", "/up012345678912/json?poll=1", "", nil, subscriber2Fn) require.Equal(t, 200, rr.Code) require.Equal(t, "", rr.Body.String()) @@ -1958,12 +1973,12 @@ func TestServer_SubscriberRateLimiting(t *testing.T) { rr = request(t, s, "PUT", "/subscriber1topic", "some message", nil) require.Equal(t, 429, rr.Code) - // Publish another 2 messages to "upSUB2topic" as visitor 9.9.9.9 + // Publish another 2 messages to "up012345678912" as visitor 9.9.9.9 for i := 0; i < 2; i++ { - rr := request(t, s, "PUT", "/upSUB2topic", "some message", nil) + rr := request(t, s, "PUT", "/up012345678912", "some message", nil) require.Equal(t, 200, rr.Code) // If we fail here, handlePublish is using the wrong visitor! } - rr = request(t, s, "PUT", "/upSUB2topic", "some message", nil) + rr = request(t, s, "PUT", "/up012345678912", "some message", nil) require.Equal(t, 429, rr.Code) // Hurray! At this point, visitor 9.9.9.9 has published 4 messages, even though @@ -1989,14 +2004,14 @@ func TestServer_SubscriberRateLimiting_UP_Only(t *testing.T) { subscriberFn := func(r *http.Request) { r.RemoteAddr = fmt.Sprintf("1.2.3.%d", i+1) } - rr := request(t, s, "GET", fmt.Sprintf("/upsomething%d/json?poll=1", i), "", nil, subscriberFn) + rr := request(t, s, "GET", fmt.Sprintf("/up12345678901%d/json?poll=1", i), "", nil, subscriberFn) require.Equal(t, 200, rr.Code) } // Publish 2 messages per topic for i := 0; i < 5; i++ { for j := 0; j < 2; j++ { - rr := request(t, s, "PUT", fmt.Sprintf("/upsomething%d?up=1", i), "some message", nil) + rr := request(t, s, "PUT", fmt.Sprintf("/up12345678901%d?up=1", i), "some message", nil) require.Equal(t, 200, rr.Code) } } @@ -2009,16 +2024,15 @@ func TestServer_Matrix_SubscriberRateLimiting_UP_Only(t *testing.T) { // "Register" 5 different UnifiedPush visitors for i := 0; i < 5; i++ { - subscriberFn := func(r *http.Request) { + rr := request(t, s, "GET", fmt.Sprintf("/up12345678901%d/json?poll=1", i), "", nil, func(r *http.Request) { r.RemoteAddr = fmt.Sprintf("1.2.3.%d", i+1) - } - rr := request(t, s, "GET", fmt.Sprintf("/upsomething%d/json?poll=1", i), "", nil, subscriberFn) + }) require.Equal(t, 200, rr.Code) } // Publish 2 messages per topic for i := 0; i < 5; i++ { - notification := fmt.Sprintf(`{"notification":{"devices":[{"pushkey":"http://127.0.0.1:12345/upsomething%d?up=1"}]}}`, i) + notification := fmt.Sprintf(`{"notification":{"devices":[{"pushkey":"http://127.0.0.1:12345/up12345678901%d?up=1"}]}}`, i) for j := 0; j < 2; j++ { response := request(t, s, "POST", "/_matrix/push/v1/notify", notification, nil) require.Equal(t, 200, response.Code) @@ -2026,7 +2040,7 @@ func TestServer_Matrix_SubscriberRateLimiting_UP_Only(t *testing.T) { } response := request(t, s, "POST", "/_matrix/push/v1/notify", notification, nil) require.Equal(t, 429, response.Code, notification) - require.Equal(t, fmt.Sprintf(`{"rejected":["http://127.0.0.1:12345/upsomething%d?up=1"]}`+"\n", i), response.Body.String()) + require.Equal(t, fmt.Sprintf(`{"rejected":["http://127.0.0.1:12345/up12345678901%d?up=1"]}`+"\n", i), response.Body.String()) } } @@ -2121,13 +2135,13 @@ func TestServer_SubscriberRateLimiting_ProtectedTopics_WithDefaultReadWrite(t *t require.Nil(t, s.userManager.AllowAccess(user.Everyone, "announcements", user.PermissionRead)) // Set rate visitor as ip:1.2.3.4 on topic - // - "up1234": Allowed, because no ACLs and nobody owns the topic + // - "up123456789012": Allowed, because no ACLs and nobody owns the topic // - "announcements": NOT allowed, because it has read-only permissions for everyone - rr := request(t, s, "GET", "/up1234,announcements/json?poll=1", "", nil, func(r *http.Request) { + rr := request(t, s, "GET", "/up123456789012,announcements/json?poll=1", "", nil, func(r *http.Request) { r.RemoteAddr = "1.2.3.4" }) require.Equal(t, 200, rr.Code) - require.Equal(t, "1.2.3.4", s.topics["up1234"].rateVisitor.ip.String()) + require.Equal(t, "1.2.3.4", s.topics["up123456789012"].rateVisitor.ip.String()) require.Nil(t, s.topics["announcements"].rateVisitor) }