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

@@ -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.

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)

federationapi/routing/send.go

@@ -71,15 +71,29 @@ 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 {
-		util.GetLogger(httpReq.Context()).WithError(err).Error("t.processTransaction failed")
-		return jsonerror.InternalServerError()
-	}
-
+	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.StatusBadRequest,
+		JSON: jsonerror.BadJSON(err.Error()),
+	}
 }
 
 type txnReq struct {
@@ -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 {

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

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=