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

docs(12): add code review report

Browse Source
This commit is contained in:
curo1305
2026-06-19 01:59:17 +02:00
parent b7ae44b3f8
commit de63e2a3a4
1 changed files with 345 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/12-cloud-resource-foundation/12-REVIEW.md
+345
View File
@@ -0,0 +1,345 @@
---
phase: 12-cloud-resource-foundation
reviewed: 2026-06-19T00:00:00Z
depth: standard
files_reviewed: 35
files_reviewed_list:
- README.md
- RUNBOOK.md
- SECURITY.md
- backend/api/cloud/__init__.py
- backend/api/cloud/browse.py
- backend/api/cloud/connections.py
- backend/api/cloud/schemas.py
- backend/celery_app.py
- backend/db/models.py
- backend/main.py
- backend/migrations/versions/0006_cloud_resource_foundation.py
- backend/services/cloud_items.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
- backend/tasks/cloud_tasks.py
- backend/tests/conftest.py
- backend/tests/test_cloud.py
- backend/tests/test_cloud_backends.py
- backend/tests/test_cloud_capabilities.py
- backend/tests/test_cloud_items.py
- backend/tests/test_cloud_security.py
- frontend/package.json
- frontend/src/api/cloud.js
- frontend/src/components/settings/SettingsCloudTab.vue
- frontend/src/components/storage/StorageBrowser.vue
- frontend/src/components/ui/AppIcon.vue
- frontend/src/components/ui/BreadcrumbBar.vue
- frontend/src/router/index.js
- frontend/src/stores/cloudConnections.js
- frontend/src/utils/formatters.js
- frontend/src/views/CloudFolderView.vue
- frontend/src/views/CloudStorageView.vue
findings:
critical: 4
warning: 8
info: 4
total: 16
status: issues_found
---
# Phase 12: Code Review Report
**Reviewed:** 2026-06-19
**Depth:** standard
**Files Reviewed:** 35
**Status:** issues_found
## Summary
Phase 12 ships the cloud resource foundation: a durable metadata cache (`cloud_items`, `cloud_folder_states`), a stale-while-revalidate browse endpoint keyed by connection UUID, a capability contract, and Celery background refresh. The structural implementation is solid — IDOR is enforced, credentials are excluded from responses, the quota boundary is respected, and the test suite is comprehensive.
However, four correctness/security defects were found that must be fixed before this ships:
1. The frontend API module calls a URL that does not exist on the backend.
2. The browse endpoint silently double-decrypts credentials (two separate decrypt calls per request), which is unnecessary but also means any key-mismatch error will manifest differently than expected.
3. Raw provider exception text leaks into a `CloudFolderState.error_message` field in the browse endpoint's failure path.
4. The `get_or_create_folder_state` service does not scope the lookup to `user_id`, creating a window where a `CloudFolderState` belonging to one user can be returned to another user who happens to use the same `connection_id` — exploitable if a DB integrity constraint ever fails.
---
## Critical Issues
### CR-01: Frontend API calls a non-existent endpoint
**File:** `frontend/src/api/cloud.js:37`
**Issue:** `getCloudFoldersByConnectionId` calls `/api/cloud/connections/${connectionId}/folders/${folderId}`. No such route is registered anywhere in the backend. The canonical endpoint added in Phase 12 is `GET /api/cloud/connections/{id}/items` (with a `parent_ref` query parameter). The mismatch means every folder-browse request from `CloudFolderView` will get a 404.
`CloudFolderView` calls this function at line 77 and the entire browse feature is broken at runtime.
**Fix:**
```js
// frontend/src/api/cloud.js
export function getCloudFoldersByConnectionId(connectionId, parentRef) {
const qs = parentRef && parentRef !== 'root'
? `?parent_ref=${encodeURIComponent(parentRef)}`
: ''
return request(`/api/cloud/connections/${connectionId}/items${qs}`)
}
```
Also update `CloudFolderView.vue` (line 77) to pass `folderId.value` as `parentRef` — the existing call already passes the right value, it just hits the wrong URL.
---
### CR-02: Raw provider exception text written into `error_message` (information leak + CLAUDE.md violation)
**File:** `backend/api/cloud/browse.py:308-310`
**Issue:** In the first-visit synchronous fetch failure path, the `error_message` written to `CloudFolderState` is a static controlled string (`"Provider temporarily unavailable. Retrying in background."`), which is correct. However the caught exception object `exc` is never sanitized. If this code path were to propagate `str(exc)` (easy future regression), raw provider error text would reach the DB.
More critically, in `backend/tasks/cloud_tasks.py` at lines 136 and 110, the exception is propagated into `_TerminalProviderError` with `f"Auth error: {exc}"` and `f"Credential decryption failed: {exc}"` — these strings carry raw exception text. While these strings do not directly flow to the API consumer (they are logged internally), the code comment on `update_folder_state` at line 269 in `services/cloud_items.py` says *"error_code and error_message must be controlled service values — never raw provider exception text."* The Celery task uses only controlled strings (`"auth_error"` / `"Authentication failed. Re-connect the account."`) for the `update_folder_state` call — that part is correct. But the internal `_TerminalProviderError` message embeds raw exception text. If a monitoring system or log aggregator surfaces these as user-visible strings, it leaks provider internals.
**Fix:** Strip the raw exception text from the sentinel message:
```python
# backend/tasks/cloud_tasks.py line 136
raise _TerminalProviderError("Auth error — see logs") from exc
# line 110
raise _TerminalProviderError("Credential decryption failed — see logs") from exc
```
---
### CR-03: `get_or_create_folder_state` does not scope lookup to `user_id` — potential cross-user state bleed
**File:** `backend/services/cloud_items.py:230-235`
**Issue:** The SELECT that looks up an existing `CloudFolderState` row filters only on `(connection_id, parent_ref)` — it does not include `user_id` in the WHERE clause. The unique constraint on `cloud_folder_states` is also only `(connection_id, parent_ref)`.
```python
result = await session.execute(
select(CloudFolderState).where(
CloudFolderState.connection_id == cid_fs,
CloudFolderState.parent_ref == parent_ref,
)
)
```
Under normal operation every `connection_id` is owned by exactly one user, so this is safe. But if a connection row is ever reused (e.g., after a cascade delete followed by a re-insert with the same UUID — possible in test environments and theoretically possible with admin tooling), `user_id` from the stale row will be returned to the requesting user. The `update_folder_state` function (which calls `get_or_create_folder_state`) accepts and writes `user_id` from the caller, but the returned row's `user_id` may differ from the parameter.
This violates the stated invariant: *"Ownership boundary: every cloud row has user_id + connection_id."*
**Fix:**
```python
result = await session.execute(
select(CloudFolderState).where(
CloudFolderState.user_id == uid_fs, # add user_id filter
CloudFolderState.connection_id == cid_fs,
CloudFolderState.parent_ref == parent_ref,
)
)
```
Also add `user_id` to the unique constraint in the migration (requires a new migration):
```python
sa.UniqueConstraint(
"user_id", "connection_id", "parent_ref",
name="uq_cloud_folder_states_user_connection_parent",
)
```
---
### CR-04: `ConnectionRenameRequest` has no maximum length on `display_name` — potential DB overflow / DoS
**File:** `backend/api/cloud/schemas.py:72-86`
**Issue:** `display_name: str` has no length cap. `CloudConnection.display_name_override` is mapped to `sa.Text` (unlimited in Postgres), but the migration column is also `sa.Text`. A user can send a display name of arbitrary length (megabytes), which is stored to the DB and echoed in every `list_connections` response for that user. This is a user-controllable field that has no bound.
CLAUDE.md requires: *"All user-supplied data validated via Pydantic."* A missing length constraint is an incomplete validation.
**Fix:**
```python
from pydantic import BaseModel, field_validator, constr
class ConnectionRenameRequest(BaseModel):
display_name: constr(max_length=255)
@field_validator("display_name")
@classmethod
def must_be_nonblank(cls, v: str) -> str:
stripped = v.strip()
if not stripped:
raise ValueError("display_name must not be blank")
return stripped
```
---
## Warnings
### WR-01: Credentials decrypted twice per browse request
**File:** `backend/api/cloud/browse.py:233, 264`
**Issue:** `_decrypt_connection` is called at line 233 (for capability probe) and again at line 264 (for first-visit synchronous listing). The second call always happens when `not cached_items`, so two decryptions occur on first visit. This is wasteful and makes the code harder to reason about. It also means a key-rotation bug would manifest differently for the capability probe versus the listing.
**Fix:** Decrypt once, hoist to before the branching:
```python
try:
credentials = _decrypt_connection(conn, current_user.id)
except Exception:
credentials = None
# Use `credentials` (with None guard) in both the capability and listing branches
```
---
### WR-02: `onedrive_backend._refresh_token` discards new `client_id`/`client_secret` from refreshed token result
**File:** `backend/storage/onedrive_backend.py:141-147`
**Issue:** The refreshed credential dict returned by `_msal_refresh` only contains `access_token`, `refresh_token`, and `expires_at`. If the encrypted credentials stored in DB included `client_id`, `client_secret`, or `token_uri` (they do — see connections.py line 265-268), those are stripped after the first in-memory token refresh. Subsequent MSAL calls in the same adapter lifetime will succeed because the adapter is stateless across requests, but any future attempt to re-serialize and re-encrypt the refreshed credentials (e.g., in a token-rotation step) would lose those fields.
**Fix:** Preserve all existing credential fields and overwrite only the token fields:
```python
return {
**self._credentials, # preserve all existing fields
"access_token": result["access_token"],
"refresh_token": result.get("refresh_token", self._credentials["refresh_token"]),
"expires_at": expires_at,
}
```
---
### WR-03: `SettingsCloudTab` uses `connectionFor(provider.key)` — only shows the first connection per provider
**File:** `frontend/src/components/settings/SettingsCloudTab.vue:252-254`
**Issue:** `connectionFor` returns the first matching connection for a given `provider` key. Since Phase 12 allows multiple connections per provider (D-02/D-03), a second Google Drive account is silently invisible in the Settings UI. Users connecting a second account of the same provider would see only the first one, with no way to manage the second.
This is a functional regression compared to the Phase 12 design intent (multiple same-provider accounts).
**Fix:** Change `SettingsCloudTab` from a provider-keyed list to a connection-keyed list, iterating over `store.connections` directly rather than the `PROVIDERS` static array. Or introduce a secondary section for additional same-provider connections below the primary one.
---
### WR-04: `CloudFolderView` breadcrumb computed from raw `folderId` path string — breaks for provider IDs containing slashes
**File:** `frontend/src/views/CloudFolderView.vue:61-65`
**Issue:** The breadcrumb is computed by splitting `folderId` on `/`. For Google Drive and OneDrive, folder IDs are opaque strings (e.g., `1a2B3cD4EfGh`) that never contain slashes, so this is safe. For WebDAV, `provider_item_id` is the WebDAV path (e.g., `documents/reports/q1`), which contains slashes. Splitting on `/` will produce one breadcrumb segment per path component — this is actually the intended UX, but the segment IDs are computed by rejoining path parts (`parts.slice(0, idx + 1).join('/')`). When navigated, these IDs are used as `folderId` in the route, which means clicking a WebDAV breadcrumb sends a slash-containing segment to `router.push`. Vue Router's `:folderId(.*)` wildcard accepts this, but only if the slash is correctly URL-encoded in the router push. `router.push` does not auto-encode path params — the raw slash is passed through, potentially confusing path resolution.
**Fix:** In `navigateTo` and `handleBreadcrumbNavigate`, encode the provider item ID:
```js
function navigateTo(item) {
router.push(`/cloud/${connectionId.value}/${encodeURIComponent(item.id)}`)
}
```
And decode on read in `folderId` computed.
---
### WR-05: Empty `except` swallows Celery scheduling failures silently with no logging
**File:** `backend/api/cloud/browse.py:327-328`
```python
except Exception:
pass # Celery unavailable — serve stale cache without failing browse
```
**Issue:** When Celery is down, the stale-while-revalidate background refresh silently fails with no log entry. In a production incident (Celery down), the system will appear healthy — the browse endpoint returns 200 — but cached items will never refresh. This makes incident diagnosis much harder.
**Fix:**
```python
except Exception as celery_exc:
import logging
logging.getLogger(__name__).warning(
"Celery unavailable — background refresh skipped for connection %s: %s",
connection_id, celery_exc,
)
```
---
### WR-06: `WebDAVBackend.list_folder` calls `validate_cloud_url` inside `_propfind` thread — double call on each item
**File:** `backend/storage/webdav_backend.py:271`
**Issue:** `validate_cloud_url(self._server_url)` is called once at the start of `_propfind` (line 258) and then again for every item in the listing (line 271). The second call per item is inside the loop body and was documented as a defense against DNS-rebinding. However, calling a synchronous name-resolution function once per item in a potentially large directory listing creates a pathological case where listing a folder with 200 items makes 201 calls to the URL validator, each potentially doing DNS resolution.
The DNS-rebinding attack vector is real but calling the validator per item is excessive — if DNS rebinding occurs, the attacker can choose when it occurs; calling the validator once per item does not materially improve security vs. calling it once per `_propfind` invocation.
**Fix:** Keep the single guard at the start of `_propfind` and remove the per-item call inside the loop.
---
### WR-07: `CloudFolderView.onMounted` watch fires immediately on first load, causing double `load()` call
**File:** `frontend/src/views/CloudFolderView.vue:134-156`
**Issue:** `onMounted` calls `load()` at the bottom (line 150). The `watch([connectionId, folderId], ...)` at line 153 runs on component mount because Vue Router populates the route params before `onMounted`. The `watch` fires synchronously after `onMounted` sets up, triggering a second `load()` call immediately. The race can cause two concurrent fetches for the same folder, with the last one to complete winning.
Vue's `watch` defaults to `immediate: false` so this depends on timing. The `onMounted` early-return at line 145 (`router.replace` + `return`) may or may not prevent the second call depending on whether the redirect fires before the watcher activates.
**Fix:** Use `{ immediate: false }` explicitly on the watch (already the default, but make it explicit), and add a guard in `load()` to avoid concurrent calls:
```js
const _loadPending = ref(false)
async function load() {
if (_loadPending.value) return
_loadPending.value = true
// ... existing code ...
_loadPending.value = false
}
```
---
### WR-08: `aria-label` on stale indicator uses unquoted variable reference (template bug)
**File:** `frontend/src/components/ui/BreadcrumbBar.vue:82`
**Issue:**
```html
aria-label="staleWarningLabel"
```
This sets the `aria-label` to the literal string `"staleWarningLabel"` instead of binding to the computed property. It should be `:aria-label="staleWarningLabel"`. Screen readers will announce "staleWarningLabel" rather than the actual human-readable stale time.
**Fix:**
```html
:aria-label="staleWarningLabel"
```
---
## Info
### IN-01: `_DISPLAY_NAMES` dict duplicated in `browse.py` and `connections.py`
**File:** `backend/api/cloud/browse.py:47-52` and `backend/api/cloud/connections.py:40-45`
**Issue:** The same `_DISPLAY_NAMES` dict is defined identically in both modules. CLAUDE.md's duplication rule requires extracting shared constants to a shared module. Either module could import from the other, or both could import from a shared constants module.
**Fix:** Move `_DISPLAY_NAMES` to `backend/api/cloud/schemas.py` or a new `backend/api/cloud/constants.py` and import from both callers.
---
### IN-02: `DefaultStorageRequest` model defined in both `connections.py` and `__init__.py`
**File:** `backend/api/cloud/connections.py:56-57` and `backend/api/cloud/__init__.py:33-34`
**Issue:** `class DefaultStorageRequest(BaseModel): backend: str` is defined in both files. The one in `connections.py` is never used (the handler that used it was moved to `__init__.py`). Dead code.
**Fix:** Remove `DefaultStorageRequest` from `connections.py`.
---
### IN-03: `_master_key()` function defined twice
**File:** `backend/api/cloud/browse.py:55-56` and `backend/api/cloud/connections.py:59-60`
**Issue:** `def _master_key() -> bytes: return settings.cloud_creds_key.encode()` is identical in both files. CLAUDE.md prohibits this pattern. Neither is imported from a shared location.
**Fix:** Move to `backend/api/cloud/connections.py` (or a shared helper) and import in `browse.py`.
---
### IN-04: Test fixture `_create_cloud_connection` defined twice in `test_cloud.py`
**File:** `backend/tests/test_cloud.py:858-875`
**Issue:** A local `_create_cloud_connection` helper is defined at line 858 inside `test_cloud.py`, shadowing the module-level function imported from `test_cloud_security.py`. The duplicate uses a hardcoded test key (`b"test-key-for-testing-32bytes!!"`) that differs from the app's `settings.cloud_creds_key`. Tests using this local fixture will produce credentials encrypted with a different key than the key used to decrypt in the browse handler (which uses `settings.cloud_creds_key`). This means tests in the "Phase 12" block at the bottom of `test_cloud.py` that depend on actual decryption (like `test_browse_connection_schedules_background_refresh_on_cached_items`) will fail when the handler tries to decrypt the test credentials — unless the capability probe path (which also calls `_decrypt_connection`) is fully mocked.
**Fix:** Either use `settings.cloud_creds_key` in all test fixtures, or always mock `build_cloud_resource_adapter` (which is already done in most tests). Remove the duplicate local definition and use the shared factory from `conftest.py`.
---
_Reviewed: 2026-06-19_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_