/account/deactivate is 500ing

[ara4n]

Description

Almost all requests to /_matrix/client/r0/account/deactivate on matrix.org failing with 500.

Steps to reproduce

deactivate an account (might need to be social login); watch 500.

Homeserver

matrix.org

Synapse Version

1.65.0 (b=matrix-org-hotfixes,d20c92d2c2)

Installation Method

No response

Platform

debian

Relevant log output

File "/home/synapse/src/synapse/http/server.py", line 368, in _async_render_wrapper
    callback_return = await self._async_render(request)
  File "/home/synapse/src/synapse/http/server.py", line 574, in _async_render
    callback_return = await raw_callback_return
  File "/home/synapse/src/synapse/rest/client/_base.py", line 99, in wrapped
    return await orig(*args, **kwargs)
  File "/home/synapse/src/synapse/rest/client/account.py", line 316, in on_POST
    await self.auth_handler.validate_user_via_ui_auth(
  File "/home/synapse/src/synapse/handlers/auth.py", line 344, in validate_user_via_ui_auth
    result, params, session_id = await self.check_ui_auth(
  File "/home/synapse/src/synapse/handlers/auth.py", line 470, in check_ui_auth
    if "session" in authdict:
TypeError: argument of type 'NoneType' is not iterable

Anything else that would be useful to know?

No response

[ara4n]

Looks like this broke on Wednesday morning; the first failing req was at 2022-08-17 09:57:26,485Z

[DMRobertson]

Suspect my pydantic changes in #13188.

[reivilibre]

Ah. This should illustrate the root cause of the problem:

>>> body = DeactivateAccountRestServlet.PostBody.parse_obj({})
>>> body.dict()
{'auth': None, 'id_server': None, 'erase': False}

This then causes the following lines a problem:

authdict = clientdict.pop("auth", {})
if "session" in authdict:

If clientdict were an empty dict here, as it was before, then authdict = {}. But since Pydantic 'fills in' the missing fields with their default values (None here), then it will get None instead.
The real thing to blame is that the UI auth code is still using type unsafe Dict[str, Any]s everywhere, so this would never have been caught statically.

[reivilibre]

• Need to make sure that Complement will catch this in the future.

[DMRobertson]

It's unfortunate that the user-provided fields live at the same level on the request body, rather than being quarantined into some extras field. I.e. we have

{
    "auth": { ... },
    "id_server": "id.example.com",
    "extra_1": 1,
    "extra_2": 2,
}

rather than e.g.

{
    "auth": { ... },
    "id_server": "id.example.com",
    "extras": {
        "extra_1": 1,
        "extra_2": 2,
    }
}