From fdb18300d91e501e800169e4451eea5b37b14c35 Mon Sep 17 00:00:00 2001
From: curo1305 <curo1305@proton.me>
Date: Mon, 1 Jun 2026 14:31:21 +0200
Subject: [PATCH] docs(02): add code review report for plan 06 gap closure

---
 .../02-users-authentication/02-REVIEW.md      | 187 ++++++++++++++++++
 1 file changed, 187 insertions(+)
 create mode 100644 .planning/phases/02-users-authentication/02-REVIEW.md

diff --git a/.planning/phases/02-users-authentication/02-REVIEW.md b/.planning/phases/02-users-authentication/02-REVIEW.md
new file mode 100644
index 0000000..00ebdbe
--- /dev/null
+++ b/.planning/phases/02-users-authentication/02-REVIEW.md
@@ -0,0 +1,187 @@
+---
+phase: 02-users-authentication
+reviewed: 2026-06-01T00:00:00Z
+depth: standard
+files_reviewed: 6
+files_reviewed_list:
+  - frontend/src/components/settings/SettingsAccountTab.vue
+  - frontend/src/App.vue
+  - frontend/src/router/index.js
+  - frontend/src/views/SettingsView.vue
+  - frontend/src/components/auth/TotpEnrollment.vue
+  - frontend/package.json
+findings:
+  critical: 3
+  warning: 4
+  info: 1
+  total: 8
+status: issues_found
+---
+
+# Phase 02: Code Review Report
+
+**Reviewed:** 2026-06-01T00:00:00Z
+**Depth:** standard
+**Files Reviewed:** 6
+**Status:** issues_found
+
+## Summary
+
+The gap-closure plan (02-06) wires up layout switching, auth route guards, and the TOTP enrollment QR improvement. The layout-aware App.vue and the `meta.layout='auth'` pattern are sound. The navigation guard correctly uses the `!to.meta.public` predicate to cover all authenticated routes — including those that carry no explicit `meta.requiresAuth` flag (/, /topics, /settings, etc.) — and deduplicates concurrent refresh calls via `_refreshInFlight`. No XSS vectors were found; Vue text interpolation auto-escapes all user-controlled values throughout.
+
+Three blockers were found: the backend `change_password` and `disable_totp` endpoints do not revoke active sessions as required by CLAUDE.md line 153, and a dead `#confirm-button` slot in `SettingsAccountTab.vue` silently discards the spinner/disabled guard on "Sign out all devices," allowing double-invocation with no visual feedback. Four warnings cover a URIError crash path in `onMounted`, a TOTP re-submission window after successful verification, fragile string-matching for password error routing, and an unconditional authenticated API call on every auth page load.
+
+## Critical Issues
+
+### CR-01: Password change does not revoke active sessions (backend, auth.py)
+
+**File:** `backend/api/auth.py:520`
+**Issue:** CLAUDE.md line 153 is explicit: "Password change, TOTP enroll/revoke, and account deactivation immediately revoke all active sessions." The `change_password` endpoint updates `password_hash` and commits but performs no refresh-token revocation. An attacker who obtained a valid refresh cookie before the password change retains full access until their token naturally expires (up to 30 days). The frontend `SettingsAccountTab.vue` also makes no attempt to trigger session revocation after a successful password change.
+**Fix:**
+```python
+# After session.commit() in change_password, revoke all refresh tokens for the user:
+await session.execute(
+    delete(RefreshToken).where(RefreshToken.user_id == current_user.id)
+)
+await session.commit()
+```
+Also add a `router.push('/login')` (or `authStore.logout()`) call in the frontend `changePassword()` success path so the current session is visibly invalidated.
+
+---
+
+### CR-02: TOTP disable does not revoke active sessions (backend, auth.py)
+
+**File:** `backend/api/auth.py:641`
+**Issue:** Same CLAUDE.md line 153 requirement applies to TOTP revoke. The `disable_totp` endpoint clears `totp_secret` / `totp_enabled` and deletes backup codes but does not revoke any refresh tokens. An attacker with a hijacked session can downgrade a victim's account security by removing 2FA, and both the attacker and any existing stolen sessions remain valid afterward.
+**Fix:**
+```python
+# After deleting backup codes in disable_totp, revoke all refresh tokens:
+await session.execute(
+    delete(RefreshToken).where(RefreshToken.user_id == current_user.id)
+)
+await session.commit()
+```
+
+---
+
+### CR-03: Dead `#confirm-button` slot removes spinner and disabled guard from "Sign out all devices"
+
+**File:** `frontend/src/components/settings/SettingsAccountTab.vue:149-161`
+**Issue:** `ConfirmBlock.vue` defines no named slot — it has a single hard-coded `<button>` that emits `confirmed`. The `<template #confirm-button>` block in `SettingsAccountTab.vue` is silently ignored by Vue; the custom button (with `:disabled="signingOutAll"` and `<AppSpinner>`) is never rendered. As a result: (1) the `signingOutAll` loading guard never activates; (2) a user can click "Sign out all devices" in rapid succession, firing multiple concurrent `logoutAll()` API calls; (3) there is no spinner feedback during the operation. The `signingOutAll` ref and the slot block are dead code.
+**Fix:** Either remove the slot and rely on `@confirmed` alone (accepting no spinner), or add a `<slot name="confirm-button">` fallback to `ConfirmBlock.vue`:
+```vue
+<!-- ConfirmBlock.vue — replace the hard-coded confirm button with: -->
+<slot name="confirm-button">
+  <button
+    type="button"
+    @click="$emit('confirmed')"
+    class="flex items-center gap-2 px-4 py-2 rounded-lg text-sm font-semibold transition-colors min-h-[44px]"
+    :class="confirmClass || 'bg-red-600 hover:bg-red-700 text-white'"
+  >
+    {{ confirmLabel }}
+  </button>
+</slot>
+```
+If the slot approach is not taken, guard against double-invocation in `signOutAll()`:
+```js
+async function signOutAll() {
+  if (signingOutAll.value) return   // add this guard
+  signingOutAll.value = true
+  ...
+}
+```
+
+---
+
+## Warnings
+
+### WR-01: `decodeURIComponent` on untrusted query parameter has no error handling
+
+**File:** `frontend/src/views/SettingsView.vue:133`
+**Issue:** `decodeURIComponent(errorMsg)` throws `URIError: URI malformed` if `cloud_error` contains invalid percent-encoding (e.g. a lone `%` or `%ZZ`). The call is inside `onMounted` with no try/catch. When this throws: `router.replace` has already fired (line 125) and the URL is cleaned up, but neither `oauthError` nor `oauthSuccessProvider` is set, so the user sees a blank cloud tab with no error message and no way to diagnose the failure.
+**Fix:**
+```js
+if (errorMsg) {
+  try {
+    oauthError.value = decodeURIComponent(errorMsg)
+  } catch {
+    oauthError.value = errorMsg  // fall back to raw value; Vue escapes it in template
+  }
+}
+```
+
+---
+
+### WR-02: TOTP "Verify code" button re-enables during 800 ms success flash, allowing re-submission
+
+**File:** `frontend/src/components/auth/TotpEnrollment.vue:145-162`
+**Issue:** After `api.totpEnable()` succeeds, `verified` is set to `true` and a `setTimeout(800)` is started before transitioning to `'backup-codes'`. `loading` is cleared in `finally` before the timeout fires, and `verifyCode` is not cleared on the success path. During the 800 ms window: `loading = false` and `verifyCode.length === 6`, so the button re-enables (disable condition: `loading || verifyCode.length !== 6`). The user can click "Verify code" again, submitting the same 6-digit code to `api.totpEnable()` a second time. The backend TOTP replay prevention should reject it, but the UX is broken and the second API call is unintended.
+**Fix:** Clear `verifyCode` and set `loading = true` (or add a separate `enrolling` guard) immediately on success before the timeout:
+```js
+const data = await api.totpEnable(verifyCode.value)
+backupCodes.value = data.backup_codes
+verified.value = true
+verifyCode.value = ''        // prevent re-submission during flash window
+setTimeout(() => {
+  step.value = 'backup-codes'
+}, 800)
+```
+
+---
+
+### WR-03: Password error display uses fragile string-matching on raw API messages
+
+**File:** `frontend/src/components/settings/SettingsAccountTab.vue:80-113`
+**Issue:** UI element selection (field-level vs. form-level error block) is driven by `passwordError.includes('Current')`. An unexpected API error message that happens to contain the word "Current" (e.g., "Current session has expired", a generic gateway message, etc.) will be routed to the current-password field display instead of the form-level block, showing a misleading field highlight and hiding the actual message. The else-branch at line 208 passes `msg` (the raw API string) directly into `passwordError`, making this fragile.
+**Fix:** Use a separate ref for field-level vs. form-level errors:
+```js
+const currentPasswordError = ref(null)
+const formError = ref(null)
+
+// In catch:
+if (msg.toLowerCase().includes('current') || msg.toLowerCase().includes('incorrect')) {
+  currentPasswordError.value = 'Current password is incorrect'
+} else {
+  formError.value = msg
+}
+```
+Then bind each template block to the appropriate ref rather than testing string contents.
+
+---
+
+### WR-04: `topicsStore.fetchTopics()` fires unconditionally on every page load, including auth pages
+
+**File:** `frontend/src/App.vue:20`
+**Issue:** `onMounted(() => topicsStore.fetchTopics())` runs regardless of the current route. When a user lands on `/login`, `/register`, or `/password-reset`, `App.vue` mounts and immediately calls `api.listTopics()` against an endpoint that requires authentication. The backend responds 401; the API client attempts a token refresh (which fails because there is no refresh cookie); the auth store clears its already-null `accessToken`. The topics store swallows the error silently. This is a spurious unauthorized request + failed refresh on every auth page, which adds noise to server logs and marginally slows page load on auth views. It also risks a race condition if future code reacts to the auth-store clearing.
+**Fix:** Guard the call behind an auth check, or move it to a layout component that only mounts for authenticated routes:
+```js
+import { watch } from 'vue'
+import { useAuthStore } from './stores/auth.js'
+
+const authStore = useAuthStore()
+watch(
+  () => authStore.accessToken,
+  (token) => { if (token) topicsStore.fetchTopics() },
+  { immediate: true }
+)
+```
+Alternatively, call `fetchTopics()` from the authenticated app shell rather than the root `App.vue`.
+
+---
+
+## Info
+
+### IN-01: `qrcode` dependency uses caret range instead of exact pin
+
+**File:** `frontend/package.json:13`
+**Issue:** `"qrcode": "^1.5.4"` uses a caret range. CLAUDE.md states "Dependency pinning: `requirements.txt` and `package-lock.json` pin exact versions; no floating `>=` for security-critical packages." While `qrcode` is a lower-risk library, all dependencies should be pinned in `package.json` for reproducibility (`package-lock.json` pins the resolved version, but the manifest range allows drift on `npm install` in fresh environments). The other three `dependencies` entries (`pinia`, `vue`, `vue-router`) are also caret-pinned and were pre-existing.
+**Fix:** Pin to the exact installed version after verifying `package-lock.json`:
+```json
+"qrcode": "1.5.4"
+```
+
+---
+
+_Reviewed: 2026-06-01T00:00:00Z_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_