fix: fs metadata sanitation (#3257)

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

@@ -18,6 +18,7 @@
  */
 
 import type { Request, Response } from 'express';
+import type { Readable } from 'node:stream';
 import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest';
 import { v4 as uuidv4 } from 'uuid';
 import type { Actor } from '../../core/actor.js';
@@ -124,6 +125,14 @@ const makeRes = () => {
 const withActor = async <T>(actor: Actor, fn: () => Promise<T>): Promise<T> =>
     runWithContext({ actor }, fn);
 
+const streamToString = async (stream: Readable): Promise<string> => {
+    const chunks: Buffer[] = [];
+    for await (const chunk of stream) {
+        chunks.push(Buffer.from(chunk as Buffer));
+    }
+    return Buffer.concat(chunks).toString('utf8');
+};
+
 // ── /startBatchWrite ────────────────────────────────────────────────
 
 describe('FSController.startBatchWrites', () => {
@@ -549,9 +558,7 @@ describe('FSController.completeBatchWrites', () => {
             .body as SignedWriteResponse[];
 
         const emitSpy = vi.spyOn(server.clients.event, 'emit');
-        let updatedCall:
-            | (typeof emitSpy.mock.calls)[number]
-            | undefined;
+        let updatedCall: (typeof emitSpy.mock.calls)[number] | undefined;
         try {
             await withActor(actor, () =>
                 controller.completeBatchWrites(
@@ -726,9 +733,9 @@ describe('FSController.statEntry', () => {
             ...makeReq({ body: { path: '/x' }, actor }),
             actor: undefined,
         } as unknown as Request;
-        await expect(
-            controller.statEntry(req, res),
-        ).rejects.toMatchObject({ statusCode: 401 });
+        await expect(controller.statEntry(req, res)).rejects.toMatchObject({
+            statusCode: 401,
+        });
     });
 });
 
@@ -898,9 +905,7 @@ describe('FSController.searchEntries', () => {
             ).toBe(true);
         }
         expect(
-            results.some(
-                (r) => r.path === `/${username}/Documents/${needle}`,
-            ),
+            results.some((r) => r.path === `/${username}/Documents/${needle}`),
         ).toBe(false);
     });
 
@@ -1318,10 +1323,6 @@ describe('FSController.copyEntry', () => {
 // ── /read (readEntry, full read) ────────────────────────────────────
 
 describe('FSController.readEntry (file streaming)', () => {
-    // makeRes here adds a real Writable surface so that
-    // `pipeline(download.body, res)` inside the controller can pipe
-    // the in-memory S3 stream into the test's response and capture
-    // bytes for assertions.
     const makeStreamingRes = () => {
         const captured = {
             statusCode: 200,
@@ -1329,7 +1330,8 @@ describe('FSController.readEntry (file streaming)', () => {
             bodyChunks: [] as Buffer[],
         };
         // eslint-disable-next-line @typescript-eslint/no-require-imports
-        const { Writable } = require('node:stream') as typeof import('node:stream');
+        const { Writable } =
+            require('node:stream') as typeof import('node:stream');
         const writable = new Writable({
             write(chunk: Buffer, _enc, cb) {
                 captured.bodyChunks.push(chunk);
@@ -1389,7 +1391,9 @@ describe('FSController.readEntry (file streaming)', () => {
         // Pipeline awaits the stream-end on success.
         expect(captured.statusCode).toBe(200);
         expect(captured.headers['Content-Type']).toMatch(/text\/plain/);
-        expect(captured.headers['Content-Length']).toBe(String(body.byteLength));
+        expect(captured.headers['Content-Length']).toBe(
+            String(body.byteLength),
+        );
         expect(captured.headers['Content-Disposition']).toMatch(
             /inline; filename=/,
         );
@@ -1833,3 +1837,158 @@ describe('FSController.mkshortcutEntry', () => {
         expect(body.isShortcut).toBe(true);
     });
 });
+
+describe('FSController metadata.objectKey injection', () => {
+    const writeFile = async (
+        actor: Actor,
+        path: string,
+        content: string,
+        metadata?: Record<string, unknown>,
+    ) => {
+        await withActor(actor, () =>
+            controller.write(
+                makeReq({
+                    body: {
+                        fileMetadata: {
+                            path,
+                            size: Buffer.byteLength(content),
+                            contentType: 'text/plain',
+                            overwrite: true,
+                            ...(metadata ? { metadata } : {}),
+                        },
+                        fileContent: content,
+                        encoding: 'utf8',
+                    },
+                    actor,
+                }) as unknown as Request<
+                    Record<string, never>,
+                    null,
+                    import('./requestTypes.js').WriteRequest
+                >,
+                makeRes().res,
+            ),
+        );
+        const entry = await server.stores.fsEntry.getEntryByPath(path, {
+            skipCache: true,
+        });
+        if (!entry) throw new Error(`entry not found after write: ${path}`);
+        return entry;
+    };
+
+    it("does not stream another user's file when a client injects metadata.objectKey on write", async () => {
+        const victim = await makeUser();
+        const attacker = await makeUser();
+        const victimSecret = 'VICTIM-TOP-SECRET-PAYLOAD';
+        const attackerDecoy = 'attacker-own-decoy-bytes';
+
+        const victimEntry = await writeFile(
+            victim.actor,
+            `/${victim.actor.user!.username}/Documents/secret.txt`,
+            victimSecret,
+        );
+
+        const victimRead = await server.services.fs.readContent(victimEntry);
+        expect(await streamToString(victimRead.body)).toBe(victimSecret);
+
+        const attackerEntry = await writeFile(
+            attacker.actor,
+            `/${attacker.actor.user!.username}/Documents/loot.txt`,
+            attackerDecoy,
+            { objectKey: victimEntry.uuid },
+        );
+
+        const persisted = attackerEntry.metadata
+            ? (JSON.parse(attackerEntry.metadata) as Record<string, unknown>)
+            : {};
+        expect(persisted.objectKey).toBeUndefined();
+
+        const attackerRead =
+            await server.services.fs.readContent(attackerEntry);
+        const got = await streamToString(attackerRead.body);
+        expect(got).toBe(attackerDecoy);
+        expect(got).not.toBe(victimSecret);
+    });
+
+    it('read path ignores a divergent metadata.objectKey on an already-poisoned row', async () => {
+        const victim = await makeUser();
+        const attacker = await makeUser();
+        const victimSecret = 'VICTIM-SECRET-FOR-POISON-TEST';
+        const attackerDecoy = 'attacker-decoy-for-poison-test';
+
+        const victimEntry = await writeFile(
+            victim.actor,
+            `/${victim.actor.user!.username}/Documents/secret2.txt`,
+            victimSecret,
+        );
+        const attackerEntry = await writeFile(
+            attacker.actor,
+            `/${attacker.actor.user!.username}/Documents/loot2.txt`,
+            attackerDecoy,
+        );
+
+        await server.stores.fsEntry.updateEntry(attackerEntry.uuid, {
+            metadata: JSON.stringify({ objectKey: victimEntry.uuid }),
+        });
+        const poisoned = await server.stores.fsEntry.getEntryByPath(
+            attackerEntry.path,
+            { skipCache: true },
+        );
+        if (!poisoned) throw new Error('poisoned entry not found');
+        expect(
+            (JSON.parse(poisoned.metadata!) as { objectKey: string }).objectKey,
+        ).toBe(victimEntry.uuid);
+
+        const read = await server.services.fs.readContent(poisoned);
+        expect(await streamToString(read.body)).toBe(attackerDecoy);
+    });
+
+    it('scrubs objectKey from move newMetadata while preserving legit trash metadata', async () => {
+        const victim = await makeUser();
+        const attacker = await makeUser();
+        const victimSecret = 'VICTIM-SECRET-FOR-MOVE-TEST';
+        const attackerDecoy = 'attacker-decoy-for-move-test';
+        const username = attacker.actor.user!.username!;
+
+        const victimEntry = await writeFile(
+            victim.actor,
+            `/${victim.actor.user!.username}/Documents/secret3.txt`,
+            victimSecret,
+        );
+        const attackerEntry = await writeFile(
+            attacker.actor,
+            `/${username}/Documents/loot3.txt`,
+            attackerDecoy,
+        );
+        const documents = await server.stores.fsEntry.getEntryByPath(
+            `/${username}/Documents`,
+            { skipCache: true },
+        );
+        if (!documents) throw new Error('Documents dir not found');
+
+        const moved = await withActor(attacker.actor, () =>
+            server.services.fs.move(attacker.userId, {
+                source: attackerEntry,
+                destinationParent: documents,
+                newName: 'loot3-moved.txt',
+                newMetadata: {
+                    original_path: `/${username}/Documents/loot3.txt`,
+                    trashed_ts: 1700000000,
+                    objectKey: victimEntry.uuid,
+                },
+            }),
+        );
+
+        const persisted = JSON.parse(moved.metadata!) as Record<
+            string,
+            unknown
+        >;
+        expect(persisted.objectKey).toBeUndefined();
+        expect(persisted.original_path).toBe(
+            `/${username}/Documents/loot3.txt`,
+        );
+        expect(persisted.trashed_ts).toBe(1700000000);
+
+        const read = await server.services.fs.readContent(moved);
+        expect(await streamToString(read.body)).toBe(attackerDecoy);
+    });
+});

src/backend/drivers/util/fileInput.ts

@@ -175,9 +175,7 @@ export async function loadFileInput(
     // `path`/`uid`/`uuid` (e.g. AI chat `puter_path` content parts) could
     // exfiltrate any user's file. Must run before the S3 read below.
     await fsService.checkFSAccess(entry, actor, 'read');
-    // S3 object key is recorded in entry.metadata.objectKey when written by
-    // fsv2; older rows fall back to the entry uuid.
-    const objectKey = deriveObjectKey(entry);
+    const objectKey = entry.uuid;
     const { body, contentType, contentLength } =
         await stores.s3Object.getObjectStream(
             {
@@ -244,29 +242,6 @@ function filenameFromMime(mime: string): string {
     return `input.${ext}`;
 }
 
-// Mirrors FSService's private deriveObjectKeyFromEntry helper. fsv2-era
-// rows persist an `objectKey` in metadata; older rows simply use the uuid.
-function deriveObjectKey(entry: {
-    uuid: string;
-    metadata: string | null;
-}): string {
-    if (entry.metadata) {
-        try {
-            const parsed = JSON.parse(entry.metadata);
-            if (
-                parsed &&
-                typeof parsed.objectKey === 'string' &&
-                parsed.objectKey.length > 0
-            ) {
-                return parsed.objectKey;
-            }
-        } catch {
-            // Not JSON — fall through.
-        }
-    }
-    return entry.uuid;
-}
-
 export function inferFilenameFromUrlOrPath(
     value: string,
     fallback = 'input',

src/backend/services/fs/FSService.ts

@@ -70,8 +70,8 @@ import { AclMode } from '../acl/ACLService.js';
 const DEFAULT_CONTENT_TYPE = 'application/octet-stream';
 const DEFAULT_SIGNED_UPLOAD_EXPIRY_SECONDS = 60 * 15;
 
-// AWS SDK v3 surfaces missing-key errors with both `name` and `Code` set to
-// "NoSuchKey". Check both — `Code` is the wire field, `name` is the JS class.
+const RESERVED_METADATA_KEYS: readonly string[] = ['objectKey'];
+
 const isNoSuchKeyError = (err: unknown): boolean => {
     if (!err || typeof err !== 'object') return false;
     const e = err as { name?: unknown; Code?: unknown };
@@ -377,7 +377,7 @@ export class FSService extends PuterService {
             size,
             contentType: metadata.contentType ?? DEFAULT_CONTENT_TYPE,
             checksumSha256: metadata.checksumSha256,
-            metadata: metadata.metadata,
+            metadata: this.#sanitizeClientMetadata(metadata.metadata),
             thumbnail: metadata.thumbnail,
             associatedAppId: metadata.associatedAppId,
             overwrite: Boolean(metadata.overwrite),
@@ -391,6 +391,50 @@ export class FSService extends PuterService {
         };
     }
 
+    #sanitizeClientMetadata(
+        metadata: FSEntryWriteInput['metadata'],
+    ): FSEntryWriteInput['metadata'] {
+        if (metadata === null || metadata === undefined) {
+            return metadata;
+        }
+        if (typeof metadata === 'string') {
+            let parsed: unknown;
+            try {
+                parsed = JSON.parse(metadata);
+            } catch {
+                return metadata;
+            }
+            if (
+                !parsed ||
+                typeof parsed !== 'object' ||
+                Array.isArray(parsed)
+            ) {
+                return metadata;
+            }
+            return JSON.stringify(
+                this.#stripReservedMetadataKeys(
+                    parsed as Record<string, unknown>,
+                ),
+            );
+        }
+        if (typeof metadata === 'object' && !Array.isArray(metadata)) {
+            return this.#stripReservedMetadataKeys(
+                metadata as Record<string, unknown>,
+            );
+        }
+        return metadata;
+    }
+
+    #stripReservedMetadataKeys(
+        record: Record<string, unknown>,
+    ): Record<string, unknown> {
+        const cleaned: Record<string, unknown> = { ...record };
+        for (const key of RESERVED_METADATA_KEYS) {
+            delete cleaned[key];
+        }
+        return cleaned;
+    }
+
     async #findDedupedPath(
         targetPath: string,
         reservedPaths: Set<string>,
@@ -2762,10 +2806,7 @@ export class FSService extends PuterService {
                 { legacyCode: 'shortcut_target_not_found' },
             );
         }
-        // Derive the S3 object key from entry metadata if present, else fall
-        // back to the uuid convention used elsewhere (objectKey defaults to
-        // uuid during write when no metadata override is set).
-        const objectKey = this.#deriveObjectKeyFromEntry(entry);
+        const objectKey = entry.uuid;
         try {
             return await this.stores.s3Object.getObjectStream(
                 {
@@ -2815,27 +2856,6 @@ export class FSService extends PuterService {
         }
     }
 
-    // Objects written by fsv2 use the pending-session's objectKey, which is
-    // persisted in FSEntry.metadata JSON under `objectKey`. Falls back to the
-    // entry uuid for entries that didn't record it (older data).
-    #deriveObjectKeyFromEntry(entry: FSEntry): string {
-        if (entry.metadata) {
-            try {
-                const parsed = JSON.parse(entry.metadata);
-                if (
-                    parsed &&
-                    typeof parsed.objectKey === 'string' &&
-                    parsed.objectKey.length > 0
-                ) {
-                    return parsed.objectKey;
-                }
-            } catch {
-                // Not JSON — fall through.
-            }
-        }
-        return entry.uuid;
-    }
-
     // -- Mutation: mkdir / touch / rename / mkshortcut ---------
 
     /**
@@ -3186,7 +3206,7 @@ export class FSService extends PuterService {
             try {
                 await this.stores.s3Object.deleteObject(
                     entry.bucket,
-                    this.#deriveObjectKeyFromEntry(entry),
+                    entry.uuid,
                     entry.bucketRegion,
                 );
             } catch {
@@ -3280,7 +3300,7 @@ export class FSService extends PuterService {
                 region: child.bucketRegion,
                 keys: [],
             };
-            group.keys.push(this.#deriveObjectKeyFromEntry(child));
+            group.keys.push(child.uuid);
             grouped.set(groupKey, group);
             // Fire individual removal events so thumbnail extension can clean up.
             this.#emitRemoveEvent(child);
@@ -3420,13 +3440,12 @@ export class FSService extends PuterService {
                 ? `/${name}`
                 : `${destinationParent.path}/${name}`;
 
-        // `metadata` column is a TEXT field; serialize when the caller sends
-        // an object, pass-through a bare string, and `null` clears it.
-        // `undefined` leaves the column untouched.
         let metadataPatch: string | null | undefined;
         if (input.newMetadata === null) metadataPatch = null;
-        else if (typeof input.newMetadata === 'object')
-            metadataPatch = JSON.stringify(input.newMetadata);
+        else if (input.newMetadata && typeof input.newMetadata === 'object')
+            metadataPatch = JSON.stringify(
+                this.#stripReservedMetadataKeys(input.newMetadata),
+            );
 
         const updated = await this.stores.fsEntry.updateEntry(source.uuid, {
             name,
@@ -3627,10 +3646,8 @@ export class FSService extends PuterService {
             });
         }
 
-        // Regular file: duplicate the S3 object under a new key (the new
-        // entry's uuid), then insert the DB row pointing at it.
         const newUuid = uuidv4();
-        const sourceObjectKey = this.#deriveObjectKeyFromEntry(source);
+        const sourceObjectKey = source.uuid;
         const resolvedBucket = this.stores.s3Object.resolveBucket(
             source.bucket,
         );
@@ -3644,14 +3661,8 @@ export class FSService extends PuterService {
             this.stores.s3Object.resolveRegion(source.bucketRegion),
         );
 
-        // Re-serialize metadata, swapping in the new objectKey.
-        const nextMetadata = this.#metadataWithObjectKey(
-            source.metadata,
-            newUuid,
-        );
+        const nextMetadata = this.#sanitizeClientMetadata(source.metadata);
 
-        // Insert as a file row. We reuse the files INSERT path (batchCreateEntries)
-        // since it handles bucket/metadata correctly. A single-row call is fine.
         const [created] = await this.stores.fsEntry.batchCreateEntries(
             [
                 {
@@ -3697,27 +3708,6 @@ export class FSService extends PuterService {
         return created;
     }
 
-    // Preserves existing metadata JSON fields, overriding only objectKey.
-    #metadataWithObjectKey(metadata: string | null, objectKey: string): string {
-        let parsed: Record<string, unknown> = {};
-        if (metadata) {
-            try {
-                const tentative = JSON.parse(metadata);
-                if (
-                    tentative &&
-                    typeof tentative === 'object' &&
-                    !Array.isArray(tentative)
-                ) {
-                    parsed = tentative as Record<string, unknown>;
-                }
-            } catch {
-                // Non-JSON legacy metadata — drop and replace.
-            }
-        }
-        parsed.objectKey = objectKey;
-        return JSON.stringify(parsed);
-    }
-
     /**
      * This method checks if the specified actor has permission to access the entry provided. It will throw an error if the actor is not permitted
      */