curo1305
15 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 06.2-close-v1-sharing-cloud-delete-csv-export-gaps | 2026-05-31T00:00:00Z | standard | 9 |
|
|
issues_found |
Phase 06.2: Code Review Report
Reviewed: 2026-05-31T00:00:00Z Depth: standard Files Reviewed: 9 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.
Critical Issues
CR-01: AuditLogTab 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:
# backend/api/audit.py — at all three filter call sites (lines 120, 159, 284):
if event_type is not None:
q = q.where(AuditLog.event_type.like(f"{event_type}%"))
# Also fix the count_q inline block in list_audit_log (lines 283-284):
if event_type is not None:
count_q = count_q.where(AuditLog.event_type.like(f"{event_type}%"))
CR-02: download_daily_export accesses backend._client without a MinIOBackend guard — AttributeError crash on non-MinIO deployments
File: backend/api/audit.py:219-228
Issue: list_daily_exports (line 182) correctly checks
isinstance(backend, MinIOBackend) and returns an empty list for non-MinIO storage.
download_daily_export (line 219) calls get_storage_backend() and immediately uses
backend._client with no type guard. On deployments where the primary storage backend
is Google Drive, OneDrive, or Nextcloud, backend._client does not exist on those
classes, raising AttributeError. The broad except Exception on line 232 catches
this and re-raises it as a 404, masking a real misconfiguration entirely — the admin
sees "Export not found" when the real problem is a storage type mismatch.
Fix: Mirror the guard from list_daily_exports:
@router.get("/audit-log/daily-exports/{date}")
async def download_daily_export(
date: str,
_admin: User = Depends(get_current_admin),
) -> StreamingResponse:
if not re.fullmatch(r"\d{4}-\d{2}-\d{2}", date):
raise HTTPException(status_code=404, detail="Invalid date format")
backend = get_storage_backend()
if not isinstance(backend, MinIOBackend): # <-- add this guard
raise HTTPException(status_code=404, detail="Export not found")
key = f"audit-logs/{date}.csv"
...
CR-03: CSV export writes raw Python dict repr for metadata_ — not valid JSON
File: backend/api/audit.py:372
Issue: csv.DictWriter.writerow() receives the dict returned by
_audit_to_dict_with_handles(), which includes "metadata_": entry.metadata_ where
entry.metadata_ is a Python dict (SQLAlchemy deserializes JSONB columns to native
Python objects). csv.DictWriter calls str() on values it cannot serialize natively,
producing Python repr syntax with single quotes ({'size_bytes': 100}) rather than
valid JSON ({"size_bytes": 100}). Any downstream system that tries to parse the
metadata_ CSV column as JSON will fail.
Reproduced:
import csv, io
output = io.StringIO()
writer = csv.DictWriter(output, fieldnames=["metadata_"])
writer.writeheader()
writer.writerow({"metadata_": {"size_bytes": 100}})
# output: "{'size_bytes': 100}" ← Python repr, NOT JSON
Fix:
import json
# In export_audit_log(), at the writerow call site (line 372):
for row in rows:
entry, user_handle_val, actor_handle_val = row[0], row[1], row[2]
record = _audit_to_dict_with_handles(entry, user_handle_val, actor_handle_val)
record["metadata_"] = json.dumps(record["metadata_"]) if record["metadata_"] is not None else ""
writer.writerow(record)
The test_audit_log_export_csv test does not assert the content of the metadata cell
so this bug currently passes all tests while corrupting exported data.
CR-04: Three admin fetch functions lack 401-refresh-retry — silently fail on token expiry
File: frontend/src/api/client.js:398-483
Issue: adminExportAuditLogCsv, adminListDailyExports, and
adminDownloadDailyExport all use raw fetch() calls without the 401-refresh-then-retry
logic that request() (line 27-30) and fetchDocumentContent() (lines 520-529) both
implement. When the 15-minute access token expires while an admin is on the audit log
page, all three functions immediately throw (e.g. Error("Export failed: 401")) with
no session recovery. The user's session is still valid (they have a live refresh token)
but the export/list operations fail hard. The auth store is not cleared, so the user
cannot tell why the operation failed.
Note: adminListDailyExports returns JSON and could be routed through request()
directly, which already has the retry logic.
Fix:
// Simplest fix for adminListDailyExports — route through request() which handles 401 retry:
export function adminListDailyExports() {
return request('/api/admin/audit-log/daily-exports')
}
// For CSV-returning functions, add the same retry block as fetchDocumentContent:
// In adminExportAuditLogCsv and adminDownloadDailyExport, replace:
// if (!res.ok) throw new Error(...)
// with:
if (res.status === 401) {
try {
await authStore.refresh()
return adminExportAuditLogCsv(params) // retry
} catch {
authStore.accessToken = null
authStore.user = null
throw new Error('Session expired')
}
}
if (!res.ok) throw new Error(`Export failed: ${res.status}`)
Warnings
WR-01: Temporary password generation has a fixed suffix that leaks 3 bytes of entropy
File: frontend/src/components/admin/AdminUsersTab.vue:301
Issue: generateRandomPassword() generates 16 bytes of random data mapped through
a 65-character charset, then unconditionally overwrites the last 4 characters with the
literal 'A1!':
pw = pw.slice(0, 12) + 'A1!'
The generated password is always exactly 15 characters with the last 3 bytes fixed
(A, 1, !). An attacker who reverse-engineers this algorithm can reduce the
brute-force search space significantly — the last 3 positions are always known. There is
also modulo bias: 256 % 65 = 61, meaning the first 61 characters of the charset are
slightly overrepresented (each maps from 4 byte values instead of 3). While these
temporary passwords must be changed on first login, they are the sole credential
protecting accounts between creation and first use.
Fix:
function generateRandomPassword() {
const upper = 'ABCDEFGHJKLMNPQRSTUVWXYZ'
const lower = 'abcdefghijkmnpqrstuvwxyz'
const digits = '23456789'
const special = '!@#$%^&*'
const all = upper + lower + digits + special // 58 chars — 256 % 58 = 24, bias reduced
const arr = new Uint32Array(16)
crypto.getRandomValues(arr)
let chars = Array.from(arr.slice(0, 12), v => all[v % all.length])
// Guarantee one char from each required class by replacing 4 specific positions
const positions = new Uint32Array(4)
crypto.getRandomValues(positions)
chars[positions[0] % 12] = upper[positions[0] % upper.length]
chars[positions[1] % 12] = lower[positions[1] % lower.length]
chars[positions[2] % 12] = digits[positions[2] % digits.length]
chars[positions[3] % 12] = special[positions[3] % special.length]
return chars.join('')
}
WR-02: format query parameter is accepted but ignored — always returns CSV regardless
File: backend/api/audit.py:313
Issue: The export_audit_log endpoint declares format: str = Query(default="csv")
but the variable format is never read in the handler body. Passing ?format=json
silently returns a CSV response. The parameter is either dead code or a future extension
stub. Either way the current behaviour is incorrect: the API contract implies multiple
formats are accepted when only CSV exists. Any string passes FastAPI's validation,
meaning callers receive misleading 200 OK responses for unsupported format values.
Fix (if only CSV will ever be supported): Remove the parameter entirely.
Fix (if future extension is planned): Validate at the handler level:
from typing import Literal
format: Literal["csv"] = Query(default="csv"), # noqa: A002
WR-03: Pagination "Next" button uses wrong heuristic — breaks on last page when total is exact multiple of page size
File: frontend/src/components/admin/AuditLogTab.vue:137,266
Issue: The "Next" button is disabled when entries.value.length < perPage (line 137
in template, line 266 in script). If the last page contains exactly perPage (50)
entries, the button stays enabled. Clicking it dispatches a page request that returns an
empty items list, leaving the user on a blank page with the page counter incremented. The
total ref is populated from data.total (line 227) but is never used for pagination.
Fix:
<!-- Template, line 137: -->
:disabled="page * perPage >= total"
// Script, nextPage():
function nextPage() {
if (page.value * perPage < total.value) {
page.value++
fetchLog()
}
}
WR-04: loadDailyExports swallows errors silently — admin sees "No daily exports" instead of an error
File: frontend/src/components/admin/AuditLogTab.vue:289-299
Issue: The loadDailyExports catch block sets dailyExports.value = [] but does
not set exportsError.value. The template renders <p v-if="exportsError"> for
download errors, but load failures produce no user-visible message. An admin whose MinIO
is misconfigured sees "No daily exports available" — identical to a legitimately empty
bucket — with no indication that an error occurred.
Fix:
} catch (e) {
dailyExports.value = []
exportsError.value = 'Failed to load daily exports. Please try again.' // add
}
WR-05: URL.revokeObjectURL called synchronously before browser initiates download — potential silent cancellation
File: frontend/src/api/client.js:425-426, 481-482
Issue: In both adminExportAuditLogCsv and adminDownloadDailyExport:
a.click()
URL.revokeObjectURL(url) // immediate — race condition
a.click() triggers the download asynchronously. Revoking the blob URL before the
browser has transferred it to the OS download manager can silently cancel the download
on slow or memory-constrained devices (Chrome has a documented race here; this is a
known pattern issue). The anchor element is also not appended to the DOM (document.body.appendChild(a))
which causes the download to fail entirely in some Firefox versions.
Fix:
document.body.appendChild(a)
a.click()
document.body.removeChild(a)
setTimeout(() => URL.revokeObjectURL(url), 1000)
WR-06: listShares uses raw string interpolation in query string — inconsistent and fragile
File: frontend/src/api/client.js:353
Issue:
export function listShares(docId) {
return request(`/api/shares?document_id=${docId}`)
}
docId is always a UUID in normal use, but it is not encoded via URLSearchParams.
All other query-parameter functions in this file (listDocuments, adminListAuditLog,
listFolders) use URLSearchParams consistently. While the practical risk from a UUID
is low, the pattern is inconsistent and breaks if the value ever contains special
characters.
Fix:
export function listShares(docId) {
const params = new URLSearchParams({ document_id: docId })
return request(`/api/shares?${params}`)
}
Info
IN-01: _build_filtered_query is defined but never called — dead code
File: backend/api/audit.py:97-121
Issue: _build_filtered_query is documented as providing the COUNT query helper
(to avoid multi-column JOIN ambiguity), but list_audit_log (lines 276-284) manually
re-implements identical filter logic inline without calling this helper. The function
is never referenced anywhere in the file. It is dead code that will confuse maintainers.
Fix: Either delete _build_filtered_query or refactor the inline count query in
list_audit_log to use it.
IN-02: Unused import re in test_documents.py
File: backend/tests/test_documents.py:9
Issue: import re is present at line 9 but re. is never used anywhere in the
module. Dead import from a previous version of the test file.
Fix: Remove import re from line 9.
IN-03: deleteDocumentRemoveOnly is a trivial one-liner wrapper — unnecessary exported symbol
File: frontend/src/api/client.js:80-82
Issue:
export function deleteDocumentRemoveOnly(id) {
return deleteDocument(id, true)
}
This adds a second exported name for deleteDocument(id, true) with no additional
behaviour. Having two symbols for the same operation creates confusion about which to
use. The wrapper itself provides no documentation, type safety, or default-arg isolation.
Fix: Remove the wrapper and call deleteDocument(id, true) directly at callsites,
or keep only the wrapper and make removeOnly an implementation detail.
IN-04: test_delete_cloud_remove_only does not assert quota is unchanged after DB-only removal
File: backend/tests/test_documents.py:897-925
Issue: The test verifies the DB row is removed but does not verify that the user's
used_bytes quota was not decremented. Cloud documents are not quota-tracked, so a
future regression that incorrectly decrements quota on remove_only paths would go
undetected by this test.
Fix:
# Add after the deleted-row assertion:
from db.models import Quota
quota = await db_session.get(Quota, auth_user["user"].id)
# Cloud docs are not quota-tracked — used_bytes must remain 0
assert quota.used_bytes == 0, (
f"remove_only should not decrement quota, got used_bytes={quota.used_bytes}"
)
Reviewed: 2026-05-31T00:00:00Z Reviewer: Claude (gsd-code-reviewer) Depth: standard