diff --git a/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW.md b/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW.md index 20b7d7d..be1ba30 100644 --- a/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW.md +++ b/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW.md @@ -1,139 +1,118 @@ --- phase: 06.2-close-v1-sharing-cloud-delete-csv-export-gaps -reviewed: 2026-05-31T00:00:00Z +reviewed: 2026-05-31T12:00:00Z depth: standard -files_reviewed: 9 +files_reviewed: 27 files_reviewed_list: + - backend/api/admin.py - backend/api/audit.py + - backend/api/documents.py + - backend/api/shares.py + - backend/services/storage.py + - backend/tests/test_admin_api.py - backend/tests/test_audit.py + - backend/tests/test_constant_time_auth.py - backend/tests/test_documents.py + - backend/tests/test_quota.py + - backend/tests/test_security.py + - backend/tests/test_security_headers.py - backend/tests/test_shares.py + - backend/tests/test_totp_replay.py - frontend/src/api/client.js - frontend/src/components/admin/AdminUsersTab.vue - frontend/src/components/admin/AuditLogTab.vue + - frontend/src/components/admin/__tests__/AdminAiConfigTab.test.js + - frontend/src/components/admin/__tests__/AdminQuotasTab.test.js + - frontend/src/components/admin/__tests__/AdminUsersTab.test.js + - frontend/src/components/auth/__tests__/PasswordStrengthBar.test.js + - frontend/src/components/documents/DocumentCard.vue + - frontend/src/components/sharing/ShareModal.vue + - frontend/src/stores/__tests__/auth.test.js + - frontend/src/stores/documents.js - frontend/src/views/AccountView.vue - frontend/src/views/CloudFolderView.vue findings: - critical: 4 - warning: 6 - info: 4 - total: 14 + critical: 7 + warning: 8 + info: 5 + total: 20 status: issues_found --- # Phase 06.2: Code Review Report -**Reviewed:** 2026-05-31T00:00:00Z +**Reviewed:** 2026-05-31T12:00:00Z **Depth:** standard -**Files Reviewed:** 9 +**Files Reviewed:** 27 **Status:** issues_found ## Summary -This phase closes v1 gaps across document sharing, cloud delete, and audit/CSV-export -functionality. The backend audit API is well-structured with appropriate admin guards, -parameterized queries, and a whitelist serializer that correctly prevents sensitive field -leakage. The share and document test coverage is comprehensive. However, four critical -bugs were found that either break features silently in production or expose runtime -crashes, plus six warnings covering security hygiene and robustness. +Phase 06.2 closes v1 gaps across document sharing (SHARE-03, SHARE-05), cloud-delete propagation, admin audit log (ADMIN-06), and CSV export. This review covers the full 27-file scope including backend APIs, services, frontend stores and views, and backend/frontend test suites. + +The core security invariants are consistently implemented: every document endpoint asserts `resource.user_id == current_user.id`, the admin whitelist serializer (`_user_to_dict`, `_doc_to_dict`, `_audit_to_dict_with_handles`) correctly excludes sensitive fields, and the sharing IDOR protections (`owner_id == current_user.id`) are in place for both PATCH and DELETE on shares. + +Seven blocker-level issues were found: + +1. **Audit log event-type filter is silently broken** — the frontend sends category prefixes (`"auth"`, `"document"`) but the backend does exact-match against dot-namespaced types (`"auth.login"`, `"document.uploaded"`). Every filter selection returns zero results. +2. **`download_daily_export` crashes on non-MinIO deployments** — no `isinstance` guard before accessing `backend._client`. +3. **CSV export serializes `metadata_` as Python repr** — `csv.DictWriter` calls `str()` on the dict, producing `{'key': val}` instead of valid JSON. +4. **Three audit CSV/download functions bypass the 401-refresh-retry path** — session expiry silently breaks exports without session recovery. +5. **UUID format mismatch in quota SQL** — `confirm_upload` strips dashes from the UUID before the SQL bind parameter, while PostgreSQL expects standard dashed UUID format; quota enforcement is unreliable. +6. **`Content-Disposition` filename is not RFC 5987-encoded** — special characters in user-supplied filenames can inject extra header fields. +7. **`PATCH /api/shares/{share_id}` writes no audit log** — permission escalations on shares are unrecorded. --- ## Critical Issues -### CR-01: AuditLogTab event_type filter always returns zero results — feature non-functional +### CR-01: Audit log event-type filter always returns zero results — feature non-functional **File:** `frontend/src/components/admin/AuditLogTab.vue:37-41` -**Issue:** The filter dropdown submits bare category prefixes (`"auth"`, `"document"`, -`"folder"`, `"share"`, `"admin"`) to the backend. The backend applies exact-equality -filtering (`AuditLog.event_type == event_type` — `backend/api/audit.py:120, 159, 284`). -Every actual event type in the codebase uses dot-notation: -`"auth.login"`, `"auth.logout"`, `"document.uploaded"`, `"document.deleted"`, -`"share.granted"`, `"share.revoked"`, `"admin.user_created"`, `"admin.quota_changed"`, etc. - -An exact match of `"auth"` against `"auth.login"` never fires. Selecting any category -from the dropdown silently returns an empty list instead of filtered results — the entire -filter feature is non-functional. - -**Fix:** Change the backend filter from exact equality to a prefix `LIKE` match (most -flexible) or update the dropdown to send exact event-type strings: +**Issue:** The filter `