From 3c2e6f967b44d895e2145d91b2f3414c9451f181 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Thu, 16 Apr 2020 17:59:55 +0100 Subject: [PATCH] Federation fixes and error handling (#970) * Improve error handling in federation /send endpoint a bit * Remove unknownRoomError, use unmarshalError when unable to get room ID * Swap out a couple more internal server errors * Update gomatrixserverlib * Update gomatrixserverlib * Update gomatrixserverlib * Update gomatrixserverlib * Update gomatrixserverlib * Update gomatrixserverlib * Return bad limit in error * Same with domain/userid --- federationapi/routing/backfill.go | 6 +++- federationapi/routing/profile.go | 12 +++++-- federationapi/routing/send.go | 52 +++++++++++++++++++++++-------- go.mod | 3 +- go.sum | 4 +-- 5 files changed, 56 insertions(+), 21 deletions(-) diff --git a/federationapi/routing/backfill.go b/federationapi/routing/backfill.go index 7092c436..62471b8a 100644 --- a/federationapi/routing/backfill.go +++ b/federationapi/routing/backfill.go @@ -16,6 +16,7 @@ package routing import ( "encoding/json" + "fmt" "net/http" "strconv" "time" @@ -73,7 +74,10 @@ func Backfill( } if req.Limit, err = strconv.Atoi(limit); err != nil { util.GetLogger(httpReq.Context()).WithError(err).Error("strconv.Atoi failed") - return jsonerror.InternalServerError() + return util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.InvalidArgumentValue(fmt.Sprintf("limit %q is invalid format", limit)), + } } // Query the roomserver. diff --git a/federationapi/routing/profile.go b/federationapi/routing/profile.go index 01a70c01..9a81c1b3 100644 --- a/federationapi/routing/profile.go +++ b/federationapi/routing/profile.go @@ -15,6 +15,7 @@ package routing import ( + "fmt" "net/http" appserviceAPI "github.com/matrix-org/dendrite/appservice/api" @@ -46,12 +47,17 @@ func GetProfile( _, domain, err := gomatrixserverlib.SplitID('@', userID) if err != nil { util.GetLogger(httpReq.Context()).WithError(err).Error("gomatrixserverlib.SplitID failed") - return jsonerror.InternalServerError() + return util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.MissingArgument(fmt.Sprintf("Format of user ID %q is invalid", userID)), + } } if domain != cfg.Matrix.ServerName { - util.GetLogger(httpReq.Context()).WithError(err).Error("domain != cfg.Matrix.ServerName failed") - return jsonerror.InternalServerError() + return util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.InvalidArgumentValue(fmt.Sprintf("Domain %q does not match this server", domain)), + } } profile, err := appserviceAPI.RetrieveUserProfile(httpReq.Context(), userID, asAPI, accountDB) diff --git a/federationapi/routing/send.go b/federationapi/routing/send.go index 1013a44c..5a9766f8 100644 --- a/federationapi/routing/send.go +++ b/federationapi/routing/send.go @@ -71,14 +71,28 @@ func Send( util.GetLogger(httpReq.Context()).Infof("Received transaction %q containing %d PDUs, %d EDUs", txnID, len(t.PDUs), len(t.EDUs)) resp, err := t.processTransaction() - if err != nil { + switch err.(type) { + // No error? Great! Send back a 200. + case nil: + return util.JSONResponse{ + Code: http.StatusOK, + JSON: resp, + } + // Handle known error cases as we will return a 400 error for these. + case roomNotFoundError: + case unmarshalError: + case verifySigError: + // Handle unknown error cases. Sending 500 errors back should be a last + // resort as this can make other homeservers back off sending federation + // events. + default: util.GetLogger(httpReq.Context()).WithError(err).Error("t.processTransaction failed") return jsonerror.InternalServerError() } - + // Return a 400 error for bad requests as fallen through from above. return util.JSONResponse{ - Code: http.StatusOK, - JSON: resp, + Code: http.StatusBadRequest, + JSON: jsonerror.BadJSON(err.Error()), } } @@ -93,6 +107,8 @@ type txnReq struct { } func (t *txnReq) processTransaction() (*gomatrixserverlib.RespSend, error) { + results := make(map[string]gomatrixserverlib.PDUResult) + var pdus []gomatrixserverlib.HeaderedEvent for _, pdu := range t.PDUs { var header struct { @@ -100,28 +116,27 @@ func (t *txnReq) processTransaction() (*gomatrixserverlib.RespSend, error) { } if err := json.Unmarshal(pdu, &header); err != nil { util.GetLogger(t.context).WithError(err).Warn("Transaction: Failed to extract room ID from event") - return nil, err + return nil, unmarshalError{err} } verReq := api.QueryRoomVersionForRoomRequest{RoomID: header.RoomID} verRes := api.QueryRoomVersionForRoomResponse{} if err := t.query.QueryRoomVersionForRoom(t.context, &verReq, &verRes); err != nil { util.GetLogger(t.context).WithError(err).Warn("Transaction: Failed to query room version for room", verReq.RoomID) - return nil, err + return nil, roomNotFoundError{verReq.RoomID} } event, err := gomatrixserverlib.NewEventFromUntrustedJSON(pdu, verRes.RoomVersion) if err != nil { util.GetLogger(t.context).WithError(err).Warnf("Transaction: Failed to parse event JSON of event %q", event.EventID()) - return nil, err + return nil, unmarshalError{err} } if err := gomatrixserverlib.VerifyAllEventSignatures(t.context, []gomatrixserverlib.Event{event}, t.keys); err != nil { util.GetLogger(t.context).WithError(err).Warnf("Transaction: Couldn't validate signature of event %q", event.EventID()) - return nil, err + return nil, verifySigError{event.EventID(), err} } pdus = append(pdus, event.Headered(verRes.RoomVersion)) } // Process the events. - results := map[string]gomatrixserverlib.PDUResult{} for _, e := range pdus { err := t.processEvent(e.Unwrap()) if err != nil { @@ -141,7 +156,7 @@ func (t *txnReq) processTransaction() (*gomatrixserverlib.RespSend, error) { // If we bail and stop processing then we risk wedging incoming // transactions from that server forever. switch err.(type) { - case unknownRoomError: + case roomNotFoundError: case *gomatrixserverlib.NotAllowed: default: // Any other error should be the result of a temporary error in @@ -162,11 +177,22 @@ func (t *txnReq) processTransaction() (*gomatrixserverlib.RespSend, error) { return &gomatrixserverlib.RespSend{PDUs: results}, nil } -type unknownRoomError struct { +type roomNotFoundError struct { roomID string } +type unmarshalError struct { + err error +} +type verifySigError struct { + eventID string + err error +} -func (e unknownRoomError) Error() string { return fmt.Sprintf("unknown room %q", e.roomID) } +func (e roomNotFoundError) Error() string { return fmt.Sprintf("room %q not found", e.roomID) } +func (e unmarshalError) Error() string { return fmt.Sprintf("unable to parse event: %s", e.err) } +func (e verifySigError) Error() string { + return fmt.Sprintf("unable to verify signature of event %q: %s", e.eventID, e.err) +} func (t *txnReq) processEDUs(edus []gomatrixserverlib.EDU) { for _, e := range edus { @@ -213,7 +239,7 @@ func (t *txnReq) processEvent(e gomatrixserverlib.Event) error { // that this server is unaware of. // However generally speaking we should reject events for rooms we // aren't a member of. - return unknownRoomError{e.RoomID()} + return roomNotFoundError{e.RoomID()} } if !stateResp.PrevEventsExist { diff --git a/go.mod b/go.mod index 849ad1f5..401492ae 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/matrix-org/go-http-js-libp2p v0.0.0-20200318135427-31631a9ef51f github.com/matrix-org/go-sqlite3-js v0.0.0-20200325174927-327088cdef10 github.com/matrix-org/gomatrix v0.0.0-20190528120928-7df988a63f26 - github.com/matrix-org/gomatrixserverlib v0.0.0-20200416113012-dafb32a889ea + github.com/matrix-org/gomatrixserverlib v0.0.0-20200416165239-837ed63a0046 github.com/matrix-org/naffka v0.0.0-20200127221512-0716baaabaf1 github.com/matrix-org/util v0.0.0-20190711121626-527ce5ddefc7 github.com/mattn/go-sqlite3 v2.0.3+incompatible @@ -27,7 +27,6 @@ require ( github.com/pierrec/lz4 v2.5.0+incompatible // indirect github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.4.1 - github.com/prometheus/common v0.9.1 github.com/rcrowley/go-metrics v0.0.0-20200313005456-10cdbea86bc0 // indirect github.com/sirupsen/logrus v1.4.2 github.com/tidwall/gjson v1.6.0 // indirect diff --git a/go.sum b/go.sum index f4b4e4ff..3bf623a9 100644 --- a/go.sum +++ b/go.sum @@ -364,8 +364,8 @@ github.com/matrix-org/gomatrix v0.0.0-20190528120928-7df988a63f26 h1:Hr3zjRsq2bh github.com/matrix-org/gomatrix v0.0.0-20190528120928-7df988a63f26/go.mod h1:3fxX6gUjWyI/2Bt7J1OLhpCzOfO/bB3AiX0cJtEKud0= github.com/matrix-org/gomatrixserverlib v0.0.0-20200124100636-0c2ec91d1df5 h1:kmRjpmFOenVpOaV/DRlo9p6z/IbOKlUC+hhKsAAh8Qg= github.com/matrix-org/gomatrixserverlib v0.0.0-20200124100636-0c2ec91d1df5/go.mod h1:FsKa2pWE/bpQql9H7U4boOPXFoJX/QcqaZZ6ijLkaZI= -github.com/matrix-org/gomatrixserverlib v0.0.0-20200416113012-dafb32a889ea h1:aiBD966UX0l4rTPgAu+u8CaiMfy+N8qWlZ2TIBp+0eM= -github.com/matrix-org/gomatrixserverlib v0.0.0-20200416113012-dafb32a889ea/go.mod h1:FsKa2pWE/bpQql9H7U4boOPXFoJX/QcqaZZ6ijLkaZI= +github.com/matrix-org/gomatrixserverlib v0.0.0-20200416165239-837ed63a0046 h1:R7iYuS8hhXdrqs5OSNF3Y3chaIFWU9KLpnzjjsdJkMU= +github.com/matrix-org/gomatrixserverlib v0.0.0-20200416165239-837ed63a0046/go.mod h1:FsKa2pWE/bpQql9H7U4boOPXFoJX/QcqaZZ6ijLkaZI= github.com/matrix-org/naffka v0.0.0-20200127221512-0716baaabaf1 h1:osLoFdOy+ChQqVUn2PeTDETFftVkl4w9t/OW18g3lnk= github.com/matrix-org/naffka v0.0.0-20200127221512-0716baaabaf1/go.mod h1:cXoYQIENbdWIQHt1SyCo6Bl3C3raHwJ0wgVrXHSqf+A= github.com/matrix-org/util v0.0.0-20171127121716-2e2df66af2f5 h1:W7l5CP4V7wPyPb4tYE11dbmeAOwtFQBTW0rf4OonOS8=