Ensure we return 429s for Matrix endpoints too; return proper error codes

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

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)
 }
 

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())
 }
 

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())
 	}
 }