curo1305
9.3 KiB
phase, fixed_at, review_path, iteration, findings_in_scope, fixed, skipped, status
| phase | fixed_at | review_path | iteration | findings_in_scope | fixed | skipped | status |
|---|---|---|---|---|---|---|---|
| 06.2-close-v1-sharing-cloud-delete-csv-export-gaps | 2026-06-01T00:00:00Z | .planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-REVIEW.md | 1 | 15 | 15 | 0 | 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 rawfetch()toreturn request('/api/admin/audit-log/daily-exports')— therequest()helper already has full 401-refresh-retry logic built in.adminExportAuditLogCsvandadminDownloadDailyExport: added a_retryparameter and aif (res.status === 401 && !_retry)block that callsauthStore.refresh()then retries once, or clears auth state and throws'Session expired'on refresh failure — matching the pattern infetchDocumentContent.
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 (nginxproxy_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