From 4220a374cabbc1a885d9c79037fcf42e14fef677 Mon Sep 17 00:00:00 2001
From: Kegsay <kegan@matrix.org>
Date: Tue, 23 Jun 2020 11:47:48 +0100
Subject: [PATCH] Fix room checks for /state and /state_ids (#1155)

We would return a 403 first (as the server would not be allowed to
see this event) and only then return a 404 if the event is not in
the given room. We now invert those checks for /state and /state_ids
to make the tests pass.
---
 federationapi/routing/events.go | 27 ++++++++++++++++++---------
 federationapi/routing/state.go  |  8 ++++++--
 sytest-whitelist                |  2 ++
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/federationapi/routing/events.go b/federationapi/routing/events.go
index ced9e3d5..6fa28f69 100644
--- a/federationapi/routing/events.go
+++ b/federationapi/routing/events.go
@@ -33,7 +33,11 @@ func GetEvent(
 	eventID string,
 	origin gomatrixserverlib.ServerName,
 ) util.JSONResponse {
-	event, err := getEvent(ctx, request, rsAPI, eventID)
+	err := allowedToSeeEvent(ctx, request.Origin(), rsAPI, eventID)
+	if err != nil {
+		return *err
+	}
+	event, err := fetchEvent(ctx, rsAPI, eventID)
 	if err != nil {
 		return *err
 	}
@@ -47,35 +51,40 @@ func GetEvent(
 	}}
 }
 
-// getEvent returns the requested event,
+// allowedToSeeEvent returns no error if the server is allowed to see this event,
 // otherwise it returns an error response which can be sent to the client.
-func getEvent(
+func allowedToSeeEvent(
 	ctx context.Context,
-	request *gomatrixserverlib.FederationRequest,
+	origin gomatrixserverlib.ServerName,
 	rsAPI api.RoomserverInternalAPI,
 	eventID string,
-) (*gomatrixserverlib.Event, *util.JSONResponse) {
+) *util.JSONResponse {
 	var authResponse api.QueryServerAllowedToSeeEventResponse
 	err := rsAPI.QueryServerAllowedToSeeEvent(
 		ctx,
 		&api.QueryServerAllowedToSeeEventRequest{
 			EventID:    eventID,
-			ServerName: request.Origin(),
+			ServerName: origin,
 		},
 		&authResponse,
 	)
 	if err != nil {
 		resErr := util.ErrorResponse(err)
-		return nil, &resErr
+		return &resErr
 	}
 
 	if !authResponse.AllowedToSeeEvent {
 		resErr := util.MessageResponse(http.StatusForbidden, "server not allowed to see event")
-		return nil, &resErr
+		return &resErr
 	}
 
+	return nil
+}
+
+// fetchEvent fetches the event without auth checks. Returns an error if the event cannot be found.
+func fetchEvent(ctx context.Context, rsAPI api.RoomserverInternalAPI, eventID string) (*gomatrixserverlib.Event, *util.JSONResponse) {
 	var eventsResponse api.QueryEventsByIDResponse
-	err = rsAPI.QueryEventsByID(
+	err := rsAPI.QueryEventsByID(
 		ctx,
 		&api.QueryEventsByIDRequest{EventIDs: []string{eventID}},
 		&eventsResponse,
diff --git a/federationapi/routing/state.go b/federationapi/routing/state.go
index 04a18904..28dfad84 100644
--- a/federationapi/routing/state.go
+++ b/federationapi/routing/state.go
@@ -98,13 +98,17 @@ func getState(
 	roomID string,
 	eventID string,
 ) (*gomatrixserverlib.RespState, *util.JSONResponse) {
-	event, resErr := getEvent(ctx, request, rsAPI, eventID)
+	event, resErr := fetchEvent(ctx, rsAPI, eventID)
 	if resErr != nil {
 		return nil, resErr
 	}
 
 	if event.RoomID() != roomID {
-		return nil, &util.JSONResponse{Code: http.StatusNotFound, JSON: nil}
+		return nil, &util.JSONResponse{Code: http.StatusNotFound, JSON: jsonerror.NotFound("event does not belong to this room")}
+	}
+	resErr = allowedToSeeEvent(ctx, request.Origin(), rsAPI, eventID)
+	if resErr != nil {
+		return nil, resErr
 	}
 
 	var response api.QueryStateAndAuthChainResponse
diff --git a/sytest-whitelist b/sytest-whitelist
index 7edc79e6..77ec6cd9 100644
--- a/sytest-whitelist
+++ b/sytest-whitelist
@@ -350,3 +350,5 @@ setting 'm.room.name' respects room powerlevel
 Syncing a new room with a large timeline limit isn't limited
 Left rooms appear in the leave section of sync
 Banned rooms appear in the leave section of sync
+Getting state checks the events requested belong to the room
+Getting state IDs checks the events requested belong to the room