curo/kite
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(13): add code review report

Browse Source
This commit is contained in:
curo1305
2026-06-23 00:24:20 +02:00
parent cd5bd82260
commit fcb38c50b1
1 changed files with 317 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/13-virtual-local-cloud-operations/13-REVIEW.md
+317
View File
@@ -0,0 +1,317 @@
---
phase: 13-virtual-local-cloud-operations
reviewed: 2026-06-23T00:00:00Z
depth: standard
files_reviewed: 15
files_reviewed_list:
- backend/api/audit.py
- backend/api/cloud/connections.py
- backend/api/cloud/operations.py
- backend/api/cloud/schemas.py
- backend/services/cloud_operations.py
- backend/storage/cloud_backend_factory.py
- backend/storage/cloud_base.py
- backend/storage/google_drive_backend.py
- backend/storage/onedrive_backend.py
- backend/storage/webdav_backend.py
- frontend/src/api/cloud.js
- frontend/src/components/settings/SettingsCloudTab.vue
- frontend/src/components/storage/StorageBrowser.vue
- frontend/src/stores/cloudConnections.js
- frontend/src/views/CloudFolderView.vue
findings:
critical: 6
warning: 8
info: 3
total: 17
status: 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.
```python
# 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:
```python
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:**
```python
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:
```python
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:
```python
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:
```python
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:
```python
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:
1. A 401 credential decryption failure masquerades as `unsupported_preview` with HTTP 200.
2. 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:**
```python
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 using `keep_both_name`) and upload with a suffixed name.
- `replace`: skip the conflict check and call `adapter.upload_file` directly, 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.
```javascript
// 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:**
```javascript
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_