From acd6ea3c758e3afdbf5e013fc2f80bf54dacfcd4 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 6 Oct 2026 10:55:19 -0400 Subject: [PATCH] fix: do not spend a consent prompt on an app root dir that is not the caller's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `perms.request('appRootDir', { app: })` could never succeed — only the app itself may claim its root dir — but the SDK read the refusal as "not granted yet", prompted the user, took the grant, then polled until it gave up and resolved undefined. A prompt the user cannot act on, and a live grant for a directory the app will never reach. The route now says which kind of refusal it is. `forbidden` alone could not carry it: the SDK already treats that as the refusal a grant is about to lift, and a unit test pins exactly that sequence, so a new `reason: 'app_mismatch'` field rides alongside instead of changing the code clients read. An entry refused that way is held back from the pooled grant and resolves undefined without a prompt, which is what it resolved to before. --- .../controllers/fs/LegacyFSController.ts | 16 +- src/puter-js/src/modules/perms/request.js | 195 ++++++++++++------ .../src/modules/perms/request.test.js | 52 +++-- 3 files changed, 176 insertions(+), 87 deletions(-) diff --git a/src/backend/controllers/fs/LegacyFSController.ts b/src/backend/controllers/fs/LegacyFSController.ts index 6bcdb4899..ebb958dd0 100644 --- a/src/backend/controllers/fs/LegacyFSController.ts +++ b/src/backend/controllers/fs/LegacyFSController.ts @@ -940,9 +940,7 @@ export class LegacyFSController extends PuterController { // Trash, and `null`/`{}` when restoring. See // `src/gui/src/helpers.js` → `window.move_items`. newMetadata: (body.new_metadata ?? undefined) as - | Record - | null - | undefined, + Record | null | undefined, }); const oldPath = source.path; await this.#emitGuiEvent('outer.gui.item.moved', moved, { @@ -1399,8 +1397,7 @@ export class LegacyFSController extends PuterController { } type SignedOrEmpty = - | (SignedFile & { path?: string }) - | Record; + (SignedFile & { path?: string }) | Record; const result: { signatures: SignedOrEmpty[]; token?: string } = { signatures: [], }; @@ -1962,10 +1959,12 @@ export class LegacyFSController extends PuterController { // claiming a root dir is the app itself asking, and a token is not it. const callerApp = isAppActor(actor) ? actor.effectiveApp : null; if (callerApp?.uid !== appUid) { + // `reason` separates this from a refusal a grant could lift, so a + // client does not spend a consent prompt on a grant that cannot work. throw new HttpError( 403, 'Only the app itself may request its root dir', - { legacyCode: 'forbidden' }, + { legacyCode: 'forbidden', fields: { reason: 'app_mismatch' } }, ); } const userId = this.#getActorUserId(req); @@ -2001,10 +2000,7 @@ export class LegacyFSController extends PuterController { const subjectRef = body.subject; const appRef = body.app; const mode = (getString(body, 'mode') ?? 'read') as - | 'see' - | 'list' - | 'read' - | 'write'; + 'see' | 'list' | 'read' | 'write'; if (!subjectRef || !appRef) throw new HttpError(400, '`subject` and `app` are required', { legacyCode: 'bad_request', diff --git a/src/puter-js/src/modules/perms/request.js b/src/puter-js/src/modules/perms/request.js index fbe8234ab..1be6e65d0 100644 --- a/src/puter-js/src/modules/perms/request.js +++ b/src/puter-js/src/modules/perms/request.js @@ -5,6 +5,7 @@ import { pollAppRootDir, statAppRootDir, } from './appRootDir.js'; +import { PuterJSError } from '../../lib/PuterJSError.js'; import { folderPathFor, folderReadable } from './folders.js'; import { checkPermissions } from './lib/holds.js'; import { @@ -14,7 +15,11 @@ import { fsPermission, subdomainsPermission, } from './lib/permissionStrings.js'; -import { assertAccess, assertFolderName, invalidArgument } from './lib/validate.js'; +import { + assertAccess, + assertFolderName, + invalidArgument, +} from './lib/validate.js'; import { requestPermissions } from './permissions.js'; /** @typedef {import('./index.js').PermsModule} PermsModule */ @@ -63,26 +68,34 @@ const folderNameOf = (details) => assertFolderName(details.name); */ const permissionsOf = (details) => { const { permission, permissions, create } = details; - if ( create !== undefined - && create !== true && create !== false - && create !== 'dir' && create !== 'file' ) { + if ( + create !== undefined && + create !== true && + create !== false && + create !== 'dir' && + create !== 'file' + ) { throw invalidArgument('`create` must be true, false, "dir", or "file"'); } - if ( permissions !== undefined ) { - if ( permission !== undefined ) { - throw invalidArgument('pass `permission` or `permissions`, not both'); + if (permissions !== undefined) { + if (permission !== undefined) { + throw invalidArgument( + 'pass `permission` or `permissions`, not both', + ); } - if ( ! Array.isArray(permissions) || permissions.length === 0 ) { + if (!Array.isArray(permissions) || permissions.length === 0) { throw invalidArgument('`permissions` must be a non-empty array'); } - for ( const entry of permissions ) { - if ( typeof entry !== 'string' || entry === '' ) { - throw invalidArgument('`permissions` entries must be non-empty strings'); + for (const entry of permissions) { + if (typeof entry !== 'string' || entry === '') { + throw invalidArgument( + '`permissions` entries must be non-empty strings', + ); } } return [...new Set(/** @type {string[]} */ (permissions))]; } - if ( typeof permission !== 'string' || permission === '' ) { + if (typeof permission !== 'string' || permission === '') { throw invalidArgument('`permission` must be a non-empty string'); } return [permission]; @@ -96,12 +109,12 @@ const permissionsOf = (details) => { * @property {Record} details * @property {string[]} permissions - What this entry needs, already resolved. * @property {(permissions: string[]) => Promise} holds - Answered from - * the call's one pooled permission read. + * the call's one pooled permission read. * @property {boolean} prompt - Whether this is a `request`. A resource whose - * check would otherwise repeat work the request is about to do anyway can spend - * the round trip once and hand the result on through `scratch`. + * check would otherwise repeat work the request is about to do anyway can + * spend the round trip once and hand the result on through `scratch`. * @property {Record} scratch - Per-entry, passed from `check` - * to `resolve`. + * to `resolve`. */ /** @@ -111,9 +124,19 @@ const permissionsOf = (details) => { * asks the server itself, so its strings stay out of the pooled read. * * @typedef {Object} PermsResourceHandler - * @property {(query: { ctx: PermsContext, details: Record }) => Promise} permissions + * @property {(query: { + * ctx: PermsContext; + * details: Record; + * }) => Promise} permissions * @property {(query: PermsQuery) => Promise} check - * @property {(query: { ctx: PermsContext, details: Record, scratch: Record }, held: boolean) => Promise} resolve + * @property {( + * query: { + * ctx: PermsContext; + * details: Record; + * scratch: Record; + * }, + * held: boolean, + * ) => Promise} resolve * @property {boolean} [pooled] */ @@ -131,15 +154,15 @@ const RESOURCES = Object.assign(Object.create(null), { ], check: async ({ ctx, permissions, holds }) => { // The grant is what puts the field on `whoami`, so `null` is granted. - if ( (await ctx.whoami()).email !== undefined ) return true; + if ((await ctx.whoami()).email !== undefined) return true; return holds(permissions); }, resolve: async ({ ctx }, held) => { - if ( ! held ) return undefined; + if (!held) return undefined; // Already there before the prompt, or released by it — one more // read only in the second case. const whoami = await ctx.whoami(); - if ( whoami.email !== undefined ) return whoami.email; + if (whoami.email !== undefined) return whoami.email; return (await ctx.reread()).email; }, }, @@ -147,22 +170,25 @@ const RESOURCES = Object.assign(Object.create(null), { folder: { permissions: async ({ ctx, details }) => [ fsPermission( - folderPathFor((await ctx.whoami()).username, folderNameOf(details)), + folderPathFor( + (await ctx.whoami()).username, + folderNameOf(details), + ), accessOf(details), ), ], check: async ({ ctx, details, permissions, holds }) => { - if ( accessOf(details) !== 'write' ) { + if (accessOf(details) !== 'write') { const path = folderPathFor( (await ctx.whoami()).username, folderNameOf(details), ); - if ( await folderReadable(ctx.puter, path) ) return true; + if (await folderReadable(ctx.puter, path)) return true; } return holds(permissions); }, resolve: async ({ ctx, details }, held) => { - if ( ! held ) return undefined; + if (!held) return undefined; return folderPathFor( (await ctx.whoami()).username, folderNameOf(details), @@ -191,7 +217,9 @@ const RESOURCES = Object.assign(Object.create(null), { appDataRequest( ctx.puter, /** @type {string} */ (details.app), - /** @type {import('./types.js').AppDataScopes} */ (details.scopes), + /** @type {import('./types.js').AppDataScopes} */ ( + details.scopes + ), ), // An empty list is its own data, which it may always use. check: async ({ permissions, holds }) => @@ -210,13 +238,23 @@ const RESOURCES = Object.assign(Object.create(null), { const appUid = appUidOf(details.app); const access = accessOf(details); // A check must not provision the directory just for asking. - if ( ! prompt ) { + if (!prompt) { return await checkAppRootDir(ctx.puter, appUid, access); } // A request is going to claim it either way, so the claim is the // check — one round trip, as the shipped method has always made. const result = await statAppRootDir(ctx.puter, appUid, access); - if ( result.error ) return false; + // Permanent, unlike a plain refusal a grant is about to lift. + if (result.error && result.reason === 'app_mismatch') { + throw new PuterJSError( + typeof result.message === 'string' + ? result.message + : 'Only the app itself may request its root dir', + 'forbidden', + { permsUngrantable: true }, + ); + } + if (result.error) return false; scratch.entry = result; return true; }, @@ -224,8 +262,8 @@ const RESOURCES = Object.assign(Object.create(null), { // check claimed it; otherwise asked for now, riding out the cache lag // behind a fresh grant. resolve: async ({ ctx, details, scratch }, held) => { - if ( ! held ) return undefined; - if ( scratch.entry ) return scratch.entry; + if (!held) return undefined; + if (scratch.entry) return scratch.entry; return await pollAppRootDir( ctx.puter, appUidOf(details.app), @@ -253,22 +291,27 @@ const RESOURCE_NAMES = Object.keys(RESOURCES); * * @param {unknown} resource * @param {unknown} details - * @returns {{ resource: string, details: Record }} + * @returns {{ resource: string; details: Record }} */ const singleEntry = (resource, details) => { - if ( typeof resource !== 'string' || resource === '' ) { + if (typeof resource !== 'string' || resource === '') { throw invalidArgument('resource must be a non-empty string'); } - if ( details !== undefined && (typeof details !== 'object' || details === null || Array.isArray(details)) ) { + if ( + details !== undefined && + (typeof details !== 'object' || + details === null || + Array.isArray(details)) + ) { throw invalidArgument('details must be an object'); } - if ( RESOURCES[resource] ) { + if (RESOURCES[resource]) { return { resource, details: /** @type {Record} */ (details ?? {}), }; } - if ( details !== undefined ) { + if (details !== undefined) { throw invalidArgument( `unknown resource: ${resource} (expected one of: ${RESOURCE_NAMES.join(', ')})`, ); @@ -282,20 +325,24 @@ const singleEntry = (resource, details) => { * * @param {unknown} entry * @param {number} index - * @returns {{ resource: string, details: Record }} + * @returns {{ resource: string; details: Record }} */ const batchEntry = (entry, index) => { - if ( typeof entry !== 'object' || entry === null || Array.isArray(entry) ) { + if (typeof entry !== 'object' || entry === null || Array.isArray(entry)) { throw invalidArgument(`requests[${index}] must be an object`); } - const { resource, ...details } = /** @type {Record} */ (entry); - if ( typeof resource !== 'string' || resource === '' ) { - throw invalidArgument(`requests[${index}].resource must be a non-empty string`); + const { resource, ...details } = /** @type {Record} */ ( + entry + ); + if (typeof resource !== 'string' || resource === '') { + throw invalidArgument( + `requests[${index}].resource must be a non-empty string`, + ); } - if ( ! RESOURCES[resource] ) { + if (!RESOURCES[resource]) { throw invalidArgument( `requests[${index}]: unknown resource: ${resource} ` + - `(expected one of: ${RESOURCE_NAMES.join(', ')})`, + `(expected one of: ${RESOURCE_NAMES.join(', ')})`, ); } return { resource, details }; @@ -311,13 +358,13 @@ const batchEntry = (entry, index) => { * @param {string[]} permissions * @returns {(permissions: string[]) => Promise} */ -function pooledHolds (ctx, permissions) { +function pooledHolds(ctx, permissions) { const wanted = [...new Set(permissions)]; /** @type {Promise> | undefined} */ let pending; return async (needed) => { - if ( needed.length === 0 || wanted.length === 0 ) return false; + if (needed.length === 0 || wanted.length === 0) return false; const held = await (pending ??= checkPermissions(ctx.puter, wanted)); return needed.every((name) => held[name] === true); }; @@ -329,11 +376,11 @@ function pooledHolds (ctx, permissions) { * drift apart. * * @param {PermsContext} ctx - * @param {{ resource: string, details: Record }[]} entries + * @param {{ resource: string; details: Record }[]} entries * @param {boolean} prompt * @returns {Promise} */ -async function runEntries (ctx, entries, prompt) { +async function runEntries(ctx, entries, prompt) { // Resolved — and so validated — before anything is asked, so a bad entry // can't surface after a prompt has gone up for the rest. const permissions = await Promise.all( @@ -345,13 +392,15 @@ async function runEntries (ctx, entries, prompt) { // Only the raw-permission resource carries `create`, and one prompt is one // decision — so at most one distinct value may cover it. Checked here, // still before anything is asked. - const createValues = [...new Set( - entries - .filter(({ resource }) => resource === 'permission') - .map(({ details }) => details.create) - .filter((value) => value !== undefined), - )]; - if ( createValues.length > 1 ) { + const createValues = [ + ...new Set( + entries + .filter(({ resource }) => resource === 'permission') + .map(({ details }) => details.create) + .filter((value) => value !== undefined), + ), + ]; + if (createValues.length > 1) { throw invalidArgument('conflicting `create` values in one request'); } const create = createValues[0]; @@ -368,7 +417,11 @@ async function runEntries (ctx, entries, prompt) { // it did before there was a check at all. `check` has nowhere to go, and // answering "not granted" would prompt someone who already granted it. /** Per entry, for whatever its `check` wants to hand to its `resolve`. */ - const scratch = entries.map(() => /** @type {Record} */ ({})); + const scratch = entries.map( + () => /** @type {Record} */ ({}), + ); + /** Entries whose refusal no grant could lift, so they never reach a prompt. */ + const ungrantable = entries.map(() => false); const held = await Promise.all( entries.map(async ({ resource, details }, i) => { @@ -381,16 +434,24 @@ async function runEntries (ctx, entries, prompt) { prompt, scratch: scratch[i], }); - } catch ( e ) { - if ( ! prompt ) throw e; + } catch (e) { + if (e?.permsUngrantable) { + ungrantable[i] = true; + return false; + } + if (!prompt) throw e; return false; } }), ); - if ( ! prompt ) return held; + if (!prompt) return held; const missing = [ - ...new Set(entries.flatMap((_entry, i) => (held[i] ? [] : permissions[i]))), + ...new Set( + entries.flatMap((_entry, i) => + held[i] || ungrantable[i] ? [] : permissions[i], + ), + ), ]; const granted = missing.length === 0 @@ -403,7 +464,8 @@ async function runEntries (ctx, entries, prompt) { { ctx, details, scratch: scratch[i] }, // An entry that named no permission asked nothing, so a grant // covering the others says nothing about it. - held[i] || (permissions[i].length > 0 && granted), + !ungrantable[i] && + (held[i] || (permissions[i].length > 0 && granted)), ), ), ); @@ -412,11 +474,11 @@ async function runEntries (ctx, entries, prompt) { /** * @param {unknown} resource * @param {unknown} details - * @returns {{ resource: string, details: Record }[]} + * @returns {{ resource: string; details: Record }[]} */ const entriesOf = (resource, details) => { - if ( ! Array.isArray(resource) ) return [singleEntry(resource, details)]; - if ( details !== undefined ) { + if (!Array.isArray(resource)) return [singleEntry(resource, details)]; + if (details !== undefined) { throw invalidArgument('a batch takes no second argument'); } return resource.map(batchEntry); @@ -473,7 +535,10 @@ const entriesOf = (resource, details) => { * the path, `'email'` the address, `'appRootDir'` the directory, the rest a * boolean. Denied is always falsy. * - * await puter.perms.request('folder', { name: 'Documents', access: 'write' }); + * await puter.perms.request('folder', { + * name: 'Documents', + * access: 'write', + * }); * * An array asks for several at once, behind one prompt, resolving in order. * @@ -487,7 +552,7 @@ const entriesOf = (resource, details) => { * @param {PermsRequestDetails} [details] * @returns {Promise} */ -export async function request (resource, details) { +export async function request(resource, details) { const entries = entriesOf(resource, details); const results = await runEntries(makeContext(this.puter), entries, true); return Array.isArray(resource) ? results : results[0]; @@ -553,7 +618,7 @@ export async function request (resource, details) { * @param {PermsRequestDetails} [details] * @returns {Promise} */ -export async function check (resource, details) { +export async function check(resource, details) { const entries = entriesOf(resource, details); const held = /** @type {boolean[]} */ ( await runEntries(makeContext(this.puter), entries, false) diff --git a/src/puter-js/src/modules/perms/request.test.js b/src/puter-js/src/modules/perms/request.test.js index 292e00b2a..ee603a9dd 100644 --- a/src/puter-js/src/modules/perms/request.test.js +++ b/src/puter-js/src/modules/perms/request.test.js @@ -12,7 +12,9 @@ const ROOT_DIR_ROUTE = '/auth/request-app-root-dir'; const WHOAMI = { username: 'alice', uuid: 'u-1' }; const SELF_UID = 'app-00000000-0000-4000-8000-000000000001'; -const denied = async () => { throw new Error('no access'); }; +const denied = async () => { + throw new Error('no access'); +}; /** * Answer the mocked helper by route, so a test says what is held and what the @@ -23,8 +25,8 @@ const denied = async () => { throw new Error('no access'); }; const routes = ({ held = {}, rootDir = [] } = {}) => { const pending = [...rootDir]; mockReq.mockImplementation(async (_puter, route) => { - if ( route === CHECK_ROUTE ) return { permissions: held }; - if ( route === ROOT_DIR_ROUTE ) { + if (route === CHECK_ROUTE) return { permissions: held }; + if (route === ROOT_DIR_ROUTE) { return pending.length ? pending.shift() : { error: true, code: 'forbidden' }; @@ -92,7 +94,10 @@ describe('perms request(resource, details)', () => { it('resolves undefined when a folder request is denied', async () => { const mod = makeModule({ requestPermission: () => false }); expect( - await request.call(mod, 'folder', { name: 'Videos', access: 'write' }), + await request.call(mod, 'folder', { + name: 'Videos', + access: 'write', + }), ).toBeUndefined(); }); @@ -147,7 +152,10 @@ describe('perms request(resource, details)', () => { }), ).toBe(true); expect(mod.puter.ui.requestPermission).toHaveBeenCalledWith({ - permissions: ['app-data:app-target:fs:read', 'app-data:app-target:kv:read'], + permissions: [ + 'app-data:app-target:fs:read', + 'app-data:app-target:kv:read', + ], }); }); @@ -215,7 +223,7 @@ describe('perms request(resource, details)', () => { it('reads a permission string that collides with an Object member', async () => { const mod = makeModule({ requestPermission: () => true }); - for ( const name of ['constructor', 'toString', 'hasOwnProperty'] ) { + for (const name of ['constructor', 'toString', 'hasOwnProperty']) { expect(await request.call(mod, name)).toBe(true); expect(mod.puter.ui.requestPermission).toHaveBeenLastCalledWith({ permission: name, @@ -280,9 +288,9 @@ describe('perms request(resource, details)', () => { await expect( request.call(mod, 'apps', { access: 'delete' }), ).rejects.toMatchObject({ code: 'invalid_argument' }); - await expect( - request.call(mod, 'apps', 'read'), - ).rejects.toMatchObject({ code: 'invalid_argument' }); + await expect(request.call(mod, 'apps', 'read')).rejects.toMatchObject({ + code: 'invalid_argument', + }); await expect( request.call(mod, 'appRootDir', { app: 42 }), ).rejects.toMatchObject({ code: 'invalid_argument' }); @@ -435,6 +443,18 @@ describe('perms request([...]) batching', () => { expect(results).toEqual([true, undefined]); }); + // Another app's root dir can never be claimed, so asking must not cost the + // user a consent prompt or leave a grant behind. + it('does not prompt for an app root dir that is not the caller own', async () => { + routes({ rootDir: [{ error: true, reason: 'app_mismatch' }] }); + const mod = makeModule({ requestPermission: () => true }); + + expect( + await request.call(mod, [{ resource: 'appRootDir', app: 'app-2' }]), + ).toEqual([undefined]); + expect(mod.puter.ui.requestPermission).not.toHaveBeenCalled(); + }); + // A grant can lag the permission cache, so the first refusal after one // isn't final — in a batch exactly as in a single request. it('keeps asking for the app root dir after the pooled grant', async () => { @@ -489,7 +509,11 @@ describe('perms request([...]) batching', () => { await expect( request.call(mod, [ { resource: 'permission', permission: 'a:read', create: true }, - { resource: 'permission', permission: 'b:read', create: 'file' }, + { + resource: 'permission', + permission: 'b:read', + create: 'file', + }, ]), ).rejects.toMatchObject({ code: 'invalid_argument' }); expect(mod.puter.ui.requestPermission).not.toHaveBeenCalled(); @@ -535,7 +559,9 @@ describe('perms request([...]) batching', () => { it('rejects an entry with no resource, an unknown one, or a stray second argument', async () => { const mod = makeModule(); - await expect(request.call(mod, [{ name: 'Desktop' }])).rejects.toMatchObject({ + await expect( + request.call(mod, [{ name: 'Desktop' }]), + ).rejects.toMatchObject({ code: 'invalid_argument', }); await expect( @@ -685,7 +711,9 @@ describe('perms check(resource, details)', () => { expect(mod.puter.ui.requestPermission).not.toHaveBeenCalled(); // The queue is empty now, so the route refuses: not held. - expect(await check.call(mod, 'appRootDir', { app: 'app-1' })).toBe(false); + expect(await check.call(mod, 'appRootDir', { app: 'app-1' })).toBe( + false, + ); }); it('rejects an invalid `create` value the same way `request` does', async () => {