Signal/Desktop
1
0
Fork 0
mirror of https://github.com/signalapp/Signal-Desktop.git synced 2026-02-15 07:28:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

Avoid orphaning quoted attachment thumbnails

Browse Source
This commit is contained in:
trevor-signal
2025-11-11 19:11:44 -05:00
committed by GitHub
parent 677404e82d
commit 7a8f208854
4 changed files with 590 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
ts/jobs/AttachmentDownloadManager.preload.ts
View File

@@ -19,6 +19,7 @@ import {
isIncrementalMacVerificationError,
} from '../util/downloadAttachment.preload.js';
import {
deleteAttachmentData as doDeleteAttachmentData,
deleteDownloadData as doDeleteDownloadData,
processNewAttachment as doProcessNewAttachment,
} from '../util/migrations.preload.js';
@@ -39,6 +40,7 @@ import {
getUndownloadedAttachmentSignature,
isIncremental,
hasRequiredInformationForRemoteBackup,
deleteAllAttachmentFilesOnDisk,
} from '../util/Attachment.std.js';
import type { ReadonlyMessageAttributesType } from '../model-types.d.ts';
import { backupsService } from '../services/backups/index.preload.js';
@@ -48,7 +50,10 @@ import {
getMaximumIncomingAttachmentSizeInKb,
getMaximumIncomingTextAttachmentSizeInKb,
} from '../types/AttachmentSize.std.js';
import { addAttachmentToMessage } from '../messageModifiers/AttachmentDownloads.preload.js';
import {
addAttachmentToMessage,
AttachmentNotNeededForMessageError,
} from '../messageModifiers/AttachmentDownloads.preload.js';
import * as Errors from '../types/errors.std.js';
import { redactGenericText } from '../util/privacy.node.js';
import {
@@ -491,6 +496,7 @@ export class AttachmentDownloadManager extends JobManager<CoreAttachmentDownload
}
type DependenciesType = {
deleteAttachmentData: typeof doDeleteAttachmentData;
deleteDownloadData: typeof doDeleteDownloadData;
downloadAttachment: typeof downloadAttachmentUtil;
processNewAttachment: typeof doProcessNewAttachment;
@@ -503,6 +509,7 @@ export async function runDownloadAttachmentJob({
isLastAttempt,
options,
dependencies = {
deleteAttachmentData: doDeleteAttachmentData,
deleteDownloadData: doDeleteDownloadData,
downloadAttachment: downloadAttachmentUtil,
processNewAttachment: doProcessNewAttachment,
@@ -567,6 +574,23 @@ export async function runDownloadAttachmentJob({
return { status: 'finished' };
}
if (error instanceof AttachmentNotNeededForMessageError) {
if (job.attachmentType === 'quote') {
log.info(
`${logId}: quote attachment not needed, likely the thumbnail was already copied from original message`
);
} else {
log.error(`${logId}: attachment not found on message`);
}
await deleteAllAttachmentFilesOnDisk({
deleteDownloadOnDisk: dependencies.deleteDownloadData,
deleteAttachmentOnDisk: dependencies.deleteAttachmentData,
})(error.attachment);
return { status: 'finished' };
}
if (error instanceof AttachmentSizeError) {
log.info(`${logId}: Attachment is too big.`);
await addAttachmentToMessage(
@@ -884,6 +908,11 @@ export async function runDownloadAttachmentJobInner({
if (isAbortError(error)) {
throw error;
}
if (error instanceof AttachmentNotNeededForMessageError) {
throw error;
}
if (mightHaveBackupThumbnailToDownload && !attemptBackupThumbnailFirst) {
log.error(
`${logId}: failed to download fullsize attachment, falling back to backup thumbnail`,

51
ts/messageModifiers/AttachmentDownloads.preload.ts
View File

@@ -69,6 +69,12 @@ export async function markAttachmentAsCorrupted(
});
}
export class AttachmentNotNeededForMessageError extends Error {
constructor(public readonly attachment: AttachmentType) {
super('AttachmentNotNeededForMessageError');
}
}
export async function addAttachmentToMessage(
messageId: string,
attachment: AttachmentType,
@@ -167,14 +173,15 @@ export async function addAttachmentToMessage(
await deleteAttachmentData(attachment.path);
}
if (!handledAnywhere) {
log.warn(
`${logPrefix}: Long message attachment found no matching place to apply`
);
// eslint-disable-next-line no-unsafe-finally
throw new AttachmentNotNeededForMessageError(attachment);
}
}
return;
}
let foundPlaceForAttachment = false;
const maybeReplaceAttachment = (existing: AttachmentType): AttachmentType => {
if (isDownloaded(existing)) {
return existing;
@@ -184,13 +191,13 @@ export async function addAttachmentToMessage(
return existing;
}
foundPlaceForAttachment = true;
return attachment;
};
if (type === 'attachment') {
const attachments = message.get('attachments');
let handledAnywhere = false;
let handledInEditHistory = false;
const editHistory = message.get('editHistory');
@@ -207,7 +214,6 @@ export async function addAttachmentToMessage(
attachments: edit.attachments.map(item => {
const newItem = maybeReplaceAttachment(item);
handledInEditHistory ||= item !== newItem;
handledAnywhere ||= handledInEditHistory;
return newItem;
}),
};
@@ -220,18 +226,12 @@ export async function addAttachmentToMessage(
if (attachments) {
message.set({
attachments: attachments.map(item => {
const newItem = maybeReplaceAttachment(item);
handledAnywhere ||= item !== newItem;
return newItem;
}),
attachments: attachments.map(maybeReplaceAttachment),
});
}
if (!handledAnywhere) {
log.warn(
`${logPrefix}: 'attachment' type found no matching place to apply`
);
if (!foundPlaceForAttachment) {
throw new AttachmentNotNeededForMessageError(attachment);
}
return;
@@ -243,7 +243,7 @@ export async function addAttachmentToMessage(
let handledInEditHistory = false;
const editHistory = message.get('editHistory');
if (preview && editHistory) {
if (editHistory) {
const newEditHistory = editHistory.map(edit => {
if (!edit.preview) {
return edit;
@@ -282,6 +282,10 @@ export async function addAttachmentToMessage(
});
}
if (!foundPlaceForAttachment) {
throw new AttachmentNotNeededForMessageError(attachment);
}
return;
}
@@ -290,7 +294,6 @@ export async function addAttachmentToMessage(
if (!contacts?.length) {
throw new Error(`${logPrefix}: no contacts, cannot add attachment!`);
}
let handled = false;
const newContacts = contacts.map(contact => {
if (!contact.avatar?.avatar) {
@@ -301,7 +304,6 @@ export async function addAttachmentToMessage(
const newAttachment = maybeReplaceAttachment(existingAttachment);
if (existingAttachment !== newAttachment) {
handled = true;
return {
...contact,
avatar: { ...contact.avatar, avatar: newAttachment },
@@ -310,10 +312,8 @@ export async function addAttachmentToMessage(
return contact;
});
if (!handled) {
throw new Error(
`${logPrefix}: Couldn't find matching contact with avatar attachment for message`
);
if (!foundPlaceForAttachment) {
throw new AttachmentNotNeededForMessageError(attachment);
}
message.set({ contact: newContacts });
@@ -374,6 +374,10 @@ export async function addAttachmentToMessage(
message.set({ quote: newQuote });
}
if (!foundPlaceForAttachment) {
throw new AttachmentNotNeededForMessageError(attachment);
}
return;
}
@@ -389,6 +393,11 @@ export async function addAttachmentToMessage(
data: sticker.data ? maybeReplaceAttachment(sticker.data) : attachment,
},
});
if (!foundPlaceForAttachment) {
throw new AttachmentNotNeededForMessageError(attachment);
}
return;
}

124
ts/test-electron/services/AttachmentDownloadManager_test.preload.ts
View File

@@ -41,6 +41,9 @@ import {
type ExplodePromiseResultType,
} from '../../util/explodePromise.std.js';
import { itemStorage } from '../../textsecure/Storage.preload.js';
import { composeAttachment } from '../../test-node/util/queueAttachmentDownloads_test.preload.js';
import { MessageCache } from '../../services/MessageCache.preload.js';
import { AttachmentNotNeededForMessageError } from '../../messageModifiers/AttachmentDownloads.preload.js';
const { omit } = lodash;
@@ -109,6 +112,7 @@ describe('AttachmentDownloadManager', () => {
beforeEach(async () => {
await DataWriter.removeAll();
await itemStorage.user.setAciAndDeviceId(generateAci(), 1);
MessageCache.install();
sandbox = sinon.createSandbox();
clock = sandbox.useFakeTimers();
@@ -179,6 +183,7 @@ describe('AttachmentDownloadManager', () => {
timestamp: job.sentAt,
received_at: job.receivedAt + 1,
conversationId: 'convoId',
attachments: [job.attachment],
},
{
forceSave: true,
@@ -625,6 +630,7 @@ describe('AttachmentDownloadManager', () => {
downloadStarted.resolve();
});
}),
deleteAttachmentData: sandbox.stub(),
deleteDownloadData: sandbox.stub(),
processNewAttachment: sandbox.stub(),
runDownloadAttachmentJobInner,
@@ -741,9 +747,117 @@ describe('AttachmentDownloadManager', () => {
});
});
});
describe('AttachmentDownloadManager.runDownloadAttachmentJob', () => {
let sandbox: sinon.SinonSandbox;
let deleteAttachmentData: sinon.SinonStub;
let deleteDownloadData: sinon.SinonStub;
let downloadAttachment: sinon.SinonStub;
let processNewAttachment: sinon.SinonStub;
const downloadedAttachment: Awaited<
ReturnType<typeof downloadAttachmentUtil>
> = {
path: '/path/to/file',
digest: 'digest',
plaintextHash: 'plaintextHash',
localKey: 'localKey',
version: 2,
size: 128,
};
beforeEach(async () => {
await DataWriter.removeAll();
await itemStorage.user.setAciAndDeviceId(generateAci(), 1);
sandbox = sinon.createSandbox();
downloadAttachment = sandbox
.stub()
.returns(Promise.resolve(downloadedAttachment));
deleteAttachmentData = sandbox.stub();
deleteDownloadData = sandbox.stub();
processNewAttachment = sandbox.stub().callsFake(attachment => attachment);
});
afterEach(async () => {
sandbox.restore();
});
it('will delete attachment files if attachment not found on message', async () => {
const messageId = 'messageId';
const attachment = composeAttachment();
const abortController = new AbortController();
await window.MessageCache.saveMessage(
{
id: messageId,
type: 'incoming',
sent_at: Date.now(),
timestamp: Date.now(),
received_at: Date.now(),
conversationId: 'convoId',
attachments: [attachment],
},
{
forceSave: true,
}
);
const job = composeJob({
messageId: 'messageId',
receivedAt: Date.now(),
attachmentOverrides: attachment,
});
const result = await runDownloadAttachmentJob({
job,
isLastAttempt: false,
options: {
isForCurrentlyVisibleMessage: false,
abortSignal: abortController.signal,
maxAttachmentSizeInKib: 100000,
maxTextAttachmentSizeInKib: 100000,
hasMediaBackups: false,
},
dependencies: {
downloadAttachment,
deleteAttachmentData,
deleteDownloadData,
processNewAttachment,
runDownloadAttachmentJobInner: sandbox.stub().throws(
new AttachmentNotNeededForMessageError({
contentType: MIME.IMAGE_PNG,
size: 128,
path: 'main/path',
downloadPath: '/downloadPath',
thumbnail: {
contentType: MIME.IMAGE_PNG,
size: 128,
path: 'thumbnail/path',
},
})
),
},
});
assert.strictEqual(result.status, 'finished');
assert.deepStrictEqual(
deleteAttachmentData
.getCalls()
.map(call => call.args[0])
.flat(),
['main/path', 'thumbnail/path']
);
assert.deepStrictEqual(
deleteDownloadData
.getCalls()
.map(call => call.args[0])
.flat(),
['/downloadPath']
);
});
});
describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => {
let sandbox: sinon.SinonSandbox;
let deleteAttachmentData: sinon.SinonStub;
let deleteDownloadData: sinon.SinonStub;
let downloadAttachment: sinon.SinonStub;
let processNewAttachment: sinon.SinonStub;
@@ -761,6 +875,9 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => {
};
beforeEach(async () => {
await DataWriter.removeAll();
MessageCache.install();
await itemStorage.user.setAciAndDeviceId(generateAci(), 1);
sandbox = sinon.createSandbox();
downloadAttachment = sandbox
.stub()
@@ -791,6 +908,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => {
maxAttachmentSizeInKib: 100 * MEBIBYTE,
maxTextAttachmentSizeInKib: 2 * MEBIBYTE,
dependencies: {
deleteAttachmentData,
deleteDownloadData,
downloadAttachment,
processNewAttachment,
@@ -829,6 +947,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => {
maxAttachmentSizeInKib: 100 * MEBIBYTE,
maxTextAttachmentSizeInKib: 2 * MEBIBYTE,
dependencies: {
deleteAttachmentData,
deleteDownloadData,
downloadAttachment,
processNewAttachment,
@@ -884,6 +1003,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => {
maxAttachmentSizeInKib: 100 * MEBIBYTE,
maxTextAttachmentSizeInKib: 2 * MEBIBYTE,
dependencies: {
deleteAttachmentData,
deleteDownloadData,
downloadAttachment,
processNewAttachment,
@@ -921,6 +1041,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => {
maxAttachmentSizeInKib: 100 * MEBIBYTE,
maxTextAttachmentSizeInKib: 2 * MEBIBYTE,
dependencies: {
deleteAttachmentData,
deleteDownloadData,
downloadAttachment,
processNewAttachment,
@@ -959,6 +1080,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => {
maxAttachmentSizeInKib: 100 * MEBIBYTE,
maxTextAttachmentSizeInKib: 2 * MEBIBYTE,
dependencies: {
deleteAttachmentData,
deleteDownloadData,
downloadAttachment,
processNewAttachment,
@@ -995,6 +1117,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => {
maxAttachmentSizeInKib: 100 * MEBIBYTE,
maxTextAttachmentSizeInKib: 2 * MEBIBYTE,
dependencies: {
deleteAttachmentData,
deleteDownloadData,
downloadAttachment,
processNewAttachment,
@@ -1050,6 +1173,7 @@ describe('AttachmentDownloadManager.runDownloadAttachmentJobInner', () => {
maxAttachmentSizeInKib: 100 * MEBIBYTE,
maxTextAttachmentSizeInKib: 2 * MEBIBYTE,
dependencies: {
deleteAttachmentData,
deleteDownloadData,
downloadAttachment,
processNewAttachment,

406
ts/test-electron/util/addAttachmentToMessage_test.preload.ts Normal file
View File

@@ -0,0 +1,406 @@
// Copyright 2025 Signal Messenger, LLC
// SPDX-License-Identifier: AGPL-3.0-only
import { assert } from 'chai';
import { emptyDir } from 'fs-extra';
import { randomBytes } from 'node:crypto';
import { readdir, writeFile } from 'node:fs/promises';
import { v7 } from 'uuid';
import * as MIME from '../../types/MIME.std.js';
import { composeAttachment } from '../../test-node/util/queueAttachmentDownloads_test.preload.js';
import { addAttachmentToMessage } from '../../messageModifiers/AttachmentDownloads.preload.js';
import { getMessageById } from '../../messages/getMessageById.preload.js';
import { MessageCache } from '../../services/MessageCache.preload.js';
import { getPath } from '../../../app/attachments.node.js';
import { getAbsoluteAttachmentPath } from '../../util/migrations.preload.js';
import { DataWriter } from '../../sql/Client.preload.js';
import type { MessageAttributesType } from '../../model-types.js';
import { itemStorage } from '../../textsecure/Storage.preload.js';
import { generateAci } from '../../types/ServiceId.std.js';
describe('addAttachmentToMessage', () => {
beforeEach(async () => {
await DataWriter.removeAll();
await itemStorage.user.setAciAndDeviceId(generateAci(), 1);
MessageCache.install();
});
afterEach(async () => {
await emptyDir(getPath(window.SignalContext.config.userDataPath));
});
async function saveMessage(
messageOverrides: Partial<MessageAttributesType>
): Promise<MessageAttributesType> {
const message: MessageAttributesType = {
id: v7(),
type: 'incoming',
sent_at: Date.now(),
timestamp: Date.now(),
received_at: Date.now(),
conversationId: 'convoId',
...messageOverrides,
};
await window.MessageCache.saveMessage(message, {
forceSave: true,
});
return message;
}
async function writeAttachmentToDisk(path: string, text: string) {
await writeFile(getAbsoluteAttachmentPath(path), text);
}
async function listAttachmentsOnDisk(): Promise<Array<string>> {
return readdir(getPath(window.SignalContext.config.userDataPath));
}
it('replaces attachment on message', async () => {
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
attachments: [attachment],
});
await addAttachmentToMessage(
messageId,
{
...attachment,
path: '/path/to/attachment',
},
'logid',
{
type: 'attachment',
}
);
const message = await getMessageById(messageId);
assert.deepStrictEqual(message?.attributes.attachments?.[0], {
...attachment,
path: '/path/to/attachment',
});
});
it('throws error if matching attachment not found', async () => {
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
attachments: [attachment],
});
await assert.isRejected(
addAttachmentToMessage(
messageId,
{ ...attachment, digest: randomBytes(32).toString('base64') },
'logid',
{
type: 'attachment',
}
),
'AttachmentNotNeededForMessageError'
);
});
it('throws error if attachment found but already downloaded', async () => {
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
path: '/path/to/attachment',
});
const { id: messageId } = await saveMessage({
attachments: [attachment],
});
await assert.isRejected(
addAttachmentToMessage(messageId, attachment, 'logid', {
type: 'attachment',
}),
'AttachmentNotNeededForMessageError'
);
});
it('replaces preview', async () => {
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
preview: [{ url: 'url', image: attachment }],
});
await addAttachmentToMessage(
messageId,
{
...attachment,
path: '/path/to/attachment',
},
'logid',
{
type: 'preview',
}
);
const message = await getMessageById(messageId);
assert.deepStrictEqual(message?.attributes.preview?.[0].image, {
...attachment,
path: '/path/to/attachment',
});
});
it('replaces preview in edithistory', async () => {
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
editHistory: [
{
timestamp: 1,
received_at: 1,
preview: [{ url: 'url', image: attachment }],
},
],
});
await addAttachmentToMessage(
messageId,
{
...attachment,
path: '/path/to/attachment',
},
'logid',
{
type: 'preview',
}
);
const message = await getMessageById(messageId);
assert.deepStrictEqual(
message?.attributes.editHistory?.[0].preview?.[0].image,
{
...attachment,
path: '/path/to/attachment',
}
);
});
it('replaces quote thumbnail', async () => {
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
quote: {
id: null,
isViewOnce: false,
referencedMessageNotFound: false,
attachments: [{ contentType: MIME.IMAGE_PNG, thumbnail: attachment }],
},
});
await addAttachmentToMessage(
messageId,
{
...attachment,
path: '/path/to/attachment',
},
'logid',
{
type: 'quote',
}
);
const message = await getMessageById(messageId);
assert.deepStrictEqual(
message?.attributes.quote?.attachments[0].thumbnail,
{
...attachment,
path: '/path/to/attachment',
}
);
});
it('replaces quote thumbnail in edit history', async () => {
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
editHistory: [
{
timestamp: 1,
received_at: 1,
quote: {
id: null,
isViewOnce: false,
referencedMessageNotFound: false,
attachments: [
{ contentType: MIME.IMAGE_PNG, thumbnail: attachment },
],
},
},
],
});
await addAttachmentToMessage(
messageId,
{
...attachment,
path: '/path/to/attachment',
},
'logid',
{
type: 'quote',
}
);
const message = await getMessageById(messageId);
assert.deepStrictEqual(
message?.attributes.editHistory?.[0].quote?.attachments[0].thumbnail,
{
...attachment,
path: '/path/to/attachment',
}
);
});
it('replaces sticker', async () => {
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
sticker: {
packId: 'packId',
stickerId: 1,
packKey: 'packKey',
data: attachment,
},
});
await addAttachmentToMessage(
messageId,
{
...attachment,
path: '/path/to/attachment',
},
'logid',
{
type: 'sticker',
}
);
const message = await getMessageById(messageId);
assert.deepStrictEqual(message?.attributes.sticker?.data, {
...attachment,
path: '/path/to/attachment',
});
});
it('replaces contact avatar', async () => {
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
contact: [
{
avatar: {
isProfile: false,
avatar: attachment,
},
},
],
});
await addAttachmentToMessage(
messageId,
{
...attachment,
path: '/path/to/attachment',
},
'logid',
{
type: 'contact',
}
);
const message = await getMessageById(messageId);
assert.deepStrictEqual(message?.attributes.contact?.[0]?.avatar?.avatar, {
...attachment,
path: '/path/to/attachment',
});
});
it('replaces body attachment and deletes file on disk', async () => {
await writeAttachmentToDisk('bodyAttachmentPath', 'attachmenttext');
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
bodyAttachment: attachment,
});
await addAttachmentToMessage(
messageId,
{
...attachment,
path: 'bodyAttachmentPath',
},
'logid',
{
type: 'long-message',
}
);
const message = await getMessageById(messageId);
assert.deepStrictEqual(message?.attributes.body, 'attachmenttext');
assert.deepStrictEqual(message?.attributes.bodyAttachment, {
...attachment,
path: 'bodyAttachmentPath',
});
// attachment is deleted from disk after download
assert.deepEqual(await listAttachmentsOnDisk(), []);
});
it('replaces body attachment in edit history and deletes file on disk', async () => {
await writeAttachmentToDisk('bodyAttachmentPath', 'attachmenttext');
const attachment = composeAttachment({
digest: randomBytes(32).toString('base64'),
});
const { id: messageId } = await saveMessage({
editHistory: [
{
timestamp: 1,
received_at: 1,
bodyAttachment: attachment,
},
],
});
await addAttachmentToMessage(
messageId,
{
...attachment,
path: 'bodyAttachmentPath',
},
'logid',
{
type: 'long-message',
}
);
const message = await getMessageById(messageId);
assert.deepStrictEqual(
message?.attributes.editHistory?.[0].body,
'attachmenttext'
);
assert.deepStrictEqual(
message?.attributes.editHistory?.[0].bodyAttachment,
{
...attachment,
path: 'bodyAttachmentPath',
}
);
// attachment is deleted from disk after download
assert.deepEqual(await listAttachmentsOnDisk(), []);
});
});