---
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_