fix: do not spend a consent prompt on an app root dir that is not the caller's

`perms.request('appRootDir', { app: <someone else's> })` 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.
This commit is contained in:
Juan Castro committed 2026-10-06 10:55:19 -04:00
1 parent d6c606fb21
commit acd6ea3c75
3 files changed
+176 -87

No files matched your search

@@ -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<string, unknown>
| null
| undefined,
Record<string, unknown> | 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<string, never>;
(SignedFile & { path?: string }) | Record<string, never>;
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',
+130 -65
View File
@@ -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<string, unknown>} details
* @property {string[]} permissions - What this entry needs, already resolved.
* @property {(permissions: string[]) => Promise<boolean>} 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<string, unknown>} 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<string, unknown> }) => Promise<string[]>} permissions
* @property {(query: {
* ctx: PermsContext;
* details: Record<string, unknown>;
* }) => Promise<string[]>} permissions
* @property {(query: PermsQuery) => Promise<boolean>} check
* @property {(query: { ctx: PermsContext, details: Record<string, unknown>, scratch: Record<string, unknown> }, held: boolean) => Promise<unknown>} resolve
* @property {(
* query: {
* ctx: PermsContext;
* details: Record<string, unknown>;
* scratch: Record<string, unknown>;
* },
* held: boolean,
* ) => Promise<unknown>} 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<string, unknown> }}
* @returns {{ resource: string; details: Record<string, unknown> }}
*/
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<string, unknown>} */ (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<string, unknown> }}
* @returns {{ resource: string; details: Record<string, unknown> }}
*/
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<string, unknown>} */ (entry);
if ( typeof resource !== 'string' || resource === '' ) {
throw invalidArgument(`requests[${index}].resource must be a non-empty string`);
const { resource, ...details } = /** @type {Record<string, unknown>} */ (
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<boolean>}
*/
function pooledHolds (ctx, permissions) {
function pooledHolds(ctx, permissions) {
const wanted = [...new Set(permissions)];
/** @type {Promise<Record<string, boolean>> | 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<string, unknown> }[]} entries
* @param {{ resource: string; details: Record<string, unknown> }[]} entries
* @param {boolean} prompt
* @returns {Promise<unknown[]>}
*/
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<string, unknown>} */ ({}));
const scratch = entries.map(
() => /** @type {Record<string, unknown>} */ ({}),
);
/** 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<string, unknown> }[]}
* @returns {{ resource: string; details: Record<string, unknown> }[]}
*/
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<unknown>}
*/
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<boolean | boolean[]>}
*/
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)
+40 -12
View File
@@ -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 () => {