From 3eb682790d1f819ab0223e23a18fef9e3bd84f34 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Fri, 14 Aug 2026 14:33:14 -0400 Subject: [PATCH] fix(fs): refuse to rename an entry owned by another user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remove and move both refuse to act on an entry the caller does not own, even when the ACL allows the write — rename had no such guard, so a write-mode share recipient could rename the owner's file, or the shared folder itself, rewriting the owner's whole subtree's paths. rename now takes the acting user and applies the same policy. Co-Authored-By: Claude Fable 5 --- src/backend/controllers/fs/FSController.ts | 3 +- .../controllers/fs/LegacyFSController.ts | 9 +++-- src/backend/services/fs/FSService.test.ts | 34 ++++++++++++++----- src/backend/services/fs/FSService.ts | 15 +++++++- 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/backend/controllers/fs/FSController.ts b/src/backend/controllers/fs/FSController.ts index 3db7e81bb..0a675329e 100644 --- a/src/backend/controllers/fs/FSController.ts +++ b/src/backend/controllers/fs/FSController.ts @@ -1466,7 +1466,8 @@ export class FSController extends PuterController { const entry = await this.#resolveEntryForRequest(body); await this.#assertAccess(actor, entry.path, 'write'); - const renamed = await this.services.fs.rename(entry, newName); + const userId = this.#getActorUserId(req); + const renamed = await this.services.fs.rename(userId, entry, newName); this.#emitGuiItemUpdated(renamed); res.json(this.#toClientEntry(renamed)); } diff --git a/src/backend/controllers/fs/LegacyFSController.ts b/src/backend/controllers/fs/LegacyFSController.ts index d4e1ac734..c89d74229 100644 --- a/src/backend/controllers/fs/LegacyFSController.ts +++ b/src/backend/controllers/fs/LegacyFSController.ts @@ -946,7 +946,8 @@ export class LegacyFSController extends PuterController { 'write', ); - const renamed = await this.services.fs.rename(entry, newName); + const userId = this.#getActorUserId(req); + const renamed = await this.services.fs.rename(userId, entry, newName); await this.#emitGuiEvent('outer.gui.item.updated', renamed); res.json(await toLegacyEntry(this.clients.event, renamed)); }; @@ -1522,7 +1523,11 @@ export class LegacyFSController extends PuterController { targetEntry.path, 'write', ); - const renamed = await this.services.fs.rename(targetEntry, newName); + const renamed = await this.services.fs.rename( + userId, + targetEntry, + newName, + ); await this.#emitGuiEvent('outer.gui.item.updated', renamed); res.json({ ...signEntry(renamed, signingCfg, { diff --git a/src/backend/services/fs/FSService.test.ts b/src/backend/services/fs/FSService.test.ts index eef0a94f1..61889d65a 100644 --- a/src/backend/services/fs/FSService.test.ts +++ b/src/backend/services/fs/FSService.test.ts @@ -2111,7 +2111,7 @@ describe('FSService mkdir, touch, rename and shortcuts', () => { `${user.home}/Documents/before.txt`, 'x', ); - const renamed = await fs.rename(entry, 'after.txt'); + const renamed = await fs.rename(user.userId, entry, 'after.txt'); expect(renamed.name).toBe('after.txt'); expect(renamed.path).toBe(`${user.home}/Documents/after.txt`); @@ -2124,7 +2124,7 @@ describe('FSService mkdir, touch, rename and shortcuts', () => { }); await writeFile(user, `${user.home}/Documents/olddir/inner.txt`, 'x'); - await fs.rename(dir, 'newdir'); + await fs.rename(user.userId, dir, 'newdir'); expect( await entryAt(user, '/Documents/newdir/inner.txt'), @@ -2140,18 +2140,34 @@ describe('FSService mkdir, touch, rename and shortcuts', () => { ); await writeFile(user, `${user.home}/Documents/taken.txt`, 'x'); - expect((await caught(() => fs.rename(entry, 'a/b'))).message).toBe( + expect((await caught(() => fs.rename(user.userId, entry, 'a/b'))).message).toBe( 'Name cannot contain a slash', ); - expect((await caught(() => fs.rename(entry, ' '))).message).toBe( + expect((await caught(() => fs.rename(user.userId, entry, ' '))).message).toBe( 'Name cannot be empty', ); expect( - (await caught(() => fs.rename(entry, 'taken.txt'))).statusCode, + (await caught(() => fs.rename(user.userId, entry, 'taken.txt'))).statusCode, ).toBe(409); // Renaming to the current name is a no-op that returns the same row. - await expect(fs.rename(entry, 'ren.txt')).resolves.toBe(entry); + await expect(fs.rename(user.userId, entry, 'ren.txt')).resolves.toBe(entry); + }); + + it("refuses to rename another user's entry, matching remove and move", async () => { + const other = await makeUser(); + const entry = await writeFile( + user, + `${user.home}/Documents/theirs.txt`, + 'x', + ); + + // A write-mode share recipient passes the ACL but must not be able to + // restructure the owner's tree — the same policy remove/move enforce. + await expect( + fs.rename(other.userId, entry, 'renamed.txt'), + ).rejects.toMatchObject({ statusCode: 403 }); + expect(await entryAt(user, '/Documents/theirs.txt')).not.toBeNull(); }); it('creates a shortcut, conflicts on a taken name and dedupes on request', async () => { @@ -2707,7 +2723,7 @@ describe('FSService copy', () => { expect(first.path).toBe(`${user.home}/Desktop/phantom.txt`); // Renaming the occupant frees the path... - await fs.rename(first, 'phantom-renamed.txt'); + await fs.rename(user.userId, first, 'phantom-renamed.txt'); // ...so an immediate re-copy must succeed. A stale path-cache entry // for the old name used to surface a phantom conflict here — and a @@ -3081,7 +3097,7 @@ describe('FSService — cross-app AppData access', () => { asCalendar(() => fs.remove(owner.userId, { entry: contactsFile })), ).rejects.toMatchObject({ statusCode: 403 }); await expect( - asCalendar(() => fs.rename(contactsFile, 'renamed.json')), + asCalendar(() => fs.rename(owner.userId, contactsFile, 'renamed.json')), ).rejects.toMatchObject({ statusCode: 403 }); const desktop = (await server.stores.fsEntry.getEntryByPath( @@ -3110,7 +3126,7 @@ describe('FSService — cross-app AppData access', () => { it('allows rename once the delete class is granted', async () => { await grant(appDataPermission(contacts.uid, 'fs', 'delete')); const renamed = await asCalendar(() => - fs.rename(contactsFile, 'renamed.json'), + fs.rename(owner.userId, contactsFile, 'renamed.json'), ); expect(renamed.name).toBe('renamed.json'); }); diff --git a/src/backend/services/fs/FSService.ts b/src/backend/services/fs/FSService.ts index de6f5332f..b23ba30a5 100644 --- a/src/backend/services/fs/FSService.ts +++ b/src/backend/services/fs/FSService.ts @@ -3246,7 +3246,20 @@ export class FSService extends PuterService { * Rename an entry in place. The name changes and path rewrites; if the * entry is a directory, descendant paths are rewritten too. */ - async rename(entry: FSEntry, newName: string): Promise { + async rename( + userId: number, + entry: FSEntry, + newName: string, + ): Promise { + if (entry.userId !== userId) { + // Same policy as remove/move: ACL write on a shared entry does not + // extend to restructuring the owner's tree. + throw new HttpError( + 403, + 'Cannot rename an entry owned by another user', + { legacyCode: 'forbidden' }, + ); + } if (newName.includes('/')) throw new HttpError(400, 'Name cannot contain a slash', { legacyCode: 'bad_request',