fix(totp): require a current passcode to disable totp

frontend/src/client/generated/sdk.gen.ts

@@ -4078,7 +4078,7 @@ export const totpGet = <ThrowOnError extends boolean = true>(options?: Options<T
 /**
  * Disable totp
  *
- * Removes all totp settings for the authenticated user. Requires the current password for confirmation. Local accounts only.
+ * 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.
  */
 export const totpDisable = <ThrowOnError extends boolean = true>(options: Options<TotpDisableData, ThrowOnError>): RequestResult<TotpDisableResponses, TotpDisableErrors, ThrowOnError> => (options.client ?? client).post<TotpDisableResponses, TotpDisableErrors, ThrowOnError>({
     security: [{

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 = {

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()
 })

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<Totp>(totpKeys.current, current => current ? {enabled: false} : current)
 			client.removeQueries({queryKey: totpKeys.qr})

frontend/src/views/user/settings/TOTP.vue

@@ -65,6 +65,16 @@
 					type="password"
 					@keyup.enter="totpDisable"
 				/>
+				<FormField
+					id="totpDisablePasscode"
+					v-model="totpDisablePasscode"
+					:label="$t('user.settings.totp.passcode')"
+					autocomplete="one-time-code"
+					:placeholder="$t('user.settings.totp.passcodePlaceholder')"
+					type="text"
+					inputmode="numeric"
+					@keyup.enter="totpDisable"
+				/>
 				<XButton
 					danger
 					:loading="disableMutation.isPending.value"
@@ -112,6 +122,7 @@ useTitle(() => `${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
 	}

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()

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'

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

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)
 	}

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)

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,

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)
+	})
+}