diff --git a/src/backend/controllers/oidc/OIDCController.test.ts b/src/backend/controllers/oidc/OIDCController.test.ts index 2f2a25cfc..390e20737 100644 --- a/src/backend/controllers/oidc/OIDCController.test.ts +++ b/src/backend/controllers/oidc/OIDCController.test.ts @@ -401,6 +401,97 @@ describe('OIDCController GET /auth/oidc/:provider/start', () => { } }); + // A share email lands on `/?shared=…` and its recipient often has to sign + // in first, so the link has to survive the round trip to the provider. + describe('share links in return_to', () => { + const SHARE_UUID = '11111111-2222-3333-4444-555555555555'; + const sharedPath = (name: string) => `/alice/${SHARE_UUID}/${name}`; + + const redirectUriFor = async (return_to: string) => { + const { res, captured } = makeRes(); + await callRoute( + 'get', + '/auth/oidc/:provider/start', + makeReq({ + params: { provider: 'custom' }, + query: { return_to }, + }), + res, + ); + const state = new URL(captured.redirectUrl ?? '').searchParams.get( + 'state', + ); + return String(oidc().verifyState(state!)?.redirect_uri); + }; + + const returnToFor = (paths: string[], path = '/') => { + const params = new URLSearchParams(); + for (const p of paths) params.append('shared', p); + return `${path}?${params.toString()}`; + }; + + it('carries a share link back to the root', async () => { + const uri = await redirectUriFor( + returnToFor([sharedPath('Report.pdf')]), + ); + const url = new URL(uri); + expect(url.origin + url.pathname).toBe(`${TEST_ORIGIN}/`); + expect(url.searchParams.getAll('shared')).toEqual([ + sharedPath('Report.pdf'), + ]); + }); + + it('carries every item, deduplicated, on any whitelisted page', async () => { + const uri = await redirectUriFor( + returnToFor( + [ + sharedPath('a.txt'), + sharedPath('b.txt'), + sharedPath('a.txt'), + ], + '/desktop', + ), + ); + const url = new URL(uri); + expect(url.origin + url.pathname).toBe(`${TEST_ORIGIN}/desktop`); + expect(url.searchParams.getAll('shared')).toEqual([ + sharedPath('a.txt'), + sharedPath('b.txt'), + ]); + }); + + it('carries no more items than a share link may name', async () => { + const paths = Array.from({ length: 25 }, (_, i) => + sharedPath(`file-${i}.txt`), + ); + const uri = await redirectUriFor(returnToFor(paths)); + expect(new URL(uri).searchParams.getAll('shared')).toEqual( + paths.slice(0, 20), + ); + }); + + it('refuses a query it does not fully recognize', async () => { + const bad_values = [ + // not a masked share path: no uuid, no item after it, or a + // hand-edited absolute path + returnToFor(['/alice/Documents/Report.pdf']), + returnToFor([`/alice/${SHARE_UUID}`]), + returnToFor([`/alice/${SHARE_UUID}/`]), + returnToFor(['']), + // a parameter that isn't `shared`, alone or alongside one + '/?x=1', + `${returnToFor([sharedPath('a.txt')])}&x=1`, + // the root is only a destination when it names something + '/', + // still no origin smuggling, share link or not + `//evil.test${returnToFor([sharedPath('a.txt')])}`, + ]; + for (const return_to of bad_values) { + expect(await redirectUriFor(return_to)).toBe(TEST_ORIGIN); + } + }); + }); + it('signs revalidate-flow state with user_uuid + flow=revalidate', async () => { const userUuid = uuidv4(); const { res, captured } = makeRes(); @@ -693,6 +784,77 @@ describe('OIDCController login callback', () => { expect(captured.cookies).toHaveLength(0); }); + it('redirects back to a share link after sign-in', async () => { + const shared = '/alice/11111111-2222-3333-4444-555555555555/Report.pdf'; + const state = oidc().signState({ + provider: 'custom', + redirect_uri: `${TEST_ORIGIN}/?shared=${encodeURIComponent(shared)}`, + }); + const sub = `sub-${Math.random().toString(36).slice(2, 8)}`; + const email = `share-${Math.random().toString(36).slice(2, 8)}@test.local`; + vi.spyOn(oidc(), 'exchangeCodeForTokens').mockResolvedValue({ + access_token: 'access', + id_token: 'id', + } as never); + vi.spyOn(oidc(), 'getUserInfo').mockResolvedValue({ + sub, + email, + email_verified: true, + } as never); + + const { res, captured } = makeRes(); + await callRoute( + 'get', + '/auth/oidc/callback/login', + makeReq({ query: { code: 'c', state } }), + res, + ); + expect(captured.redirectStatus).toBe(302); + const url = new URL(captured.redirectUrl ?? ''); + expect(url.origin + url.pathname).toBe(`${TEST_ORIGIN}/`); + expect(url.searchParams.getAll('shared')).toEqual([shared]); + }); + + it('keeps a share link on the error page so a retry still lands on it', async () => { + const shared = '/alice/11111111-2222-3333-4444-555555555555/Report.pdf'; + const sub = `sub-${Math.random().toString(36).slice(2, 8)}`; + const email = `sus-${Math.random().toString(36).slice(2, 8)}@test.local`; + const created = await runWithContext({ req: makeReq({}) }, () => + oidc().createUserFromOIDC('custom', { + sub, + email, + email_verified: true, + }), + ); + await server.stores.user.update(created.user!.id, { suspended: 1 }); + + const state = oidc().signState({ + provider: 'custom', + redirect_uri: `${TEST_ORIGIN}/?shared=${encodeURIComponent(shared)}`, + }); + vi.spyOn(oidc(), 'exchangeCodeForTokens').mockResolvedValue({ + access_token: 'access', + id_token: 'id', + } as never); + vi.spyOn(oidc(), 'getUserInfo').mockResolvedValue({ + sub, + email, + email_verified: true, + } as never); + + const { res, captured } = makeRes(); + await callRoute( + 'get', + '/auth/oidc/callback/login', + makeReq({ query: { code: 'c', state } }), + res, + ); + const url = new URL(captured.redirectUrl ?? ''); + expect(url.searchParams.get('auth_error')).toBe('1'); + expect(url.searchParams.get('action')).toBe('login'); + expect(url.searchParams.getAll('shared')).toEqual([shared]); + }); + it('redirects back to an /app/ landing after sign-in', async () => { const state = oidc().signState({ provider: 'custom', diff --git a/src/backend/controllers/oidc/OIDCController.ts b/src/backend/controllers/oidc/OIDCController.ts index cce1700fa..4162f53ef 100644 --- a/src/backend/controllers/oidc/OIDCController.ts +++ b/src/backend/controllers/oidc/OIDCController.ts @@ -23,6 +23,11 @@ import { HttpError } from '../../core/http/HttpError.js'; import type { PuterRouter } from '../../core/http/PuterRouter.js'; import { PuterController } from '../types.js'; import { sessionCookieFlags } from '../../util/cookieFlags.js'; +import { parseMaskedSharePath } from '../../services/fs/sharePathMask.js'; +import { + SHARE_DEEP_LINK_ITEMS_LIMIT, + SHARE_DEEP_LINK_PARAM, +} from '../../services/share/shareDeepLink.js'; const REVALIDATION_COOKIE_NAME = 'puter_revalidation'; const REVALIDATION_EXPIRY_SEC = 300; @@ -64,18 +69,84 @@ function resolutionErrorCode(code: string | undefined): string { : 'signup_blocked'; } -// GUI pages an OIDC flow may return to: /desktop, /dashboard, and direct app -// landings (/app/ and its desktop-booted twin /desktop/app/, -// mirroring APP_NAME_REGEX in AppDriver). Strict whitelist — never a -// client-supplied URL (no open redirect). +// GUI pages an OIDC flow may return to: the root (where a share email lands), +// /desktop, /dashboard, and direct app landings (/app/ and its +// desktop-booted twin /desktop/app/, mirroring APP_NAME_REGEX in +// AppDriver). Strict whitelist — never a client-supplied URL (no open +// redirect). function isWhitelistedReturnPath(path: string): boolean { return ( + path === '/' || path === '/desktop' || path === '/dashboard' || /^(\/desktop)?\/app\/[a-zA-Z0-9_-]{1,100}$/.test(path) ); } +/** + * The share items a return target's query names, or null when the query is + * anything else at all. + * + * A share email lands on `/?shared=…`, and its recipient usually has to sign in + * before they can see what was shared. An OIDC flow leaves the origin and comes + * back to a URL this server builds, so the parameter travels through the flow + * or the recipient returns to a bare Home with nothing to say what they were + * sent. + * + * `shared` is the only parameter that makes the trip, and only values shaped + * like the masked path the mail was built from — the value is user-visible + * text, so a hand-edited one is refused rather than reflected back into the + * browser. + */ +function sharedPathsFromReturnQuery(query: string): string[] | null { + const paths: string[] = []; + for (const [key, value] of new URLSearchParams(query)) { + if (key !== SHARE_DEEP_LINK_PARAM) return null; + const parsed = parseMaskedSharePath(value); + // The segment after the uuid is the shared item itself. A mask without + // one addresses the owner's parent directory, which is not the + // recipient's to open. + if (!parsed || !parsed.tail) return null; + // Same rule the link builder follows: the first items are the ones + // that travel, so what gets highlighted reads as the top of the list. + if ( + paths.length < SHARE_DEEP_LINK_ITEMS_LIMIT && + !paths.includes(value) + ) { + paths.push(value); + } + } + return paths; +} + +/** + * A client-supplied `return_to` reduced to what will actually be redirected to, + * or null when it isn't a page an OIDC flow returns to. + * + * The path is matched as given, never parsed as a URL: a protocol-relative + * value (`//evil.test/desktop`) has to fail the whitelist rather than smuggle + * an origin through as a `pathname`. The query is rebuilt from the values that + * survived, so nothing reaches the redirect verbatim. + */ +function sanitizeReturnTo(raw: string): string | null { + const separator = raw.indexOf('?'); + const path = separator === -1 ? raw : raw.slice(0, separator); + if (!isWhitelistedReturnPath(path)) return null; + + const shared = + separator === -1 + ? [] + : sharedPathsFromReturnQuery(raw.slice(separator + 1)); + if (shared === null) return null; + // The root is only a destination when it names something: on its own it is + // where the flow already lands. + if (shared.length === 0) return path === '/' ? null : path; + + const params = new URLSearchParams(); + for (const value of shared) params.append(SHARE_DEEP_LINK_PARAM, value); + return `${path}?${params.toString()}`; +} + function buildErrorRedirectUrl( origin: string, sourceFlow: string, @@ -100,11 +171,17 @@ function buildErrorRedirectUrl( // /app/ landing) so the retry — and the eventual success — keeps // the user's destination. redirect_uri comes from the signed state and // was built server-side, but re-check the path against the whitelist. + // A share link's items come back too: the retry happens on that page, and + // its success reloads it, so they have to be on it to survive. let pagePath = '/'; + let sharedPaths: string[] = []; if (typeof stateDecoded?.redirect_uri === 'string') { try { - const statePath = new URL(stateDecoded.redirect_uri).pathname; - if (isWhitelistedReturnPath(statePath)) pagePath = statePath; + const stateUrl = new URL(stateDecoded.redirect_uri); + if (isWhitelistedReturnPath(stateUrl.pathname)) { + pagePath = stateUrl.pathname; + sharedPaths = sharedPathsFromReturnQuery(stateUrl.search) ?? []; + } } catch { // unparsable redirect_uri: fall back to the root page } @@ -145,6 +222,9 @@ function buildErrorRedirectUrl( if (requestCode) { params.set('request_code', requestCode); } + for (const path of sharedPaths) { + params.append(SHARE_DEEP_LINK_PARAM, path); + } return `${base}${pagePath}?${params.toString()}`; } @@ -289,16 +369,17 @@ export class OIDCController extends PuterController { let appRedirectUri = flowRedirects[flow] ?? (origin || '/'); // Optional GUI return path so login started from /desktop, - // /dashboard, or an /app/ landing lands back there. + // /dashboard, an /app/ landing, or a share link lands + // back there. const rawReturnTo = Array.isArray(req.query.return_to) ? req.query.return_to[0] : req.query.return_to; - if ( - (flow === 'login' || flow === 'signup') && - typeof rawReturnTo === 'string' && - isWhitelistedReturnPath(rawReturnTo) - ) { - appRedirectUri = `${origin}${rawReturnTo}`; + const returnTo = + typeof rawReturnTo === 'string' + ? sanitizeReturnTo(rawReturnTo) + : null; + if ((flow === 'login' || flow === 'signup') && returnTo) { + appRedirectUri = `${origin}${returnTo}`; } // Popup support diff --git a/src/gui/src/helpers/auth_redirect.js b/src/gui/src/helpers/auth_redirect.js index af13730ef..44c99240b 100644 --- a/src/gui/src/helpers/auth_redirect.js +++ b/src/gui/src/helpers/auth_redirect.js @@ -17,6 +17,8 @@ * along with this program. If not, see . */ +import parse_shared_path, { SHARED_PATH_PARAM } from './parse_shared_path.js'; + /** * Where to send the user after a successful login/signup started from the * current page. Keeps the user on the page they authenticated from — most @@ -49,18 +51,15 @@ export const get_auth_redirect_url = () => { }; /** - * The `return_to` path to send along when starting an OIDC flow, or null if - * the current page isn't one the backend will return to. The backend strictly - * whitelists these (never a client-supplied URL): `/desktop`, `/dashboard`, - * and direct app landings (`/app/`, plus the desktop-booted - * `/desktop/app/`), so OIDC login started from an app landing comes back - * to the app — and to the same interface it was opened in. + * The pathname part of an OIDC `return_to`, or null when the current page isn't + * one the backend will return to. The root is in here only for the share links + * below — on its own it is where the flow already lands. * - * @returns {string|null} whitelistable pathname, or null + * @returns {string|null} */ -export const get_oidc_return_to = () => { +const oidc_return_path = () => { const pathname = window.location.pathname; - if ( pathname === '/desktop' || pathname === '/dashboard' ) { + if ( pathname === '/' || pathname === '/desktop' || pathname === '/dashboard' ) { return pathname; } // app landing: normalize away a trailing slash to match the backend whitelist @@ -69,3 +68,36 @@ export const get_oidc_return_to = () => { } return null; }; + +/** + * The `return_to` to send along when starting an OIDC flow, or null if the + * current page isn't one the backend will return to. The backend strictly + * whitelists these (never a client-supplied URL): `/desktop`, `/dashboard`, + * and direct app landings (`/app/`, plus the desktop-booted + * `/desktop/app/`), so OIDC login started from an app landing comes back + * to the app — and to the same interface it was opened in. + * + * A share link (`?shared=`, from an email) is carried along with the path: the + * recipient usually has to sign in before they can see what was shared, and an + * OIDC round trip leaves the origin, so the parameter has to travel through the + * flow or they come back to a bare Home. Only well-formed values go — the + * backend refuses the rest, and a hand-edited link is no one's destination. + * + * @returns {string|null} whitelistable path, with its share items, or null + */ +export const get_oidc_return_to = () => { + const path = oidc_return_path(); + if ( path === null ) return null; + + const shared = new URLSearchParams(window.location.search ?? '') + .getAll(SHARED_PATH_PARAM) + .filter(value => parse_shared_path(value) !== null); + if ( shared.length === 0 ) { + // The root is only a destination when it names something. + return path === '/' ? null : path; + } + + const params = new URLSearchParams(); + for ( const value of shared ) params.append(SHARED_PATH_PARAM, value); + return `${path}?${params.toString()}`; +}; diff --git a/src/gui/src/helpers/auth_redirect.test.js b/src/gui/src/helpers/auth_redirect.test.js index 691d8db61..fa2d95f54 100644 --- a/src/gui/src/helpers/auth_redirect.test.js +++ b/src/gui/src/helpers/auth_redirect.test.js @@ -20,11 +20,21 @@ import { describe, it, expect, afterEach } from 'vitest'; import { get_oidc_return_to } from './auth_redirect.js'; -const at = (pathname) => { - globalThis.window = { location: { pathname } }; +const at = (pathname, search = '') => { + globalThis.window = { location: { pathname, search } }; return get_oidc_return_to(); }; +const SHARE_UUID = '11111111-2222-3333-4444-555555555555'; +const shared_path = (name) => `/alice/${SHARE_UUID}/${name}`; + +/** `window.location.search` for a page opened by a share link. */ +const share_search = (...paths) => { + const params = new URLSearchParams(); + for ( const path of paths ) params.append('shared', path); + return `?${params.toString()}`; +}; + afterEach(() => { delete globalThis.window; }); @@ -45,6 +55,29 @@ describe('get_oidc_return_to', () => { expect(at('/desktop/app/editor/')).toBe('/desktop/app/editor'); }); + it('carries a share link so the item survives the round trip', () => { + expect(at('/', share_search(shared_path('Report.pdf')))).toBe( + `/${share_search(shared_path('Report.pdf'))}`, + ); + expect(at('/desktop', share_search(shared_path('Report.pdf')))).toBe( + `/desktop${share_search(shared_path('Report.pdf'))}`, + ); + expect( + at('/', share_search(shared_path('a.txt'), shared_path('b.txt'))), + ).toBe(`/${share_search(shared_path('a.txt'), shared_path('b.txt'))}`); + }); + + it('leaves behind everything that is not a share link', () => { + // a hand-edited value the backend would refuse anyway + expect(at('/', share_search('/alice/Documents/Report.pdf'))).toBe(null); + expect(at('/', '?shared=')).toBe(null); + // other parameters are not ours to carry + expect(at('/desktop', '?app=editor')).toBe('/desktop'); + expect( + at('/desktop', `${share_search(shared_path('a.txt'))}&app=editor`), + ).toBe(`/desktop${share_search(shared_path('a.txt'))}`); + }); + it('returns null for anything the backend would reject', () => { expect(at('/')).toBe(null); expect(at('/settings')).toBe(null);