diff --git a/src/gui/src/UI/UIWindowLogin.js b/src/gui/src/UI/UIWindowLogin.js index 15295aabe..43668edd5 100644 --- a/src/gui/src/UI/UIWindowLogin.js +++ b/src/gui/src/UI/UIWindowLogin.js @@ -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(); diff --git a/src/gui/src/UI/UIWindowSignup.js b/src/gui/src/UI/UIWindowSignup.js index 2e65d6e03..189ea0308 100644 --- a/src/gui/src/UI/UIWindowSignup.js +++ b/src/gui/src/UI/UIWindowSignup.js @@ -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`, ); diff --git a/src/gui/src/initgui.js b/src/gui/src/initgui.js index 582c8dc80..882d8f166 100644 --- a/src/gui/src/initgui.js +++ b/src/gui/src/initgui.js @@ -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 { diff --git a/src/gui/src/util/popupAuth.js b/src/gui/src/util/popupAuth.js index 327bcfbd0..26aa81897 100644 --- a/src/gui/src/util/popupAuth.js +++ b/src/gui/src/util/popupAuth.js @@ -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/` or `?action=`); 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/` or `?action=`); 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); diff --git a/src/gui/src/util/popupAuth.test.js b/src/gui/src/util/popupAuth.test.js index 93db460ce..b3c4d911b 100644 --- a/src/gui/src/util/popupAuth.test.js +++ b/src/gui/src/util/popupAuth.test.js @@ -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); + } + }); +}); diff --git a/src/puter-js/tests/e2e/specs/requestPermission.spec.js b/src/puter-js/tests/e2e/specs/requestPermission.spec.js index 8f182ecf2..766079818 100644 --- a/src/puter-js/tests/e2e/specs/requestPermission.spec.js +++ b/src/puter-js/tests/e2e/specs/requestPermission.spec.js @@ -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 = '
' + + '
' + + '

' + + '
'; + 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 });