20 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 13-virtual-local-cloud-operations | 2026-06-23T00:00:00Z | standard | 15 |
|
|
issues_found |
Phase 13: Code Review Report
Reviewed: 2026-06-23 Depth: standard Files Reviewed: 15 Status: issues_found
Summary
Phase 13 implements virtual cloud operations: create-folder, rename, move, delete, upload, open, preview, download, connection health, reconnect, and disconnect. The architectural boundaries are well-structured — providers do not write to the DB, credentials never appear in responses, and the typed MUT_KIND_* vocabulary is used consistently. However, six blockers were found: two security-class issues (XSS via Content-Disposition injection and path traversal in filename construction), two correctness blockers (audit log written before commit, missing DB item soft-delete on cloud delete success), and two protocol-correctness issues (reconnect flow does not commit the audit log; Content-Disposition header is missing on the preview response). Eight warnings cover logic gaps and code quality.
Structural Findings (fallow)
No structural pre-pass was provided for this review.
Narrative Findings (AI reviewer)
Critical Issues
CR-01: Content-Disposition header injection via unescaped filename
File: backend/api/cloud/operations.py:364
Issue: The download endpoint builds a Content-Disposition: attachment; filename="<safe_filename>" header by replacing " with '. This is insufficient. A filename containing ;, newline (\r\n), or other HTTP header-special characters can break out of the filename= parameter and inject arbitrary response headers. RFC 6266 requires either filename*=UTF-8''<percent-encoded> encoding, or at minimum stripping all non-printable and non-ASCII characters. The current str.replace('"', "'") is a security measure that is too narrow.
# Current (insufficient — only removes double-quote)
safe_filename = filename.replace('"', "'")
return Response(
...
headers={"Content-Disposition": f'attachment; filename="{safe_filename}"'},
)
# Fix — use RFC 6266 percent-encoding via the standard library
import urllib.parse
encoded = urllib.parse.quote(filename, safe=" !#$&'()*+,-./:;<=>?@[]^_`{|}~")
return Response(
...
headers={"Content-Disposition": f"attachment; filename*=UTF-8''{encoded}"},
)
CR-02: Path traversal in WebDAV upload_file — unsanitized filename injected into path
File: backend/storage/webdav_backend.py:630-631
Issue: upload_file constructs the provider path by concatenating parent_ref and filename directly without sanitizing filename for path traversal characters:
object_path = f"{parent_ref.rstrip('/')}/{filename}"
A filename containing .., /, or %2F (depending on how the WebDAV client handles encoding) can traverse directories above parent_ref. The put_object method correctly uses Path(original_filename).name, but upload_file does not apply the same guard. The service layer currently passes upload_filename = filename or file.filename or "upload" without sanitization. An attacker who controls the filename form field can write files outside the intended parent directory.
Fix:
from pathlib import PurePosixPath
# Strip all path components — keep basename only
safe_filename = PurePosixPath(filename).name
if not safe_filename or safe_filename in ('.', '..'):
safe_filename = 'upload'
object_path = f"{parent_ref.rstrip('/')}/{safe_filename}" if parent_ref else safe_filename
Apply the same guard in WebDAVBackend.rename() when computing new_path from user-supplied new_name (line 512).
CR-03: Audit log written before session.commit() — phantom audit events on transaction rollback
File: backend/api/cloud/operations.py:800-815 (move), lines 904-920 (delete), lines 1066-1082 (upload)
Issue: The pattern in move, delete, and upload is:
await write_audit_log(session, ...) # ← writes to the same session
await session.commit() # ← commits both the mutation and the log
This ordering is safe for those three paths. However, in the upload path (lines 1066-1082), write_audit_log is called inside the if provider_item_id: block and followed by await session.commit() at line 1082 — so if the commit fails (e.g. DB constraint violation after the provider upload succeeded), the audit row is lost and the upsert_cloud_item write also fails. The upload succeeds at the provider level but no metadata or audit record exists in DocuVault. This is a data-loss risk: a file is uploaded to the cloud but DocuVault has no record of it.
More critically, if upsert_cloud_item raises before write_audit_log is reached, the commit never happens and the folder-state invalidation is lost too. The function should be structured so that if any DB operation fails, a compensating action can occur. At minimum, the audit log should not be treated as evidence of success before the commit that records the item.
Fix: Wrap the entire DB mutation block in a try/except with a rollback and return an error result if any DB step fails after the provider operation succeeds. Document the "provider succeeded but DB failed" residual risk.
CR-04: Successful cloud delete does not soft-delete the CloudItem row — orphaned metadata
File: backend/api/cloud/operations.py:888-929
Issue: On a successful adapter.delete() returning MUT_KIND_DELETED, the endpoint calls update_folder_state (to mark the parent as non-fresh) and writes an audit log, but it never marks the CloudItem row as deleted (sets deleted_at). The upsert_cloud_item call is only present for successful creates, renames, and moves — not for deletes.
The result is that after a user deletes a cloud file, the CloudItem row persists with deleted_at IS NULL. The next browse will re-list from the provider (parent folder freshness is downgraded), but until then the item remains in the metadata table. Any query that uses CloudItem.deleted_at.is_(None) (e.g. the upload conflict check, the preview endpoint, the download endpoint) will continue to "see" the deleted file. The upload conflict check at operations.py:980-990 would falsely report a conflict for a file that was just deleted.
Fix: After kind == MUT_KIND_DELETED, soft-delete the row:
from sqlalchemy import update
from db.models import CloudItem
from datetime import datetime, timezone
await session.execute(
update(CloudItem)
.where(
CloudItem.connection_id == connection_id,
CloudItem.provider_item_id == item_id,
CloudItem.user_id == current_user.id,
CloudItem.deleted_at.is_(None),
)
.values(deleted_at=datetime.now(timezone.utc))
)
CR-05: reconnect_connection endpoint writes audit log but does NOT commit it
File: backend/api/cloud/connections.py:644-655
Issue: The reconnect endpoint calls:
result = await _svc_reconnect(...) # commits credentials update inside the service
...
await write_audit_log(session, event_type="cloud.reconnect", ...) # writes audit
# ← NO session.commit() follows
reconnect_connection in services/cloud_operations.py calls session.commit() at line 271 to persist the new credentials. The router then calls write_audit_log on the same session after that commit. However, there is no session.commit() after write_audit_log in the router. The audit row is written to an uncommitted transaction that will be rolled back when the session closes. The audit event for every reconnect is silently lost.
Fix: Add await session.commit() after the write_audit_log call in the reconnect endpoint, or restructure so write_audit_log and the commit happen in the same unit of work.
CR-06: preview_cloud_file swallows HTTPException from _resolve_and_get_adapter and returns 200 with a non-bytes body
File: backend/api/cloud/operations.py:293-300
Issue: Step 4 of preview_cloud_file catches HTTPException from _resolve_and_get_adapter (e.g. the 401 from credential decryption failure or the 404 from a race between the ownership check and the adapter build) and returns an _unsupported_preview_response dict rather than re-raising:
except HTTPException:
# Re-raise 404 from ownership check in _resolve_and_get_adapter ...
return _unsupported_preview_response(connection_id, item_id, "provider_error")
The comment says "Re-raise 404" but the code returns a dict instead of re-raising. This means:
- A 401 credential decryption failure masquerades as
unsupported_previewwith HTTP 200. - A 404 (foreign connection race) silently returns 200 instead of 404, defeating the IDOR guard that was applied in Step 1.
The comment is misleading. Since the ownership check was already done in Step 1, the HTTPException from Step 4 can only be a 401 (credential failure). It should be re-raised or translated to a typed reauth response.
Fix:
try:
conn, adapter = await _resolve_and_get_adapter(session, connection_id, current_user.id)
file_bytes = await adapter.get_object(item_id)
except HTTPException:
raise # re-raise 401/404 rather than masking as unsupported_preview
except Exception:
return _unsupported_preview_response(connection_id, item_id, "provider_error")
Warnings
WR-01: keep_both_name used as conflict-retry suffix but replace action is not implemented server-side
File: backend/api/cloud/operations.py:1092-1098 and frontend/src/views/CloudFolderView.vue:343-374
Issue: The frontend conflict dialog offers both "Keep both" and "Replace" options. For "Keep both", the frontend re-calls uploadCloudFile with conflict_action='keep_both'. For "Replace", it re-calls with conflict_action='replace'. However, the backend upload_cloud_file endpoint reads conflict_action from the form data (line 943 Form(None)) but does nothing with it — the conflict_action parameter is parsed but never used. Both "Keep both" and "Replace" will hit the conflict check again and return another kind: 'conflict' response, creating an infinite conflict loop. The "keep_both" path in the frontend (CloudFolderView.vue:352) also sends the original filename without the counter suffix, so the backend conflict check will detect it again.
Fix: The backend must read conflict_action and branch:
keep_both: skip the cache conflict check (or rename usingkeep_both_name) and upload with a suffixed name.replace: skip the conflict check and calladapter.upload_filedirectly, allowing the provider to overwrite.
WR-02: _attempt_credential_refresh silently stores empty dict on decryption failure — ACTIVE connection with empty credentials
File: backend/services/cloud_operations.py:307-311
Issue: When credentials cannot be decrypted, _attempt_credential_refresh returns {} (empty dict). The caller re-encrypts this empty dict and stores it as credentials_enc, then sets status to ACTIVE. On the next provider call, the credential decryption succeeds (it returns {}), but _dict_to_google_creds or the WebDAV constructor will raise KeyError on d["access_token"] / credentials["username"]. The connection is now permanently broken with ACTIVE status — the user has no indication to reconnect, and health-test or browse will error out cryptically rather than returning requires_reauth.
Fix: Return None on decryption failure and handle it in the caller: if _attempt_credential_refresh returns None, do not commit, and set status to AUTH_FAILED instead of ACTIVE.
WR-03: list_audit_log — missing event_type filter validation for paginated listing route (passes but does nothing on invalid prefix)
File: backend/api/audit.py:96-98
Issue: _validate_event_type is correct — it raises 422 for an event_type not in _VALID_EVENT_PREFIXES. However, the event_type filter in _apply_audit_filters uses AuditLog.event_type.like(f"{event_type}.%") (line 116). This filters for event types that start with event_type + "." but will never match an exact top-level event (e.g. if the stored value is "cloud" rather than "cloud.connected"). If stored events ever use the bare prefix as the event type, the filter silently returns zero rows. The filter is correct for prefixes that always have a dot-separated suffix, but the code has no assertion that stored values always contain a dot. This is a minor correctness concern but could cause confusing empty result sets.
Fix: Document the convention that all audit event types must be prefix.suffix format, and add a DB constraint or application-level assertion to that effect.
WR-04: OneDriveBackend.upload_file — upload session URL never validated for SSRF
File: backend/storage/onedrive_backend.py:737
Issue: The uploadUrl returned by Microsoft Graph for a resumable upload session is sent directly to client.put() without host validation. Although the URL originates from Graph's createUploadSession, it could theoretically be manipulated (DNS rebinding, misconfiguration of the @odata.nextLink-like URL). The WebDAV backend validates destination URLs; the OneDrive backend validates @odata.nextLink hosts (line 401-405), but does not validate the uploadUrl. The risk is low (Graph controls the URL) but is inconsistent with the defense-in-depth stance taken in the WebDAV backend.
Fix: Validate uploadUrl against _GRAPH_HOST before uploading chunks, similar to the nextLink validation on line 401-405.
WR-05: reconnect_connection (service) accepts new_credentials from the router but the router never passes them — dead parameter
File: backend/services/cloud_operations.py:215 and backend/api/cloud/connections.py:637
Issue: reconnect_connection accepts an optional new_credentials: Optional[dict] parameter. The router calls it without providing new_credentials. The parameter is documented as "Optional new credential dict from the caller (e.g. POST body)" but the POST body for the reconnect endpoint carries no credential payload (the endpoint takes no body). The parameter is dead code that could mislead future developers into thinking credential injection through the reconnect endpoint is supported.
Fix: Remove new_credentials from the reconnect service function signature, or add a schema and validation if caller-supplied credentials are intended.
WR-06: testConnection store action reads result?.state but the server returns result?.status
File: frontend/src/stores/cloudConnections.js:253
Issue: The server's health/test endpoints return { status: "healthy" | "degraded" | "auth_failed" | "offline" }. The store's testConnection function reads result?.state (line 253), which will always be undefined. The state passed to setConnectionHealth will be undefined, which then falls through to the 'unknown' default. Health tests never update the displayed state.
// Current — wrong field name
const state = result?.state ?? 'unknown'
// Fix — correct field name from server
const state = result?.status ?? 'unknown'
Note that the server uses status in get_connection_health and test_connection returns, while the store's internal vocabulary uses state. The translation must happen at the store boundary.
WR-07: onFileOpen in CloudFolderView calls downloadCloudFile but ignores the response — file never downloaded
File: frontend/src/views/CloudFolderView.vue:398-401
Issue: When the backend returns kind: 'unsupported_preview', the fallback calls api.downloadCloudFile(...) but never uses the result. The downloadCloudFile API function makes a GET request to the backend's download endpoint, which streams bytes in the response body. The frontend needs to consume those bytes and trigger a browser download (e.g. by creating a Blob URL). Currently the bytes are fetched and immediately discarded, so no file download occurs and the user sees nothing.
Fix:
const blob = await api.downloadCloudFile(connectionId.value, file.provider_item_id)
// Then trigger download:
const url = URL.createObjectURL(blob)
const a = document.createElement('a')
a.href = url
a.download = file.name ?? 'download'
a.click()
URL.revokeObjectURL(url)
This requires downloadCloudFile to return a Blob rather than a parsed JSON response — align the API function accordingly.
WR-08: StorageBrowser.vue onOutsideClick uses .closest('.relative') — overly broad selector closes picker on any click inside any .relative div
File: frontend/src/components/storage/StorageBrowser.vue:838-843
Issue: The folder picker close-on-outside-click handler checks !e.target.closest('.relative') to determine if the click was "inside". The .relative class is a Tailwind utility class applied to many elements in the component (line 401 wraps the Move button in <div class="relative">). A click on any element anywhere in the file list that happens to be inside a relative-positioned div will prevent the picker from closing. This is incorrect: the guard should check for the picker trigger button specifically, not the Tailwind class.
Fix: Give the trigger button a data-test="folder-picker-trigger" attribute and close the picker using !e.target.closest('[data-test="folder-picker-trigger"]'), or maintain a ref to the trigger element.
Info
IN-01: audit.py CSV export — user_email field included but potentially exposes PII without explicit confirmation
File: backend/api/audit.py:56 (_CSV_FIELDS)
Issue: The CSV export includes user_email joined from the User table. Per the CLAUDE.md security protocol, user PII (email) is encrypted at rest. The User.email column referenced in the query (line 149) should be the encrypted column. If the ORM model stores ciphertext in email and plaintext in email_hmac, the CSV will contain ciphertext. If the model stores plaintext (decrypted at load time), the export leaks PII. This needs verification against the User model definition.
IN-02: _validate_event_type raises HTTPException from a pure helper function
File: backend/api/audit.py:96-98
Issue: _validate_event_type raises HTTPException directly. Per the CLAUDE.md code standards: "Service layer raises ValueError (or domain exceptions), never HTTPException. Only the router layer raises HTTPException." This is a pure validation helper used inside _apply_audit_filters which is called by both endpoint handlers and internal helpers. The function is currently only called from router-adjacent code, so the impact is limited, but it violates the layering rule.
IN-03: CloudFolderView.vue — watch([connectionId, folderId]) fires on mount AND onMounted also calls load() — double load on initial render
File: frontend/src/views/CloudFolderView.vue:430-433
Issue: onMounted calls load() (unless redirected to a saved folder). The watch([connectionId, folderId]) is set with the default { immediate: false }, so it does not fire on mount. However, router.push inside navigateTo (line 110) triggers the route change, and then loadFolder is also called explicitly (line 112). When the route watcher fires after the push, it calls load() again with the new folderId. This means navigating to a subfolder triggers two simultaneous requests for the same content — one from loadFolder (line 112) and one from the watcher. The second response will overwrite the first with the same data, which is benign, but wasteful and may cause visible flicker. Consider guarding the watcher to not fire when the navigation was initiated by navigateTo.
Reviewed: 2026-06-23 Reviewer: Claude (gsd-code-reviewer) Depth: standard