kite/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW.md

---
phase: 06.2-close-v1-sharing-cloud-delete-csv-export-gaps
reviewed: 2026-05-31T00:00:00Z
depth: standard
files_reviewed: 9
files_reviewed_list:
  - backend/api/audit.py
  - backend/tests/test_audit.py
  - backend/tests/test_documents.py
  - backend/tests/test_shares.py
  - frontend/src/api/client.js
  - frontend/src/components/admin/AdminUsersTab.vue
  - frontend/src/components/admin/AuditLogTab.vue
  - frontend/src/views/AccountView.vue
  - frontend/src/views/CloudFolderView.vue
findings:
  critical: 4
  warning: 6
  info: 4
  total: 14
status: 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:

```python
# 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}%"))
```

```python
# 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`:

```python
@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:
```python
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:**
```python
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:**
```js
// 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!'`:
```js
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:**
```js
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:
```python
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:**
```html
<!-- Template, line 137: -->
:disabled="page * perPage >= total"
```
```js
// 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:**
```js
  } 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`:
```js
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:**
```js
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:**
```js
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:**
```js
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:**
```js
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:**
```python
    # 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_