docs(13): add phase 13 planning artifacts — virtual local cloud operations
Plans 01–11, CONTEXT, PATTERNS, RESEARCH, REVIEW-FIX, and updated SUMMARY and CONTEXT for the virtual-local-cloud-operations phase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
73328eee0b
commit
b67e77dd69
@@ -0,0 +1,107 @@
|
||||
---
|
||||
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''<percent-encoded>` 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_
|
||||
Reference in New Issue
Block a user