From 57e1104afb0dad14db55cfdcfb7b26cf7dbb4f05 Mon Sep 17 00:00:00 2001
From: binwiederhier <pheckel@datto.com>
Date: Thu, 23 Feb 2023 15:38:45 -0500
Subject: [PATCH] Ensure we return 429s for Matrix endpoints too; return proper
error codes
---
server/server.go | 4 ++--
server/server_matrix.go | 5 +++++
server/server_matrix_test.go | 2 +-
server/server_test.go | 23 +++++++++++++++++++----
4 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/server/server.go b/server/server.go
index 57f084f6..0466cd23 100644
--- a/server/server.go
+++ b/server/server.go
@@ -405,7 +405,6 @@ func (s *Server) handleInternal(w http.ResponseWriter, r *http.Request, v *visit
} else if r.Method == http.MethodOptions {
return s.limitRequests(s.handleOptions)(w, r, v) // Should work even if the web app is not enabled, see #598
} else if (r.Method == http.MethodPut || r.Method == http.MethodPost) && r.URL.Path == "/" {
- // So I don't *really* have to switch this order, since this is unrelated to UP; But, making this and matrix inconsistent is just calling for confusion, no?
return s.transformBodyJSON(s.limitRequestsWithTopic(s.authorizeTopicWrite(s.handlePublish)))(w, r, v)
} else if r.Method == http.MethodPost && r.URL.Path == matrixPushPath {
return s.transformMatrixJSON(s.limitRequestsWithTopic(s.authorizeTopicWrite(s.handlePublishMatrix)))(w, r, v)
@@ -1487,10 +1486,11 @@ func (s *Server) transformMatrixJSON(next handleFunc) handleFunc {
return func(w http.ResponseWriter, r *http.Request, v *visitor) error {
newRequest, err := newRequestFromMatrixJSON(r, s.config.BaseURL, s.config.MessageLimit)
if err != nil {
- logvr(v, r).Tag(tagMatrix).Err(err).Trace("Invalid Matrix request")
+ logvr(v, r).Tag(tagMatrix).Err(err).Debug("Invalid Matrix request")
return err
}
if err := next(w, newRequest, v); err != nil {
+ logvr(v, r).Tag(tagMatrix).Err(err).Debug("Error handling Matrix request")
return &errMatrix{pushKey: newRequest.Header.Get(matrixPushKeyHeader), err: err}
}
return nil
diff --git a/server/server_matrix.go b/server/server_matrix.go
index 28ca7337..2ec77546 100644
--- a/server/server_matrix.go
+++ b/server/server_matrix.go
@@ -147,6 +147,11 @@ func writeMatrixDiscoveryResponse(w http.ResponseWriter) error {
// writeMatrixError logs and writes the errMatrix to the given http.ResponseWriter as a matrixResponse
func writeMatrixError(w http.ResponseWriter, r *http.Request, v *visitor, err *errMatrix) error {
logvr(v, r).Tag(tagMatrix).Err(err).Debug("Matrix gateway error")
+ if httpErr, ok := err.err.(*errHTTP); ok {
+ w.Header().Set("X-Ntfy-Error-Code", fmt.Sprintf("%d", httpErr.Code))
+ w.Header().Set("X-Ntfy-Error-Message", httpErr.Message)
+ w.WriteHeader(httpErr.HTTPCode)
+ }
return writeMatrixResponse(w, err.pushKey)
}
diff --git a/server/server_matrix_test.go b/server/server_matrix_test.go
index 883a8c1f..73a4460c 100644
--- a/server/server_matrix_test.go
+++ b/server/server_matrix_test.go
@@ -74,7 +74,7 @@ func TestMatrix_WriteMatrixError(t *testing.T) {
r, _ := http.NewRequest("POST", "http://ntfy.example.com/_matrix/push/v1/notify", nil)
v := newVisitor(newTestConfig(t), nil, nil, netip.MustParseAddr("1.2.3.4"), nil)
require.Nil(t, writeMatrixError(w, r, v, &errMatrix{"https://ntfy.example.com/upABCDEFGHI?up=1", errHTTPBadRequestMatrixPushkeyBaseURLMismatch}))
- require.Equal(t, 200, w.Result().StatusCode)
+ require.Equal(t, 400, w.Result().StatusCode)
require.Equal(t, `{"rejected":["https://ntfy.example.com/upABCDEFGHI?up=1"]}`+"\n", w.Body.String())
}
diff --git a/server/server_test.go b/server/server_test.go
index 97762545..b26cf780 100644
--- a/server/server_test.go
+++ b/server/server_test.go
@@ -1206,8 +1206,10 @@ func TestServer_MatrixGateway_Push_Failure_InvalidPushkey(t *testing.T) {
s := newTestServer(t, newTestConfig(t))
notification := `{"notification":{"devices":[{"pushkey":"http://wrong-base-url.com/mytopic?up=1"}]}}`
response := request(t, s, "POST", "/_matrix/push/v1/notify", notification, nil)
- require.Equal(t, 200, response.Code)
+ require.Equal(t, 400, response.Code)
require.Equal(t, `{"rejected":["http://wrong-base-url.com/mytopic?up=1"]}`+"\n", response.Body.String())
+ require.Equal(t, "40020", response.Header().Get("X-Ntfy-Error-Code"))
+ require.Equal(t, "invalid request: push key must be prefixed with base URL, received push key: http://wrong-base-url.com/mytopic?up=1, configured base URL: http://127.0.0.1:12345", response.Header().Get("X-Ntfy-Error-Message"))
response = request(t, s, "GET", "/mytopic/json?poll=1", "", nil)
require.Equal(t, 200, response.Code)
@@ -1279,6 +1281,22 @@ func TestServer_PublishAsJSON(t *testing.T) {
require.True(t, m.Time < time.Now().Unix()+31*60)
}
+func TestServer_PublishAsJSON_RateLimit(t *testing.T) {
+ // Publishing as JSON follows a different path. This ensures that rate
+ // limiting works for this endpoint as well
+ c := newTestConfig(t)
+ c.VisitorMessageDailyLimit = 3
+ s := newTestServer(t, c)
+
+ for i := 0; i < 3; i++ {
+ response := request(t, s, "PUT", "/", `{"topic":"mytopic","message":"A message"}`, nil)
+ require.Equal(t, 200, response.Code)
+ }
+ response := request(t, s, "PUT", "/", `{"topic":"mytopic","message":"A message"}`, nil)
+ require.Equal(t, 429, response.Code)
+ require.Equal(t, 42908, toHTTPError(t, response.Body.String()).Code)
+}
+
func TestServer_PublishAsJSON_WithEmail(t *testing.T) {
mailer := &testMailer{}
s := newTestServer(t, newTestConfig(t))
@@ -2008,9 +2026,6 @@ 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)
- // FIXME this is because we switched the order of the "limitRequests" handler
- // FIXME there should be tests for the 429s on the "/" and "/_matrix.." endpoint
-
require.Equal(t, fmt.Sprintf(`{"rejected":["http://127.0.0.1:12345/upsomething%d?up=1"]}`+"\n", i), response.Body.String())
}
}