docs(02): add code review report for plan 06 gap closure
This commit is contained in:
curo1305
@@ -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_
|
||||
Reference in New Issue
Block a user