Previously we required every alias in a canonical alias event sent by a
client to be valid, and would only validate local aliases. This
prevented clients from adding/removing canonical aliases if there were
existing remote or invalid aliases.
Previously, we would only attempt to validate the aliases in the event
content if we were able to parse the event, and would silently allow it
otherwise.
The previous logic would increment the backoff counter both when a
request actually fails and when we do not make a request because the
server was already in backoff. This lead to a positive feedback loop
where every request made while a server is in backoff increases the
backoff delay, making it impossible to recover from backoff unless the
entire backoff delay elapses with zero requests.
Failing to reset the backoff state resulted in a monotonically
increasing backoff delay. If a remote server was temporarily
unavailable, we would have a persistently increased rate of key query
failures until the backoff state was reset by a server restart. If
enough key queries were attempted while the remote was unavailable, it
can accumulate an arbitrarily long backoff delay and effectively block
all future key queries to this server.
This is pure code-motion, with no behavior changes. The new structure
will make it easier to fix the backoff behavior, and makes the code
somewhat less of a nightmare to follow.
Previously attempting to delete an MXC that is only associated with
dangling thumbnails would fail, because it assumes that every thumbnail
must have a corresponding original in the db, and errors out if it can't
find the original. This is incorrect because we create dangling
thumbnails when requesting a remote thumbnail over federation when we
don't have the original file.
When requesting remote thumbnails over federation, we can end up with a
thumbnail in the media db without an associated original file. Because
of this, skipping thumbnails is insufficient to get a list of all MXCs.
Previously we were only skipping thumbnails that had both dimensions
nonzero. I think the assumption was that only the dimensions allowed by
services::media::thumbnail_properties would be used. This is not the
case because we have used arbitrary thumbnail dimensions when requesting
remote thumbnails.
Now that we are able to distinguish between corrupted media keys and
missing files, it makes more sense to propagate the corrupted keys up to
the caller.
This is useful to easily distinguish missing files from corrupted keys.
All existing usage sites have been modified so there is no behavior
change in this commit.
We *should* ensure that media deletion is always successful, but when a
bug causes a single object to fail deletion it's better to try to delete
the remaining objects than to give up entirely.
Our current code should never write these, because we have an allowlist
of thumbnail sizes. None of the allowed sizes contain a 0xFF byte. We
have observed keys with a 0xFF in the thumbnail size a couple times on
real servers, and believe an early version of conduit wrote these before
the allowlist was added.
These keys were originally handled correctly, and were broken by
e2cba15ed2. Before that commit, we were
parsing media keys backwards, and never tried to read the thumbnail size
or mxc url.
This became a problem because #foundation-office:matrix.org has a
malformed create event with its `predecessor` set to a string instead of
a map.
The solution to this is, unfortunately, to do more shotgun parsing to
extract only the desired fields rather than trying to parse the entire
content every time. To prevent this kind of problem from happening
again, `RoomCreateEventContent` must only be used for creating new PDUs,
existing PDUs must be shotgun-parsed.
This gets rid of 3 instances of re-parsing the room version.
There's one place where we need the event ID of the room create event to
verify federation responses, so now we just look up the event ID at that
point instead.
This will break backwards compatibility of configurations, but
ensures that a previously-configured setting won't get dropped
arbitrarily. Pretty much worth it, I think.
