fix: thumbnail res (#3465)

extensions/thumbnails.test.ts

@@ -3,6 +3,7 @@ import {
     PutObjectCommand,
     type S3Client,
 } from '@aws-sdk/client-s3';
+import crypto from 'node:crypto';
 import {
     afterAll,
     beforeAll,
@@ -26,6 +27,9 @@ const TINY_PNG_BASE64 =
 
 const BUCKET = 'puter-local';
 
+// Keys the extension will accept back: its own `thumbnails/<uuid>` namespace.
+const mintedKey = () => `thumbnails/${crypto.randomUUID()}`;
+
 const streamToBuffer = async (
     body: { transformToByteArray: () => Promise<Uint8Array> } | undefined,
 ): Promise<Buffer> => {
@@ -195,7 +199,7 @@ describe('thumbnails extension — handleThumbnailRead', () => {
         // Seed an object so the presigned URL points at something real
         // (the signer itself doesn't validate existence, but this keeps
         // the test honest).
-        const key = 'thumb-read-test';
+        const key = mintedKey();
         await s3.send(
             new PutObjectCommand({
                 Bucket: BUCKET,
@@ -220,6 +224,57 @@ describe('thumbnails extension — handleThumbnailRead', () => {
         expect((entry.thumbnail as string).startsWith('http')).toBe(true);
     });
 
+    // `fsentries.thumbnail` is writable through the FS API, so a stored
+    // pointer is attacker input. Signing one the extension didn't mint would
+    // hand out a presigned read of an arbitrary object — including another
+    // user's file, whose key is its fsentry uuid in this same bucket.
+    it.each([
+        // Shaped exactly like an fs object key (and like a legacy thumbnail
+        // row) — indistinguishable from a planted pointer, so it fails closed.
+        [
+            "another user's file object",
+            `s3://${BUCKET}/${crypto.randomUUID()}`,
+        ],
+        [
+            'a key outside the thumbnails namespace',
+            `s3://${BUCKET}/secrets/dump`,
+        ],
+        [
+            'a namespace-lookalike key',
+            `s3://${BUCKET}/thumbnails/../${crypto.randomUUID()}`,
+        ],
+        ['a non-uuid inside the namespace', `s3://${BUCKET}/thumbnails/etc`],
+    ])('refuses to presign a pointer naming %s', async (_label, thumbnail) => {
+        const entry: Record<string, unknown> = { thumbnail };
+        await handleThumbnailRead(entry, {
+            s3,
+            s3Presign,
+            bucketName: BUCKET,
+            bucketEndpoint: 'http://127.0.0.1:4566/puter-local/',
+            db: stubDb,
+        });
+        expect(entry.thumbnail).toBeNull();
+    });
+
+    it('signs against its own bucket, ignoring the one in the pointer', async () => {
+        const key = mintedKey();
+        const entry: Record<string, unknown> = {
+            thumbnail: `s3://attacker-named-bucket/${key}`,
+        };
+        await handleThumbnailRead(entry, {
+            s3,
+            s3Presign,
+            bucketName: BUCKET,
+            bucketEndpoint: 'http://127.0.0.1:4566/puter-local/',
+            db: stubDb,
+        });
+        // Signed for OUR bucket; `attacker-named-bucket` never reached S3.
+        const signed = entry.thumbnail as string;
+        expect(signed.startsWith('http')).toBe(true);
+        expect(signed).not.toContain('attacker-named-bucket');
+        expect(signed).toContain(BUCKET);
+    });
+
     it('leaves the thumbnail untouched when not s3/https/data', async () => {
         const entry: Record<string, unknown> = { thumbnail: 'about:blank' };
         await handleThumbnailRead(entry, {
@@ -285,7 +340,7 @@ describe('thumbnails extension — handleFsRemoveNodeThumbnail', () => {
     });
 
     it('deletes the S3 object referenced by an s3:// thumbnail URL', async () => {
-        const key = 'thumb-remove-test';
+        const key = mintedKey();
         await s3.send(
             new PutObjectCommand({
                 Bucket: BUCKET,
@@ -297,7 +352,7 @@ describe('thumbnails extension — handleFsRemoveNodeThumbnail', () => {
 
         await handleFsRemoveNodeThumbnail(
             { target: { thumbnail: `s3://${BUCKET}/${key}` } },
-            { s3 },
+            { s3, bucketName: BUCKET },
         );
 
         // GetObject should now error because the key was deleted.
@@ -306,15 +361,43 @@ describe('thumbnails extension — handleFsRemoveNodeThumbnail', () => {
         ).rejects.toThrow();
     });
 
+    // The destructive half of the same confused deputy: the stored pointer
+    // decides which object is deleted, so a key the extension didn't mint
+    // must never reach DeleteObject.
+    it('does not delete an object the pointer names but we did not mint', async () => {
+        const victimKey = crypto.randomUUID(); // shaped like an fs object key
+        await s3.send(
+            new PutObjectCommand({
+                Bucket: BUCKET,
+                Key: victimKey,
+                Body: Buffer.from(TINY_PNG_BASE64, 'base64'),
+                ContentType: 'image/png',
+            }),
+        );
+
+        await handleFsRemoveNodeThumbnail(
+            { target: { thumbnail: `s3://${BUCKET}/${victimKey}` } },
+            { s3, bucketName: BUCKET },
+        );
+
+        const survivor = await s3.send(
+            new GetObjectCommand({ Bucket: BUCKET, Key: victimKey }),
+        );
+        expect(survivor.ContentType).toBe('image/png');
+    });
+
     it('is a no-op when the target has no thumbnail', async () => {
         // Should not throw or attempt a delete.
-        await handleFsRemoveNodeThumbnail({ target: {} }, { s3 });
+        await handleFsRemoveNodeThumbnail(
+            { target: {} },
+            { s3, bucketName: BUCKET },
+        );
     });
 
     it('is a no-op when the thumbnail URL is not an s3:// pointer', async () => {
         await handleFsRemoveNodeThumbnail(
             { target: { thumbnail: 'https://cdn.example.com/x.png' } },
-            { s3 },
+            { s3, bucketName: BUCKET },
         );
     });
 });

extensions/thumbnails.ts

@@ -13,6 +13,54 @@ const clients = extension.import('client');
 const MAX_THUMBNAIL_BYTES = 2 * 1024 * 1024;
 const MAX_THUMBNAIL_PIXELS = 64e6;
 
+// Namespace every object this extension writes. An fs object's key is its
+// bare fsentry uuid and the default config points `thumbnailStore.name` at
+// the same bucket as `s3_bucket`, so without a prefix of our own there is no
+// way to tell a thumbnail we minted from any other object in the deployment.
+const THUMBNAIL_KEY_PREFIX = 'thumbnails/';
+const UUID_PATTERN =
+    /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
+
+const mintThumbnailKey = (): string =>
+    `${THUMBNAIL_KEY_PREFIX}${crypto.randomUUID()}`;
+
+/**
+ * Extract the object key from a stored thumbnail pointer, or null when the
+ * pointer isn't one this extension minted.
+ *
+ * `fsentries.thumbnail` is writable through the FS API, so neither half of the
+ * stored string is trusted: the bucket is discarded (callers always pass their
+ * own) and the key must sit under {@link THUMBNAIL_KEY_PREFIX} with a random
+ * uuid. Honouring an arbitrary key would lend this extension's storage
+ * credentials to whatever object the caller named — in the shared-bucket layout
+ * that is every user's file, since an fs object's key is its fsentry uuid.
+ * Legacy bare-uuid thumbnails fail the check and are treated as absent; they
+ * are indistinguishable from a planted pointer, so there is nothing safer to do
+ * with them than stop signing them.
+ */
+const resolveThumbnailKey = (pointer: string): string | null => {
+    let key: string;
+    if (pointer.startsWith('s3://')) {
+        const rest = pointer.slice('s3://'.length);
+        const slash = rest.indexOf('/');
+        if (slash === -1) return null;
+        key = rest.slice(slash + 1);
+    } else {
+        let pathname: string;
+        try {
+            pathname = new URL(pointer).pathname;
+        } catch {
+            return null;
+        }
+        const segments = pathname.replace(/^\/+/, '').split('/');
+        segments.shift(); // bucket
+        key = segments.join('/');
+    }
+    if (!key.startsWith(THUMBNAIL_KEY_PREFIX)) return null;
+    const id = key.slice(THUMBNAIL_KEY_PREFIX.length);
+    return UUID_PATTERN.test(id) ? key : null;
+};
+
 // S3 client + bucket config — lazily resolved after boot from config.
 let s3Client: S3Client | null = null;
 let s3PresignClient: S3Client | null = null;
@@ -112,7 +160,7 @@ export async function handleThumbnailCreated(
         return;
     }
 
-    const key = crypto.randomUUID();
+    const key = mintThumbnailKey();
     event.url = `s3://${deps.bucketName}/${key}`;
 
     await deps.s3.send(
@@ -151,7 +199,7 @@ export const handleThumbnailUploadPrepare = async (
                 continue;
         }
 
-        const key = crypto.randomUUID();
+        const key = mintThumbnailKey();
         const command = new PutObjectCommand({
             Bucket: deps.bucketName,
             Key: key,
@@ -178,27 +226,27 @@ export const handleThumbnailRead = async (
     if (typeof thumb !== 'string' || !thumb) return;
     const presignClient = deps.s3Presign;
 
-    if (thumb.startsWith('s3://')) {
-        const [bucket, key] = thumb.slice(5).split('/');
-        entry.thumbnail = await getSignedUrl(
-            presignClient,
-            new GetObjectCommand({ Bucket: bucket, Key: key }),
-            { expiresIn: 604800 },
-        );
-    } else if (
-        thumb.startsWith('https') &&
-        thumb.includes(new URL(deps.bucketEndpoint).hostname)
-    ) {
+    if (
+        thumb.startsWith('s3://') ||
         // Legacy format — remove after full migration
-        const [bucket, key] = new URL(thumb).pathname.slice(1).split('/');
+        (thumb.startsWith('https') &&
+            thumb.includes(new URL(deps.bucketEndpoint).hostname))
+    ) {
+        const key = resolveThumbnailKey(thumb);
+        if (!key) {
+            // Not a pointer we minted — refuse to sign it rather than hand
+            // out a presigned read of whatever object it names.
+            entry.thumbnail = null;
+            return;
+        }
         entry.thumbnail = await getSignedUrl(
             presignClient,
-            new GetObjectCommand({ Bucket: bucket, Key: key }),
+            new GetObjectCommand({ Bucket: deps.bucketName, Key: key }),
             { expiresIn: 604800 },
         );
     } else if (thumb.startsWith('data')) {
         // Inline data-URL migration: upload to S3 and update the DB entry.
-        const key = crypto.randomUUID();
+        const key = mintThumbnailKey();
         const { mimeType, data } = base64ParseDataUrl(thumb);
         const newUrl = `s3://${deps.bucketName}/${key}`;
 
@@ -233,14 +281,22 @@ export const handleThumbnailRead = async (
 };
 
 export const handleFsRemoveNodeThumbnail = async (
-    payload: { target: Record<string, unknown> },
-    deps: { s3: S3Client },
+    payload: { target: { thumbnail?: string | null } },
+    deps: { s3: S3Client; bucketName: string },
 ): Promise<void> => {
-    const thumbnailUrl = payload.target.thumbnail as string | undefined;
-    if (!thumbnailUrl || !thumbnailUrl.startsWith('s3://')) return;
+    const thumbnailUrl = payload.target.thumbnail;
+    if (!thumbnailUrl) return;
 
-    const [bucket, key] = thumbnailUrl.slice(5).split('/');
-    await deps.s3.send(new DeleteObjectCommand({ Bucket: bucket, Key: key }));
+    // Same trust rule as the read path, and load-bearing for the same reason:
+    // the pointer decides which object gets deleted, so a key we didn't mint
+    // would let the owner of one file destroy an object belonging to someone
+    // else just by naming it here.
+    const key = resolveThumbnailKey(thumbnailUrl);
+    if (!key) return;
+
+    await deps.s3.send(
+        new DeleteObjectCommand({ Bucket: deps.bucketName, Key: key }),
+    );
 };
 
 extension.on(
@@ -282,9 +338,9 @@ extension.on('thumbnail.read', async (_key, entry: Record<string, unknown>) => {
 // -- fs.remove.node --------------------------------------------------
 // Delete S3 thumbnail when the file is removed.
 
-extension.on(
-    'fs.remove.node',
-    async (_key, payload: { target: Record<string, unknown> }) => {
-        await handleFsRemoveNodeThumbnail(payload, { s3: getClient() });
-    },
-);
+extension.on('fs.remove.node', async (_key, payload) => {
+    await handleFsRemoveNodeThumbnail(payload, {
+        s3: getClient(),
+        bucketName: thumbnailBucketName,
+    });
+});

src/backend/controllers/fs/LegacyFSController.test.ts

@@ -2743,6 +2743,48 @@ describe('LegacyFSController.updateFsentryThumbnail', () => {
         const body = captured.body as { thumbnail: string };
         expect(typeof body.thumbnail).toBe('string');
     });
+
+    // The write ACL here covers the entry being annotated, not whatever the
+    // thumbnail string points at. A storage pointer stored verbatim is later
+    // presigned (and deleted) by the thumbnails extension using the server's
+    // own credentials, so the owner of one file could name another user's
+    // object — an fs object's key is its fsentry uuid — and have the server
+    // read or destroy it. Only inline image data is accepted.
+    it.each([
+        ['an s3:// pointer', 's3://puter-local/00000000-0000-4000-8000-000000000000'],
+        ['an https URL', 'https://cdn.example.com/x.png'],
+        ['a bare object key', 'thumbnails/whatever'],
+    ])('rejects %s instead of storing it verbatim', async (_label, thumbnail) => {
+        const { actor } = await makeUser();
+        const username = actor.user!.username!;
+        const target = `/${username}/Documents/thumbme3-${uuidv4()}.txt`;
+        await withActor(actor, () =>
+            controller.touch(
+                makeReq({
+                    body: { path: target, set_modified_to_now: true },
+                    actor,
+                }),
+                makeRes().res,
+            ),
+        );
+        const entry = await server.stores.fsEntry.getEntryByPath(target);
+
+        const { res } = makeRes();
+        await expect(
+            withActor(actor, () =>
+                controller.updateFsentryThumbnail(
+                    makeReq({
+                        body: { uid: entry!.uuid, thumbnail },
+                        actor,
+                    }),
+                    res,
+                ),
+            ),
+        ).rejects.toMatchObject({ statusCode: 400 });
+
+        const after = await server.stores.fsEntry.getEntryByUuid(entry!.uuid);
+        expect(after?.thumbnail ?? null).toBeNull();
+    });
 });
 
 // ── GET /get-launch-apps ────────────────────────────────────────────

src/backend/controllers/fs/LegacyFSController.ts

@@ -863,6 +863,14 @@ export class LegacyFSController extends PuterController {
             throw new HttpError(400, 'Missing `thumbnail`', {
                 legacyCode: 'bad_request',
             });
+        // Only inline image data. Clients generate the thumbnail themselves
+        // and the thumbnails extension is what turns it into a storage
+        // pointer; accepting a pointer here would let a caller name an object
+        // the server would then sign reads of, and delete, on their behalf.
+        if (!thumbnail.startsWith('data:'))
+            throw new HttpError(400, '`thumbnail` must be a data: URL', {
+                legacyCode: 'bad_request',
+            });
 
         const entry = await this.stores.fsEntry.getEntryByUuid(uid);
         if (!entry || !entry.path)