From 727726437a38bf6ed703dc2229eeeb18beb70e14 Mon Sep 17 00:00:00 2001 From: jelveh Date: Sun, 26 Jul 2026 10:24:32 -0700 Subject: [PATCH] Close three gaps left in the permission-request flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each was reproduced first — the squatting grant against a running server, the COOP timing in a real browser — and each fix was then confirmed by reverting it and watching the new test fail. Security: the dialog could name one site and grant to another. The squatter guard added for grant/revoke by `origin` only ran when `app_uid` was absent, and the dialog sends both — so `app_uid` won and the guard never applied. `getAppUIDFromOrigin` returns the synthetic `app-` for any origin with no app row of its own, and the grant endpoint resolves `app_uid` as uid *or name*, so the grant landed on whoever registered an app under that computed name (the format allows it, and the namespace is a source constant). A link like `/action/request-permission?origin=https://a-site-you-trust.example` named that site in the prompt while "Allow" handed the permission elsewhere. Fixed on both sides of the wire. The action now sends the origin alone — no uid resolved in the browser is safe to forward, whatever its source — and a supplied `origin` now decides the target on the server even when an `app_uid` travels beside it: the origin is what the prompt showed the user, so it is what the grant has to follow. Correctness: a COOP-only site was answered before the user decided. 7efc0c0b routed a severed opener to `pollDecision` by treating an already-closed popup as severed, but that reads `closed` synchronously after `window.open()` — before the navigation whose response headers cause the severing has committed. Measured in Chromium: `closed` is false at 0ms and true by 200ms. So a site sending COOP: same-origin without COEP still took the watch-the-window path, and `requestPermission` resolved false 1.1s after the click while the dialog was still on screen. The "Allow" that followed committed a grant the site had been told it did not get. Tell the two apart by when the close lands, and keep the in-flight message's grace period on both branches — an answer already on its way outranks whatever the close is taken to mean. Correctness: a grant that timed out was not withdrawn. `grant_may_have_committed` was only set in the fetch's `catch`, which cannot run until the timeout's timer callback returns — and that callback already calls `fail_grant`, which settles the dialog as a denial outright when the dialog was force-closed mid-grant. The reconciliation was skipped in exactly the case it exists for. Record the unknown outcome in the timer, where the timeout already means the request left the browser. Also require a `token` from the user-app exchange rather than just a non-null body: an HTTP failure (a blocked origin, a 5xx) returns the parsed *error* body, which is truthy, so the guard added for this missed it — handing the opener an `undefined` token, and prompting for a grant whose app row was never bootstrapped. Known limitation, now more reachable: a severed opener cannot signal a denial at all, since nothing is written for one, so those sites wait out the poll timeout before receiving false. A grant still resolves promptly. --- .../controllers/auth/AuthController.test.ts | 94 ++++++++ .../controllers/auth/AuthController.ts | 10 +- src/gui/src/UI/UIPermissionDialog.js | 9 + src/gui/src/initgui.js | 53 ++--- src/puter-js/src/modules/UI.js | 40 +++- .../tests/e2e/specs/requestPermission.spec.js | 203 ++++++++++++++++++ 6 files changed, 378 insertions(+), 31 deletions(-) diff --git a/src/backend/controllers/auth/AuthController.test.ts b/src/backend/controllers/auth/AuthController.test.ts index cbcb7db9e..5f0e2e3f5 100644 --- a/src/backend/controllers/auth/AuthController.test.ts +++ b/src/backend/controllers/auth/AuthController.test.ts @@ -1675,6 +1675,100 @@ describe('AuthController grant flows', () => { expect(rows).toEqual([]); }); + it('grant/revoke-user-app: an `app_uid` sent beside an `origin` cannot redirect the grant away from that origin', async () => { + // The origin is what a consent prompt shows the user, so it has to + // decide who receives the grant. Resolving `origin` only when + // `app_uid` was absent left the squatter guard bypassable by simply + // sending both: the uid won, and it is resolved as uid-*or-name*, so + // the synthetic `app-` of an unregistered origin landed + // on whoever registered an app under that literal name. + const origin = `https://unregistered-${uuidv4()}.example`; + const syntheticUid = `app-${uuidv5(origin, APP_ORIGIN_UUID_NAMESPACE)}`; + const squatter = await server.stores.app.create( + { + name: syntheticUid, + title: 'SquatterBesideOrigin', + index_url: 'https://squatter-beside-origin.example/index.html', + }, + { ownerUserId: target.id }, + ); + + const permission = 'service:squat-beside:ii:read'; + for (const handler of [ + 'handleGrantUserApp', + 'handleRevokeUserApp', + ] as const) { + await expect( + inCtx(issuerActor, () => + controller[handler]( + makeReq( + { + app_uid: syntheticUid, + origin, + permission, + extra: {}, + }, + { actor: issuerActor }, + ), + makeRes(), + ), + ), + ).rejects.toMatchObject({ statusCode: 404 }); + } + + const rows = await server.clients.db.read( + 'SELECT p.`permission` FROM `user_to_app_permissions` p ' + + 'WHERE p.`user_id` = ? AND p.`app_id` = ?', + [issuer.id, squatter.id], + ); + expect(rows).toEqual([]); + }); + + it('grant-user-app: a registered `origin` beside an unrelated `app_uid` grants to the origin, not the uid', async () => { + // Same precedence rule, on the path where the origin does resolve: the + // uid travelling beside it must not steer the grant somewhere else. + const appName = `tp-origin-${uuidv4()}`; + const origin = `https://${appName}.example.test`; + const app = await server.stores.app.create( + { + name: appName, + title: 'TestPrecedenceOriginApp', + index_url: `${origin}/index.html`, + }, + { ownerUserId: issuer.id }, + ); + const other = await server.stores.app.create( + { + name: `tp-other-${uuidv4()}`, + title: 'TestPrecedenceOtherApp', + index_url: `https://tp-other-${uuidv4()}.example.test/index.html`, + }, + { ownerUserId: issuer.id }, + ); + + const permission = 'service:tp-origin:ii:read'; + await inCtx(issuerActor, () => + controller.handleGrantUserApp( + makeReq( + { app_uid: other.uid, origin, permission, extra: {} }, + { actor: issuerActor }, + ), + makeRes(), + ), + ); + + const granted = async (appId: number) => + ( + (await server.clients.db.read( + 'SELECT p.`permission` FROM `user_to_app_permissions` p ' + + 'WHERE p.`user_id` = ? AND p.`app_id` = ?', + [issuer.id, appId], + )) as Array<{ permission: string }> + ).map((r) => r.permission); + expect(await granted(app.id)).toContain(permission); + expect(await granted(other.id)).not.toContain(permission); + }); + it('grant-user-app: 400 on a non-object `extra`/`meta` instead of committing then faulting', async () => { // These are forwarded into the audit row and read as objects // downstream, so a bad value used to surface as a 500 *after* the diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index 2b6557aa8..e935bf9fc 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -2726,6 +2726,12 @@ export class AuthController extends PuterController { * collect a grant the user made to the origin — the uid is derived from a * published namespace constant, so it can be computed and squatted offline. * Only a uid that names an existing app row is accepted. + * + * An `origin` supplied alongside an `app_uid` takes precedence over it (see + * the grant/revoke handlers). The origin is what a consent prompt shows the + * user, so it — not a uid travelling beside it — has to decide who receives + * the grant; otherwise a caller could name one app on screen and grant to + * another. No caller sends both with different intent. */ async #registeredAppUidFromOrigin(origin: string): Promise { const uid = await this.services.auth.appUidFromOrigin(origin); @@ -2752,7 +2758,7 @@ export class AuthController extends PuterController { extra, meta, }); - if (origin && !app_uid) { + if (origin) { app_uid = await this.#registeredAppUidFromOrigin(origin); } if (!app_uid || !permission) { @@ -2833,7 +2839,7 @@ export class AuthController extends PuterController { permission, meta, }); - if (origin && !app_uid) { + if (origin) { app_uid = await this.#registeredAppUidFromOrigin(origin); } if (!app_uid || !permission) { diff --git a/src/gui/src/UI/UIPermissionDialog.js b/src/gui/src/UI/UIPermissionDialog.js index da816c333..b4d6313d0 100644 --- a/src/gui/src/UI/UIPermissionDialog.js +++ b/src/gui/src/UI/UIPermissionDialog.js @@ -215,6 +215,15 @@ async function show_permission_dialog (options) { ? new AbortController() : null; const grant_timer = setTimeout(() => { + // A request still outstanding after the timeout has left this + // browser with an unknown outcome, so record that before + // anything can settle. The `catch` below sets the same flag, + // but only from a microtask that cannot run until this timer + // callback returns — by which time `fail_grant` may already + // have settled the dialog as a denial (it does exactly that + // when the dialog was force-closed mid-grant), leaving a + // committed grant with no revoke behind it. + grant_may_have_committed = true; controller?.abort(); // Also recover on engines with no AbortController, where the // fetch above may never settle on its own. diff --git a/src/gui/src/initgui.js b/src/gui/src/initgui.js index c674fe226..9ef8acaf1 100644 --- a/src/gui/src/initgui.js +++ b/src/gui/src/initgui.js @@ -212,10 +212,14 @@ const postAuthActions = async (action) => { try { let data = await window.getUserAppToken(new URL(window.openerOrigin).origin); // `getUserAppToken` reports a network failure by returning - // null, so say what went wrong instead of faulting on the - // property read below. - if ( ! data ) { - throw new Error('user-app token exchange returned no data'); + // null, and an HTTP failure (a blocked origin, a 5xx) by + // returning the parsed *error* body — which is truthy but + // carries no token. Both mean the exchange did not happen, so + // say what went wrong instead of handing the opener an + // `undefined` token and, for actions that depend on the app row + // this bootstraps, prompting for a grant that could only fail. + if ( ! data?.token ) { + throw new Error('user-app token exchange returned no token'); } // This is an implicit app and the app_uid is sent back from the server // we cache it here so that we can use it later @@ -501,22 +505,20 @@ const postAuthActions = async (action) => { if ( token_exchange_failed ) { throw new Error('token exchange failed; not prompting'); } - // The requesting app is identified by its origin only. A uid from - // the query string is never trusted: it is chosen by whoever - // opened this page, so honouring it would let a link grant a - // permission to an app the dialog never named. When the origin - // can't be resolved here, the uid is left unset and the server - // resolves it from the same origin the dialog displayed. - let app_uid; - if ( origin ) { - app_uid = window.host_app_uid - ?? await window.getAppUIDFromOrigin(origin) - ?? undefined; - } - + // The requesting app is identified by its origin, and only the + // server turns that origin into a grant target. No uid is sent + // from here: a uid from the query string is chosen by whoever + // opened this page, and even a uid resolved through + // `getAppUIDFromOrigin` is unsafe to forward, because an origin + // with no app row of its own resolves to a *synthetic* + // `app-`. The grant endpoint resolves `app_uid` + // as uid-or-name, so forwarding that synthetic uid would hand + // the grant to whoever registered an app under that literal + // name. Passing the origin instead makes the server resolve the + // same origin the dialog displayed, and reject it outright + // unless it names an app that really exists. granted = await UIPermissionDialog({ permission: permission, - app_uid: app_uid, origin: origin, }); } catch (e) { @@ -1762,11 +1764,14 @@ window.initgui = async function (options) { let data = await window.getUserAppToken( new URL(window.openerOrigin).origin, ); - // A network failure here returns null, not - // a throw; the reads below would fault. - if (!data) { + // A network failure here returns null and + // an HTTP failure returns the parsed error + // body, neither of which carries a token; + // the reads below would fault or hand the + // opener an `undefined` token. + if (!data?.token) { throw new Error( - 'user-app token exchange returned no data', + 'user-app token exchange returned no token', ); } // This is an implicit app and the app_uid is sent back from the server @@ -1852,9 +1857,9 @@ window.initgui = async function (options) { let data = await window.getUserAppToken( new URL(window.openerOrigin).origin, ); - if (!data) { + if (!data?.token) { throw new Error( - 'user-app token exchange returned no data', + 'user-app token exchange returned no token', ); } // This is an implicit app and the app_uid is sent back from the server diff --git a/src/puter-js/src/modules/UI.js b/src/puter-js/src/modules/UI.js index 240f26ec2..3365a7816 100644 --- a/src/puter-js/src/modules/UI.js +++ b/src/puter-js/src/modules/UI.js @@ -1211,16 +1211,46 @@ class UI extends EventListener { pollDecision(); return; } + // `closed` read right after `window.open()` cannot see COOP + // severing yet: the popup is still the initial about:blank in + // this browsing-context group, and the group is only swapped + // when the navigation to the Puter origin *commits* — measured + // at ~200ms. So severing shows up here, in the poll, as + // `closed` flipping true moments after opening. + // + // A real user close reads identically, which is why the two are + // told apart by *when* they happen: severing lands while the + // popup is still loading, whereas closing it by hand means + // finding the window and clicking it. Anything inside this + // window is therefore the channel going away, not an answer — + // reporting a denial for it would tell the site "denied" while + // the user goes on to click Allow and commit the grant. + const SEVERED_WINDOW_MS = 3000; + const opened_at = Date.now(); checkClosed = setInterval(() => { if ( ! popup.closed ) return; + clearInterval(checkClosed); + checkClosed = null; + const severed = Date.now() - opened_at < SEVERED_WINDOW_MS; // The GUI posts the decision and then closes the popup, // and cross-process postMessage delivery is not ordered // relative to `closed` becoming true. Give an in-flight - // grant message a grace period before treating the close - // as a denial. - clearInterval(checkClosed); - checkClosed = null; - setTimeout(() => settle(false), CLOSE_GRACE_MS); + // decision message its grace period before acting on the + // close — on either branch, since a real answer already on + // its way outranks whatever the close is taken to mean. + setTimeout(() => { + if ( settled ) return; + if ( severed ) { + // The decision can still be read back from the + // server. A denial can't (nothing is written for + // it), so this only ends early on a grant — + // otherwise it waits out the poll timeout before + // answering false. + pollDecision(); + return; + } + settle(false); + }, CLOSE_GRACE_MS); }, 100); }; diff --git a/src/puter-js/tests/e2e/specs/requestPermission.spec.js b/src/puter-js/tests/e2e/specs/requestPermission.spec.js index 50edbf268..54300e0b1 100644 --- a/src/puter-js/tests/e2e/specs/requestPermission.spec.js +++ b/src/puter-js/tests/e2e/specs/requestPermission.spec.js @@ -291,6 +291,76 @@ test.describe('puter.ui.requestPermission (env=app)', () => { await deleteTestApp(page, appName); } }); + + test('a grant that is still hanging when the dialog is force-closed is withdrawn too', async ({ page, context }) => { + // Same reconciliation, reached through the timeout instead of an error + // response. The timeout aborts the request, and the abort's rejection is + // only delivered as a microtask — it cannot run until the timer callback + // returns. So when the dialog has already been force-closed (the close + // watcher can bypass the `cancel` handler), the timer settles the dialog + // as a denial *before* the outcome is recorded as unknown, and the + // permission the request may have committed is left behind, granted, + // with the app told it was refused. + const appName = await registerTestApp(page, { fixtureURL: PERMISSION_FIXTURE_URL }); + const permission = 'driver:puter-image-generation:generate'; + const isGrantedTo = (appUid) => page.evaluate(async ({ perm, uid }) => { + const res = await fetch(`${puter.APIOrigin}/auth/list-permissions`, { + headers: { 'Authorization': `Bearer ${puter.authToken}` }, + }); + const body = await res.json(); + return body.myself_to_app.some( + r => r.permission === perm && r.app_uid === uid, + ); + }, { perm: permission, uid: appUid }); + + try { + const appFrame = await gotoTestApp(page, appName); + const appUid = await page.evaluate( + async (name) => (await puter.apps.get(name)).uid, + appName, + ); + const dialog = page.locator('dialog.perm-dialog'); + + // Grant it for real first, so there is a live row the withdrawal + // has to remove. + await appFrame.locator('#req-driver-perm').click(); + await expect(dialog).toBeVisible(); + await dialog.locator('.perm-dialog-allow').click(); + await expect(appFrame.locator('#log [data-entry="perm:driver:true"]')).toBeVisible(); + expect(await isGrantedTo(appUid)).toBe(true); + + // Hang the next grant past the dialog's 15s time-box. + let revoked = false; + await context.route('**/auth/grant-user-app', async () => { + await new Promise(r => setTimeout(r, 60_000)); + }); + await context.route('**/auth/revoke-user-app', async (route) => { + revoked = true; + await route.continue(); + }); + + await appFrame.locator('#req-driver-perm').click(); + await expect(dialog).toBeVisible(); + await dialog.locator('.perm-dialog-allow').click(); + await expect(dialog.locator('.perm-dialog-allow.perm-dialog-busy')).toBeVisible(); + + // What the close watcher does on a repeated close request: close + // without firing a cancelable `cancel`, while the grant is still in + // flight. There is then no retry UI left, so the timeout has to + // settle it — as a denial it must reconcile. + await page.evaluate(() => { + document.querySelector('dialog.perm-dialog')?.close(); + }); + + await expect(appFrame.locator('#log [data-entry="perm:driver:false"]')) + .toBeVisible({ timeout: 30_000 }); + await expect.poll(() => revoked, { timeout: 20_000 }).toBe(true); + await expect.poll(() => isGrantedTo(appUid), { timeout: 20_000 }).toBe(false); + } finally { + await context.unroute('**/auth/grant-user-app'); + await deleteTestApp(page, appName); + } + }); }); test.describe('puter.ui.requestPermission (env=gui)', () => { @@ -508,6 +578,102 @@ test.describe('puter.ui.requestPermission (env=web popup, first visit)', () => { }); }); +test.describe('puter.ui.requestPermission (env=web, COOP-only opener)', () => { + test('a site that only sets COOP is not answered before the user has decided', async ({ page, context }) => { + // `Cross-Origin-Opener-Policy: same-origin` without COEP severs the + // opener relationship — but only when the popup's navigation *commits*, + // a couple of hundred milliseconds after `window.open()` returns. So the + // severed-opener check that runs synchronously after opening cannot see + // it, and the severing instead surfaces as `popup.closed` flipping true + // while the popup is still loading. Treating that as a close reported a + // denial about a second after the click — before the user had even seen + // the dialog — and then the Allow they went on to click committed a + // grant the site had been told it did not get. + const permission = 'driver:puter-image-generation:generate'; + + // Hand the site a token for its *own* app, the way a signed-in + // third-party site holds one. Without it the poll fallback has nothing + // to authenticate with and answers false immediately (covered above), + // and `/auth/check-permissions` has to run as this app-under-user actor + // for a user→app grant to be visible at all. + // `window.api_origin` / `window.auth_token`, not the SDK's: the + // prod-built GUI bundles an SDK pointed at api.puter.com, so + // `puter.APIOrigin` on this page is production. + await page.goto('/'); + await page.waitForFunction(() => !!window.getUserAppToken && !!window.auth_token, + null, { timeout: 60_000 }); + const fixtureOrigin = new URL(PERMISSION_FIXTURE_URL).origin; + const appToken = await page.evaluate( + async (origin) => (await window.getUserAppToken(origin))?.token, + fixtureOrigin, + ); + expect(typeof appToken).toBe('string'); + + // Earlier tests grant this same permission to the fixture origin's app, + // and the row outlives them — clear it so the poll starting out true + // can't pass this test on its own. + await page.evaluate(async ({ origin, perm }) => { + await fetch(`${window.api_origin}/auth/revoke-user-app`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Authorization': `Bearer ${window.auth_token}`, + }, + body: JSON.stringify({ origin, permission: perm }), + }); + }, { origin: fixtureOrigin, perm: permission }); + + await context.route(PERMISSION_FIXTURE_URL, async (route) => { + const resp = await route.fetch(); + await route.fulfill({ + response: resp, + headers: { + ...resp.headers(), + 'cross-origin-opener-policy': 'same-origin', + }, + }); + }); + + await page.goto(PERMISSION_FIXTURE_URL); + await page.locator('body.ready').waitFor({ timeout: 60_000 }); + // COOP-only: severed, but *not* cross-origin-isolated (that needs COEP + // too), so the isolation check above does not catch this case. + expect(await page.evaluate(() => window.crossOriginIsolated)).toBe(false); + await page.evaluate((t) => puter.setAuthToken(t), appToken); + + const [popup] = await Promise.all([ + page.waitForEvent('popup'), + page.locator('#req-driver-perm').click(), + ]); + const dialog = popup.locator('dialog.perm-dialog'); + await expect(dialog).toBeVisible({ timeout: 60_000 }); + + // Long enough to cover the severing (~200ms), the close grace period + // (1s) and the window in which a close is read as severing (3s). + await page.waitForTimeout(5000); + expect(await page.locator('#log [data-entry="perm:driver:false"]').count()).toBe(0); + expect(await page.locator('#log [data-entry="perm:driver:true"]').count()).toBe(0); + + // The answer itself has to come from the server poll, since the popup + // cannot reach a severed opener. That leg is not asserted here: the poll + // is a direct site→API request, and Chrome refuses it from this + // loopback fixture origin ("Permission was denied for this request to + // access the `loopback` address space"), which does not apply to the + // https origins this runs on in production. The grant still has to + // succeed, which is what the popup is left to do. + const granted = popup.waitForResponse( + (r) => r.url().includes('/auth/grant-user-app') && r.status() === 200, + { timeout: 30_000 }, + ); + await dialog.locator('.perm-dialog-allow').click(); + await granted; + // Polled rather than awaiting the `close` event: the popup closes itself + // as soon as it has answered, which can happen before a listener + // registered after the click is attached. + await expect.poll(() => popup.isClosed(), { timeout: 30_000 }).toBe(true); + }); +}); + test.describe('puter.ui.requestPermission (env=web, cross-origin-isolated)', () => { test('a signed-out isolated site is answered promptly instead of waiting out the poll', async ({ page, context }) => { // A cross-origin-isolated opener can't be reached by postMessage, so the @@ -558,6 +724,43 @@ test.describe('request-permission action hardening', () => { await expect(page.locator('dialog.perm-dialog')).toHaveCount(0); }); + test('the grant names the requester by origin only, never by a browser-computed uid', async ({ page, context }) => { + // A uid computed in the browser is unsafe to forward even when it came + // from the server: an origin with no app row of its own resolves to a + // *synthetic* `app-`, and the grant endpoint resolves + // `app_uid` as uid-*or-name*. Forwarding it would hand the grant to + // whoever registered an app under that literal name — a name derived + // from a published namespace constant, so it can be squatted offline — + // while the dialog named the origin. Sending the origin alone is what + // makes the server resolve the same requester the user was shown, and + // reject it outright unless it names an app that really exists. + await page.goto('/'); + await page.waitForFunction(() => !!window.puter?.authToken, null, { timeout: 60_000 }); + await page.goto(PERMISSION_FIXTURE_URL); + await page.locator('body.ready').waitFor({ timeout: 60_000 }); + + const grantBodies = []; + await context.route('**/auth/grant-user-app', async (route) => { + grantBodies.push(route.request().postDataJSON()); + await route.continue(); + }); + + const [popup] = await Promise.all([ + page.waitForEvent('popup'), + page.locator('#req-driver-perm').click(), + ]); + const dialog = popup.locator('dialog.perm-dialog'); + await expect(dialog).toBeVisible({ timeout: 60_000 }); + await dialog.locator('.perm-dialog-allow').click(); + await expect(page.locator('#log [data-entry="perm:driver:true"]')).toBeVisible(); + + expect(grantBodies.length).toBeGreaterThan(0); + for ( const body of grantBodies ) { + expect(body.app_uid).toBeUndefined(); + expect(typeof body.origin).toBe('string'); + } + }); + test('`cross_origin_isolated` cannot turn a permission prompt into a token grant', async ({ page }) => { // That flag routes the user-app token to the opener via /login/set, // which the unauthenticated /login/wait then hands to whoever knows the