docs(phase-09): add VERIFICATION.md + REVIEW.md — phase 9 complete
5/5 automated must-haves verified. 4 human runtime checks pending. Code review: 4 blockers (CR-01 testAiConnection POST/GET mismatch, CR-02 open redirect, CR-03 unvalidated role field, CR-04 password shuffle bias), 6 warnings, 3 info. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
curo1305
co-authored by
Claude Sonnet 4.6
Claude Sonnet 4.6
parent
baa462870d
commit
6f9f045a9a
@@ -0,0 +1,283 @@
|
||||
---
|
||||
phase: 09-admin-panel-rearchitecture
|
||||
reviewed: 2026-06-12T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 24
|
||||
files_reviewed_list:
|
||||
- backend/api/admin/__init__.py
|
||||
- backend/api/admin/overview.py
|
||||
- backend/api/admin/ai.py
|
||||
- backend/api/admin/quotas.py
|
||||
- backend/api/admin/users.py
|
||||
- backend/api/admin/shared.py
|
||||
- backend/api/auth/password.py
|
||||
- backend/api/auth/shared.py
|
||||
- backend/api/auth/tokens.py
|
||||
- backend/api/auth/totp.py
|
||||
- backend/api/documents/content.py
|
||||
- backend/api/documents/crud.py
|
||||
- backend/api/documents/upload.py
|
||||
- backend/tests/test_admin_overview.py
|
||||
- frontend/src/api/admin.js
|
||||
- frontend/src/components/admin/AdminSidebar.vue
|
||||
- frontend/src/layouts/AdminLayout.vue
|
||||
- frontend/src/router/index.js
|
||||
- frontend/src/views/admin/AdminAiView.vue
|
||||
- frontend/src/views/admin/AdminAuditView.vue
|
||||
- frontend/src/views/admin/AdminOverviewView.vue
|
||||
- frontend/src/views/admin/AdminQuotasView.vue
|
||||
- frontend/src/views/admin/AdminUsersView.vue
|
||||
- frontend/src/views/auth/LoginView.vue
|
||||
- frontend/tailwind.config.js
|
||||
findings:
|
||||
critical: 4
|
||||
warning: 6
|
||||
info: 3
|
||||
total: 13
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 9: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-06-12
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 24
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Phase 9 rearchitects the admin panel into a focused sub-router tree and reworks several auth and document endpoints. The overall structure is sound: router aggregation is clean, the `get_current_admin` dep is consistently applied, and the data-leakage protections in `_user_to_dict` and `_ai_config_to_dict` are correctly implemented.
|
||||
|
||||
Four blockers were found:
|
||||
|
||||
1. `testAiConnection` in `admin.js` sends a GET request with query-string parameters, but the backend endpoint `POST /api/admin/ai-config/test-connection` expects a POST body — the call never works as written.
|
||||
2. The router guard's open redirect: `route.query.redirect` is used without validation, allowing an attacker to redirect a freshly authenticated admin or user to an arbitrary external URL.
|
||||
3. The admin user-create flow generates a handle client-side from an untrusted email string; the back-end `UserCreate` model accepts any role string including "admin" without restriction.
|
||||
4. The Fisher-Yates shuffle in `generateRandomPassword` uses a four-byte `posArr` for shuffling a 16-element array, producing severely biased shuffles for indices 4–15.
|
||||
|
||||
---
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: `testAiConnection` sends GET but backend expects POST
|
||||
|
||||
**File:** `frontend/src/api/admin.js:75-79`
|
||||
**Issue:** `testAiConnection` issues a GET request to `/api/admin/ai-config/test-connection?provider_id=...` with no body. The backend registers `POST /api/admin/ai-config/test-connection` (see `backend/api/admin/ai.py:150`) which reads a `TestConnectionRequest` Pydantic body. A GET to a POST endpoint returns 405 Method Not Allowed. The overrides object passed from `AdminAiView.vue:298` (`{api_key, base_url, model_name}`) is also silently ignored because it is never serialised.
|
||||
|
||||
**Fix:**
|
||||
```js
|
||||
export function testAiConnection(providerId, overrides = {}) {
|
||||
return request('/api/admin/ai-config/test-connection', {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ provider_id: providerId, ...overrides }),
|
||||
})
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CR-02: Open redirect via unvalidated `route.query.redirect`
|
||||
|
||||
**File:** `frontend/src/views/auth/LoginView.vue:213`
|
||||
**Issue:** After a successful login, the view unconditionally calls `router.push(route.query.redirect || defaultRedirect)`. An attacker can craft a phishing link such as:
|
||||
```
|
||||
https://app.example.com/login?redirect=https://evil.example.com
|
||||
```
|
||||
Vue Router's `push()` accepts absolute URLs and will navigate the browser to the external site, potentially exfiltrating the just-issued access token from memory or triggering further credential harvesting.
|
||||
|
||||
**Fix:** Validate that the redirect target is an internal path before using it:
|
||||
```js
|
||||
function isSafeRedirect(url) {
|
||||
if (!url) return false
|
||||
// Accept only paths starting with / but not // (protocol-relative)
|
||||
return url.startsWith('/') && !url.startsWith('//')
|
||||
}
|
||||
|
||||
const redirect = isSafeRedirect(route.query.redirect)
|
||||
? route.query.redirect
|
||||
: defaultRedirect
|
||||
await router.push(redirect)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CR-03: `UserCreate` model allows admin role escalation via POST /api/admin/users
|
||||
|
||||
**File:** `backend/api/admin/users.py:31-35`
|
||||
**Issue:** `UserCreate.role` is a plain `str` with default `"user"`. The endpoint accepts and persists whatever role value is submitted:
|
||||
```python
|
||||
class UserCreate(BaseModel):
|
||||
...
|
||||
role: str = "user"
|
||||
```
|
||||
Nothing prevents `{"role": "superadmin"}` or any other invented value from being stored in the DB. More critically, even within the defined roles, any admin operator can create another admin account — there is no separate privilege gate on admin creation. This is only a policy gap rather than an external injection vector (the endpoint requires `get_current_admin`), but it violates the principle that role strings should be strictly validated.
|
||||
|
||||
**Fix:** Use a `Literal` type to constrain the field:
|
||||
```python
|
||||
from typing import Literal
|
||||
class UserCreate(BaseModel):
|
||||
handle: str
|
||||
email: EmailStr
|
||||
password: str
|
||||
role: Literal["user", "admin"] = "user"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CR-04: Biased Fisher-Yates shuffle in `generateRandomPassword`
|
||||
|
||||
**File:** `frontend/src/views/admin/AdminUsersView.vue:308-314`
|
||||
**Issue:** The shuffle uses a pre-fetched four-byte `posArr` (indices 4–7) to cover 15 loop iterations (`i` from 15 down to 1). `posArr[4 + (i % 4)]` maps every four consecutive `i` values to the same byte. For `i >= 8`, bytes 4–7 are reused with a different modulus, making many swap destinations statistically impossible. Positions 8–15 of the character array end up with a heavily constrained distribution. The stated claim "no modulo bias" refers only to charset selection, not to the shuffle.
|
||||
|
||||
**Fix:** Fetch fresh random bytes for each swap step:
|
||||
```js
|
||||
for (let i = chars.length - 1; i > 0; i--) {
|
||||
const swapBuf = new Uint8Array(1)
|
||||
crypto.getRandomValues(swapBuf)
|
||||
const j = swapBuf[0] % (i + 1)
|
||||
;[chars[i], chars[j]] = [chars[j], chars[i]]
|
||||
}
|
||||
```
|
||||
Or use a 16-byte array fetched once before the loop:
|
||||
```js
|
||||
const shuffleBuf = new Uint8Array(chars.length)
|
||||
crypto.getRandomValues(shuffleBuf)
|
||||
for (let i = chars.length - 1; i > 0; i--) {
|
||||
const j = shuffleBuf[i] % (i + 1)
|
||||
;[chars[i], chars[j]] = [chars[j], chars[i]]
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `requires_password_change` redirects to `/account` which does not exist
|
||||
|
||||
**File:** `frontend/src/views/auth/LoginView.vue:223-225`
|
||||
**Issue:** When the server returns `requires_password_change: true`, the view routes the user to `/account`. The router (`frontend/src/router/index.js:39`) defines `{ path: '/account', redirect: '/settings' }`. A redirect to `/settings` will trigger the D-09 guard (`isAdmin && !isAdminRoute → redirect to /admin`) for admin users who have `password_must_change=True`. Admin users trapped in this state can never change their password from the UI. For regular users it works but silently relies on an indirect redirect chain, which is fragile.
|
||||
|
||||
**Fix:** Route to `/settings` directly, and verify the Settings view exposes the password-change form to users whose `password_must_change` is true.
|
||||
|
||||
---
|
||||
|
||||
### WR-02: Login redirect guard fires before the token is set in the store
|
||||
|
||||
**File:** `frontend/src/router/index.js:95-114`
|
||||
**Issue:** The guard reads `authStore.accessToken` before the refresh call on line 97. If the refresh succeeds the guard continues. However, the D-09 admin lock-in check on lines 112–114 reads `authStore.user?.role`. If `authStore.refresh()` sets the token asynchronously but the user object is populated from the response in the same tick, there is no race. But if the store's `refresh()` sets only the token and fetches user data in a separate step, the `isAdmin` check on line 105 could be `false` for an admin, allowing them through to a non-admin route for one navigation cycle. This should be verified against `stores/auth.js` but the logic is subtle enough to warrant inspection.
|
||||
|
||||
**Fix:** Ensure `authStore.refresh()` atomically populates both `accessToken` and `user` before the guard's role check, and add a test that covers the reload-as-admin scenario.
|
||||
|
||||
---
|
||||
|
||||
### WR-03: `test_overview_non_admin_forbidden` overrides `get_current_user` not `get_current_admin`
|
||||
|
||||
**File:** `backend/tests/test_admin_overview.py:106-113`
|
||||
**Issue:** The test overrides `get_current_user` with a regular user, then calls `GET /api/admin/overview`. That endpoint depends on `get_current_admin`, not `get_current_user`. The override has no effect — the request will be rejected by `get_current_admin` because there is no authentication credential in the test client, not because of the regular-user role check. The test passes for the wrong reason: it tests unauthenticated rejection, not role-based rejection. The intended invariant (regular user JWT → 403) is untested.
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
@pytest.mark.asyncio
|
||||
async def test_overview_non_admin_forbidden(db_session):
|
||||
from main import app
|
||||
from deps.auth import get_current_admin
|
||||
from fastapi import HTTPException
|
||||
|
||||
regular = await make_regular_user(db_session)
|
||||
app.dependency_overrides[get_db] = lambda: db_session
|
||||
# Simulate a real get_current_admin dep that raises 403 for non-admins
|
||||
def _reject():
|
||||
raise HTTPException(status_code=403, detail="Admin required")
|
||||
app.dependency_overrides[get_current_admin] = _reject
|
||||
|
||||
async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as c:
|
||||
resp = await c.get("/api/admin/overview")
|
||||
|
||||
app.dependency_overrides.clear()
|
||||
assert resp.status_code == 403
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-04: `update_user_quota` (PATCH) does not validate that the target user exists before reading the Quota row
|
||||
|
||||
**File:** `backend/api/admin/quotas.py:53-97`
|
||||
**Issue:** `session.get(Quota, user_id)` succeeds only if a Quota row exists. If the user exists but has no Quota row (possible during partial seed/migration), the endpoint returns 404 "Quota not found" with no indication that the user itself exists. The more dangerous direction: if `user_id` is a valid UUID that does not correspond to any user at all, the same generic 404 is returned. This is technically correct (admin still gets a 404) but makes the API confusing and provides no guard against operating on orphan Quota rows (possible with direct DB manipulation). A pre-flight user lookup would harden the endpoint.
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
user = await session.get(User, user_id)
|
||||
if user is None:
|
||||
raise HTTPException(status_code=404, detail="User not found")
|
||||
quota = await session.get(Quota, user_id)
|
||||
if quota is None:
|
||||
raise HTTPException(status_code=404, detail="Quota not found")
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-05: `create_user` response leaks plaintext email without going through `_user_to_dict`
|
||||
|
||||
**File:** `backend/api/admin/users.py:137-143`
|
||||
**Issue:** The `create_user` endpoint builds its response dict manually instead of calling `_user_to_dict(new_user)`. It happens to omit sensitive fields in this instance, but it diverges from the established single-point-of-truth serialiser. If `_user_to_dict` is later updated (e.g., to encrypt email or add/remove a field), the `create_user` response will silently fall out of sync.
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
await session.commit()
|
||||
await session.refresh(new_user)
|
||||
return _user_to_dict(new_user)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-06: `update_user_status` response builds its own dict and returns plaintext email
|
||||
|
||||
**File:** `backend/api/admin/users.py:200-205`
|
||||
**Issue:** Same pattern as WR-05. The response is hand-built and bypasses `_user_to_dict`. Additionally, this response omits `role`, `totp_enabled`, and other fields the UI may rely on for list refresh, creating an inconsistency between the `GET /users` list shape and the `PATCH /users/{id}/status` response shape.
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
await session.commit()
|
||||
await session.refresh(user)
|
||||
return _user_to_dict(user)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `AdminAiView.vue` imports from both `../../api/client.js` and the wildcard `* as api` from the same file
|
||||
|
||||
**File:** `frontend/src/views/admin/AdminAiView.vue:207-208`
|
||||
**Issue:** Line 207 imports `* as api from '../../api/client.js'` and line 208 imports named exports `{ getAiConfig, saveAiConfig, testAiConnection }` from the same module. The named imports shadow the namespace imports for those three identifiers, meaning two bindings exist for the same value. This is redundant and makes it unclear which form is canonical.
|
||||
|
||||
**Fix:** Remove the named import line and use `api.getAiConfig`, `api.saveAiConfig`, `api.testAiConnection` consistently, or remove the `* as api` import and use only named imports throughout the component.
|
||||
|
||||
---
|
||||
|
||||
### IN-02: `formatMB` in `AdminQuotasView.vue` duplicates shared `formatSize` from `utils/formatters.js`
|
||||
|
||||
**File:** `frontend/src/views/admin/AdminQuotasView.vue:99-102`
|
||||
**Issue:** The view defines its own local `formatMB(bytes)` helper instead of importing `formatSize` from `src/utils/formatters.js`. CLAUDE.md explicitly prohibits defining local `formatSize` variants. The output format differs slightly (always "N MB" vs. the shared helper's adaptive unit), but the duplication violates the shared module rule.
|
||||
|
||||
**Fix:** Import `formatSize` from `../../utils/formatters.js` and either use it directly or derive the MB-only display from it. If MB-only display is intentional, document why and consider adding a `formatMB` export to `formatters.js` so it becomes the one canonical definition.
|
||||
|
||||
---
|
||||
|
||||
### IN-03: `formatTimestamp` in `AdminAuditView.vue` duplicates `formatDate` from `utils/formatters.js`
|
||||
|
||||
**File:** `frontend/src/views/admin/AdminAuditView.vue:322-328`
|
||||
**Issue:** A local `formatTimestamp` function is defined that parses an ISO string and returns a formatted date — the same transformation performed by the shared `formatDate` helper. CLAUDE.md prohibits components from defining their own `formatDate`. The only difference is the output format (`YYYY-MM-DD HH:MM:SS` vs. whatever `formatDate` produces). If the format differs intentionally, `formatDate` should be extended; otherwise replace this function with the shared import.
|
||||
|
||||
**Fix:**
|
||||
```js
|
||||
import { formatDate } from '../../utils/formatters.js'
|
||||
// Replace formatTimestamp(entry.created_at) with formatDate(entry.created_at) in template
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-06-12_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user