docs(06.2): add code review fix report
This commit is contained in:
curo1305
+162
@@ -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 `<p v-if="exportsError">` 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_
|
||||
Reference in New Issue
Block a user