docs(06.2): update review status after fixes — all 15 CR/WR findings resolved
This commit is contained in:
curo1305
@@ -36,7 +36,8 @@ findings:
|
||||
warning: 8
|
||||
info: 5
|
||||
total: 20
|
||||
status: issues_found
|
||||
status: fixed
|
||||
fixed_at: 2026-06-01T00:00:00Z
|
||||
---
|
||||
|
||||
# Phase 06.2: Code Review Report
|
||||
@@ -68,6 +69,7 @@ Seven blocker-level issues were found:
|
||||
|
||||
### CR-01: Audit log event-type filter always returns zero results — feature non-functional
|
||||
|
||||
**Status:** fixed — commit a3ad36c
|
||||
**File:** `frontend/src/components/admin/AuditLogTab.vue:37-41`
|
||||
|
||||
**Issue:** The filter `<select>` emits bare category strings (`"auth"`, `"document"`, `"folder"`, `"share"`, `"admin"`). The backend applies exact equality (`AuditLog.event_type == event_type`) at `backend/api/audit.py:120, 159, 284`. All actual event types use dot-namespaced format: `"auth.login"`, `"document.uploaded"`, `"share.granted"`, `"admin.user_created"`, etc. An exact match of `"auth"` against `"auth.login"` never fires. Selecting any category silently returns an empty list — the entire filter feature is non-functional.
|
||||
@@ -91,6 +93,7 @@ if event_type is not None:
|
||||
|
||||
### CR-02: `download_daily_export` accesses `backend._client` without MinIOBackend guard — AttributeError on non-MinIO deployments
|
||||
|
||||
**Status:** fixed — commit 50859bb
|
||||
**File:** `backend/api/audit.py:219-228`
|
||||
|
||||
**Issue:** `list_daily_exports` (line 182) correctly guards with `isinstance(backend, MinIOBackend)` and returns empty for non-MinIO storage. `download_daily_export` at line 219 calls `get_storage_backend()` and directly accesses `backend._client` with no type guard. On Google Drive, OneDrive, Nextcloud, or WebDAV storage backends, `_client` does not exist — an `AttributeError` is raised and swallowed by the broad `except Exception` at line 232, returning a misleading 404 "Export not found" to the admin.
|
||||
@@ -105,6 +108,7 @@ key = f"audit-logs/{date}.csv"
|
||||
|
||||
### CR-03: CSV export serializes `metadata_` as Python repr — not valid JSON
|
||||
|
||||
**Status:** fixed — commit 792d463
|
||||
**File:** `backend/api/audit.py:372`
|
||||
|
||||
**Issue:** `csv.DictWriter.writerow()` calls `str()` on values it cannot natively serialize. `entry.metadata_` is a Python `dict` (SQLAlchemy deserializes JSONB to native Python), producing `{'size_bytes': 100}` — Python repr with single quotes — rather than valid JSON `{"size_bytes": 100}`. Any downstream consumer that parses the `metadata_` column as JSON will fail. The test `test_audit_log_export_csv` does not assert on the cell content so this bug passes the test suite.
|
||||
@@ -122,6 +126,7 @@ for row in rows:
|
||||
|
||||
### CR-04: Three audit export functions bypass 401-refresh-retry — exports silently break on token expiry
|
||||
|
||||
**Status:** fixed — commit 3fa7e8b
|
||||
**File:** `frontend/src/api/client.js:398-483`
|
||||
|
||||
**Issue:** `adminExportAuditLogCsv`, `adminListDailyExports`, and `adminDownloadDailyExport` all use raw `fetch()` with no 401-refresh-then-retry logic. When the 15-minute access token expires mid-session, all three functions throw immediately (`Error("Export failed: 401")`) with no session recovery. The auth store is not cleared, so the user cannot distinguish a token expiry from a network error. The `request()` helper (lines 27-30) and `fetchDocumentContent()` (lines 520-529) both implement this retry correctly.
|
||||
@@ -148,6 +153,7 @@ if (res.status === 401 && !options?._retry) {
|
||||
|
||||
### CR-05: UUID format mismatch in quota SQL — quota enforcement unreliable in `confirm_upload`
|
||||
|
||||
**Status:** fixed — commit 653cb3a
|
||||
**File:** `backend/api/documents.py:348-356`
|
||||
|
||||
**Issue:** The atomic quota `UPDATE` in `confirm_upload` strips dashes from the user UUID:
|
||||
@@ -167,6 +173,7 @@ PostgreSQL stores UUIDs in native `uuid` type (dashed format). Binding a 32-hex-
|
||||
|
||||
### CR-06: `Content-Disposition` filename not RFC 5987-encoded — header injection via special characters
|
||||
|
||||
**Status:** fixed — commit 1a34209
|
||||
**File:** `backend/api/documents.py:791`
|
||||
|
||||
**Issue:**
|
||||
@@ -188,6 +195,7 @@ headers = {
|
||||
|
||||
### CR-07: `PATCH /api/shares/{share_id}` writes no audit log — permission escalations unrecorded
|
||||
|
||||
**Status:** fixed — commit 1f2cec9
|
||||
**File:** `backend/api/shares.py:246-270`
|
||||
|
||||
**Issue:** `update_share_permission` changes the effective access level on a document share (e.g. `"view"` → `"edit"`) but writes no audit log entry. Every other share mutation — `grant_share` (logs `share.granted`) and `revoke_share` (logs `share.revoked`) — writes to the audit log. A permission escalation on a high-value document is therefore invisible in the ADMIN-06 audit trail. The endpoint also has no `Request` parameter, so IP address cannot be captured.
|
||||
@@ -223,6 +231,7 @@ async def update_share_permission(
|
||||
|
||||
### WR-01: `generateRandomPassword` discards 4 random chars and appends a fixed suffix
|
||||
|
||||
**Status:** fixed — commit 1cba903
|
||||
**File:** `frontend/src/components/admin/AdminUsersTab.vue:299-302`
|
||||
|
||||
**Issue:**
|
||||
@@ -235,6 +244,7 @@ The 16-char random password is truncated to 12, then `'A1!'` is always appended.
|
||||
|
||||
### WR-02: `format` query parameter on `/audit-log/export` is accepted but ignored — dead parameter
|
||||
|
||||
**Status:** fixed — commit 683670a
|
||||
**File:** `backend/api/audit.py:313`
|
||||
|
||||
**Issue:** `format: str = Query(default="csv")` is declared but the variable `format` is never read in the handler. Any caller passing `?format=json` receives a CSV response with HTTP 200 and no error. This is misleading API design — the parameter should either be used or removed.
|
||||
@@ -246,6 +256,7 @@ format: Literal["csv"] = Query(default="csv"), # noqa: A002
|
||||
|
||||
### WR-03: Pagination "Next" button uses wrong heuristic — breaks when total is exact multiple of page size
|
||||
|
||||
**Status:** fixed — commit 2542c81
|
||||
**File:** `frontend/src/components/admin/AuditLogTab.vue:137,266`
|
||||
|
||||
**Issue:** The "Next" button is disabled when `entries.value.length < perPage`. If the last page has exactly `perPage` entries, the button remains enabled. Clicking it fetches an empty page and leaves the user on a blank audit log view with the page counter incremented. The `total` ref is populated from `data.total` but is never used for pagination control.
|
||||
@@ -266,6 +277,7 @@ function nextPage() {
|
||||
|
||||
### WR-04: `loadDailyExports` swallows errors silently — admin sees "no exports" instead of error message
|
||||
|
||||
**Status:** fixed — commit 2542c81
|
||||
**File:** `frontend/src/components/admin/AuditLogTab.vue:289-299`
|
||||
|
||||
**Issue:** The catch block sets `dailyExports.value = []` but never sets `exportsError.value`. An admin whose MinIO bucket is unreachable sees "No daily exports available" — identical to a legitimately empty bucket — with no error indication.
|
||||
@@ -280,6 +292,7 @@ function nextPage() {
|
||||
|
||||
### WR-05: `URL.revokeObjectURL` called synchronously before browser download begins — potential silent cancellation
|
||||
|
||||
**Status:** fixed — commit 3fa7e8b (combined with CR-04)
|
||||
**File:** `frontend/src/api/client.js:425-426,481-482`
|
||||
|
||||
**Issue:** Both CSV download functions call `a.click()` then immediately `URL.revokeObjectURL(url)`. The `click()` is asynchronous relative to the OS download manager handoff; revoking before the handoff is complete can silently cancel the download on some browser/OS combinations. The `<a>` element is also never appended to the DOM, which causes silent failure in Firefox.
|
||||
@@ -294,6 +307,7 @@ setTimeout(() => URL.revokeObjectURL(url), 1000)
|
||||
|
||||
### WR-06: `listShares` uses raw template string for query params — inconsistent with all other API functions
|
||||
|
||||
**Status:** fixed — commit 9e8f8d5
|
||||
**File:** `frontend/src/api/client.js:353`
|
||||
|
||||
**Issue:**
|
||||
@@ -312,6 +326,7 @@ export function listShares(docId) {
|
||||
|
||||
### WR-07: `X-Forwarded-For` used as trusted client IP without validation — IP spoofing in audit logs
|
||||
|
||||
**Status:** fixed — commit 50b6e7f
|
||||
**File:** `backend/api/admin.py:249,301,411,456,517`, `backend/api/documents.py:379,635`, `backend/api/shares.py:67`
|
||||
|
||||
**Issue:** All audit log IP captures use:
|
||||
@@ -324,6 +339,7 @@ request.headers.get("X-Forwarded-For") or (request.client.host if request.client
|
||||
|
||||
### WR-08: `storage.delete_document` commits inside the service, then `delete_document` API handler commits again — split-transaction audit log risk
|
||||
|
||||
**Status:** fixed — commit 2072c3d
|
||||
**File:** `backend/api/documents.py:654-668` and `backend/services/storage.py:182`
|
||||
|
||||
**Issue:** `storage.delete_document` calls `await session.commit()` at line 182, which ends the transaction. The API handler then calls `write_audit_log` and `await session.commit()` at lines 659-668, which commits in a *separate* transaction. If any statement between the two commits raises an exception, the document row is gone but the audit log entry is never written — a silent gap in the audit trail. This is a transaction atomicity violation.
|
||||
|
||||
Reference in New Issue
Block a user