curo/kite
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(06.2): add code review report

Browse Source
This commit is contained in:
curo1305
2026-05-31 20:38:59 +02:00
parent aa957d6c50
commit 97314ce486
1 changed files with 237 additions and 264 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW.md
+237 -264
View File
@@ -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 `<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 — at all three filter call sites (lines 120, 159, 284):
# 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}%"))
```
```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}%"))
**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 a MinIOBackend guard — AttributeError crash on non-MinIO deployments
### CR-02: `download_daily_export` accesses `backend._client` without MinIOBackend guard — AttributeError 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`:
**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
@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"
...
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 writes raw Python `dict` repr for `metadata_` — not valid JSON
### CR-03: CSV export serializes `metadata_` as Python repr — 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
```
**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
# 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)
@@ -141,135 +120,142 @@ for row in rows:
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
### CR-04: Three audit export functions bypass 401-refresh-retry — exports silently break 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.
**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
// Simplest fix for adminListDailyExports — route through request() which handles 401 retry:
// adminListDailyExports — route through request() which has retry built in:
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')
}
// 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')
}
if (!res.ok) throw new Error(`Export failed: ${res.status}`)
}
```
### CR-05: UUID format mismatch in quota SQL — quota enforcement unreliable in `confirm_upload`
**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
**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
**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: Temporary password generation has a fixed suffix that leaks 3 bytes of entropy
### WR-01: `generateRandomPassword` discards 4 random chars and appends a fixed suffix
**File:** `frontend/src/components/admin/AdminUsersTab.vue:301`
**File:** `frontend/src/components/admin/AdminUsersTab.vue:299-302`
**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
**Issue:**
```javascript
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.
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:**
```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('')
}
```
**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 is accepted but ignored — always returns CSV regardless
### WR-02: `format` query parameter on `/audit-log/export` is accepted but ignored — dead parameter
**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.
**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 (if only CSV will ever be supported):** Remove the parameter entirely.
**Fix (if future extension is planned):** Validate at the handler level:
**Fix (simplest):** Remove the parameter. If JSON export is planned for later, add a `Literal["csv"]` constraint:
```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
### WR-03: Pagination "Next" button uses wrong heuristic — breaks 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.
**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: -->
<!-- Template line 137: -->
:disabled="page * perPage >= total"
```
```js
// Script, nextPage():
```javascript
function nextPage() {
if (page.value * perPage < total.value) {
page.value++
@@ -278,77 +264,72 @@ function nextPage() {
}
```
---
### WR-04: `loadDailyExports` swallows errors silently — admin sees "No daily exports" instead of an error
### WR-04: `loadDailyExports` swallows errors silently — admin sees "no exports" instead of error message
**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.
**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:**
```js
} catch (e) {
dailyExports.value = []
exportsError.value = 'Failed to load daily exports. Please try again.' // add
}
```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
### WR-05: `URL.revokeObjectURL` called synchronously before browser initiates download — potential silent cancellation
**File:** `frontend/src/api/client.js:425-426,481-482`
**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.
**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:**
```js
```javascript
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
### WR-06: `listShares` uses raw template string for query params — inconsistent with all other API functions
**File:** `frontend/src/api/client.js:353`
**Issue:**
```js
export function listShares(docId) {
return request(`/api/shares?document_id=${docId}`)
}
```javascript
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.
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:**
```js
```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
**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
**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
@@ -357,68 +338,60 @@ export function listShares(docId) {
**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.
**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:** Either delete `_build_filtered_query` or refactor the inline count query in
`list_audit_log` to use it.
**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
### IN-02: Unused `import re` in `test_documents.py`
**File:** `backend/api/admin.py:101`
**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.
**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
# 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}"
)
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-31T00:00:00Z_
_Reviewed: 2026-05-31T12:00:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_