From fdc0860252180711d93ccdfc7f5d6c723221e973 Mon Sep 17 00:00:00 2001 From: kolaente Date: Wed, 14 May 2025 22:51:45 +0200 Subject: [PATCH] fix(auth): only use query parameters instead of local storage for password reset token (#770) Resolves https://github.com/go-vikunja/vikunja/issues/682 --- .vscode/settings.json | 2 +- .../cypress/e2e/user/password-reset.spec.ts | 57 +++++++++++++++++++ frontend/cypress/factories/project.ts | 10 +++- frontend/cypress/factories/token.ts | 29 ++++++++++ frontend/cypress/factories/user.ts | 19 +++++-- frontend/src/App.vue | 4 +- frontend/src/i18n/lang/en.json | 3 +- frontend/src/models/passwordReset.ts | 4 +- frontend/src/router/index.ts | 17 ++++-- frontend/src/stores/auth.ts | 7 +++ frontend/src/views/user/PasswordReset.vue | 21 +++++-- 11 files changed, 154 insertions(+), 19 deletions(-) create mode 100644 frontend/cypress/e2e/user/password-reset.spec.ts create mode 100644 frontend/cypress/factories/token.ts diff --git a/.vscode/settings.json b/.vscode/settings.json index 535b3821f..8a613b1b2 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -11,7 +11,7 @@ "editor.defaultFormatter": "dbaeumer.vscode-eslint" }, "[typescript]": { - "editor.defaultFormatter": "dbaeumer.vscode-eslint" + "editor.defaultFormatter": "vscode.typescript-language-features" }, // https://eslint.vuejs.org/user-guide/#editor-integrations diff --git a/frontend/cypress/e2e/user/password-reset.spec.ts b/frontend/cypress/e2e/user/password-reset.spec.ts new file mode 100644 index 000000000..3a7e26ad8 --- /dev/null +++ b/frontend/cypress/e2e/user/password-reset.spec.ts @@ -0,0 +1,57 @@ +import {UserFactory, type UserAttributes} from '../../factories/user' +import {TokenFactory, type TokenAttributes} from '../../factories/token' + +context('Password Reset', () => { + let user: UserAttributes + + beforeEach(() => { + UserFactory.truncate() + TokenFactory.truncate() + user = UserFactory.create(1)[0] as UserAttributes + }) + + it('Should allow a user to reset their password with a valid token', () => { + const tokenArray = TokenFactory.create(1, {user_id: user.id as number, kind: 1}) + const token: TokenAttributes = tokenArray[0] as TokenAttributes + + cy.visit(`/?userPasswordReset=${token.token}`) + cy.url().should('include', `/password-reset?token=${token.token}`) + + const newPassword = 'newSecurePassword123' + cy.get('input[id=password]').type(newPassword) + cy.get('button').contains('Reset your password').click() + + cy.get('.message.success').should('contain', 'The password was updated successfully.') + cy.get('.button').contains('Login').click() + cy.url().should('include', '/login') + + // Try to login with the new password + cy.get('input[id=username]').type(user.username) + cy.get('input[id=password]').type(newPassword) + cy.get('.button').contains('Login').click() + cy.url().should('not.include', '/login') + }) + + it('Should show an error for an invalid token', () => { + cy.visit('/?userPasswordReset=invalidtoken123') + cy.url().should('include', '/password-reset?token=invalidtoken123') + + // Attempt to reset password + const newPassword = 'newSecurePassword123' + cy.get('input[id=password]').type(newPassword) + cy.get('button').contains('Reset your password').click() + + cy.get('.message').should('contain', 'Invalid token') + }) + + it('Should redirect to login if no token is present in query param when visiting /password-reset directly', () => { + cy.visit('/password-reset') + cy.url().should('not.include', '/password-reset') + cy.url().should('include', '/login') + }) + + it('Should redirect to login if userPasswordReset token is not present in query param when visiting root', () => { + cy.visit('/') + cy.url().should('include', '/login') + }) +}) \ No newline at end of file diff --git a/frontend/cypress/factories/project.ts b/frontend/cypress/factories/project.ts index 8068027cb..8530dc1dd 100644 --- a/frontend/cypress/factories/project.ts +++ b/frontend/cypress/factories/project.ts @@ -1,10 +1,18 @@ import {Factory} from '../support/factory' import {faker} from '@faker-js/faker' +export interface ProjectAttributes { + id: number | string; // Allow string for '{increment}' + title: string; + owner_id: number; + created: string; + updated: string; +} + export class ProjectFactory extends Factory { static table = 'projects' - static factory() { + static factory(): Omit & { id: string } { // id is '{increment}' before seeding const now = new Date() return { diff --git a/frontend/cypress/factories/token.ts b/frontend/cypress/factories/token.ts new file mode 100644 index 000000000..a51692fd0 --- /dev/null +++ b/frontend/cypress/factories/token.ts @@ -0,0 +1,29 @@ +import {faker} from '@faker-js/faker' +import {Factory} from '../support/factory' + +export interface TokenAttributes { + id: number | string; // Allow string for '{increment}' + user_id: number; + token: string; + kind: number; + created: string; +} + +export class TokenFactory extends Factory { + static table = 'user_tokens' + + // The factory method itself produces an object where id is '{increment}' (a string) + // before it gets processed by the main create() method in the base Factory class. + static factory(attrs?: Partial>): Omit & { id: string } { + const now = new Date() + + return { + id: '{increment}', // This is a string + user_id: 1, // Default user_id + token: faker.string.alphanumeric(64), + kind: 1, // TokenPasswordReset + created: now.toISOString(), + ...(attrs ?? {}), + } + } +} \ No newline at end of file diff --git a/frontend/cypress/factories/user.ts b/frontend/cypress/factories/user.ts index 78eaa12e1..a685e24ac 100644 --- a/frontend/cypress/factories/user.ts +++ b/frontend/cypress/factories/user.ts @@ -2,16 +2,27 @@ import {faker} from '@faker-js/faker' import {Factory} from '../support/factory' +export interface UserAttributes { + id: number | string; + username: string; + password?: string; + status: number; + issuer: string; + language: string; + created: string; + updated: string; +} + export class UserFactory extends Factory { static table = 'users' - static factory() { + static factory(): Omit & { id: string; password?: string } { const now = new Date() return { - id: '{increment}', - username: faker.lorem.word(10) + faker.string.uuid(), - password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.', // 1234 + id: '{increment}', + username: faker.lorem.word(10) + faker.string.uuid(), + password: '$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To.', // 1234 status: 0, issuer: 'local', language: 'en', diff --git a/frontend/src/App.vue b/frontend/src/App.vue index 3cd19a5a4..e4db31377 100644 --- a/frontend/src/App.vue +++ b/frontend/src/App.vue @@ -84,8 +84,8 @@ watch(userPasswordReset, (userPasswordReset) => { return } - localStorage.setItem('passwordResetToken', userPasswordReset) - router.push({name: 'user.password-reset.reset'}) + authStore.setPasswordResetToken(userPasswordReset) + router.push({name: 'user.password-reset.reset', query: { token: userPasswordReset }}) }, { immediate: true }) // setup email verification redirect diff --git a/frontend/src/i18n/lang/en.json b/frontend/src/i18n/lang/en.json index e8607e7d2..1c2805868 100644 --- a/frontend/src/i18n/lang/en.json +++ b/frontend/src/i18n/lang/en.json @@ -67,7 +67,8 @@ "noAccountYet": "Don't have an account yet?", "alreadyHaveAnAccount": "Already have an account?", "remember": "Stay logged in", - "registrationDisabled": "Registration is disabled." + "registrationDisabled": "Registration is disabled.", + "passwordResetTokenMissing": "Password reset token is missing." }, "settings": { "title": "Settings", diff --git a/frontend/src/models/passwordReset.ts b/frontend/src/models/passwordReset.ts index 0c2d3cd87..86f9ed452 100644 --- a/frontend/src/models/passwordReset.ts +++ b/frontend/src/models/passwordReset.ts @@ -11,6 +11,8 @@ export default class PasswordResetModel extends AbstractModel im super() this.assignData(data) - this.token = localStorage.getItem('passwordResetToken') + if (data.token) { + this.token = data.token + } } } \ No newline at end of file diff --git a/frontend/src/router/index.ts b/frontend/src/router/index.ts index e3a20cf93..4328b415f 100644 --- a/frontend/src/router/index.ts +++ b/frontend/src/router/index.ts @@ -394,6 +394,17 @@ export async function getAuthForRoute(to: RouteLocation, authStore) { return } + // Check if password reset token is in query params + const resetToken = to.query.userPasswordReset as string | undefined + if (resetToken) { + authStore.setPasswordResetToken(resetToken) + } + + // Redirect to password reset page if we have a token stored + if (authStore.passwordResetToken && to.name !== 'user.password-reset.reset') { + return {name: 'user.password-reset.reset', query: { token: authStore.passwordResetToken }} + } + // 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 = ![ @@ -404,7 +415,7 @@ export async function getAuthForRoute(to: RouteLocation, authStore) { 'link-share.auth', 'openid.auth', ].includes(to.name as string) && - localStorage.getItem('passwordResetToken') === null && + authStore.passwordResetToken === null && localStorage.getItem('emailConfirmToken') === null && !(to.name === 'home' && (typeof to.query.userPasswordReset !== 'undefined' || typeof to.query.userEmailConfirm !== 'undefined')) @@ -423,10 +434,6 @@ export async function getAuthForRoute(to: RouteLocation, authStore) { return {name: 'user.login'} } - if(localStorage.getItem('passwordResetToken') !== null && to.name !== 'user.password-reset.reset') { - return {name: 'user.password-reset.reset'} - } - if(localStorage.getItem('emailConfirmToken') !== null && to.name !== 'user.login') { return {name: 'user.login'} } diff --git a/frontend/src/stores/auth.ts b/frontend/src/stores/auth.ts index 25477ed29..21ea84ea5 100644 --- a/frontend/src/stores/auth.ts +++ b/frontend/src/stores/auth.ts @@ -68,6 +68,7 @@ export const useAuthStore = defineStore('auth', () => { const authenticated = ref(false) const isLinkShareAuth = ref(false) const needsTotpPasscode = ref(false) + const passwordResetToken = ref(null) const info = ref(null) const avatarUrl = ref('') @@ -149,6 +150,10 @@ export const useAuthStore = defineStore('auth', () => { needsTotpPasscode.value = newNeedsTotpPasscode } + function setPasswordResetToken(token: string | null) { + passwordResetToken.value = token + } + function reloadAvatar() { if (!info.value) return avatarUrl.value = `${getAvatarUrl(info.value)}&=${new Date().valueOf()}` @@ -444,6 +449,7 @@ export const useAuthStore = defineStore('auth', () => { authenticated: readonly(authenticated), isLinkShareAuth: readonly(isLinkShareAuth), needsTotpPasscode: readonly(needsTotpPasscode), + passwordResetToken: readonly(passwordResetToken), info: readonly(info), avatarUrl: readonly(avatarUrl), @@ -466,6 +472,7 @@ export const useAuthStore = defineStore('auth', () => { setAuthenticated, setIsLinkShareAuth, setNeedsTotpPasscode, + setPasswordResetToken, reloadAvatar, updateLastUserRefresh, diff --git a/frontend/src/views/user/PasswordReset.vue b/frontend/src/views/user/PasswordReset.vue index fe3636af6..9eb3d6777 100644 --- a/frontend/src/views/user/PasswordReset.vue +++ b/frontend/src/views/user/PasswordReset.vue @@ -52,6 +52,9 @@