mirror of
https://github.com/HeyPuter/puter.git
synced 2026-08-25 15:37:12 +00:00
Take a permission popup's requester from the browser, not the link
`app_uid` was removed from the request-permission URL because the uid names who receives the grant, so it has to come from the requesting origin rather than from whoever built the link. Two other parameters still carried exactly that identity, and the origin is the identity twice over: it is the name the dialog attributes the request to, and it is what the server resolves into the app the grant is written against. `opener_origin` is believed on every popup boot, ahead of the referrer. `origin` was the fallback in the request-permission block itself, reached whenever there is no opener at all. Either one lets a bare link raise a consent prompt in some other app's name — the token exchange bootstraps an app row for whatever origin was typed, so the grant then commits against it — while the user is looking at a domain the requester does not control. That is the whole dialog defeated: a site can name `docs.google.com` and have the Allow land on Google's app row. So a permission popup now takes only an origin the browser vouches for: `document.referrer`, or the opener's own reply to the `requestOrigin` handshake. Neither can be forged to another origin. Nothing legitimate relied on the parameters — the SDK sends neither, and the OIDC redirect `opener_origin` exists for drops `action` too, so no permission flow can arrive through one. The `origin` fallback only ever fired when there was no opener, which is to say when there was no requester either; that path now reports its denial as usual instead of prompting. The referrer is safe here precisely because `action` is only `request-permission` on the popup's first load, so it is always the opener's. Restoring the action across an OIDC hop would break that — the returning navigation's referrer is the identity provider — which is why the round trip is withheld rather than repaired: A popup that signs in through OIDC comes back to a redirect URI the server hard-codes to `/action/sign-in`, so it returns believing it is a plain sign-in popup. It posts `puter.token`, which the SDK's global listener feeds straight into `setAuthToken()`, and never runs the action it was opened for. For a permission prompt that is the exact outcome `deliversTokenToOpener` exists to prevent: the site is signed in without ever asking, the user is never shown the permission they were brought there to decide, and the request resolves as a denial. Nothing in the returned URL says what the popup was for, so it cannot recover on its own. Until the redirect can carry the action back — and the opener origin can be re-established from the handshake on return — a popup whose purpose cannot survive the hop does not offer the hop. Email sign-in stays in the window and is unaffected. Pinned by e2e tests that spoof each parameter and expect the prompt to name the real opener, or not to appear at all. The long-hostname test drove the dialog through `origin=`, which no longer produces one, so it now asserts the CSS elision contract directly while the popup test asserts the real flow applies the `perm-dialog-entity-host` class that contract keys on.
This commit is contained in:
@@ -22,6 +22,7 @@ import UIWindow from './UIWindow.js';
|
||||
import UIWindowRecoverPassword from './UIWindowRecoverPassword.js';
|
||||
import UIWindowSignup from './UIWindowSignup.js';
|
||||
import { KNOWN_OIDC_PROVIDERS, OIDC_GENERIC_PROVIDER_ICON, humanizeOidcProviderId } from '../util/openid.js';
|
||||
import { offersFederatedSignInInPopup } from '../util/popupAuth.js';
|
||||
|
||||
// ── 2FA Login CSS (injected once) ───────────────────────────────────────────
|
||||
const LOGIN_2FA_CSS = `
|
||||
@@ -400,6 +401,14 @@ async function UIWindowLogin (options) {
|
||||
|
||||
(async () => {
|
||||
try {
|
||||
// A federated hop navigates this popup away and the provider
|
||||
// returns it as a plain sign-in popup, losing whatever the popup
|
||||
// was opened to do — and, for a permission prompt, handing the
|
||||
// opener a token instead of a decision. Don't offer it there.
|
||||
if ( window.embedded_in_popup
|
||||
&& ! offersFederatedSignInInPopup(window.gui_action) ) {
|
||||
return;
|
||||
}
|
||||
const res = await fetch(`${window.api_origin}/auth/oidc/providers`);
|
||||
if ( ! res.ok ) return;
|
||||
const data = await res.json();
|
||||
|
||||
@@ -24,6 +24,7 @@ import UIWindowPhoneVerificationRequired from './UIWindowPhoneVerificationRequir
|
||||
import UIWindowCardVerificationRequired from './UIWindowCardVerificationRequired.js';
|
||||
import UIWindowLogin from './UIWindowLogin.js';
|
||||
import { KNOWN_OIDC_PROVIDERS, OIDC_GENERIC_PROVIDER_ICON, humanizeOidcProviderId } from '../util/openid.js';
|
||||
import { offersFederatedSignInInPopup } from '../util/popupAuth.js';
|
||||
|
||||
function UIWindowSignup(options) {
|
||||
options = options ?? {};
|
||||
@@ -216,6 +217,17 @@ function UIWindowSignup(options) {
|
||||
|
||||
(async () => {
|
||||
try {
|
||||
// A federated hop navigates this popup away and the
|
||||
// provider returns it as a plain sign-in popup, losing
|
||||
// whatever the popup was opened to do — and, for a
|
||||
// permission prompt, handing the opener a token instead
|
||||
// of a decision. Don't offer it there.
|
||||
if (
|
||||
window.embedded_in_popup &&
|
||||
!offersFederatedSignInInPopup(window.gui_action)
|
||||
) {
|
||||
return;
|
||||
}
|
||||
const res = await fetch(
|
||||
`${window.api_origin}/auth/oidc/providers`,
|
||||
);
|
||||
|
||||
+25
-4
@@ -58,7 +58,10 @@ import { ThemeService } from './services/ThemeService.js';
|
||||
// factory name so a bare `privacy_aware_path(path)` call in this module can't
|
||||
// silently resolve to the factory — use `window.privacy_aware_path` instead.
|
||||
import { privacy_aware_path as privacy_aware_path_factory } from './util/desktop.js';
|
||||
import { deliversTokenToOpener } from './util/popupAuth.js';
|
||||
import {
|
||||
deliversTokenToOpener,
|
||||
trustsOpenerOriginParam,
|
||||
} from './util/popupAuth.js';
|
||||
|
||||
const postAuthActions = async (action) => {
|
||||
// Set when a popup's user-app token exchange fails. The exchange is what
|
||||
@@ -499,7 +502,15 @@ const postAuthActions = async (action) => {
|
||||
if ( action === 'request-permission' ) {
|
||||
const permission = window.url_query_params.get('permission');
|
||||
const msg_id = window.url_query_params.get('msg_id');
|
||||
const origin = window.openerOrigin ?? window.url_query_params.get('origin');
|
||||
// Browser-attested only: `openerOrigin` is the referrer, or the opener's
|
||||
// own reply to the `requestOrigin` handshake. There is deliberately no
|
||||
// query-string fallback — the origin is the requester's identity, naming
|
||||
// who the dialog attributes the request to and picking the app the grant
|
||||
// is written against, so a link must not get to state it. That is the
|
||||
// same rule that keeps `app_uid` out of this URL, and the SDK never sends
|
||||
// an origin either. Without one there is nothing to prompt about and the
|
||||
// denial below is reported as usual.
|
||||
const origin = window.openerOrigin;
|
||||
|
||||
// Whatever happens, the requester must get an answer and the popup
|
||||
// must close — otherwise the popup wedges open with the caller's
|
||||
@@ -967,6 +978,11 @@ window.initgui = async function (options) {
|
||||
} else if (window.url_query_params.has('action')) {
|
||||
action = window.url_query_params.get('action').toLowerCase();
|
||||
}
|
||||
// Published for the windows that open mid-flow and have to know what this
|
||||
// page is for — the login/signup windows consult it before offering a
|
||||
// federated sign-in hop that would navigate the popup away and lose the
|
||||
// action. See util/popupAuth.js.
|
||||
window.gui_action = action;
|
||||
|
||||
//--------------------------------------------------------------------------------------
|
||||
// Determine if we are in full-page mode
|
||||
@@ -1058,8 +1074,13 @@ window.initgui = async function (options) {
|
||||
$('body').addClass('embedded-in-popup');
|
||||
|
||||
// determine the origin of the opener (preserved across OIDC redirect via URL param, else referrer or messaging)
|
||||
const openerOriginFromUrl =
|
||||
window.url_query_params.get('opener_origin');
|
||||
// A permission prompt is the exception: there the opener's origin names
|
||||
// the requester on the dialog and picks the app the grant is written to,
|
||||
// so it may only come from a source the browser vouches for. See
|
||||
// util/popupAuth.js.
|
||||
const openerOriginFromUrl = trustsOpenerOriginParam(action)
|
||||
? window.url_query_params.get('opener_origin')
|
||||
: null;
|
||||
if (openerOriginFromUrl) {
|
||||
window.openerOrigin = openerOriginFromUrl;
|
||||
} else {
|
||||
|
||||
@@ -50,3 +50,64 @@ const NON_AUTH_POPUP_ACTIONS = new Set(['request-permission']);
|
||||
*/
|
||||
export const deliversTokenToOpener = (action) =>
|
||||
!NON_AUTH_POPUP_ACTIONS.has(action);
|
||||
|
||||
/**
|
||||
* Popup actions where the opener's origin *is* the requester's identity, rather
|
||||
* than just the address an answer is sent back to.
|
||||
*/
|
||||
const OPENER_IS_THE_REQUESTER_ACTIONS = new Set(['request-permission']);
|
||||
|
||||
/**
|
||||
* Whether a popup running `action` may take its opener's origin from the
|
||||
* `opener_origin` URL parameter.
|
||||
*
|
||||
* That parameter exists so a sign-in popup can carry the opener's origin across
|
||||
* an OIDC redirect, which drops the rest of the query. It is chosen by whoever
|
||||
* built the link, though, and for a permission prompt the opener's origin is the
|
||||
* requester's identity twice over: it is the name the dialog attributes the
|
||||
* request to, and it is what the server resolves into the app the grant is
|
||||
* written against. Honouring a link-supplied one would let any site prompt in
|
||||
* another app's name and commit the user's grant to it — the same hole that
|
||||
* `app_uid` was removed from this URL to close.
|
||||
*
|
||||
* So a permission popup takes only a browser-attested origin: `document.referrer`
|
||||
* or the opener's own reply to the `requestOrigin` handshake. Nothing is lost —
|
||||
* the SDK never sends this parameter, and the OIDC redirect it exists for drops
|
||||
* `action` too, so no permission flow can reach here through one.
|
||||
*
|
||||
* @param {string|null|undefined} action - The popup's `action`, as parsed from
|
||||
* the URL (`/action/<name>` or `?action=<name>`); undefined for a plain
|
||||
* sign-in popup.
|
||||
* @returns {boolean} `true` if `opener_origin` may be believed.
|
||||
*/
|
||||
export const trustsOpenerOriginParam = (action) =>
|
||||
!OPENER_IS_THE_REQUESTER_ACTIONS.has(action);
|
||||
|
||||
/**
|
||||
* Whether a popup running `action` may offer federated (OIDC) sign-in.
|
||||
*
|
||||
* An OIDC hop navigates the popup away and the provider sends it back to a
|
||||
* redirect URI the server builds, which is hard-coded to `/action/sign-in`
|
||||
* (OIDCController's popup branch). The popup therefore comes back believing it
|
||||
* is a plain sign-in popup: it posts `puter.token` to the opener — which the
|
||||
* SDK's global listener feeds straight into `setAuthToken()` — and never runs
|
||||
* the action it was opened for. For a permission prompt that is the exact
|
||||
* outcome `deliversTokenToOpener` exists to prevent: the site is signed in
|
||||
* without asking, and the user is never shown the permission they were meant to
|
||||
* decide on (the request resolves as a denial).
|
||||
*
|
||||
* Nothing in the returned URL says what the popup was originally for, so the
|
||||
* popup cannot re-establish it. Restoring the action through the redirect is
|
||||
* also not enough on its own: the returning navigation's referrer is the
|
||||
* provider, not the opener, so `trustsOpenerOriginParam`'s browser-attested
|
||||
* origin would have to come from the `requestOrigin` handshake instead. Until
|
||||
* that exists, a popup whose purpose cannot survive the round trip does not
|
||||
* offer the round trip. Email sign-in stays in the window and works normally.
|
||||
*
|
||||
* @param {string|null|undefined} action - The popup's `action`, as parsed from
|
||||
* the URL (`/action/<name>` or `?action=<name>`); undefined for a plain
|
||||
* sign-in popup.
|
||||
* @returns {boolean} `true` if OIDC buttons may be shown in this popup.
|
||||
*/
|
||||
export const offersFederatedSignInInPopup = (action) =>
|
||||
!NON_AUTH_POPUP_ACTIONS.has(action);
|
||||
|
||||
@@ -18,7 +18,11 @@
|
||||
*/
|
||||
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { deliversTokenToOpener } from './popupAuth.js';
|
||||
import {
|
||||
deliversTokenToOpener,
|
||||
offersFederatedSignInInPopup,
|
||||
trustsOpenerOriginParam,
|
||||
} from './popupAuth.js';
|
||||
|
||||
describe('deliversTokenToOpener', () => {
|
||||
it('withholds the token from a permission prompt', () => {
|
||||
@@ -47,3 +51,45 @@ describe('deliversTokenToOpener', () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('offersFederatedSignInInPopup', () => {
|
||||
it('withholds the OIDC hop from a permission prompt', () => {
|
||||
// The provider returns the popup to a hard-coded `/action/sign-in`, so it
|
||||
// comes back believing it is a sign-in popup: it hands the opener a token
|
||||
// — the very thing `deliversTokenToOpener` refuses — and never shows the
|
||||
// permission the user was there to decide on.
|
||||
expect(offersFederatedSignInInPopup('request-permission')).toBe(false);
|
||||
});
|
||||
|
||||
it('offers it in the popups that exist to sign the user in', () => {
|
||||
for ( const action of [undefined, 'sign-in', 'login', 'signup'] ) {
|
||||
expect(offersFederatedSignInInPopup(action)).toBe(true);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('trustsOpenerOriginParam', () => {
|
||||
it('disbelieves a link-supplied origin on a permission prompt', () => {
|
||||
// The opener's origin is the requester's identity there: it is the name
|
||||
// the dialog shows and the app the server writes the grant to. Taking it
|
||||
// from the link would let any site prompt in another app's name — the
|
||||
// same hole `app_uid` was removed from this URL to close.
|
||||
expect(trustsOpenerOriginParam('request-permission')).toBe(false);
|
||||
});
|
||||
|
||||
it('believes it for the flows the parameter exists to carry', () => {
|
||||
// It survives an OIDC redirect, which drops the rest of the query.
|
||||
// `undefined` is a plain sign-in popup, which carries no action.
|
||||
for ( const action of [
|
||||
undefined,
|
||||
'sign-in',
|
||||
'login',
|
||||
'signup',
|
||||
'show-open-file-picker',
|
||||
'show-directory-picker',
|
||||
'show-save-file-picker',
|
||||
] ) {
|
||||
expect(trustsOpenerOriginParam(action)).toBe(true);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -439,6 +439,11 @@ test.describe('puter.ui.requestPermission (env=web popup)', () => {
|
||||
await expect(dialog).toBeVisible({ timeout: 60_000 });
|
||||
// Sites are identified by their origin host.
|
||||
await expect(dialog.locator('.perm-dialog-entity-name')).toContainText('localhost');
|
||||
// And marked as a host, which is what buys the left-elision asserted on
|
||||
// further down. Without this class a long host truncates on the wrong
|
||||
// end, so the real flow has to be the thing that applies it.
|
||||
await expect(dialog.locator('.perm-dialog-entity-name'))
|
||||
.toHaveClass(/perm-dialog-entity-host/);
|
||||
await dialog.locator('.perm-dialog-deny').click();
|
||||
await expect(page.locator('#log [data-entry="perm:driver:false"]')).toBeVisible();
|
||||
|
||||
@@ -1132,16 +1137,80 @@ test.describe('request-permission action hardening', () => {
|
||||
expect(await page.evaluate(() => window.__leaked)).toBe('waiting');
|
||||
});
|
||||
|
||||
test('`opener_origin` cannot rename the requester on the prompt', async ({ page }) => {
|
||||
// That parameter exists so a sign-in popup can carry its opener's origin
|
||||
// across an OIDC redirect. On a permission prompt the opener's origin is
|
||||
// the requester's identity, so honouring a link-supplied one would let any
|
||||
// site raise a prompt in another app's name — and land the grant on that
|
||||
// app. The popup must name the origin the browser attests to instead.
|
||||
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 spoofed = 'https://not-the-requester.example';
|
||||
const [popup] = await Promise.all([
|
||||
page.waitForEvent('popup'),
|
||||
page.evaluate((o) => {
|
||||
window.open(
|
||||
`${puter.defaultGUIOrigin}/action/request-permission?embedded_in_popup=true`
|
||||
+ `&opener_origin=${encodeURIComponent(o)}`
|
||||
+ '&permission=driver%3Aputer-image-generation%3Agenerate&msg_id=88',
|
||||
'perm-opener-origin-probe',
|
||||
'width=600,height=700',
|
||||
);
|
||||
}, spoofed),
|
||||
]);
|
||||
|
||||
const name = popup.locator('dialog.perm-dialog .perm-dialog-entity-name');
|
||||
await expect(name).toBeVisible({ timeout: 60_000 });
|
||||
await expect(name).toContainText('localhost');
|
||||
await expect(name).not.toContainText('not-the-requester.example');
|
||||
});
|
||||
|
||||
test('a URL-supplied origin never produces a prompt either', async ({ page }) => {
|
||||
// Same rule as the `app_uid` case above, for the other identifier a link
|
||||
// can carry. The origin is the requester's identity — the name the dialog
|
||||
// shows *and* what the server resolves into the app the grant is written
|
||||
// against — so it may only come from a source the browser vouches for
|
||||
// (the referrer, or the opener's reply to the `requestOrigin`
|
||||
// handshake). Believing the query string would let a bare link raise a
|
||||
// consent prompt in any app's name and commit the user's grant to it.
|
||||
await page.goto(
|
||||
'/action/request-permission?permission=driver%3Aputer-image-generation%3Agenerate' +
|
||||
`&origin=${encodeURIComponent('https://not-the-requester.example/')}`,
|
||||
);
|
||||
await page.waitForFunction(() => !!window.puter?.authToken, null, { timeout: 60_000 });
|
||||
await page.locator('.desktop').waitFor({ timeout: 60_000 });
|
||||
await expect(page.locator('dialog.perm-dialog')).toHaveCount(0);
|
||||
});
|
||||
|
||||
test('a long hostname keeps its registrable domain visible', async ({ page }) => {
|
||||
// The identity line is the only thing naming the requester, so it must
|
||||
// not elide the end of the host: `accounts.google.com.attacker.example`
|
||||
// truncated on the right reads as `accounts.google.com…`.
|
||||
//
|
||||
// Driven against the dialog's own markup rather than through a request,
|
||||
// because there is no longer any way to hand the flow an arbitrary host:
|
||||
// the origin has to be browser-attested, and the fixture's opener is
|
||||
// whatever host the test server runs on. What is asserted here is the CSS
|
||||
// contract that produces the elision; that the real flow marks a site's
|
||||
// identity line with `perm-dialog-entity-host` — the class the contract
|
||||
// keys on — is asserted in the popup test above.
|
||||
const host = 'accounts.google.com.attacker-run-domain.example';
|
||||
await page.goto(
|
||||
'/action/request-permission?permission=driver%3Aputer-image-generation%3Agenerate' +
|
||||
`&origin=${encodeURIComponent(`https://${host}/`)}`,
|
||||
);
|
||||
await page.goto('/');
|
||||
await page.waitForFunction(() => !!window.puter?.authToken, null, { timeout: 60_000 });
|
||||
await page.evaluate((h) => {
|
||||
const el = document.createElement('dialog');
|
||||
el.className = 'perm-dialog';
|
||||
el.innerHTML = '<div class="perm-dialog-body">'
|
||||
+ '<div class="perm-dialog-identity">'
|
||||
+ '<h1 class="perm-dialog-entity-name perm-dialog-entity-host"></h1>'
|
||||
+ '</div></div>';
|
||||
el.querySelector('h1').textContent = h;
|
||||
document.body.appendChild(el);
|
||||
el.showModal();
|
||||
}, host);
|
||||
const name = page.locator('dialog.perm-dialog .perm-dialog-entity-name');
|
||||
await expect(name).toBeVisible({ timeout: 60_000 });
|
||||
|
||||
|
||||
Reference in New Issue
Block a user