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-31T12:00:00Z
depth: standard
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: 7
  warning: 8
  info: 5
  total: 20
status: fixed
fixed_at: 2026-06-01T00:00:00Z
---

# Phase 06.2: Code Review Report

**Reviewed:** 2026-05-31T12:00:00Z
**Depth:** standard
**Files Reviewed:** 27
**Status:** issues_found

## Summary

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: 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.

**Fix (preferred — change backend to prefix match):**
```python
# backend/api/audit.py — lines 120, 159, and the count_q block at 283-284:
if event_type is not None:
    q = q.where(AuditLog.event_type.like(f"{event_type}%"))
```

**Fix (alternative — use exact event-type strings in the frontend):**
```html
<option value="auth.login">Login</option>
<option value="document.uploaded">Document uploaded</option>
<option value="document.deleted">Document deleted</option>
<option value="share.granted">Share granted</option>
<option value="share.revoked">Share revoked</option>
<option value="admin.user_created">Admin: user created</option>
```

### 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.

**Fix:**
```python
backend = get_storage_backend()
if not isinstance(backend, MinIOBackend):
    raise HTTPException(status_code=404, detail="Export not found")
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.

**Fix:**
```python
import json

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)
```

### 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.

**Fix:**
```js
// adminListDailyExports — route through request() which has retry built in:
export function adminListDailyExports() {
  return request('/api/admin/audit-log/daily-exports')
}

// adminExportAuditLogCsv and adminDownloadDailyExport — add after the fetch() call:
if (res.status === 401 && !options?._retry) {
  try {
    await authStore.refresh()
    return adminExportAuditLogCsv(params)  // retry once
  } catch {
    authStore.accessToken = null
    authStore.user = null
    throw new Error('Session expired')
  }
}
```

### 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:
```python
{"delta": size, "uid": str(doc.user_id).replace("-", "")}
```
PostgreSQL stores UUIDs in native `uuid` type (dashed format). Binding a 32-hex-char undashed string against a `uuid`-typed column via `text()` produces inconsistent type coercion behavior across psycopg driver versions. In contrast, `services/storage.py:178` passes the UUID with dashes (no `.replace("-", "")`). If the quota row is not found by the UPDATE, `row` is `None` and every confirm call returns HTTP 413 (quota exceeded) even when the user has available quota — making all uploads fail.

**Fix:**
```python
# Line 348 — remove .replace("-", ""):
{"delta": size, "uid": str(doc.user_id)}

# Line 356 — same fix:
{"uid": str(doc.user_id)}
```

### 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:**
```python
"content-disposition": f'inline; filename="{doc.filename}"',
```
`doc.filename` is user-supplied and stored verbatim. A filename containing `"` or `\r\n` can inject additional HTTP header fields. The filename validator at line 86-89 only blocks `/` and `\` — it does not block quotes or CRLF sequences. RFC 5987 encoding is required for non-ASCII and special-character filenames.

**Fix:**
```python
import urllib.parse
safe_name = urllib.parse.quote(doc.filename, safe='')
headers = {
    ...
    "content-disposition": f"inline; filename*=UTF-8''{safe_name}",
    ...
}
```

### 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.

**Fix:**
```python
@router.patch("/{share_id}", status_code=200)
async def update_share_permission(
    share_id: str,
    body: SharePermissionPatch,
    request: Request,                          # add
    session: AsyncSession = Depends(get_db),
    current_user: User = Depends(get_regular_user),
) -> dict:
    ...
    share.permission = body.permission

    await write_audit_log(
        session=session,
        event_type="share.permission_changed",
        user_id=current_user.id,
        actor_id=current_user.id,
        resource_id=share.document_id,
        ip_address=_ip(request),
        metadata_={"share_id": str(share.id), "new_permission": body.permission},
    )
    await session.commit()
```

---

## Warnings

### 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:**
```javascript
pw = pw.slice(0, 12) + 'A1!'
```
The 16-char random password is truncated to 12, then `'A1!'` is always appended. The last 3 characters carry zero entropy. A brute-force attacker who knows the generation algorithm needs to search only 12 random positions, not 15. These passwords protect accounts between admin creation and first login.

**Fix:** Replace the fixed-suffix approach with positional injection of required character classes within the random portion, keeping all positions random. See also note: the charset length is 64, so `256 % 64 == 0` — no modulo bias — but the truncation from 16 to 12 chars before appending is still an entropy loss.

### 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.

**Fix (simplest):** Remove the parameter. If JSON export is planned for later, add a `Literal["csv"]` constraint:
```python
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.

**Fix:**
```html
<!-- Template line 137: -->
:disabled="page * perPage >= total"
```
```javascript
function nextPage() {
  if (page.value * perPage < total.value) {
    page.value++
    fetchLog()
  }
}
```

### 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.

**Fix:**
```javascript
} catch (e) {
  dailyExports.value = []
  exportsError.value = 'Failed to load daily exports. Please try again.'
}
```

### 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.

**Fix:**
```javascript
document.body.appendChild(a)
a.click()
document.body.removeChild(a)
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:**
```javascript
return request(`/api/shares?document_id=${docId}`)
```
All other functions in this file use `URLSearchParams`. While `docId` is always a UUID in practice (low injection risk), this is inconsistent and fragile. The pattern would break if `docId` ever contained `+`, `&`, or `=`.

**Fix:**
```javascript
export function listShares(docId) {
  const params = new URLSearchParams({ document_id: docId })
  return request(`/api/shares?${params}`)
}
```

### 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:
```python
request.headers.get("X-Forwarded-For") or (request.client.host if request.client else None)
```
`X-Forwarded-For` is a client-controlled header. Any actor can forge it: `X-Forwarded-For: 127.0.0.1`. This allows an attacker to record any IP address in the audit log for their actions, defeating one of the audit trail's primary forensic values.

**Fix:** Deploy a reverse proxy that overwrites `X-Forwarded-For` with the real remote IP before it reaches FastAPI (e.g. nginx `proxy_set_header X-Forwarded-For $remote_addr;`), or use a trusted-proxy middleware that only reads the header when the request originates from a known proxy CIDR. Document this deployment requirement prominently.

### 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.

**Fix:** Move the audit log write into `storage.delete_document`, or refactor `storage.delete_document` to not commit internally (let the caller control commit boundaries, passing `auto_commit=False`).

---

## 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 the COUNT-query helper to avoid JOIN ambiguity, but `list_audit_log` manually re-implements the same filter logic inline (lines 276-284) without calling this function. It is never referenced anywhere in the file.

**Fix:** Delete `_build_filtered_query`, or refactor the inline count query in `list_audit_log` to use it.

### IN-02: `UserCreate.role` accepts arbitrary strings — no allowlist validation

**File:** `backend/api/admin.py:101`

**Issue:** `role: str = "user"` accepts any string. An admin can inadvertently create a user with `role="superuser"` or any future privileged role string. If a new role is added later, the API silently accepts it before guards are updated.

**Fix:**
```python
from typing import Literal

class UserCreate(BaseModel):
    handle: str
    email: EmailStr
    password: str
    role: Literal["user", "admin"] = "user"
```

### IN-03: `import re` unused in `test_documents.py`

**File:** `backend/tests/test_documents.py:9`

**Issue:** `import re` is present but `re` is never used in the test file.

**Fix:** Remove the import.

### IN-04: `initiate_password_reset` writes no audit log

**File:** `backend/api/admin.py:330-359`

**Issue:** All other admin operations log an audit entry. `initiate_password_reset` does not record which admin triggered a reset for which user, making it impossible to investigate suspicious reset activity post-incident. This is an ADMIN-03 gap.

**Fix:** Add `write_audit_log` with `event_type="admin.password_reset_initiated"`, `user_id=user.id`, `actor_id=_admin.id`. This requires also adding `request: Request` as a parameter.

### IN-05: `test_delete_cloud_remove_only` does not assert quota is unchanged

**File:** `backend/tests/test_documents.py:897-925`

**Issue:** The test verifies the DB row is deleted but does not verify that `used_bytes` was not decremented. Cloud documents are not quota-tracked; a future regression that incorrectly decrements quota on the `remove_only` path would go undetected.

**Fix:**
```python
from db.models import Quota
quota = await db_session.get(Quota, auth_user["user"].id)
assert quota.used_bytes == 0, (
    f"remove_only must not decrement quota, got used_bytes={quota.used_bytes}"
)
```

---

_Reviewed: 2026-05-31T12:00:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_