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