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.
