From e7e32560b0ed38be8435c972967836c8198bec28 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Thu, 13 Aug 2026 19:07:18 -0400 Subject: [PATCH] feat(permissions): let manage inherit down the filesystem tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Access already reached descendants through the ancestor chain while authority did not, so someone trusted to manage a shared folder could re-share the folder but nothing inside it, and could not see who had access to a file within it. A manage-inherits-from-ancestor implicator resolves it in the permission layer, beside is-owner, so every caller agrees rather than just ShareService. It consults only the immediate parent — resolving that re-enters one level up, making a chain of depth d cost d checks rather than d². That makes two cascade gaps reachable, both fixed here. A revoke now walks the subtree, since a grant on a descendant can rest on authority held at the folder. And it stops at a delegate whose authority survives another issuer, because what they granted was never theirs to lose. Also pins that manage is not transitive: granting it needs manage:manage:fs:, which only the owner holds, so delegation is one level deep by construction. --- src/backend/services/fs/FSService.ts | 43 +++++ .../services/share/ShareService.test.ts | 172 ++++++++++++++++++ src/backend/services/share/ShareService.ts | 63 +++++-- src/backend/stores/share/ShareStore.js | 22 +++ 4 files changed, 281 insertions(+), 19 deletions(-) diff --git a/src/backend/services/fs/FSService.ts b/src/backend/services/fs/FSService.ts index 8ef917570..de6f5332f 100644 --- a/src/backend/services/fs/FSService.ts +++ b/src/backend/services/fs/FSService.ts @@ -240,6 +240,49 @@ export class FSService extends PuterService { }, }); + // -- manage-inherits-from-ancestor ----------------------------- + // `manage` on a directory covers what is inside it, the way `fs:*` + // access already reaches descendants through the ancestor chain. + // Without this the two are asymmetric: someone trusted to manage a + // shared folder can re-share the folder itself but nothing in it, and + // cannot even see who has access to a file within it. + // + // Only the parent is consulted; resolving it re-enters one level up, + // so a chain of depth d costs d checks rather than d². + permissions.registerImplicator({ + id: 'manage-inherits-from-ancestor', + shortcut: true, + matches: (permission: string): boolean => + permission.startsWith(`${MANAGE_PERM_PREFIX}:fs:`), + check: async ({ actor, permission }): Promise => { + // Apps are bounded by their user through a separate path; + // widening them here would let one outrun that bound. + if (actor.app || actor.accessToken) return undefined; + if (!actor.user?.id) return undefined; + + const stripped = permission.replaceAll( + `${MANAGE_PERM_PREFIX}:`, + '', + ); + const uid = PermissionUtil.split(stripped)[1]; + if (!uid) return undefined; + + const entry = await fsEntryStore.getEntryByUuid(uid); + if (!entry) return undefined; + + const [, parent] = await this.getAncestorChain(entry.path); + if (!parent) return undefined; + + // uuids carry no `:`, so swapping it in leaves the manage + // prefixes and the mode suffix exactly as they were. + const held = await permissions.check( + actor, + permission.replace(`fs:${uid}`, `fs:${parent.uid}`), + ); + return held ? {} : undefined; + }, + }); + // -- app-owns-appdata ----------------------------------------- // Mirror of the ACLService short-circuit at ACLService.check: // an app-under-user actor implicitly holds fs::* on any diff --git a/src/backend/services/share/ShareService.test.ts b/src/backend/services/share/ShareService.test.ts index 6768a1519..f8aad82c7 100644 --- a/src/backend/services/share/ShareService.test.ts +++ b/src/backend/services/share/ShareService.test.ts @@ -402,6 +402,178 @@ describe('ShareService', () => { expect(await canRead(third.actor, file.path)).toBe(false); }); + describe('manage inherits down the tree', () => { + it('lets a folder delegate re-share and inspect a file inside it', async () => { + const owner = await makeUser(); + const delegate = await makeUser(); + const third = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: delegate.email }, + mode: 'manage', + }); + + // Authority now reaches the child the way access already did. + const rows = await server.services.share.listSharesOf( + delegate.actor, + { uid: file.uuid }, + ); + expect(rows.map((r) => r.holder.username)).toContain( + delegate.user.username, + ); + + await share(delegate.actor, { + uid: file.uuid, + recipient: { email: third.email }, + mode: 'read', + }); + expect(await canRead(third.actor, file.path)).toBe(true); + }); + + it('revokes what a folder delegate re-shared from inside it', async () => { + const owner = await makeUser(); + const delegate = await makeUser(); + const third = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: delegate.email }, + mode: 'manage', + }); + await share(delegate.actor, { + uid: file.uuid, + recipient: { email: third.email }, + mode: 'read', + }); + expect(await canRead(third.actor, file.path)).toBe(true); + + // The grant on the child came from authority held on the folder, + // so withdrawing that authority has to reach down to it. + await unshare(owner.actor, { + uid: dir.uuid, + recipient: { username: delegate.user.username }, + }); + + expect(await canRead(delegate.actor, file.path)).toBe(false); + expect(await canRead(third.actor, file.path)).toBe(false); + }); + + it('does not let plain access on a folder manage what is inside', async () => { + const owner = await makeUser(); + const reader = await makeUser(); + const third = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + + await share(owner.actor, { + uid: dir.uuid, + recipient: { email: reader.email }, + mode: 'write', + }); + + // `write` reaches the child, but managing is a separate namespace. + // 403 rather than 404 here: they can already see the file, so + // hiding it would protect nothing. + expect(await canRead(reader.actor, file.path)).toBe(true); + await expect( + share(reader.actor, { + uid: file.uuid, + recipient: { email: third.email }, + mode: 'read', + }), + ).rejects.toMatchObject({ statusCode: 403 }); + }); + + it('does not let manage on a file leak up to its folder', async () => { + const owner = await makeUser(); + const delegate = await makeUser(); + const third = await makeUser(); + const { dir, file } = await makeDirWithFile(owner.user); + + await share(owner.actor, { + uid: file.uuid, + recipient: { email: delegate.email }, + mode: 'manage', + }); + + // Inheritance runs one way; the parent is not implied by the child. + await expect( + share(delegate.actor, { + uid: dir.uuid, + recipient: { email: third.email }, + mode: 'read', + }), + ).rejects.toMatchObject({ statusCode: 404 }); + }); + }); + + it('does not let a delegate pass on `manage` itself', async () => { + const owner = await makeUser(); + const delegate = await makeUser(); + const third = await makeUser(); + const file = await makeFile(owner.user); + + await share(owner.actor, { + uid: file.uuid, + recipient: { email: delegate.email }, + mode: 'manage', + }); + + // Granting `manage` needs `manage:manage:fs:`, which only the + // owner holds — so delegation is one level deep by construction. + await expect( + share(delegate.actor, { + uid: file.uuid, + recipient: { email: third.email }, + mode: 'manage', + }), + ).rejects.toMatchObject({ statusCode: 403 }); + }); + + it('leaves a delegate alone when their authority survives another issuer', async () => { + const owner = await makeUser(); + const delegate = await makeUser(); + const middle = await makeUser(); + const leaf = await makeUser(); + const file = await makeFile(owner.user); + + // `middle` manages by the owner's grant, and separately holds a plain + // read the delegate handed out. + await share(owner.actor, { + uid: file.uuid, + recipient: { email: delegate.email }, + mode: 'manage', + }); + await share(owner.actor, { + uid: file.uuid, + recipient: { email: middle.email }, + mode: 'manage', + }); + await share(delegate.actor, { + uid: file.uuid, + recipient: { email: middle.email }, + mode: 'read', + }); + await share(middle.actor, { + uid: file.uuid, + recipient: { email: leaf.email }, + mode: 'read', + }); + + await unshare(owner.actor, { + uid: file.uuid, + recipient: { username: delegate.user.username }, + }); + + // Withdrawing the delegate costs `middle` nothing it was relying on, + // so what `middle` granted must stand. + expect(await canRead(delegate.actor, file.path)).toBe(false); + expect(await canRead(middle.actor, file.path)).toBe(true); + expect(await canRead(leaf.actor, file.path)).toBe(true); + }); + it('lets a delegate clear only what it issued', async () => { const owner = await makeUser(); const delegate = await makeUser(); diff --git a/src/backend/services/share/ShareService.ts b/src/backend/services/share/ShareService.ts index 7d0cfed76..f13d5b549 100644 --- a/src/backend/services/share/ShareService.ts +++ b/src/backend/services/share/ShareService.ts @@ -233,14 +233,7 @@ export class ShareService extends PuterService { ); if (isNewShare) await this.#assertDailyQuota(issuerId); - const holderActor: Actor = { - user: { - id: holder.id, - uuid: holder.uuid, - username: holder.username, - } as Actor['user'], - effectiveApp: null, - }; + const holderActor: Actor = this.#actorFor(holder); await this.services.acl.setUserUser( actor, @@ -367,27 +360,30 @@ export class ShareService extends PuterService { if (seen.has(issuerId)) return 0; seen.add(issuerId); - const rows = (await this.stores.share.listByFsentry(entry.id)).filter( + // The whole subtree, not just this node: `manage` inherits downwards, + // so a grant on a descendant can rest on authority held here. + const rows = ( + await this.stores.share.listByFsentrySubtree(entry.id, entry.path) + ).filter( (row: { issuer_user_id: number }) => Number(row.issuer_user_id) === issuerId, ); + if (rows.length === 0) return 0; + + const nodes = await this.stores.fsEntry.getEntriesByIds( + rows.map((row: { fsentry_id: number }) => Number(row.fsentry_id)), + ); let revoked = 0; for (const row of rows) { const holderId = Number(row.holder_user_id); + const node = nodes.get(Number(row.fsentry_id)); const downstream = await this.stores.user.getById(holderId); - if (!downstream?.username) continue; - - revoked += await this.#revokeDownstream( - actor, - entry, - holderId, - seen, - ); + if (!node || !downstream?.username) continue; const { revoked: didRevoke, authorized } = await this.#revokeFor( actor, - entry, + node, downstream.username, issuerId, ); @@ -395,10 +391,27 @@ export class ShareService extends PuterService { if (authorized) { await this.stores.share.deleteActive({ holderUserId: holderId, - fsentryId: entry.id, + fsentryId: node.id, issuerUserId: issuerId, }); } + + // Only carry on down if this actually cost them their authority. + // A delegate granted `manage` by two people keeps it when one + // withdraws, and what they granted is not theirs to lose. + const stillHolds = + await this.services.permission.canManagePermission( + this.#actorFor(downstream), + `fs:${node.uuid}:read`, + ); + if (stillHolds) continue; + + revoked += await this.#revokeDownstream( + actor, + entry, + holderId, + seen, + ); } return revoked; } @@ -602,6 +615,18 @@ export class ShareService extends PuterService { }; } + /** A plain user actor, for asking the permission layer about someone else. */ + #actorFor(user: { id: number; uuid?: string; username?: string }): Actor { + return { + user: { + id: user.id, + uuid: user.uuid, + username: user.username, + } as Actor['user'], + effectiveApp: null, + }; + } + #requireUserId(actor: Actor): number { const id = actor?.user?.id; if (typeof id !== 'number') { diff --git a/src/backend/stores/share/ShareStore.js b/src/backend/stores/share/ShareStore.js index d2987d193..ff137e365 100644 --- a/src/backend/stores/share/ShareStore.js +++ b/src/backend/stores/share/ShareStore.js @@ -110,6 +110,28 @@ export class ShareStore extends PuterStore { return rows.map((r) => this.#normalizeRow(r)); } + /** + * Active shares on a directory and everything beneath it. `manage` inherits + * downwards, so a revoke here has to see what rests on it. + * + * @param {number} fsentryId + * @param {string} path Directory path, used to match descendants. + */ + async listByFsentrySubtree(fsentryId, path) { + // `!` escapes the LIKE wildcards so a directory named with `%` or `_` + // cannot widen the match into siblings. + const prefix = `${String(path).replace(/([!%_])/g, '!$1')}/%`; + const rows = await this.clients.db.read( + 'SELECT `share`.* FROM `share` ' + + 'JOIN `fsentries` ON `fsentries`.`id` = `share`.`fsentry_id` ' + + 'WHERE `share`.`holder_user_id` IS NOT NULL AND ' + + "(`share`.`fsentry_id` = ? OR `fsentries`.`path` LIKE ? ESCAPE '!') " + + 'ORDER BY `share`.`id`', + [fsentryId, prefix], + ); + return rows.map((r) => this.#normalizeRow(r)); + } + async countByHolder(holderUserId) { const rows = await this.clients.db.read( 'SELECT COUNT(*) AS `count` FROM `share` WHERE `holder_user_id` = ?',