Handling Non-Local `user_id` in Room Actions Causes Internal Server Error - validation

[TrevisGordan]

Description

I've identified an issue with the [POST /_matrix/client/v3/rooms/{roomId}/-action-] endpoint, where submitting a payload containing a user_id from a different home server or an invalid (non-local) user ID results in an Internal Server Error.

The root cause of this issue lies within roommember.py in the function get_local_current_membership_for_user_in_room, which raises an unhandled standard Exception when encountering a non-local user.

Current exception raising:

if not self.hs.is_mine_id(user_id):
	raise Exception(
		"Cannot call 'get_local_current_membership_for_user_in_room' on "
		"non-local user %s" % (user_id,),
	)

(Source: synapse / get_local_current_membership_for_user_in_room)

To resolve this issue, I suggest two potential approaches:

1. Modify the Existing Exception Handling: Replace the standard Exception within get_local_current_membership_for_user_in_room with a SynapseError, providing a clear and specific error message to the API consumer. For instance:

   message = f"Provided user_id {user_id} is a non-local user"
   raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.M_INVALID_USERNAME)
   

2. Handle the Exception at the Caller Function: Alternatively, the exception could be caught and handled appropriately within the functions that call get_local_current_membership_for_user_in_room, ensuring that a SynapseError is raised there instead.

This should improve error handling and user feedback when encountering non-local user_ids in room action requests, and better input validation preventing the generic Internal Server Error.

Steps to reproduce

• Post non local user id to rooms endpoint.
• Reproduce with:

curl -X POST -H 'Content-Type: application/json' -d '{"reason": "They'"'"'ve been banned long enough", "user_id": "@cheeky_monkey:ma!trix.org"}' 'http://matrix.localhost/_matrix/client/v3/rooms/!e42d8c:matrix.org/unban?access_token=<TOKEN>'

Homeserver

local

Synapse Version

1.94.0

Installation Method

Docker (matrixdotorg/synapse)

Database

PostgreSQL

Workers

Multiple workers

Platform

K8t

Configuration

No response

Relevant log output

'''
2024-01-31 08:26:30,491 - synapse.http.server - 140 - ERROR - POST-122111 - Failed handle request via 'RoomMembershipRestServlet': <XForwardedForRequest at 0x7fffbe6206d0 method='POST' uri='/_matrix/client/v3/rooms/!e42d8c:matrix.org/unban?access_token=<redacted>' clientproto='HTTP/1.1' site='8083'>
'''
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/dist-packages/synapse/http/server.py", line 326, in _async_render_wrapper
    callback_return = await self._async_render(request)
  File "/usr/local/lib/python3.9/dist-packages/synapse/http/server.py", line 538, in _async_render
    callback_return = await raw_callback_return
  File "/usr/local/lib/python3.9/dist-packages/synapse/rest/client/room.py", line 1072, in on_POST
    return await self._do(request, requester, room_id, membership_action, None)
  File "/usr/local/lib/python3.9/dist-packages/synapse/rest/client/room.py", line 1045, in _do
    await self.room_member_handler.update_membership(
  File "/usr/local/lib/python3.9/dist-packages/synapse/handlers/room_member.py", line 640, in update_membership
    result = await self.update_membership_locked(
  File "/usr/local/lib/python3.9/dist-packages/synapse/handlers/room_member.py", line 1024, in update_membership_locked
    ) = await self.store.get_local_current_membership_for_user_in_room(
  File "/usr/local/lib/python3.9/dist-packages/synapse/storage/databases/main/roommember.py", line 540, in get_local_current_membership_for_user_in_room
    raise Exception(
Exception: Cannot call 'get_local_current_membership_for_user_in_room' on non-local user @cheeky_monkey:matrix.org

Anything else that would be useful to know?

No response

[dklimpel]

IMO here is Synapse not compliant to the Spec:

https://spec.matrix.org/v1.10/client-server-api/#post_matrixclientv3roomsroomidinvite

400	The request is invalid. A meaningful errcode and description error text will be returned. Example reasons for rejection include:The request body is malformed (errcode set to M_BAD_JSON or M_NOT_JSON).One or more users being invited to the room are residents of a homeserver which does not support the requested room version. The errcode will be M_UNSUPPORTED_ROOM_VERSION in these cases.

The other endpoints do not have a defined error for this.

[anoadragon453]

I think Solution 1 is fine. However M_INVALID_USERNAME is intended to only be used during registration, according to the spec. I would instead use M_BAD_JSON as @dklimpel suggested.

Note that I think this is only an issue for the following endpoints, which must be completed by a local user:

• POST /_matrix/client/v3/rooms/{roomId}/join
• POST /_matrix/client/v3/rooms/{roomId}/leave

(get_local_current_membership_for_user_in_room is only called in those cases.)

For other actions, such as /_matrix/client/v3/rooms/{roomId}/unban in your reproduction example, it's perfectly valid to kick/unban a remote user from a room. If this fails due to the user not being local, then that's a separate bug.

It would also be good to have a regression test which fails on the current codebase, but succeeds after the fix.

[TrevisGordan]

Hey @anoadragon453, This seems not quite right, there is a little twist.

To reproduce this behavior and get a 500 error, a few conditions must be met:

1. The action indeed must be either KICK or UNBAN.
2. The room (room_id) must be incorrect or non-existent.

The function update_membership_locked treats KICK or UNBAN as effective_membership_state = leave (source). When the room does not exist, is_host_in_room will return False, causing the condition:

elif effective_membership_state == Membership.LEAVE:

(source)

To call get_local_current_membership_for_user_in_room for kick and unban events, leading to a 500 error in the function's "Paranoia check" when the user does not exist.

The question is whether it makes more sense to tackle the issue at its root, which involves the is_host_in_room check. The issue is not only that the USER ID is non-local but also that the room does not exist. The is_host_in_room behavior stems from Federation functionality. Therefore, it may be smarter to leave it as is and just fix the get_local_current_membership_for_user_in_room error, and solve the 500.

I implemented the said Synapse error, and created a Test. Just double checking whether this solution is still sufficient, before creating the PR.

[TrevisGordan]

@anoadragon453 do you have thoughts on this? ;)

[anoadragon453]

Sorry for taking a while to respond to this. I think the diff you posted is sufficient, and suggest going ahead and opening a PR.

In the test, I would have a one check for the a non-local user ID and another for the invalid room ID, instead of including both in a single request. I'd also double-check that the errcode is M_BAD_JSON.

Thanks!