From 2fdd67c72e68442fadcebd05b46ec8787e916e8e Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Tue, 15 Sep 2026 10:33:47 -0700 Subject: [PATCH] fix: tighten up fs perm strings (#3860) --- src/backend/services/fs/FSService.test.ts | 68 +++++++++++++++++++ .../permission/PermissionService.test.ts | 8 +++ .../services/permission/PermissionService.ts | 26 ++++++- .../services/permission/permissionUtil.ts | 11 +++ src/gui/src/UI/UIPermissionDialog.js | 8 +++ 5 files changed, 120 insertions(+), 1 deletion(-) diff --git a/src/backend/services/fs/FSService.test.ts b/src/backend/services/fs/FSService.test.ts index a7c83b8d8..c5b03af17 100644 --- a/src/backend/services/fs/FSService.test.ts +++ b/src/backend/services/fs/FSService.test.ts @@ -3852,6 +3852,74 @@ describe('FSService permission rules', () => { ); expect(higher).toContain(`manage:fs:${file.uuid}`); }); + + it('never offers the bare entry as a parent of a moded permission', async () => { + const higher = await server.services.permission.getHigherPermissions( + `fs:${file.uuid}:write`, + ); + expect(higher).not.toContain(`fs:${file.uuid}`); + }); + + it('refuses to grant an entry with no mode', async () => { + const stranger = await makeUser(); + for (const specifier of [file.path, file.uuid]) { + const error = await caught(() => + server.services.permission.grantUserUserPermission( + user.actor, + stranger.username, + `fs:${specifier}`, + ), + ); + expect(error.statusCode).toBe(400); + expect(error.legacyCode).toBe('bad_request'); + } + }); + + // A row an earlier bug could still have written: it names the entry and no + // mode, so the parent walk once let it answer read, write and delete. + it('resolves nothing for a stored permission with no mode', async () => { + const app = (await server.stores.app.create( + { + name: `perm-${uuidv4()}`, + title: 'FS permission test', + index_url: 'https://perm.test/', + }, + { ownerUserId: user.userId }, + )) as { id: number; uid: string }; + const appActor = makeActor({ + user: user.actor.user, + app: { uid: app.uid, id: app.id }, + }); + + await server.stores.permission.upsertUserAppPerm( + user.userId, + app.id, + `fs:${file.uuid}`, + {}, + ); + + for (const mode of ['see', 'list', 'read', 'write']) { + await expect( + server.services.permission.check( + appActor, + `fs:${file.uuid}:${mode}`, + ), + ).resolves.toBe(false); + } + + // The same row with a mode on it still resolves, and only that far. + await server.services.permission.grantUserAppPermission( + user.actor, + app.uid, + `fs:${file.uuid}:read`, + ); + await expect( + server.services.permission.check(appActor, `fs:${file.uuid}:read`), + ).resolves.toBe(true); + await expect( + server.services.permission.check(appActor, `fs:${file.uuid}:write`), + ).resolves.toBe(false); + }); }); // -- Cross-app AppData (app-data::fs:) ---------------------- diff --git a/src/backend/services/permission/PermissionService.test.ts b/src/backend/services/permission/PermissionService.test.ts index 5ff455125..6ffe92882 100644 --- a/src/backend/services/permission/PermissionService.test.ts +++ b/src/backend/services/permission/PermissionService.test.ts @@ -161,6 +161,14 @@ describe('PermissionService.getHigherPermissions', () => { expect(higher).toEqual(expect.arrayContaining(['a:b:c', 'a:b', 'a'])); }); + it('drops the bare fs parents, which no grant may hold', async () => { + const service = createPermissionService(); + const higher = await service.getHigherPermissions('fs:some-uuid:write'); + expect(higher).toContain('fs:some-uuid:write'); + expect(higher).not.toContain('fs:some-uuid'); + expect(higher).not.toContain('fs'); + }); + it('expands via registered exploders when the parent matches', async () => { const service = createPermissionService(); service.registerExploder({ diff --git a/src/backend/services/permission/PermissionService.ts b/src/backend/services/permission/PermissionService.ts index 4288c199b..05f640aee 100644 --- a/src/backend/services/permission/PermissionService.ts +++ b/src/backend/services/permission/PermissionService.ts @@ -30,6 +30,7 @@ import { PERMISSION_SCAN_CACHE_TTL_SECONDS, } from './consts'; import { + isBareFsPermission, PermissionUtil, readingHasTerminal, type PermissionExploder, @@ -126,7 +127,9 @@ export class PermissionService extends PuterService { for (const p of more) higher.add(p); } } - return [...higher]; + // The parent walk reaches `fs:`, which no grant is allowed to + // hold; drop it so a stored one can't answer a check for any mode. + return [...higher].filter((p) => !isBareFsPermission(p)); } getParentPermissions(permission: string): string[] { @@ -140,6 +143,22 @@ export class PermissionService extends PuterService { return parents; } + /** + * Grants only: an `fs:` permission has to name a mode. Without one the row + * sits above every mode, and a request that simply omits it reads to the + * user as a narrower grant than it is. Revokes stay unguarded so an + * existing bare row can still be withdrawn. + */ + assertGrantableFsPermission(permission: string): void { + if (isBareFsPermission(permission)) { + throw new HttpError( + 400, + 'Invalid `permission`: `fs` requires an access mode', + { legacyCode: 'bad_request' }, + ); + } + } + // -- Public check / scan API -------------------------------------- async check( @@ -825,6 +844,7 @@ export class PermissionService extends PuterService { meta: GrantMeta = {}, ): Promise { permission = await this.rewritePermission(permission); + this.assertGrantableFsPermission(permission); const user = await this.stores.user.getByUsername(username); if (!user) throw new HttpError(404, `user_does_not_exist: ${username}`, { @@ -933,6 +953,7 @@ export class PermissionService extends PuterService { ): Promise { // First: the rewrite decides the row's width and what a revoke matches. permission = await this.rewritePermission(permission); + this.assertGrantableFsPermission(permission); if (permission.length > PERMISSION_MAX_LEN) { throw new HttpError(400, 'permission is too long', { legacyCode: 'bad_request', @@ -1287,6 +1308,7 @@ export class PermissionService extends PuterService { */ async assertUserAppPermissionWritable(permission: string): Promise { const rewritten = await this.#rewriteForUserAppWrite(permission); + this.assertGrantableFsPermission(rewritten); if (rewritten.length > PERMISSION_MAX_LEN) { throw new HttpError(400, 'Invalid `permission`', { legacyCode: 'bad_request', @@ -1302,6 +1324,7 @@ export class PermissionService extends PuterService { meta: GrantMeta = {}, ): Promise { permission = await this.#rewriteForUserAppWrite(permission); + this.assertGrantableFsPermission(permission); // Checked after the rewrite, because the rewrite is what decides how // wide the row actually is: `fs:/deep/path:read` collapses to // `fs::read`. Reject here rather than let an oversized string @@ -1448,6 +1471,7 @@ export class PermissionService extends PuterService { meta: GrantMeta = {}, ): Promise { permission = await this.rewritePermission(permission); + this.assertGrantableFsPermission(permission); const app = await this.stores.app.resolveApp(appIdentifier); if (!app) throw new HttpError(404, `entity_not_found: app:${appIdentifier}`, { diff --git a/src/backend/services/permission/permissionUtil.ts b/src/backend/services/permission/permissionUtil.ts index db7b00a9d..4e1a3a5c8 100644 --- a/src/backend/services/permission/permissionUtil.ts +++ b/src/backend/services/permission/permissionUtil.ts @@ -165,6 +165,17 @@ export const PermissionUtil = { }, }; +/** + * Whether a permission names an fs entry but no access mode. `fs:` is a + * parent of `fs::`, so one answers every mode over the entry and + * everything under it. Nothing grants one deliberately, so both the grant path + * and the check path treat it as invalid rather than as a wildcard. + */ +export const isBareFsPermission = (permission: string): boolean => { + const parts = PermissionUtil.split(permission); + return parts[0] === 'fs' && parts.length < 3; +}; + /** * Check whether a reading includes any terminal node (an `option`, or a `path` * that itself transitively terminates). diff --git a/src/gui/src/UI/UIPermissionDialog.js b/src/gui/src/UI/UIPermissionDialog.js index 35aae78e3..8c2dab99a 100644 --- a/src/gui/src/UI/UIPermissionDialog.js +++ b/src/gui/src/UI/UIPermissionDialog.js @@ -552,6 +552,9 @@ function permission_icon_svg (icon) { return icons[icon] ?? icons.shield; } +/** Access modes an `fs:` permission can ask for; anything else is not shown. */ +const FS_ACCESS_MODES = ['see', 'list', 'read', 'write']; + /** * Generates a user-friendly description of a permission string. * @@ -568,6 +571,11 @@ async function get_permission_description (permission, options = {}) { const [resource_type, resource_id, action, interface_name = null] = parts; if ( resource_type === 'fs' ) { + // No mode means no verb to put in front of the user, and a + // modeless `fs:` grant is one the backend refuses anyway. + if ( ! FS_ACCESS_MODES.includes(action) ) { + return null; + } // Check for standard folders using whoami().directories const standard_folder_description = await get_standard_folder_description(resource_id, action); if ( standard_folder_description ) {