diff --git a/frontend/src/router/index.ts b/frontend/src/router/index.ts index 580ade3c5..02e5ad580 100644 --- a/frontend/src/router/index.ts +++ b/frontend/src/router/index.ts @@ -473,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 @@ -586,6 +598,12 @@ 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 { hash: to.hash, ...newRoute, diff --git a/frontend/tests/e2e/user/oauth-authorize.spec.ts b/frontend/tests/e2e/user/oauth-authorize.spec.ts index 24228df3f..572802e46 100644 --- a/frontend/tests/e2e/user/oauth-authorize.spec.ts +++ b/frontend/tests/e2e/user/oauth-authorize.spec.ts @@ -37,7 +37,15 @@ test.describe('OAuth 2.0 Authorization Flow', () => { // query param, to keep the OAuth params out of access logs). await page.goto(`/oauth/authorize?${authorizeParams}`) await expect(page).toHaveURL(/\/login#redirect=/) - expect(decodeURIComponent(new URL(page.url()).hash)).toContain('/oauth/authorize') + + // 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 @@ -79,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) + }) })