fix(auth): honor oauth.authorize redirect hash when already authenticated (#2654)

getAuthForRoute short-circuited with an early return for any signed-in
browser before the #redirect= hash was inspected. So when a user copied
the /login#redirect=<encoded /oauth/authorize URL> into a browser that
was already signed in to Vikunja, Login.vue pushed them to home and the
OAuth authorize never ran — meaning the native/desktop client never got
its code. "Already signed in in my main browser, the desktop opened a
different default browser" is the primary #2654 scenario, so this broke
the feature's main path.

Detect the redirect hash up front and, in the authenticated branch,
return the decoded destination (e.g. /oauth/authorize?...) so the guard
re-enters, early-returns for the authed session, and mounts
OAuthAuthorize.vue with the existing token. The destination carries no
redirect hash, so there is no loop. The unauthenticated path is
unchanged (it still folds the hash into localStorage and proceeds to
/login so redirectIfSaved() runs after login/register/OIDC).

Add an e2e for the already-signed-in browser (authenticatedPage fixture)
that asserts it lands on /oauth/authorize and completes the PKCE flow,
and strengthen both tests to assert the OAuth params (response_type,
client_id, code_challenge, state) survive in the decoded redirect
destination rather than only checking the path.
This commit is contained in:
kolaente 2026-06-19 19:31:13 +02:00
parent 6dc7f8ba1e
commit be08179979
2 changed files with 94 additions and 2 deletions

View File

@ -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=<oauth.authorize> 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,

View File

@ -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=<oauth.authorize> 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)
})
})