kite/.planning/phases/02-users-authentication/02-REVIEW.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
02-users-authentication	2026-06-01T00:00:00Z	standard	6	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	critical warning info total 3 4 1 8	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:

# 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:

# 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:

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

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:

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:

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:

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:

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:

"qrcode": "1.5.4"

---

Reviewed: 2026-06-01T00:00:00Z Reviewer: Claude (gsd-code-reviewer) Depth: standard