diff --git a/src/backend/controllers/fs/FSController.test.ts b/src/backend/controllers/fs/FSController.test.ts index 2231940d7..4fbec0c0b 100644 --- a/src/backend/controllers/fs/FSController.test.ts +++ b/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 (actor: Actor, fn: () => Promise): Promise => runWithContext({ actor }, fn); +const streamToString = async (stream: Readable): Promise => { + 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, + ) => { + 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, + 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) + : {}; + 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); + }); +}); diff --git a/src/backend/drivers/util/fileInput.ts b/src/backend/drivers/util/fileInput.ts index 32ef4fa92..0e8b91347 100644 --- a/src/backend/drivers/util/fileInput.ts +++ b/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', diff --git a/src/backend/services/fs/FSService.ts b/src/backend/services/fs/FSService.ts index 691d0268b..adfea5e65 100644 --- a/src/backend/services/fs/FSService.ts +++ b/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, + ), + ); + } + if (typeof metadata === 'object' && !Array.isArray(metadata)) { + return this.#stripReservedMetadataKeys( + metadata as Record, + ); + } + return metadata; + } + + #stripReservedMetadataKeys( + record: Record, + ): Record { + const cleaned: Record = { ...record }; + for (const key of RESERVED_METADATA_KEYS) { + delete cleaned[key]; + } + return cleaned; + } + async #findDedupedPath( targetPath: string, reservedPaths: Set, @@ -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 = {}; - if (metadata) { - try { - const tentative = JSON.parse(metadata); - if ( - tentative && - typeof tentative === 'object' && - !Array.isArray(tentative) - ) { - parsed = tentative as Record; - } - } 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 */