From b4b464554b76fb2bdff2b7eef9f8dfc44553daa3 Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Mon, 21 Sep 2026 15:00:02 -0400 Subject: [PATCH] fix: settle a URL file-access prompt from a grant the app already holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening `/app/?file=` asked for consent on every launch: the gate prompted unconditionally and never read the `fs::write` grant its own Allow had written. `/auth/check-permissions` takes an optional `app_uid` so the account can ask what one of its apps holds, and the launch gate skips the dialog when the answer is yes. Sessions only — an app or a scoped token asking would be a window onto its neighbours' grants. --- .../controllers/auth/AuthController.test.ts | 116 ++++++++++++++++++ .../controllers/auth/AuthController.ts | 40 +++++- src/gui/src/helpers/confirmUrlFileAccess.js | 20 ++- .../src/helpers/confirmUrlFileAccess.test.js | 28 ++++- src/gui/src/helpers/holdsPermissions.js | 111 +++++++++++------ src/gui/src/helpers/holdsPermissions.test.js | 35 +++++- 6 files changed, 302 insertions(+), 48 deletions(-) diff --git a/src/backend/controllers/auth/AuthController.test.ts b/src/backend/controllers/auth/AuthController.test.ts index 7d468b6af..40aecb1fb 100644 --- a/src/backend/controllers/auth/AuthController.test.ts +++ b/src/backend/controllers/auth/AuthController.test.ts @@ -5865,6 +5865,122 @@ describe('AuthController.handleCheckPermissions + handleListPermissions', () => }); }); + // What lets a launch settle a consent prompt it holds no app token for. + it('check-permissions: `app_uid` answers for that app, not for the asking user', async () => { + const { user, actor } = await makeUserAndActor(); + const app = await server.stores.app.create( + { + name: `cpa-${uuidv4()}`, + title: 'TestCheckPermsAppUid', + index_url: 'https://check-perms-uid.example.test/index.html', + }, + { ownerUserId: user.id }, + ); + const permission = `user:${user.uuid}:email:read`; + + // Held by the user, so asking as the user would answer `true`. + const asUser = makeRes(); + await inCtx(actor, () => + controller.handleCheckPermissions( + makeReq({ permissions: [permission] }, { actor }), + asUser, + ), + ); + expect(asUser.body).toEqual({ permissions: { [permission]: true } }); + + const before = makeRes(); + await inCtx(actor, () => + controller.handleCheckPermissions( + makeReq( + { permissions: [permission], app_uid: app.uid }, + { actor }, + ), + before, + ), + ); + expect(before.body).toEqual({ permissions: { [permission]: false } }); + + await inCtx(actor, () => + controller.handleGrantUserApp( + makeReq( + { app_uid: app.uid, permission, extra: {} }, + { actor }, + ), + makeRes(), + ), + ); + + const after = makeRes(); + await inCtx(actor, () => + controller.handleCheckPermissions( + makeReq( + { permissions: [permission], app_uid: app.uid }, + { actor }, + ), + after, + ), + ); + expect(after.body).toEqual({ permissions: { [permission]: true } }); + }); + + it('check-permissions: an app cannot ask about another app, and an unknown app 404s', async () => { + const { user, actor } = await makeUserAndActor(); + const app = await server.stores.app.create( + { + name: `cpx-${uuidv4()}`, + title: 'TestCheckPermsCrossApp', + index_url: 'https://check-perms-x.example.test/index.html', + }, + { ownerUserId: user.id }, + ); + const appActor = makeActor({ + user: actor.user, + app: { id: app.id, uid: app.uid }, + }); + + await expect( + inCtx(appActor, () => + controller.handleCheckPermissions( + makeReq( + { permissions: ['service:foo:ii:read'], app_uid: app.uid }, + { actor: appActor }, + ), + makeRes(), + ), + ), + ).rejects.toMatchObject({ statusCode: 403 }); + + await expect( + inCtx(actor, () => + controller.handleCheckPermissions( + makeReq( + { + permissions: ['service:foo:ii:read'], + app_uid: `app-${uuidv4()}`, + }, + { actor }, + ), + makeRes(), + ), + ), + ).rejects.toMatchObject({ statusCode: 404 }); + + await expect( + inCtx(actor, () => + controller.handleCheckPermissions( + makeReq( + { + permissions: ['service:foo:ii:read'], + app_uid: { not: 'a string' } as unknown as string, + }, + { actor }, + ), + makeRes(), + ), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + it('list-permissions: returns the shape and includes a user→app grant with its app_uid', async () => { const { user, actor } = await makeUserAndActor(); const app = await server.stores.app.create( diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index 2acdc7ff8..7aefa1a7b 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -33,6 +33,7 @@ import { hasVerifiedPhone, } from '../../core/http/middleware/gates.js'; import type { Actor } from '../../core/actor.js'; +import { isPlainUserActor, makeActor } from '../../core/actor.js'; import { checkRateLimit } from '../../core/http/middleware/rateLimit.js'; import { signStepUpToken, @@ -3535,27 +3536,56 @@ export class AuthController extends PuterController { // -- Permission checks ------------------------------------------- + /** + * The caller's account acting as `appIdentifier`, resolved uid-or-name like + * the grant handlers. + */ + async #appUnderUserActor( + actor: Actor, + appIdentifier: unknown, + ): Promise { + this.#validateAppPermissionParams({ app_uid: appIdentifier }); + // Sessions only: for an app, this would be a window onto its neighbours' grants. + if (!isPlainUserActor(actor)) { + throw new HttpError(403, 'actor must be a user', { + legacyCode: 'forbidden', + }); + } + const app = await this.stores.app.resolveApp(appIdentifier as string); + if (!app) { + throw new HttpError(404, `App ${appIdentifier} does not exist`, { + legacyCode: 'not_found', + }); + } + return makeActor({ + user: actor.user, + app: { id: app.id, uid: app.uid }, + }); + } + @Post('/auth/check-permissions', { subdomain: 'api', requireAuth: true, rateLimit: AUTH_CHECK_LIMIT, }) async handleCheckPermissions(req: Request, res: Response): Promise { - const { permissions } = req.body ?? {}; + const { permissions, app_uid } = req.body ?? {}; if (!Array.isArray(permissions)) { throw new HttpError(400, 'Missing or invalid `permissions` array', { legacyCode: 'bad_request', }); } + // `app_uid` asks what an app of mine holds, not what I hold. + const actor = app_uid + ? await this.#appUnderUserActor(req.actor!, app_uid) + : req.actor!; + const unique = [...new Set(permissions)] as string[]; const result: Record = {}; let granted: Map; try { - granted = await this.services.permission.checkMany( - req.actor!, - unique, - ); + granted = await this.services.permission.checkMany(actor, unique); } catch { granted = new Map(); } diff --git a/src/gui/src/helpers/confirmUrlFileAccess.js b/src/gui/src/helpers/confirmUrlFileAccess.js index e72879c7c..8d3344531 100644 --- a/src/gui/src/helpers/confirmUrlFileAccess.js +++ b/src/gui/src/helpers/confirmUrlFileAccess.js @@ -18,6 +18,7 @@ */ import UIPermissionDialog from '../UI/UIPermissionDialog.js'; +import { appHoldsPermissions } from './holdsPermissions.js'; import { isUuid } from './sharePaths.js'; /** @@ -52,14 +53,16 @@ export const urlFileLaunchOptions = (value) => { * @param {object} [deps] Injectable seams for tests. * @param {(target: { path?: string, uid?: string }) => Promise<{ uid?: string, path?: string, is_dir?: boolean }>} [deps.stat] * @param {(options: object) => Promise} [deps.permissionDialog] + * @param {(permissions: string[], appUid: string) => Promise} [deps.holdsPermissions] * @returns {Promise<{ uid: string, path?: string } | null>} The file as - * stat'd, only if the user allowed it; `null` otherwise. + * stat'd, only if the user allowed it, or if they already had; `null` otherwise. */ export const confirmUrlFileAccess = async ( { path, uid, appUid, appName }, { stat = (target) => puter.fs.stat({ ...target, consistency: 'eventual' }), permissionDialog = UIPermissionDialog, + holdsPermissions = appHoldsPermissions, } = {}, ) => { if ( (! path && ! uid) || ! appUid ) return null; @@ -79,16 +82,23 @@ export const confirmUrlFileAccess = async ( return null; } + // By uid: it is what the grant is stored against, and a recipient's path is masked. + const permission = `fs:${fsentry.uid}:write`; + const file = { uid: fsentry.uid, path: fsentry.path }; + + // Consent already given is not a question to ask again on every launch. + if ( await holdsPermissions([permission], appUid) ) { + return file; + } + const granted = await permissionDialog({ app_uid: appUid, app_name: appName, - // By uid: it is what the grant is stored against, and a path a - // recipient sees is a masked stand-in for the owner's. - permission: `fs:${fsentry.uid}:write`, + permission, // The entry was just stat'd; nothing here should bring one into being. create: false, }); - return granted === true ? { uid: fsentry.uid, path: fsentry.path } : null; + return granted === true ? file : null; }; export default confirmUrlFileAccess; diff --git a/src/gui/src/helpers/confirmUrlFileAccess.test.js b/src/gui/src/helpers/confirmUrlFileAccess.test.js index dd2de1100..67582ede6 100644 --- a/src/gui/src/helpers/confirmUrlFileAccess.test.js +++ b/src/gui/src/helpers/confirmUrlFileAccess.test.js @@ -31,10 +31,12 @@ const FILE = '/alice/Documents/notes.txt'; const FILE_UID = '2b7d8c1e-4f3a-4b6c-9d1e-0a1b2c3d4e5f'; let permissionDialog; -const deps = (stat) => ({ stat, permissionDialog }); +let holdsPermissions; +const deps = (stat) => ({ stat, permissionDialog, holdsPermissions }); beforeEach(() => { permissionDialog = vi.fn(async () => true); + holdsPermissions = vi.fn(async () => false); vi.spyOn(console, 'warn').mockImplementation(() => {}); }); @@ -78,6 +80,30 @@ describe('confirmUrlFileAccess', () => { })); }); + // The reason this gate stopped asking on every launch of the same link. + it('hands the file over without prompting when the app already holds the grant', async () => { + const stat = vi.fn(async () => ({ uid: FILE_UID, path: FILE, is_dir: false })); + holdsPermissions = vi.fn(async () => true); + + await expect(confirmUrlFileAccess( + { path: FILE, appUid: APP, appName: 'notepad' }, deps(stat), + )).resolves.toEqual({ uid: FILE_UID, path: FILE }); + + expect(holdsPermissions).toHaveBeenCalledWith([`fs:${FILE_UID}:write`], APP); + expect(permissionDialog).not.toHaveBeenCalled(); + }); + + // A check that couldn't be made is not consent. + it('still prompts when the check for an existing grant fails', async () => { + const stat = vi.fn(async () => ({ uid: FILE_UID, path: FILE, is_dir: false })); + holdsPermissions = vi.fn(async () => false); + + await expect(confirmUrlFileAccess( + { path: FILE, appUid: APP }, deps(stat), + )).resolves.toEqual({ uid: FILE_UID, path: FILE }); + expect(permissionDialog).toHaveBeenCalled(); + }); + it('reports a refusal when the user denies', async () => { permissionDialog = vi.fn(async () => false); const stat = vi.fn(async () => ({ uid: FILE_UID, path: FILE, is_dir: false })); diff --git a/src/gui/src/helpers/holdsPermissions.js b/src/gui/src/helpers/holdsPermissions.js index da7139b81..e966ca72e 100644 --- a/src/gui/src/helpers/holdsPermissions.js +++ b/src/gui/src/helpers/holdsPermissions.js @@ -20,6 +20,60 @@ // The dialog waits behind this check, so a stalled read must not hold it up. const CHECK_TIMEOUT_MS = 5000; +/** + * One `/auth/check-permissions` round trip: `token` says whose access is in question, unless `appUid` names an app under that user. + * + * @param {string[]} permissions + * @param {object} options + * @param {string} options.token + * @param {string} [options.appUid] + * @param {typeof fetch} [options.fetchImpl] + * @param {string} [options.apiOrigin] + * @param {number} [options.timeoutMs] + * @returns {Promise} + */ +const queryHeld = async ( + permissions, + { + token, + appUid, + fetchImpl = globalThis.fetch?.bind(globalThis), + apiOrigin = window.api_origin, + timeoutMs = CHECK_TIMEOUT_MS, + }, +) => { + if ( ! token || ! Array.isArray(permissions) || permissions.length === 0 ) { + return false; + } + const controller = typeof AbortController !== 'undefined' + ? new AbortController() + : null; + const expiry = setTimeout(() => controller?.abort(), timeoutMs); + try { + const resp = await fetchImpl(`${apiOrigin}/auth/check-permissions`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${token}`, + }, + body: JSON.stringify({ + permissions: [...new Set(permissions)], + ...(appUid ? { app_uid: appUid } : {}), + }), + ...(controller ? { signal: controller.signal } : {}), + }); + if ( ! resp.ok ) return false; + const held = (await resp.json())?.permissions ?? {}; + // Every scope: one prompt is one decision, so partly-held is unheld. + return permissions.every((p) => held[p] === true); + } catch (e) { + console.error('Failed to check held permissions', e); + return false; + } finally { + clearTimeout(expiry); + } +}; + /** * Whether every one of these permissions is already held by whoever `token` * identifies — an app-under-user token, so the answer is about that app's @@ -37,42 +91,27 @@ const CHECK_TIMEOUT_MS = 5000; * @param {number} [deps.timeoutMs] * @returns {Promise} */ -export const holdsPermissions = async ( - permissions, - token, - { - fetchImpl = globalThis.fetch?.bind(globalThis), - apiOrigin = window.api_origin, - timeoutMs = CHECK_TIMEOUT_MS, - } = {}, -) => { - if ( ! token || ! Array.isArray(permissions) || permissions.length === 0 ) { - return false; - } - const controller = typeof AbortController !== 'undefined' - ? new AbortController() - : null; - const expiry = setTimeout(() => controller?.abort(), timeoutMs); - try { - const resp = await fetchImpl(`${apiOrigin}/auth/check-permissions`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - Authorization: `Bearer ${token}`, - }, - body: JSON.stringify({ permissions: [...new Set(permissions)] }), - ...(controller ? { signal: controller.signal } : {}), - }); - if ( ! resp.ok ) return false; - const held = (await resp.json())?.permissions ?? {}; - // Every scope: one prompt is one decision, so partly-held is unheld. - return permissions.every((p) => held[p] === true); - } catch (e) { - console.error('Failed to check held permissions', e); - return false; - } finally { - clearTimeout(expiry); - } +export const holdsPermissions = (permissions, token, deps = {}) => + queryHeld(permissions, { ...deps, token }); + +/** + * The same question asked as the user, for flows holding no app token yet. + * + * A missing uid reports not held: asked as the user alone, their own file answers `true`. + * + * @param {string[]} permissions + * @param {string} appUid + * @param {object} [deps] Injectable seams for tests. + * @param {string} [deps.authToken] + * @param {typeof fetch} [deps.fetchImpl] + * @param {string} [deps.apiOrigin] + * @param {number} [deps.timeoutMs] + * @returns {Promise} + */ +export const appHoldsPermissions = (permissions, appUid, deps = {}) => { + if ( ! appUid ) return Promise.resolve(false); + const { authToken = window.auth_token, ...rest } = deps; + return queryHeld(permissions, { ...rest, appUid, token: authToken }); }; export default holdsPermissions; diff --git a/src/gui/src/helpers/holdsPermissions.test.js b/src/gui/src/helpers/holdsPermissions.test.js index 984effc66..06c4aa515 100644 --- a/src/gui/src/helpers/holdsPermissions.test.js +++ b/src/gui/src/helpers/holdsPermissions.test.js @@ -18,7 +18,7 @@ */ import { describe, it, expect } from 'vitest'; -import { holdsPermissions } from './holdsPermissions.js'; +import { appHoldsPermissions, holdsPermissions } from './holdsPermissions.js'; const EMAIL = 'user:u-1:email:read'; const APPS = 'apps-of-user:u-1:read'; @@ -102,3 +102,36 @@ describe('holdsPermissions', () => { expect(calls).toHaveLength(0); }); }); + +const FILE = 'fs:2b7d8c1e-4f3a-4b6c-9d1e-0a1b2c3d4e5f:write'; +const appDeps = (fetchImpl) => ({ ...deps(fetchImpl), authToken: 'user-token' }); + +describe('appHoldsPermissions', () => { + it('asks as the user about the named app', async () => { + const { fetchImpl, calls } = makeFetch({ held: { [FILE]: true } }); + + await expect(appHoldsPermissions([FILE], 'app-uid-1', appDeps(fetchImpl))) + .resolves.toBe(true); + + expect(calls[0].headers.Authorization).toBe('Bearer user-token'); + expect(calls[0].body).toEqual({ permissions: [FILE], app_uid: 'app-uid-1' }); + }); + + // Asked as the user with no app named, the user's own file answers `true`. + it('reports not held, and asks nothing, without an app to name', async () => { + const { fetchImpl, calls } = makeFetch({ held: { [FILE]: true } }); + + await expect(appHoldsPermissions([FILE], undefined, appDeps(fetchImpl))) + .resolves.toBe(false); + await expect(appHoldsPermissions([FILE], '', appDeps(fetchImpl))) + .resolves.toBe(false); + + expect(calls).toHaveLength(0); + }); + + it('reports not held when the app has not been granted it', async () => { + const { fetchImpl } = makeFetch({ held: { [FILE]: false } }); + await expect(appHoldsPermissions([FILE], 'app-uid-1', appDeps(fetchImpl))) + .resolves.toBe(false); + }); +});