From c8d476a3cca2fe0850373c0276144eea65d0a219 Mon Sep 17 00:00:00 2001
From: Kegsay <kegan@matrix.org>
Date: Mon, 27 Jul 2020 09:20:09 +0100
Subject: [PATCH] Return HTTP errors when trying to kick invalid users (#1221)

Room integrity was never compromised as GMSL does auth checks,
but we would incorrectly 200 OK the request instead of 403ing.
---
 clientapi/routing/membership.go | 12 +++++++++---
 clientapi/routing/routing.go    |  2 +-
 sytest-whitelist                |  2 ++
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/clientapi/routing/membership.go b/clientapi/routing/membership.go
index a9a8fa00..90ddb699 100644
--- a/clientapi/routing/membership.go
+++ b/clientapi/routing/membership.go
@@ -96,6 +96,7 @@ func SendKick(
 	req *http.Request, accountDB accounts.Database, device *userapi.Device,
 	roomID string, cfg *config.Dendrite,
 	rsAPI roomserverAPI.RoomserverInternalAPI, asAPI appserviceAPI.AppServiceQueryAPI,
+	stateAPI currentstateAPI.CurrentStateInternalAPI,
 ) util.JSONResponse {
 	body, evTime, roomVer, reqErr := extractRequestData(req, roomID, rsAPI)
 	if reqErr != nil {
@@ -108,6 +109,11 @@ func SendKick(
 		}
 	}
 
+	errRes := checkMemberInRoom(req.Context(), stateAPI, device.UserID, roomID)
+	if errRes != nil {
+		return *errRes
+	}
+
 	var queryRes roomserverAPI.QueryMembershipForUserResponse
 	err := rsAPI.QueryMembershipForUser(req.Context(), &roomserverAPI.QueryMembershipForUserRequest{
 		RoomID: roomID,
@@ -116,11 +122,11 @@ func SendKick(
 	if err != nil {
 		return util.ErrorResponse(err)
 	}
-	// kick is only valid if the user is not currently banned
-	if queryRes.Membership == "ban" {
+	// kick is only valid if the user is not currently banned or left (that is, they are joined or invited)
+	if queryRes.Membership != "join" && queryRes.Membership != "invite" {
 		return util.JSONResponse{
 			Code: 403,
-			JSON: jsonerror.Unknown("cannot /kick banned users"),
+			JSON: jsonerror.Unknown("cannot /kick banned or left users"),
 		}
 	}
 	// TODO: should we be using SendLeave instead?
diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go
index c9ed5ea5..311f64d1 100644
--- a/clientapi/routing/routing.go
+++ b/clientapi/routing/routing.go
@@ -155,7 +155,7 @@ func Setup(
 			if err != nil {
 				return util.ErrorResponse(err)
 			}
-			return SendKick(req, accountDB, device, vars["roomID"], cfg, rsAPI, asAPI)
+			return SendKick(req, accountDB, device, vars["roomID"], cfg, rsAPI, asAPI, stateAPI)
 		}),
 	).Methods(http.MethodPost, http.MethodOptions)
 	r0mux.Handle("/rooms/{roomID}/unban",
diff --git a/sytest-whitelist b/sytest-whitelist
index 5bf6d68b..234eae39 100644
--- a/sytest-whitelist
+++ b/sytest-whitelist
@@ -413,3 +413,5 @@ A full_state incremental update returns only recent timeline
 A prev_batch token can be used in the v1 messages API
 We don't send redundant membership state across incremental syncs by default
 Typing notifications don't leak
+Users cannot kick users from a room they are not in
+Users cannot kick users who have already left a room