diff --git a/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW-FIX.md b/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW-FIX.md new file mode 100644 index 0000000..05e90ef --- /dev/null +++ b/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW-FIX.md @@ -0,0 +1,162 @@ +--- +phase: 06.2-close-v1-sharing-cloud-delete-csv-export-gaps +fixed_at: 2026-06-01T00:00:00Z +review_path: .planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW.md +iteration: 1 +findings_in_scope: 15 +fixed: 15 +skipped: 0 +status: all_fixed +--- + +# Phase 06.2: Code Review Fix Report + +**Fixed at:** 2026-06-01T00:00:00Z +**Source review:** `.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW.md` +**Iteration:** 1 + +**Summary:** +- Findings in scope: 15 (7 Critical, 8 Warning) +- Fixed: 15 +- Skipped: 0 + +--- + +## Fixed Issues + +### CR-01: Audit log event-type filter always returns zero results + +**Files modified:** `backend/api/audit.py` +**Commit:** a3ad36c +**Applied fix:** Changed all three `AuditLog.event_type == event_type` comparisons (in `_build_filtered_query`, `_build_filtered_query_with_handles`, and the inline count query in `list_audit_log`) to `AuditLog.event_type.like(f"{event_type}%")`. This allows the frontend to send category prefixes like `"auth"` and match all dot-namespaced event types like `"auth.login"`, `"auth.logout"`, etc. + +--- + +### CR-02: `download_daily_export` crashes on non-MinIO deployments + +**Files modified:** `backend/api/audit.py` +**Commit:** 50859bb +**Applied fix:** Added `if not isinstance(backend, MinIOBackend): raise HTTPException(status_code=404, detail="Export not found")` immediately after `get_storage_backend()` in `download_daily_export`, mirroring the existing guard in `list_daily_exports`. Non-MinIO deployments now receive a clean 404 instead of an `AttributeError`. + +--- + +### CR-03: CSV export serializes `metadata_` as Python repr + +**Files modified:** `backend/api/audit.py` +**Commit:** 792d463 +**Applied fix:** Added `import json` to imports. In the CSV export loop, replaced the direct `writer.writerow(record)` call with a two-step pattern: build the record dict, then set `record["metadata_"] = json.dumps(record["metadata_"]) if record["metadata_"] is not None else ""` before writing. This produces valid JSON `{"key": "val"}` instead of Python repr `{'key': 'val'}`. + +--- + +### CR-04: Three audit export functions bypass 401-refresh-retry + +**Files modified:** `frontend/src/api/client.js` +**Commit:** 3fa7e8b (combined with WR-05) +**Applied fix:** +- `adminListDailyExports`: converted from raw `fetch()` to `return request('/api/admin/audit-log/daily-exports')` — the `request()` helper already has full 401-refresh-retry logic built in. +- `adminExportAuditLogCsv` and `adminDownloadDailyExport`: added a `_retry` parameter and a `if (res.status === 401 && !_retry)` block that calls `authStore.refresh()` then retries once, or clears auth state and throws `'Session expired'` on refresh failure — matching the pattern in `fetchDocumentContent`. + +--- + +### CR-05: UUID format mismatch in quota SQL in `confirm_upload` + +**Files modified:** `backend/api/documents.py` +**Commit:** 653cb3a +**Applied fix:** Removed both `.replace("-", "")` calls on `str(doc.user_id)` in the atomic quota UPDATE and in the fallback SELECT. PostgreSQL's native `uuid` column type expects dashed UUID format (e.g. `550e8400-e29b-41d4-a716-446655440000`). The undashed 32-hex string was causing unreliable type coercion, making every upload return HTTP 413 quota-exceeded. + +--- + +### CR-06: `Content-Disposition` filename not RFC 5987-encoded + +**Files modified:** `backend/api/documents.py` +**Commit:** 1a34209 +**Applied fix:** Added `import urllib.parse` to imports. Replaced `f'inline; filename="{doc.filename}"'` with `safe_name = urllib.parse.quote(doc.filename, safe='')` followed by `f"inline; filename*=UTF-8''{safe_name}"`. This RFC 5987 extended-value encoding prevents header injection via quotes or CRLF sequences in user-supplied filenames, and correctly handles non-ASCII characters. +**Note:** requires human verification that existing browser clients handle `filename*=UTF-8''` correctly (all modern browsers support RFC 5987). + +--- + +### CR-07: `PATCH /api/shares/{share_id}` writes no audit log + +**Files modified:** `backend/api/shares.py` +**Commit:** 1f2cec9 +**Applied fix:** Added `request: Request` parameter to `update_share_permission`. After `share.permission = body.permission`, added a `write_audit_log` call with `event_type="share.permission_changed"`, `user_id=current_user.id`, `actor_id=current_user.id`, `resource_id=share.document_id`, `ip_address=_ip(request)`, and `metadata_={"share_id": str(share.id), "new_permission": body.permission}`. The `session.commit()` now commits both the share update and the audit log entry atomically. + +--- + +### WR-01: `generateRandomPassword` discards 4 random chars and appends a fixed suffix + +**Files modified:** `frontend/src/components/admin/AdminUsersTab.vue` +**Commit:** 1cba903 +**Applied fix:** Replaced the `pw.slice(0, 12) + 'A1!'` approach with a fully-random positional injection strategy: generate 16 random characters from the 64-char charset (no modulo bias), then inject one guaranteed character from each of the four required classes (uppercase, lowercase, digit, special) at positions 0-3, then Fisher-Yates shuffle using additional random bytes from `crypto.getRandomValues`. All 16 positions carry entropy. + +--- + +### WR-02: `format` query parameter accepted but ignored + +**Files modified:** `backend/api/audit.py` +**Commit:** 683670a +**Applied fix:** Added `Literal` to the `typing` import. Changed `format: str = Query(default="csv")` to `format: Literal["csv"] = Query(default="csv")`. FastAPI now returns HTTP 422 with a validation error if a caller passes `?format=json`, making the parameter self-documenting and preventing silent misuse. + +--- + +### WR-03: Pagination "Next" button enabled when last page is exactly full + +**Files modified:** `frontend/src/components/admin/AuditLogTab.vue` +**Commit:** 2542c81 (combined with WR-04) +**Applied fix:** Changed `:disabled="entries.length < perPage"` to `:disabled="page * perPage >= total"` in the template. Updated `nextPage()` guard from `entries.value.length >= perPage` to `page.value * perPage < total.value`. The `total` ref (already populated from `data.total`) is now the authoritative pagination bound. + +--- + +### WR-04: `loadDailyExports` swallows errors silently + +**Files modified:** `frontend/src/components/admin/AuditLogTab.vue` +**Commit:** 2542c81 (combined with WR-03) +**Applied fix:** Added `exportsError.value = 'Failed to load daily exports. Please try again.'` in the catch block of `loadDailyExports()`. The `exportsError` ref is already bound to a `

` element in the template, so the error will surface to the admin immediately. + +--- + +### WR-05: `URL.revokeObjectURL` called synchronously before download handoff + +**Files modified:** `frontend/src/api/client.js` +**Commit:** 3fa7e8b (combined with CR-04) +**Applied fix:** In both `adminExportAuditLogCsv` and `adminDownloadDailyExport`, replaced `a.click(); URL.revokeObjectURL(url)` with `document.body.appendChild(a); a.click(); document.body.removeChild(a); setTimeout(() => URL.revokeObjectURL(url), 1000)`. The DOM append fixes silent failure in Firefox; the 1-second deferred revoke ensures the OS download manager has completed the handoff before the blob URL is invalidated. + +--- + +### WR-06: `listShares` uses raw template string for query params + +**Files modified:** `frontend/src/api/client.js` +**Commit:** 9e8f8d5 +**Applied fix:** Replaced `` return request(`/api/shares?document_id=${docId}`) `` with `const params = new URLSearchParams({ document_id: docId }); return request(`/api/shares?${params}`)`. Consistent with all other API functions in the file; handles edge-case characters in IDs correctly. + +--- + +### WR-07: `X-Forwarded-For` used as trusted IP without trust-boundary documentation + +**Files modified:** `backend/api/admin.py`, `backend/api/shares.py`, `backend/api/documents.py` +**Commit:** 50b6e7f +**Applied fix:** +- `shares.py`: Expanded the `_ip()` helper docstring with an explicit TRUST BOUNDARY note explaining that X-Forwarded-For is client-controlled and documenting the required production deployment (nginx `proxy_set_header X-Forwarded-For $remote_addr;` or trusted-proxy middleware with CIDR validation). +- `admin.py`: Added an `_ip()` helper function (same docstring) to DRY up the 5 inline occurrences. All 5 `_ip = request.headers.get(...)` lines replaced with `_ip_addr = _ip(request)`. +- `documents.py`: Added inline trust-boundary comments above the 2 direct usages. +**Note:** The actual IP extraction logic is unchanged by design — the deployment-level fix (reverse proxy overwrite) is documented but not implemented in application code, as it requires infrastructure configuration. + +--- + +### WR-08: Split-transaction audit log on document delete + +**Files modified:** `backend/services/storage.py`, `backend/api/documents.py` +**Commit:** 2072c3d +**Applied fix:** Added `auto_commit: bool = True` parameter to `storage.delete_document()`. When `auto_commit=False`, the function skips `await session.commit()`. In `documents.py`, the `delete_document` call now passes `auto_commit=False`, so the subsequent `write_audit_log` and `await session.commit()` run in the same transaction. If the audit log write fails for any reason, the entire transaction (including the document deletion) rolls back atomically — eliminating the gap where the document row was gone but the audit entry was missing. + +--- + +## Skipped Issues + +None — all 15 in-scope findings were fixed. + +--- + +_Fixed: 2026-06-01T00:00:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_