From a76800d693f5b4bcf41e514ff5d9a035e77364ef Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Sun, 4 Oct 2026 21:11:00 -0700 Subject: [PATCH] fix(thumbnails): duplicate a copied thumbnail with GetObject + PutObject instead of CopyObject (#4072) * fix(thumbnails): duplicate a copied thumbnail with GetObject + PutObject instead of CopyObject CopyObject names its source as `/` in a header, which skips any path in the client's endpoint, so it can miss an object that Bucket/Key requests reach. The copy handler now reads the source object (still behind the HeadObject size bound) and writes it under the copy's key with its content type. * fix(thumbnails): check the GetObject length before buffering a copied thumbnail The size check runs on a HEAD and the read on a separate GET, so an object replaced in between was read whole into memory. Reject a GET response whose length exceeds the cap and destroy its body before reading. --- extensions/thumbnails.test.ts | 194 +++++++++++++++++++++++++++++++++- extensions/thumbnails.ts | 19 +++- 2 files changed, 207 insertions(+), 6 deletions(-) diff --git a/extensions/thumbnails.test.ts b/extensions/thumbnails.test.ts index 2d745a440..8dddd097b 100644 --- a/extensions/thumbnails.test.ts +++ b/extensions/thumbnails.test.ts @@ -2,7 +2,7 @@ import { GetObjectCommand, HeadObjectCommand, PutObjectCommand, - type S3Client, + S3Client, } from '@aws-sdk/client-s3'; import type { Request, Response } from 'express'; import crypto from 'node:crypto'; @@ -563,15 +563,44 @@ describe('thumbnails extension — handleFsCopyNodeThumbnail', () => { await server?.shutdown(); }); - const copyNode = async (thumbnail: string | null, copyUuid: string) => { + const copyNode = async ( + thumbnail: string | null, + copyUuid: string, + client: S3Client = s3, + ) => { const db = { write: vi.fn().mockResolvedValue(undefined) }; await handleFsCopyNodeThumbnail( { copy: { thumbnail, uuid: copyUuid } }, - { s3, bucketName: BUCKET, bucketEndpoint: BUCKET_ENDPOINT, db }, + { + s3: client, + bucketName: BUCKET, + bucketEndpoint: BUCKET_ENDPOINT, + db, + }, ); return db; }; + // The key the copied row was repointed at, asserting it is bound to it. + const repointedKey = ( + db: Awaited>, + copyUuid: string, + ): string => { + expect(db.write).toHaveBeenCalledTimes(1); + const [sql, [pointer, uuid]] = db.write.mock.calls[0] as [ + string, + [string, string], + ]; + expect(sql).toBe( + 'UPDATE `fsentries` SET `thumbnail` = ? WHERE `uuid` = ?', + ); + expect(uuid).toBe(copyUuid); + expect(pointer).toMatch( + new RegExp(`^s3://${BUCKET}/thumbnails/${copyUuid}/`), + ); + return pointer.slice(`s3://${BUCKET}/`.length); + }; + it.each([ ['an entry-bound key', () => mintedKey()], ['a key that predates entry binding', legacyKey], @@ -603,6 +632,7 @@ describe('thumbnails extension — handleFsCopyNodeThumbnail', () => { const duplicated = await s3.send( new GetObjectCommand({ Bucket: BUCKET, Key: newKey }), ); + expect(duplicated.ContentType).toBe('image/png'); expect( (await streamToBuffer(duplicated.Body as never)).equals(body), ).toBe(true); @@ -610,6 +640,72 @@ describe('thumbnails extension — handleFsCopyNodeThumbnail', () => { }, ); + // The SDK puts an endpoint's path in front of every Bucket/Key request but + // not in front of a CopyObject source, so on such a store HeadObject finds + // the source and CopyObject reports NoSuchKey. + it('duplicates through a client whose endpoint carries a path', async () => { + const endpoint = await s3.config.endpoint!(); + const client = new S3Client({ + region: await s3.config.region(), + endpoint: `${endpoint.protocol}//${endpoint.hostname}:${endpoint.port}/${BUCKET}`, + credentials: await s3.config.credentials(), + forcePathStyle: true, + }); + const sourceKey = mintedKey(); + const body = Buffer.from(TINY_PNG_BASE64, 'base64'); + await putObject(client, sourceKey, body); + + const copyUuid = crypto.randomUUID(); + const db = await copyNode( + `s3://${BUCKET}/${sourceKey}`, + copyUuid, + client, + ); + + const duplicated = await client.send( + new GetObjectCommand({ + Bucket: BUCKET, + Key: repointedKey(db, copyUuid), + }), + ); + expect(duplicated.ContentType).toBe('image/png'); + expect( + (await streamToBuffer(duplicated.Body as never)).equals(body), + ).toBe(true); + }); + + it("lets either entry's removal leave the other's thumbnail in place", async () => { + const sourceUuid = crypto.randomUUID(); + const sourceKey = mintedKey(sourceUuid); + const sourcePointer = `s3://${BUCKET}/${sourceKey}`; + await putObject(s3, sourceKey, Buffer.from(TINY_PNG_BASE64, 'base64')); + const remove = (thumbnail: string, uuid: string) => + handleFsRemoveNodeThumbnail( + { target: { thumbnail, uuid } }, + { s3, bucketName: BUCKET, bucketEndpoint: BUCKET_ENDPOINT }, + ); + + // Removing a copy leaves the source's object... + const firstUuid = crypto.randomUUID(); + const firstKey = repointedKey( + await copyNode(sourcePointer, firstUuid), + firstUuid, + ); + await remove(`s3://${BUCKET}/${firstKey}`, firstUuid); + expect(await objectExists(s3, firstKey)).toBe(false); + expect(await objectExists(s3, sourceKey)).toBe(true); + + // ...and removing the source leaves a copy's. + const secondUuid = crypto.randomUUID(); + const secondKey = repointedKey( + await copyNode(sourcePointer, secondUuid), + secondUuid, + ); + await remove(sourcePointer, sourceUuid); + expect(await objectExists(s3, sourceKey)).toBe(false); + expect(await objectExists(s3, secondKey)).toBe(true); + }); + it('drops the pointer when the shared object is already gone', async () => { const copyUuid = crypto.randomUUID(); const db = await copyNode( @@ -638,6 +734,40 @@ describe('thumbnails extension — handleFsCopyNodeThumbnail', () => { expect(params).toEqual([copyUuid]); }); + it('drops the pointer when the object outgrows the bound after its size was checked', async () => { + const sourceKey = mintedKey(); + await putObject(s3, sourceKey, Buffer.from(TINY_PNG_BASE64, 'base64')); + const sent: unknown[] = []; + // Replaces the object between the HEAD and the GET. + const racing = { + send: async (command: unknown) => { + sent.push(command); + const result = await s3.send(command as never); + if (command instanceof HeadObjectCommand) { + await putObject( + s3, + sourceKey, + Buffer.alloc(2 * 1024 * 1024 + 1), + ); + } + return result; + }, + } as unknown as S3Client; + + const copyUuid = crypto.randomUUID(); + const db = await copyNode( + `s3://${BUCKET}/${sourceKey}`, + copyUuid, + racing, + ); + + expect(db.write).toHaveBeenCalledTimes(1); + const [sql, params] = db.write.mock.calls[0] as [string, [string]]; + expect(sql).toContain('NULL'); + expect(params).toEqual([copyUuid]); + expect(sent.some((c) => c instanceof PutObjectCommand)).toBe(false); + }); + it('does not duplicate an object the pointer names but we did not mint', async () => { const foreignKey = crypto.randomUUID(); // shaped like an fs object key const db = await copyNode( @@ -828,4 +958,62 @@ describe('thumbnails extension — signed batch upload through /fs', () => { ); expect(entry.thumbnail).toBe('https://example.com/thumb.png'); }); + + it('gives a copy its own thumbnail that survives moves and the source being removed', async () => { + const { actor, username } = await makeActor(); + const s3 = server.clients.s3.get(); + const fs = server.services.fs; + const { entry: source } = await upload( + actor, + `/${username}/Documents/photo.png`, + (s) => s.thumbnailUrl, + ); + const userId = source.userId; + const sourceKey = source.thumbnail!.slice(`s3://${BUCKET}/`.length); + const desktop = (await server.stores.fsEntry.getEntryByPath( + `/${username}/Desktop`, + ))!; + // The listener writes the row directly, so read it the same way. + const storedThumbnail = async (uuid: string) => { + const [row] = (await server.clients.db.read( + 'SELECT `thumbnail` FROM `fsentries` WHERE `uuid` = ?', + [uuid], + )) as Array<{ thumbnail: string | null }>; + return row?.thumbnail ?? null; + }; + + const copy = await runWithContext({ actor }, () => + fs.copy(userId, { source, destinationParent: desktop }), + ); + // `fs.copy.node` is fire-and-forget. + const copyPointer = await vi.waitFor(async () => { + const pointer = await storedThumbnail(copy.uuid); + expect(pointer).toMatch( + new RegExp(`^s3://${BUCKET}/thumbnails/${copy.uuid}/`), + ); + return pointer!; + }); + const copyKey = copyPointer.slice(`s3://${BUCKET}/`.length); + + const documents = (await server.stores.fsEntry.getEntryByPath( + `/${username}/Documents`, + ))!; + const moved = await runWithContext({ actor }, () => + fs.move(userId, { + source: copy, + destinationParent: documents, + newName: 'moved.png', + }), + ); + expect(moved.uuid).toBe(copy.uuid); + expect(await storedThumbnail(moved.uuid)).toBe(copyPointer); + + await runWithContext({ actor }, () => + fs.remove(userId, { entry: source }), + ); + await vi.waitFor(async () => { + expect(await objectExists(s3, sourceKey)).toBe(false); + }); + expect(await objectExists(s3, copyKey)).toBe(true); + }); }); diff --git a/extensions/thumbnails.ts b/extensions/thumbnails.ts index da24d62f3..a6674e47f 100644 --- a/extensions/thumbnails.ts +++ b/extensions/thumbnails.ts @@ -1,5 +1,4 @@ import { - CopyObjectCommand, DeleteObjectCommand, GetObjectCommand, HeadObjectCommand, @@ -10,6 +9,7 @@ import { getSignedUrl } from '@aws-sdk/s3-request-presigner'; import { extension } from '@heyputer/backend/src/extensions'; import { isMissingObjectError } from '@heyputer/backend/src/stores/fs/S3ObjectStore'; import crypto from 'node:crypto'; +import type { Readable } from 'node:stream'; import sharp from 'sharp'; const clients = extension.import('client'); @@ -396,11 +396,24 @@ export const handleFsCopyNodeThumbnail = async ( ) { throw new Error('thumbnail exceeds the size bound'); } + // Read and rewrite rather than CopyObject: a copy source skips any path + // in the client's endpoint, so it can miss an object Bucket/Key reach. + const source = await deps.s3.send( + new GetObjectCommand({ Bucket: deps.bucketName, Key: sourceKey }), + ); + // The object can be replaced after the HEAD; bound what gets buffered. + if ((source.ContentLength ?? 0) > MAX_THUMBNAIL_BYTES) { + (source.Body as Readable | undefined)?.destroy(); + throw new Error('thumbnail exceeds the size bound'); + } + const body = await source.Body?.transformToByteArray(); + if (!body) throw new Error('thumbnail has no body'); await deps.s3.send( - new CopyObjectCommand({ + new PutObjectCommand({ Bucket: deps.bucketName, - CopySource: `${deps.bucketName}/${sourceKey}`, Key: newKey, + Body: body, + ContentType: source.ContentType, }), ); } catch (err) {