From 389f5628572e0cc3d3b45d620a9863e7adcd3179 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Fri, 14 Aug 2026 17:18:45 -0400 Subject: [PATCH] fix(fs): authorize restructuring by write on the parent --- src/backend/services/fs/FSService.test.ts | 114 ++++++++++++++++++++++ src/backend/services/fs/FSService.ts | 62 ++++++------ 2 files changed, 148 insertions(+), 28 deletions(-) diff --git a/src/backend/services/fs/FSService.test.ts b/src/backend/services/fs/FSService.test.ts index 3bdee8bf4..edb2ae6c1 100644 --- a/src/backend/services/fs/FSService.test.ts +++ b/src/backend/services/fs/FSService.test.ts @@ -2762,6 +2762,120 @@ describe('FSService copy', () => { }); }); +describe('FSService restructuring a shared tree', () => { + let owner: TestUser; + let holder: TestUser; + let shared: FSEntry; + + const shareWrite = (entry: FSEntry) => + server.services.acl.setUserUser( + owner.actor, + holder.actor, + { + path: entry.path, + resolveAncestors: () => fs.getAncestorChain(entry.path), + }, + 'write', + ); + + const asHolder = (run: () => Promise): Promise => + runWithContext({ actor: holder.actor }, run); + + const stillThere = async (path: string) => + (await server.stores.fsEntry.getEntryByPath(path, { + skipCache: true, + })) !== null; + + beforeAll(async () => { + owner = await makeUser(); + holder = await makeUser(); + shared = await fs.mkdir(owner.userId, { + path: `${owner.home}/Documents/Contents`, + }); + await shareWrite(shared); + }); + + it('lets a recipient delete a file inside the shared folder', async () => { + const file = await writeFile(owner, `${shared.path}/gone.txt`, 'x'); + + await asHolder(() => fs.remove(holder.userId, { entry: file })); + + expect(await stillThere(file.path)).toBe(false); + }); + + it('lets a recipient delete a subfolder of the shared folder', async () => { + const sub = await fs.mkdir(owner.userId, { + path: `${shared.path}/sub`, + }); + await writeFile(owner, `${sub.path}/deep.txt`, 'x'); + + await asHolder(() => + fs.remove(holder.userId, { entry: sub, recursive: true }), + ); + + expect(await stillThere(sub.path)).toBe(false); + expect(await stillThere(`${sub.path}/deep.txt`)).toBe(false); + }); + + it('lets a recipient rename a file inside the shared folder', async () => { + const file = await writeFile(owner, `${shared.path}/before.txt`, 'x'); + + const renamed = await asHolder(() => + fs.rename(holder.userId, file, 'after.txt'), + ); + + expect(renamed.path).toBe(`${shared.path}/after.txt`); + expect(renamed.userId).toBe(owner.userId); + }); + + it('refuses to let a recipient delete the shared folder itself', async () => { + const error = await caught(() => + asHolder(() => + fs.remove(holder.userId, { entry: shared, recursive: true }), + ), + ); + + expect(error.statusCode).toBe(403); + expect(error.legacyCode).toBe('forbidden'); + expect(await stillThere(shared.path)).toBe(true); + }); + + it('refuses to let a recipient rename the shared folder itself', async () => { + const error = await caught(() => + asHolder(() => fs.rename(holder.userId, shared, 'Renamed')), + ); + + expect(error.statusCode).toBe(403); + expect(await stillThere(shared.path)).toBe(true); + }); + + it('refuses to let a recipient rename a file shared directly with them', async () => { + const file = await writeFile(owner, `${owner.home}/direct.txt`, 'x'); + await shareWrite(file); + + const error = await caught(() => + asHolder(() => fs.rename(holder.userId, file, 'mine.txt')), + ); + + expect(error.statusCode).toBe(403); + expect(await stillThere(file.path)).toBe(true); + }); + + it('refuses a stranger with no share at all', async () => { + const stranger = await makeUser(); + const file = await writeFile(owner, `${shared.path}/private.txt`, 'x'); + + const error = await caught(() => + runWithContext({ actor: stranger.actor }, () => + fs.remove(stranger.userId, { entry: file }), + ), + ); + + expect(error.statusCode).toBe(403); + expect(await stillThere(file.path)).toBe(true); + }); +}); + describe('FSService access checks', () => { let owner: TestUser; let stranger: TestUser; diff --git a/src/backend/services/fs/FSService.ts b/src/backend/services/fs/FSService.ts index 866804f99..428bdb750 100644 --- a/src/backend/services/fs/FSService.ts +++ b/src/backend/services/fs/FSService.ts @@ -3253,15 +3253,7 @@ export class FSService extends PuterService { 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' }, - ); - } + await this.#assertCanRestructure(entry, userId); if (newName.includes('/')) throw new HttpError(400, 'Name cannot contain a slash', { legacyCode: 'bad_request', @@ -3389,6 +3381,35 @@ export class FSService extends PuterService { } } + /** + * Rename, move and delete are authorized by `write` on the parent, not on + * the entry — which is what lets a share recipient reorganize inside a + * shared folder without reaching the shared folder itself. + */ + async #assertCanRestructure(entry: FSEntry, userId: number): Promise { + if (entry.userId === userId) return; + + const actor = Context.get('actor') as Actor | undefined; + const parentPath = pathPosix.dirname(entry.path); + if (actor && parentPath !== '/') { + const allowed = await this.services.acl.check( + actor, + { + path: parentPath, + resolveAncestors: () => this.getAncestorChain(parentPath), + }, + 'write', + ); + if (allowed) return; + } + + throw new HttpError( + 403, + 'Cannot restructure an entry owned by another user', + { legacyCode: 'forbidden' }, + ); + } + async remove( userId: number, input: { @@ -3407,20 +3428,11 @@ export class FSService extends PuterService { if (!input.systemInitiated) { await this.#assertCrossAppDeleteAllowed(entry.path); } - if (entry.userId !== userId) { - // Defensive — only the owner should be hitting this path; higher - // layers grant access via ACL, not raw ownership, but we still - // want to avoid a misrouted call taking out someone else's tree. - throw new HttpError( - 403, - 'Cannot remove an entry owned by another user', - { legacyCode: 'forbidden' }, - ); - } + await this.#assertCanRestructure(entry, userId); if (entry.isDir) { const descendants = await this.stores.fsEntry.listDescendantsByPath( - userId, + entry.userId, entry.path, ); if (descendants.length > 0 && !input.recursive) { @@ -3642,13 +3654,7 @@ export class FSService extends PuterService { // The source only: moving *into* another app's AppData is a write, and // ACL plus the fs:write class already cover that. await this.#assertCrossAppDeleteAllowed(source.path); - if (source.userId !== userId) { - throw new HttpError( - 403, - 'Cannot move an entry owned by another user', - { legacyCode: 'forbidden' }, - ); - } + await this.#assertCanRestructure(source, userId); if (!destinationParent.isDir) { throw new HttpError(400, 'Destination parent is not a directory', { legacyCode: 'dest_is_not_a_directory', @@ -3715,7 +3721,7 @@ export class FSService extends PuterService { if (source.isDir && source.path !== finalPath) { await this.stores.fsEntry.updatePathPrefixForUser( - userId, + source.userId, source.path, finalPath, );