From b6af13284598e2f0a977846267d64cfc23ba7022 Mon Sep 17 00:00:00 2001 From: Tink Date: Fri, 19 Jun 2026 19:50:47 +0200 Subject: [PATCH] fix(auth): preserve desktop authorize URL when not signed in (#2944) --- frontend/src/constants/redirectHash.ts | 12 +++ frontend/src/router/index.ts | 58 +++++++++++-- .../tests/e2e/user/oauth-authorize.spec.ts | 82 ++++++++++++++++++- 3 files changed, 143 insertions(+), 9 deletions(-) create mode 100644 frontend/src/constants/redirectHash.ts diff --git a/frontend/src/constants/redirectHash.ts b/frontend/src/constants/redirectHash.ts new file mode 100644 index 000000000..1dd648e58 --- /dev/null +++ b/frontend/src/constants/redirectHash.ts @@ -0,0 +1,12 @@ +/** + * Hash-fragment prefix used to carry a post-login destination in the URL. + * + * Unlike the localStorage redirect, this lives in the address bar so the URL + * stays copyable between browsers (needed for native OAuth clients that open + * /oauth/authorize, see #2654). It uses the hash – not a query param – so the + * embedded OAuth parameters never reach server or proxy access logs. + * + * Must stay distinct from LINK_SHARE_HASH_PREFIX, which router.beforeEach + * special-cases. + */ +export const REDIRECT_HASH_PREFIX = '#redirect=' diff --git a/frontend/src/router/index.ts b/frontend/src/router/index.ts index cc693cc48..02e5ad580 100644 --- a/frontend/src/router/index.ts +++ b/frontend/src/router/index.ts @@ -6,6 +6,7 @@ import {getProjectViewId} from '@/helpers/projectView' import {parseDateOrString} from '@/helpers/time/parseDateOrString' import {getNextWeekDate} from '@/helpers/time/getNextWeekDate' import {LINK_SHARE_HASH_PREFIX} from '@/constants/linkShareHash' +import {REDIRECT_HASH_PREFIX} from '@/constants/redirectHash' import {AUTH_ROUTE_NAMES} from '@/constants/authRouteNames' import {PRO_FEATURE} from '@/constants/proFeatures' @@ -30,7 +31,7 @@ const router = createRouter({ } // Scroll to anchor should still work - if (to.hash && !to.hash.startsWith(LINK_SHARE_HASH_PREFIX)) { + if (to.hash && !to.hash.startsWith(LINK_SHARE_HASH_PREFIX) && !to.hash.startsWith(REDIRECT_HASH_PREFIX)) { return {el: to.hash} } @@ -472,10 +473,22 @@ const router = createRouter({ }) export async function getAuthForRoute(to: RouteLocation, authStore) { + // vue-router already decoded to.hash once, so slicing off the prefix yields the original + // fullPath (e.g. /oauth/authorize?...) losslessly — no extra decodeURIComponent needed. + const redirectDest = to.name === 'user.login' && to.hash.startsWith(REDIRECT_HASH_PREFIX) + ? to.hash.slice(REDIRECT_HASH_PREFIX.length) + : '' + if (authStore.authUser || authStore.authLinkShare) { + // An already-signed-in browser that opens a copied /login#redirect= URL + // must run the OAuth flow with its existing session instead of short-circuiting to home. + // The destination has no redirect hash, so the second guard pass just early-returns (#2654). + if (redirectDest) { + return redirectDest + } return } - + // Check if password reset token is in query params const resetToken = to.query.userPasswordReset as string | undefined @@ -499,15 +512,35 @@ export async function getAuthForRoute(to: RouteLocation, authStore) { } } + // Keep the destination in the address bar (not just per-browser localStorage) so a native + // client's /oauth/authorize URL stays copyable into another browser. Hash, not query, so the + // embedded OAuth params never reach access logs (#2654). Pass fullPath raw: vue-router encodes + // the hash itself, so an extra encodeURIComponent here would be double-encoded in the URL. + if (to.name === 'oauth.authorize') { + return { + name: 'user.login', + hash: REDIRECT_HASH_PREFIX + to.fullPath, + } + } + + // Fold the hash destination into localStorage: it's the only bridge that survives the + // external OIDC round-trip out of the SPA, so redirectIfSaved() works after any auth method. + // vue-router already decoded to.hash once, so it equals the fullPath we wrote above as-is. + if (to.hash.startsWith(REDIRECT_HASH_PREFIX)) { + const destination = to.hash.slice(REDIRECT_HASH_PREFIX.length) + const resolved = router.resolve(destination) + saveLastVisited(resolved.name as string, resolved.params, resolved.query) + } + // Check if the route the user wants to go to is a route which needs authentication. We use this to // redirect the user after successful login. const isValidUserAppRoute = !AUTH_ROUTE_NAMES.has(to.name as string) && localStorage.getItem('emailConfirmToken') === null - + if (isValidUserAppRoute) { saveLastVisited(to.name as string, to.params, to.query) } - + if (isValidUserAppRoute) { return {name: 'user.login'} } @@ -565,12 +598,25 @@ router.beforeEach(async (to, from) => { const newRoute = await getAuthForRoute(to, authStore) if(newRoute) { + // A string target (the decoded redirect destination for an authed browser) already + // carries its own query/path and no redirect hash, so navigate to it verbatim — don't + // re-attach to.hash or it would re-enter the redirect loop. + if (typeof newRoute === 'string') { + return newRoute + } return { - ...newRoute, hash: to.hash, + ...newRoute, } } - + + // to.fullPath keeps the redirect hash url-encoded while to.hash is decoded, so the endsWith + // check below never matches and would re-append the hash forever. The hash is already on the + // URL here, so skip the re-attach (#2654). + if (to.hash.startsWith(REDIRECT_HASH_PREFIX)) { + return + } + if(!to.fullPath.endsWith(to.hash)) { return to.fullPath + to.hash } diff --git a/frontend/tests/e2e/user/oauth-authorize.spec.ts b/frontend/tests/e2e/user/oauth-authorize.spec.ts index 908e9ae3f..572802e46 100644 --- a/frontend/tests/e2e/user/oauth-authorize.spec.ts +++ b/frontend/tests/e2e/user/oauth-authorize.spec.ts @@ -32,10 +32,20 @@ test.describe('OAuth 2.0 Authorization Flow', () => { }) // Navigate to the OAuth authorize frontend route. - // The user is not logged in, so the router guard saves the route - // and redirects to /login. + // The user is not logged in, so the router guard redirects to /login while + // carrying the authorize destination in a copyable #redirect= hash (not a + // query param, to keep the OAuth params out of access logs). await page.goto(`/oauth/authorize?${authorizeParams}`) - await expect(page).toHaveURL(/\/login/) + await expect(page).toHaveURL(/\/login#redirect=/) + + // The decoded #redirect= destination must carry the full authorize URL, including the + // OAuth params — checking only for the path would pass even if the query were dropped. + const redirectHash = decodeURIComponent(new URL(page.url()).hash) + expect(redirectHash).toContain('/oauth/authorize') + expect(redirectHash).toContain('response_type=code') + expect(redirectHash).toContain('client_id=vikunja') + expect(redirectHash).toContain(`code_challenge=${codeChallenge}`) + expect(redirectHash).toContain(`state=${state}`) // Register the response listener BEFORE clicking Login, because after // login redirectIfSaved() navigates back to /oauth/authorize and the @@ -77,4 +87,70 @@ test.describe('OAuth 2.0 Authorization Flow', () => { expect(tokenBody.token_type).toBe('bearer') expect(tokenBody.expires_in).toBeGreaterThan(0) }) + + // The primary #2654 scenario: the native client opened a different default browser that is + // already signed in to Vikunja. Opening the copied /login#redirect= URL must + // run the OAuth flow with the existing session instead of short-circuiting to home. + test('Already-authenticated browser opening the copied login redirect runs the authorize flow', async ({authenticatedPage, apiContext, currentUser}) => { + const page = authenticatedPage + + const codeVerifier = randomBytes(32).toString('base64url') + const codeChallenge = createHash('sha256').update(codeVerifier).digest('base64url') + const state = randomBytes(16).toString('base64url') + + const authorizeParams = new URLSearchParams({ + response_type: 'code', + client_id: 'vikunja', + redirect_uri: 'vikunja-flutter://callback', + code_challenge: codeChallenge, + code_challenge_method: 'S256', + state, + }) + + // The component POSTs as soon as it mounts with the existing session, so register the + // listener before navigating. + const authorizeResponsePromise = page.waitForResponse( + response => response.url().includes('/api/v1/oauth/authorize') && response.request().method() === 'POST', + {timeout: 15000}, + ) + + // Open the copyable login URL exactly as it would be pasted from another browser + // (#redirect= is REDIRECT_HASH_PREFIX from @/constants/redirectHash, inlined here because + // the e2e runner has no @ alias). + const redirectDestination = `/oauth/authorize?${authorizeParams}` + await page.goto(`/login#redirect=${encodeURIComponent(redirectDestination)}`) + + // The authed guard must send us straight to /oauth/authorize, not home. + await expect(page).toHaveURL(/\/oauth\/authorize/) + const landed = new URL(page.url()) + expect(landed.pathname).toBe('/oauth/authorize') + expect(landed.searchParams.get('response_type')).toBe('code') + expect(landed.searchParams.get('client_id')).toBe('vikunja') + expect(landed.searchParams.get('code_challenge')).toBe(codeChallenge) + expect(landed.searchParams.get('state')).toBe(state) + + // The PKCE flow completes with the existing session — no second login. + const authorizeResponse = await authorizeResponsePromise + const authorizeBody = await authorizeResponse.json() + expect(authorizeBody.code).toBeTruthy() + expect(authorizeBody.redirect_uri).toBe('vikunja-flutter://callback') + expect(authorizeBody.state).toBe(state) + + const tokenResponse = await apiContext.post('oauth/token', { + data: { + grant_type: 'authorization_code', + code: authorizeBody.code, + client_id: 'vikunja', + redirect_uri: 'vikunja-flutter://callback', + code_verifier: codeVerifier, + }, + }) + + expect(tokenResponse.ok()).toBe(true) + const tokenBody = await tokenResponse.json() + expect(tokenBody.access_token).toBeTruthy() + expect(tokenBody.refresh_token).toBeTruthy() + expect(tokenBody.token_type).toBe('bearer') + expect(tokenBody.expires_in).toBeGreaterThan(0) + }) })