admin/drive/cleanup could delete imported custom emoji

[Johann150]

💡 Summary

/api/admin/drive/cleanup deletes all drive files where userId = NULL:

misskey/packages/backend/src/server/api/endpoints/admin/drive/cleanup.ts

Lines 15 to 17 in 149edae

	const files = await DriveFiles.find({
	userId: IsNull(),
	});

This could accidentally delete imported emoji since they are created with userId set to NULL:

misskey/packages/backend/src/server/api/endpoints/admin/emoji/copy.ts

Line 57 in 149edae

driveFile = await uploadFromUrl({ url: emoji.originalUrl, user: null, force: true });

🔧❓ Possible Solutions

1. Check that files are not used as emojis in /api/admin/drive/cleanup.
   This would probably be complicated, but could be done with the TypeORM equivalent of

... LEFT JOIN emoji ON drive_file.url = emoji."originalUrl" WHERE emoji.id IS NULL

2. Make @instance.actor the owner of imported emojis.
3. Remove this API endpoint because drive files for deleted users are already deleted when the account is deleted:

   misskey/packages/backend/src/queue/processors/db/delete-account.ts

   Line 48 in c69b72e

   { // Delete files

Additional Info

related to #7304

[syuilo]

There is also the option of not using Drive to manage file data used internally by the system, such as emoji (e.g., create a more general file table and have DriveFile refer to it).

[Johann150]

That sounds much more complicated than any of the suggestions I made. Unless you have a different use for this I would absolutely not recommend it. And even then I think option 2 would be more sensible for what you say.

[Johann150]

I think option 1 is too complicated.

If option 2 is implemented, we should also add a NOT NULL constraint to drive_file.user. Maybe the default for the column could be set after @instance.actor is created, but that sounds like a bad idea since it is not part of the usual database management with migrations, so it could easily get broken. Even without the default value of the column, it sounds like a good idea because I wouldn't be sure if other parts of Misskey do not also have different interpretations of the drive_file.user value.

However it would also implicate that @instance.actor is also subject to the drive usage limit, which could be a good and a bad thing. It would stop too many emoji being added to an instance (there are known performance issues with the emoji picker if many emojis are present). On the other hand it might also be an annoyance for instance administrators & moderators. Syuilo's suggestion would also avoid this issue, but continuing to use NULL could also avoid it and is already implemented and simpler.

It would also be possible to combine options 2 and 3.

However, in full view with the drive quota problem in mind I think option 3 is the best.

[Johann150]

If option 3 is implemented, I'm not sure if this constraint should be changed as well:

TABLE "drive_file" CONSTRAINT "FK_860fa6f6c7df5bb887249fba22e" FOREIGN KEY ("userId") REFERENCES "user"(id) ON DELETE SET NULL

Setting to null on delete does not make sense because it would move the file to the drive of the system. Cascading the delete is also not a good idea because the file on disk has to be deleted outside of the database as well.

Maybe it should be changed to ON DELETE RESTRICT to make sure all drive files are deleted before a user can be deleted. As commented above this procedure is already followed by Misskey anway, it would just correctly mirror it in the database structure.