Close three gaps left in the permission-request flow

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-<uuidv5(origin)>` 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.
This commit is contained in:
jelveh
2026-07-26 10:24:32 -07:00
parent aa7f766f53
commit 727726437a
6 changed files with 378 additions and 31 deletions
@@ -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-<uuidv5(origin)>` 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
@@ -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<string> {
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) {
+9
View File
@@ -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.
+29 -24
View File
@@ -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-<uuidv5(origin)>`. 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
+35 -5
View File
@@ -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);
};
@@ -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-<uuidv5(origin)>`, 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