diff --git a/frontend/src/client/generated/sdk.gen.ts b/frontend/src/client/generated/sdk.gen.ts index 9da6cb55c..0d91e9fdf 100644 --- a/frontend/src/client/generated/sdk.gen.ts +++ b/frontend/src/client/generated/sdk.gen.ts @@ -4078,7 +4078,7 @@ export const totpGet = (options?: Options(options: Options): RequestResult => (options.client ?? client).post({ security: [{ diff --git a/frontend/src/client/generated/types.gen.ts b/frontend/src/client/generated/types.gen.ts index 62c4dad0d..847be1bb2 100644 --- a/frontend/src/client/generated/types.gen.ts +++ b/frontend/src/client/generated/types.gen.ts @@ -2983,6 +2983,10 @@ export type TotpDisableBodyBody = { * The current user's password, required to disable totp. */ password?: string; + /** + * A current passcode from the user's authenticator, required to disable totp. + */ + totp_passcode?: string; }; export type TotpEnableBodyBody = { @@ -4748,6 +4752,10 @@ export type TotpDisableBodyBodyWritable = { * The current user's password, required to disable totp. */ password?: string; + /** + * A current passcode from the user's authenticator, required to disable totp. + */ + totp_passcode?: string; }; export type TotpEnableBodyBodyWritable = { diff --git a/frontend/src/client/queries/totp.test.ts b/frontend/src/client/queries/totp.test.ts index 49753d62b..bd4bdd026 100644 --- a/frontend/src/client/queries/totp.test.ts +++ b/frontend/src/client/queries/totp.test.ts @@ -44,8 +44,8 @@ it('drops enrollment secrets and QR data after disabling', async () => { client.setQueryData(totpKeys.current, {enabled: true, secret: 'old'}) client.setQueryData(totpKeys.qr, new Blob()) sdk.totpDisable.mockResolvedValue({data: {}}) - await client.getMutationCache().build(client, disableTotpMutationOptions()).execute('password') - expect(sdk.totpDisable).toHaveBeenCalledWith({body: {password: 'password'}}) + await client.getMutationCache().build(client, disableTotpMutationOptions()).execute({password: 'password', totp_passcode: '123456'}) + expect(sdk.totpDisable).toHaveBeenCalledWith({body: {password: 'password', totp_passcode: '123456'}}) expect(client.getQueryData(totpKeys.current)).toEqual({enabled: false}) expect(client.getQueryData(totpKeys.qr)).toBeUndefined() }) diff --git a/frontend/src/client/queries/totp.ts b/frontend/src/client/queries/totp.ts index c880b9e5e..ddcb6c465 100644 --- a/frontend/src/client/queries/totp.ts +++ b/frontend/src/client/queries/totp.ts @@ -73,7 +73,7 @@ export function useEnableTotpMutation() { export function disableTotpMutationOptions() { return contextMutationOptions({ - mutationFn: async (password: string) => (await totpDisable({body: {password}})).data, + mutationFn: async (body: {password: string, totp_passcode: string}) => (await totpDisable({body})).data, onSuccess: (_data, _input, client) => { client.setQueryData(totpKeys.current, current => current ? {enabled: false} : current) client.removeQueries({queryKey: totpKeys.qr}) diff --git a/frontend/src/views/user/settings/TOTP.vue b/frontend/src/views/user/settings/TOTP.vue index 630fad74a..5258681a3 100644 --- a/frontend/src/views/user/settings/TOTP.vue +++ b/frontend/src/views/user/settings/TOTP.vue @@ -65,6 +65,16 @@ type="password" @keyup.enter="totpDisable" /> + `${t('user.settings.totp.title')} - ${t('user.settings.title')}`) const totpConfirmPasscode = ref('') const totpDisableForm = ref(false) const totpDisablePassword = ref('') +const totpDisablePasscode = ref('') const configStore = useConfigStore() const authStore = useAuthStore() @@ -145,12 +156,16 @@ async function totpConfirm() { function closeDisableForm() { totpDisableForm.value = false totpDisablePassword.value = '' + totpDisablePasscode.value = '' } async function totpDisable() { if (disableMutation.isPending.value) return try { - await disableMutation.mutateAsync(totpDisablePassword.value) + await disableMutation.mutateAsync({ + password: totpDisablePassword.value, + totp_passcode: totpDisablePasscode.value, + }) } catch { return } diff --git a/frontend/tests/e2e/user/settings/totp.spec.ts b/frontend/tests/e2e/user/settings/totp.spec.ts index 7647e3642..59cf156ed 100644 --- a/frontend/tests/e2e/user/settings/totp.spec.ts +++ b/frontend/tests/e2e/user/settings/totp.spec.ts @@ -1,7 +1,7 @@ import {test, expect} from '../../../support/fixtures' import {generate} from 'otplib' import {gotoUserSettings} from '../../../support/userSettings' -import {TotpFactory} from '../../../factories/totp' +import {TotpFactory, FIXED_TOTP_SECRET} from '../../../factories/totp' import {TEST_PASSWORD} from '../../../support/constants' test.describe('TOTP', () => { @@ -28,6 +28,7 @@ test.describe('TOTP', () => { await page.getByRole('button', {name: 'Disable'}).click() await page.locator('#currentPassword').fill(TEST_PASSWORD) + await page.locator('#totpDisablePasscode').fill(await generate({secret: FIXED_TOTP_SECRET})) // Second "Disable" inside the form submits it. await page.locator('.card').getByRole('button', {name: 'Disable'}).last().click() @@ -38,6 +39,20 @@ test.describe('TOTP', () => { await expect(page.getByRole('button', {name: 'Enroll'})).toBeVisible() }) + test('keeps TOTP enabled when the disable passcode is wrong', async ({authenticatedPage: page, currentUser}) => { + await TotpFactory.create(1, {user_id: currentUser.id}, false) + await gotoUserSettings(page, 'totp') + + await page.getByRole('button', {name: 'Disable'}).click() + await page.locator('#currentPassword').fill(TEST_PASSWORD) + await page.locator('#totpDisablePasscode').fill('000000') + await page.locator('.card').getByRole('button', {name: 'Disable'}).last().click() + + await expect(page.locator('.global-notification .vue-notification.error')).toBeVisible() + await page.reload() + await expect(page.getByText("You've successfully set up two factor authentication!")).toBeVisible() + }) + test('rejects wrong passcode during enrollment', async ({authenticatedPage: page}) => { await gotoUserSettings(page, 'totp') await page.getByRole('button', {name: 'Enroll'}).click() diff --git a/frontend/tests/factories/totp.ts b/frontend/tests/factories/totp.ts index 5f96f6264..173330482 100644 --- a/frontend/tests/factories/totp.ts +++ b/frontend/tests/factories/totp.ts @@ -1,7 +1,7 @@ import {Factory} from '../support/factory' // Fixed base32 secret so tests can generate deterministic codes with otplib. -export const FIXED_TOTP_SECRET = 'JBSWY3DPEHPK3PXP' +export const FIXED_TOTP_SECRET = 'HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ' export class TotpFactory extends Factory { static table = 'totp' diff --git a/pkg/routes/api/v1/user_totp.go b/pkg/routes/api/v1/user_totp.go index 6e907f2c0..20a74cac1 100644 --- a/pkg/routes/api/v1/user_totp.go +++ b/pkg/routes/api/v1/user_totp.go @@ -138,15 +138,18 @@ func UserTOTPEnable(c *echo.Context) error { // UserTOTPDisable disables totp settings for the current user. // @Summary Disable totp settings -// @Description Disables any totp settings for the current user. +// @Description Disables any totp settings for the current user. Requires the current password and a current totp passcode. // @tags user // @Accept json // @Produce json // @Security JWTKeyAuth -// @Param totp body user.Login true "The current user's password (only password is enough)." +// @Param totp body user.Login true "The current user's password and a current totp passcode (totp_passcode)." // @Success 200 {object} models.Message "Successfully disabled" // @Failure 400 {object} web.HTTPError "Something's invalid." +// @Failure 403 {object} web.HTTPError "Wrong password." // @Failure 404 {object} web.HTTPError "User does not exist." +// @Failure 412 {object} web.HTTPError "Invalid totp passcode." +// @Failure 429 {object} web.HTTPError "Too many failed attempts." // @Failure 500 {object} models.Message "Internal server error." // @Router /user/settings/totp/disable [post] func UserTOTPDisable(c *echo.Context) error { @@ -166,13 +169,7 @@ func UserTOTPDisable(c *echo.Context) error { } defer s.Close() - err = user.CheckReauthPassword(u, login.Password) - if err != nil { - _ = s.Rollback() - return err - } - - err = user.DisableTOTP(s, u) + err = user.DisableTOTPWithReauth(s, u, login.Password, login.TOTPPasscode) if err != nil { _ = s.Rollback() return err diff --git a/pkg/routes/api/v2/user_totp.go b/pkg/routes/api/v2/user_totp.go index ec43fd127..a253ee8a0 100644 --- a/pkg/routes/api/v2/user_totp.go +++ b/pkg/routes/api/v2/user_totp.go @@ -42,7 +42,8 @@ type totpEnableBody struct { type totpDisableBody struct { Body struct { - Password string `json:"password" doc:"The current user's password, required to disable totp."` + Password string `json:"password" doc:"The current user's password, required to disable totp."` + TOTPPasscode string `json:"totp_passcode" doc:"A current passcode from the user's authenticator, required to disable totp."` } } @@ -101,7 +102,7 @@ func RegisterTOTPRoutes(api huma.API) { Register(api, huma.Operation{ OperationID: "totp-disable", Summary: "Disable totp", - Description: "Removes all totp settings for the authenticated user. Requires the current password for confirmation. Local accounts only.", + Description: "Removes all totp settings for the authenticated user. Requires the current password and a current totp passcode. Wrong passwords and passcodes are throttled per user (429). Local accounts only.", Method: http.MethodPost, Path: "/user/settings/totp/disable", DefaultStatus: http.StatusOK, @@ -225,11 +226,7 @@ func totpDisable(ctx context.Context, in *totpDisableBody) (*totpMessageBody, er } defer s.Close() - if err := user.CheckReauthPassword(u, in.Body.Password); err != nil { - _ = s.Rollback() - return nil, translateDomainError(err) - } - if err := user.DisableTOTP(s, u); err != nil { + if err := user.DisableTOTPWithReauth(s, u, in.Body.Password, in.Body.TOTPPasscode); err != nil { _ = s.Rollback() return nil, translateDomainError(err) } diff --git a/pkg/user/totp.go b/pkg/user/totp.go index dd06b71b7..103a92abe 100644 --- a/pkg/user/totp.go +++ b/pkg/user/totp.go @@ -141,6 +141,30 @@ func DisableTOTP(s *xorm.Session, user *User) (err error) { return } +// Needs a passcode so a stolen session plus the password can't strip the second factor. Rolls s back on failure. +func DisableTOTPWithReauth(s *xorm.Session, u *User, password, passcode string) error { + err := ThrottleReauth(u.ID, func() error { + if err := CheckUserPassword(u, password); err != nil { + return err + } + if passcode == "" { + return ErrInvalidTOTPPasscode{} + } + _, err := ValidateTOTPPasscode(s, &TOTPPasscode{User: u, Passcode: passcode}) + return err + }) + if err != nil { + // Rolled back first: HandleFailedTOTPAuth needs its own write lock on SQLite. + _ = s.Rollback() + if passcode != "" && IsErrInvalidTOTPPasscode(err) { + HandleFailedTOTPAuth(u) + } + return err + } + + return DisableTOTP(s, u) +} + // ValidateTOTPPasscode validated totp codes of users. func ValidateTOTPPasscode(s *xorm.Session, passcode *TOTPPasscode) (t *TOTP, err error) { t, err = GetTOTPForUser(s, passcode.User) diff --git a/pkg/webtests/huma_user_totp_test.go b/pkg/webtests/huma_user_totp_test.go index 6e05dac54..e3fdd74c5 100644 --- a/pkg/webtests/huma_user_totp_test.go +++ b/pkg/webtests/huma_user_totp_test.go @@ -22,6 +22,8 @@ import ( "testing" "time" + "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/modules/keyvalue" "code.vikunja.io/api/pkg/user" "github.com/pquerna/otp/totp" @@ -95,13 +97,39 @@ func TestHumaTOTP(t *testing.T) { require.Equal(t, http.StatusPreconditionFailed, rec.Code, "body: %s", rec.Body.String()) }) - t.Run("Disable with the correct password", func(t *testing.T) { + t.Run("Disable with the correct password and passcode", func(t *testing.T) { + e, err := setupTestEnv() + require.NoError(t, err) + passcode, err := totp.GenerateCode("JBSWY3DPEHPK3PXP", time.Now()) + require.NoError(t, err) + rec := humaRequest(t, e, http.MethodPost, "/api/v2/user/settings/totp/disable", + fmt.Sprintf(`{"password":"12345678","totp_passcode":%q}`, passcode), humaTokenFor(t, &testuser10), "") + require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), "disabled successfully") + db.AssertMissing(t, "totp", map[string]interface{}{"user_id": 10}) + }) + + // GHSA-49pj-52x6-8gpj: the password alone must not strip the second factor. + t.Run("Disable with the password but no passcode is refused", func(t *testing.T) { e, err := setupTestEnv() require.NoError(t, err) rec := humaRequest(t, e, http.MethodPost, "/api/v2/user/settings/totp/disable", `{"password":"12345678"}`, humaTokenFor(t, &testuser10), "") - require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) - assert.Contains(t, rec.Body.String(), "disabled successfully") + require.Equal(t, http.StatusPreconditionFailed, rec.Code, "body: %s", rec.Body.String()) + db.AssertExists(t, "totp", map[string]interface{}{"user_id": 10, "enabled": true}, false) + }) + + t.Run("Disable with a wrong passcode is refused and counted", func(t *testing.T) { + e, err := setupTestEnv() + require.NoError(t, err) + rec := humaRequest(t, e, http.MethodPost, "/api/v2/user/settings/totp/disable", + `{"password":"12345678","totp_passcode":"000000"}`, humaTokenFor(t, &testuser10), "") + require.Equal(t, http.StatusPreconditionFailed, rec.Code, "body: %s", rec.Body.String()) + db.AssertExists(t, "totp", map[string]interface{}{"user_id": 10, "enabled": true}, false) + attempts, exists, err := keyvalue.Get(testuser10.GetFailedTOTPAttemptsKey()) + require.NoError(t, err) + require.True(t, exists, "a wrong passcode must count toward the totp lockout") + assert.Equal(t, int64(1), attempts) }) t.Run("Disable with a wrong password is refused", func(t *testing.T) { @@ -123,7 +151,7 @@ func TestHumaTOTP(t *testing.T) { {http.MethodGet, "/api/v2/user/settings/totp/qrcode", ""}, {http.MethodPost, "/api/v2/user/settings/totp/enroll", ""}, {http.MethodPost, "/api/v2/user/settings/totp/enable", `{"passcode":"000000"}`}, - {http.MethodPost, "/api/v2/user/settings/totp/disable", `{"password":"12345678"}`}, + {http.MethodPost, "/api/v2/user/settings/totp/disable", `{"password":"12345678","totp_passcode":"000000"}`}, } { rec := humaRequest(t, e, tc.method, tc.path, tc.body, token, "") assert.Equal(t, http.StatusPreconditionFailed, rec.Code, diff --git a/pkg/webtests/user_totp_test.go b/pkg/webtests/user_totp_test.go index 0b6af69a3..8e68b8808 100644 --- a/pkg/webtests/user_totp_test.go +++ b/pkg/webtests/user_totp_test.go @@ -17,6 +17,7 @@ package webtests import ( + "fmt" "net/http" "testing" "time" @@ -89,3 +90,42 @@ func TestUserTOTPSecretHiddenWhenEnabled(t *testing.T) { require.NoError(t, err) }) } + +// GHSA-49pj-52x6-8gpj: the password alone must not strip the second factor. +func TestUserTOTPDisable(t *testing.T) { + t.Run("password and passcode", func(t *testing.T) { + passcode, err := totp.GenerateCode("JBSWY3DPEHPK3PXP", time.Now()) + require.NoError(t, err) + rec, err := newTestRequestWithUser(t, http.MethodPost, apiv1.UserTOTPDisable, &testuser10, + fmt.Sprintf(`{"password":"12345678","totp_passcode":%q}`, passcode), nil, nil) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + db.AssertMissing(t, "totp", map[string]interface{}{"user_id": 10}) + }) + + t.Run("password without passcode", func(t *testing.T) { + _, err := newTestRequestWithUser(t, http.MethodPost, apiv1.UserTOTPDisable, &testuser10, + `{"password":"12345678"}`, nil, nil) + require.Error(t, err) + assert.Equal(t, http.StatusPreconditionFailed, getHTTPErrorCode(err)) + db.AssertExists(t, "totp", map[string]interface{}{"user_id": 10, "enabled": true}, false) + }) + + t.Run("wrong passcode", func(t *testing.T) { + _, err := newTestRequestWithUser(t, http.MethodPost, apiv1.UserTOTPDisable, &testuser10, + `{"password":"12345678","totp_passcode":"000000"}`, nil, nil) + require.Error(t, err) + assert.Equal(t, http.StatusPreconditionFailed, getHTTPErrorCode(err)) + db.AssertExists(t, "totp", map[string]interface{}{"user_id": 10, "enabled": true}, false) + }) + + t.Run("wrong password", func(t *testing.T) { + passcode, err := totp.GenerateCode("JBSWY3DPEHPK3PXP", time.Now()) + require.NoError(t, err) + _, err = newTestRequestWithUser(t, http.MethodPost, apiv1.UserTOTPDisable, &testuser10, + fmt.Sprintf(`{"password":"wrong","totp_passcode":%q}`, passcode), nil, nil) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + db.AssertExists(t, "totp", map[string]interface{}{"user_id": 10, "enabled": true}, false) + }) +}