--- phase: 13-virtual-local-cloud-operations fixed_at: 2026-06-23T00:31:15Z review_path: .planning/phases/13-virtual-local-cloud-operations/13-REVIEW.md iteration: 1 findings_in_scope: 7 fixed: 7 skipped: 0 status: all_fixed --- # Phase 13: Code Review Fix Report **Fixed at:** 2026-06-23T00:31:15Z **Source review:** `.planning/phases/13-virtual-local-cloud-operations/13-REVIEW.md` **Iteration:** 1 **Summary:** - Findings in scope: 7 (CR-01 through CR-06, WR-06) - Fixed: 7 - Skipped: 0 --- ## Fixed Issues ### CR-06: preview_cloud_file swallows HTTPException **Files modified:** `backend/api/cloud/operations.py` **Commit:** ebc74f4 **Applied fix:** Changed `except HTTPException: return _unsupported_preview_response(...)` to `except HTTPException: raise` so 401 (credential failure) and 404 (ownership race) are re-raised rather than masked as a 200 unsupported-preview response. The `except Exception` fallthrough for genuine provider errors is unchanged. --- ### CR-01: Content-Disposition header injection via unescaped filename **Files modified:** `backend/api/cloud/operations.py` **Commit:** d4b2697 **Applied fix:** Added `import urllib.parse` at the top of the module and replaced `filename.replace('"', "'")` + bare `filename=` header with `urllib.parse.quote(filename, safe=...)` producing an RFC 6266 `filename*=UTF-8''` header. This prevents injection of newlines, semicolons, and other HTTP header-special characters that the previous single-quote substitution did not cover. --- ### CR-02: Path traversal in WebDAV upload_file and rename **Files modified:** `backend/storage/webdav_backend.py` **Commit:** a1d1c3b **Applied fix:** - Added `PurePosixPath` to the existing `from pathlib import Path` import. - In `upload_file`: applied `PurePosixPath(filename).name` before constructing `object_path`, with a fallback to `"upload"` for empty or dot-only results. Updated the returned `"name"` field to use the sanitized name. - In `rename`: applied the same `PurePosixPath(new_name).name` guard before computing `new_path`, with a fallback to the original `new_name` (caller-validated). Updated the returned `"name"` field to use the sanitized name. --- ### CR-03: Audit log/DB failure after provider upload leaves orphaned upload **Files modified:** `backend/api/cloud/operations.py` **Commit:** 405c7a6 **Applied fix:** Wrapped the entire post-upload DB block (`upsert_cloud_item` + `update_folder_state` + `write_audit_log` + `session.commit()`) in a `try/except Exception` block. On failure, the session is rolled back and a `JSONResponse(207)` with `kind: "provider_success_db_error"` is returned, documenting that the file is on the provider but DocuVault has no metadata record. This makes the failure observable and actionable rather than an unhandled 500. Note: this finding is classified as a logic/correctness concern — requires human verification that the 207 response is appropriate for the frontend conflict-action flow. --- ### CR-04: Successful cloud delete does not soft-delete the CloudItem row **Files modified:** `backend/api/cloud/operations.py` **Commit:** af0de30 **Applied fix:** After `kind == MUT_KIND_DELETED`, added a SQLAlchemy `sa_update(CloudItem).where(...).values(deleted_at=datetime.now(timezone.utc))` targeting `connection_id + provider_item_id + user_id + deleted_at.is_(None)`. This runs before `update_folder_state` and `write_audit_log` in the same transaction, so the soft-delete is committed atomically with the audit row and folder-state invalidation. Queries filtering on `deleted_at.is_(None)` (upload conflict check, preview, download) will no longer see the deleted file. Note: this is a logic/correctness fix — requires human verification that the soft-delete target columns match the CloudItem model. --- ### CR-05: reconnect_connection audit log never committed **Files modified:** `backend/api/cloud/connections.py` **Commit:** 60df855 **Applied fix:** Added `await session.commit()` immediately after `write_audit_log(...)` in the reconnect endpoint, with an explanatory comment. The service's earlier `session.commit()` (persisting credentials) is a separate unit of work; this commit persists the audit row that was written to the session after that earlier commit. --- ### WR-06: testConnection reads wrong field name (state vs status) **Files modified:** `frontend/src/stores/cloudConnections.js` **Commit:** b1a9f43 **Applied fix:** Changed `result?.state ?? 'unknown'` to `result?.status ?? 'unknown'` in `testConnection`. The server's health/test endpoints return `{ status: ... }` — the internal store vocabulary uses `state` but the translation must happen at the store boundary. Added a comment explaining the naming mismatch. --- ## Test Results **Backend cloud tests** (test_cloud_mutations, test_cloud_reconnect, test_cloud_backends, test_cloud_audit): 183 passed, 3 xfailed, 4 warnings — 0 failures. **Frontend store tests** (cloudConnections.test.js): 31 passed — 0 failures. **Full backend suite** (excluding pre-existing `test_extract_docx` missing-module failure): 766 passed, 18 skipped, 4 deselected, 10 xfailed, 65 warnings — 0 failures. The `test_extract_docx` failure is pre-existing (missing `python-docx` module in the local environment) and was failing on the main branch before any of these fixes were applied. --- _Fixed: 2026-06-23_ _Fixer: Claude (gsd-code-fixer)_ _Iteration: 1_