Details
- Reviewers
ashoat - Commits
- rCOMMf3d78381cecc: [keyserver] Fix `updateUserAvatar` query
- Set image avatar
- Set emoji avatar
- Set image avatar
- Set malformed image avatar (uploadID doesn't point to upload that can be set as image avatar... whether because it's nonsensical or belongs to another uploader or is already contained by eg a message)
Diff Detail
- Repository
- rCOMM Comm
- Branch
- arcpatch-D7437 (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
keyserver/src/updaters/account-updaters.js | ||
---|---|---|
127–141 ↗ | (On Diff #25146) | At a very high level this query is about unsetting container for upload belonging to previous image avatar. One thing we want to be careful about is unsetting container and "breaking" the previous avatar state until we're sure we can get to the intended avatar state as described by the UpdateUserAvatarRequest. The four possible new states are
In the first three cases, we can safely unset container since "everything we need" (contents of newAvatarValue) for the new avatar state will be handled by the UPDATE users query and only affect the users table. For these cases the ${mediaID} IS NULL condition will be true and all upload rows with uploader and container set to viewer.userID will be cleared. However, if the intended future avatar state is an image avatar... we want to make sure that container can be set for the upload corresponding to that future image avatar before we unlink the existing one. If, for example, a malicious user or broken client sent a request with uploadID = 329058190 without this change:
And we would be left with the old image avatar pointing to a now unlinked upload that will be swept into the void by the cron job that cleans up containerless uploads. This is a broken state. However, with this change:
And we would be where we were before the avatar change request was made. It might not be the intended state, but it's a consistent state that isn't broken. |
keyserver/src/updaters/account-updaters.js | ||
---|---|---|
148 ↗ | (On Diff #25146) | I thought it didn't hurt to add this check. We definitely don't want to assign an upload to avatar that could also show up in fetchMediaForThread. Whether this check is necessary or not is questionable... open to keeping or leaving out. |