diff --git a/.planning/phases/12-cloud-resource-foundation/12-REVIEW.md b/.planning/phases/12-cloud-resource-foundation/12-REVIEW.md new file mode 100644 index 0000000..a688cf5 --- /dev/null +++ b/.planning/phases/12-cloud-resource-foundation/12-REVIEW.md @@ -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_