docs(12): add code review report for phase 12-06 gap closure
This commit is contained in:
curo1305
@@ -1,345 +1,267 @@
|
||||
---
|
||||
phase: 12-cloud-resource-foundation
|
||||
reviewed: 2026-06-19T00:00:00Z
|
||||
reviewed: 2026-06-21T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 35
|
||||
files_reviewed: 14
|
||||
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/api/utils.js
|
||||
- frontend/src/components/cloud/CloudCredentialModal.vue
|
||||
- frontend/src/components/cloud/CloudFolderTreeItem.vue
|
||||
- frontend/src/components/cloud/CloudProviderTreeItem.vue
|
||||
- frontend/src/components/cloud/__tests__/CloudFolderTreeItem.test.js
|
||||
- frontend/src/components/cloud/__tests__/CloudProviderTreeItem.test.js
|
||||
- frontend/src/components/layout/AppSidebar.vue
|
||||
- frontend/src/components/layout/__tests__/AppSidebar.test.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
|
||||
critical: 3
|
||||
warning: 5
|
||||
info: 3
|
||||
total: 11
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 12: Code Review Report
|
||||
# Phase 12 (gap-closure pass): Code Review Report
|
||||
|
||||
**Reviewed:** 2026-06-19
|
||||
**Reviewed:** 2026-06-21T00:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 35
|
||||
**Files Reviewed:** 14
|
||||
**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.
|
||||
This pass covers the Phase 12 gap-closure files: `connections.py`, the updated frontend cloud API and components, and the extended test suite. The primary concerns are (1) an OAuth error path that reflects arbitrary provider-supplied text into a redirect URL, (2) a rename endpoint that writes to `display_name_override` but the list endpoint reads `display_name`, creating a silent write-that-never-shows-up defect, and (3) several silent `catch {}` blocks that leave users with no error feedback. Additionally the test fixture `_create_cloud_connection` in `test_cloud.py` uses a hardcoded key that differs from the app key, which can cause spurious decryption failures.
|
||||
|
||||
---
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: Frontend API calls a non-existent endpoint
|
||||
### CR-01: OAuth `error` parameter reflected unsanitised into redirect URL
|
||||
|
||||
**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.
|
||||
**File:** `backend/api/cloud/connections.py:318-319, 364-365`
|
||||
|
||||
`CloudFolderView` calls this function at line 77 and the entire browse feature is broken at runtime.
|
||||
**Issue:** In `oauth_callback`, the `error_param` received from the OAuth provider (line 315) is wrapped verbatim into a `ValueError` (line 319): `raise ValueError(f"OAuth provider returned error: {error_param}")`. This exception then reaches the bare `except Exception as exc` handler on line 364, which calls `_settings_redirect(cloud_error=str(exc))`. The result is that an attacker who controls the OAuth redirect (e.g., MITM, malicious OAuth server, forged state response) can inject arbitrary text into the `cloud_error` query parameter. If the frontend ever renders `cloud_error` via `innerHTML` or `v-html`, this becomes XSS. Even without that, injected text in a URL query param is reflected user-visible content.
|
||||
|
||||
The same `except Exception` catch on line 364 also surfaces raw Python exception messages from `google_auth_oauthlib`, `msal`, and `_oauth_credentials_from_code` to the redirect URL, exposing internal library error details.
|
||||
|
||||
**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
|
||||
# Line 318-319: sanitise provider-supplied error token
|
||||
SAFE_PROVIDER_ERRORS = frozenset({
|
||||
"access_denied", "temporarily_unavailable", "server_error",
|
||||
"invalid_request", "unsupported_response_type",
|
||||
})
|
||||
if error_param:
|
||||
safe = error_param if error_param in SAFE_PROVIDER_ERRORS else "oauth_error"
|
||||
return _settings_redirect(cloud_error=f"OAuth provider returned: {safe}")
|
||||
|
||||
# Line 364-365: never reflect raw exception text
|
||||
except Exception:
|
||||
logger.exception("OAuth callback error")
|
||||
return _settings_redirect(cloud_error="Connection failed. Please try again.")
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CR-03: `get_or_create_folder_state` does not scope lookup to `user_id` — potential cross-user state bleed
|
||||
### CR-02: `rename_connection` writes to `display_name_override` but list reads `display_name` — rename is silently a no-op
|
||||
|
||||
**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)`.
|
||||
**File:** `backend/api/cloud/connections.py:598-601`
|
||||
|
||||
**Issue:** The PATCH handler (line 598) sets `conn.display_name_override = body.display_name`. The `_connection_response` function (line 264-276) which builds every list-connections item calls `CloudConnectionOut.model_validate(conn)`, and `CloudConnectionOut` is validated from the ORM model. If `CloudConnectionOut` maps to `display_name` (the non-override column), then renames are written to `display_name_override` but never read by the list endpoint — the user sees no change after renaming.
|
||||
|
||||
The HTTP response on line 601 returns `{"id": ..., "display_name": body.display_name}` directly from the request body (not from the refreshed DB row), making this look like a success even when the value is not persisted in the column the API reads from. The test `test_rename_connection_display_name` only asserts on the PATCH response body, not on a subsequent GET — so this bug is not caught by the test suite.
|
||||
|
||||
**Fix:** After the PATCH, verify correctness by re-reading via the list path:
|
||||
```python
|
||||
result = await session.execute(
|
||||
select(CloudFolderState).where(
|
||||
CloudFolderState.connection_id == cid_fs,
|
||||
CloudFolderState.parent_ref == parent_ref,
|
||||
)
|
||||
)
|
||||
# In rename_connection, return value consistent with list endpoint
|
||||
conn.display_name_override = body.display_name
|
||||
await session.commit()
|
||||
await session.refresh(conn)
|
||||
# Return _connection_response(conn) so the response uses the same serialisation as GET
|
||||
return _connection_response(conn)
|
||||
```
|
||||
|
||||
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:**
|
||||
Also add a round-trip assertion to the test:
|
||||
```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",
|
||||
)
|
||||
list_resp = await async_client.get("/api/cloud/connections", headers=auth["headers"])
|
||||
assert any(c["display_name"] == "My Work Drive" for c in list_resp.json()["items"])
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CR-04: `ConnectionRenameRequest` has no maximum length on `display_name` — potential DB overflow / DoS
|
||||
### CR-03: Test fixture uses mismatched encryption key — integration tests can silently fail decryption
|
||||
|
||||
**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.
|
||||
**File:** `backend/tests/test_cloud.py:983-984`
|
||||
|
||||
CLAUDE.md requires: *"All user-supplied data validated via Pydantic."* A missing length constraint is an incomplete validation.
|
||||
**Issue:** The local `_create_cloud_connection` fixture (lines 978-995) encrypts credentials with the hardcoded key `b"test-key-for-testing-32bytes!!"` (30 bytes). The application's `_master_key()` in `connections.py` uses `settings.cloud_creds_key.encode()`. In any test that creates a connection via this fixture and then calls an endpoint that decrypts credentials (browse, config, credential update), decryption will fail with a key-mismatch error unless the test fixture's key matches the test-environment `CLOUD_CREDS_KEY` setting exactly. Since the app uses `settings.cloud_creds_key`, the fixture should too:
|
||||
|
||||
```python
|
||||
from config import settings
|
||||
master_key = settings.cloud_creds_key.encode()
|
||||
```
|
||||
|
||||
The earlier `cloud_connection_factory` fixture in `conftest.py` (referenced in other tests) presumably uses the settings key; this local fixture does not. The mismatched key is a latent defect that causes non-obvious `DecryptionError` failures under certain test orderings and CI configurations.
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
from pydantic import BaseModel, field_validator, constr
|
||||
async def _create_cloud_connection(session, user_id, provider: str = "google_drive", name: str = "My Drive"):
|
||||
from db.models import CloudConnection
|
||||
from storage.cloud_utils import encrypt_credentials
|
||||
from config import settings # use app key, not hardcoded
|
||||
|
||||
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
|
||||
master_key = settings.cloud_creds_key.encode()
|
||||
creds_enc = encrypt_credentials(master_key, str(user_id), {"access_token": "tok", "refresh_token": "ref"})
|
||||
...
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Credentials decrypted twice per browse request
|
||||
### WR-01: `resolvedServerUrl` can be empty on Nextcloud submit with no client-side validation
|
||||
|
||||
**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.
|
||||
**File:** `frontend/src/components/cloud/CloudCredentialModal.vue:225-234, 301-316`
|
||||
|
||||
**Fix:** Decrypt once, hoist to before the branching:
|
||||
```python
|
||||
try:
|
||||
credentials = _decrypt_connection(conn, current_user.id)
|
||||
except Exception:
|
||||
credentials = None
|
||||
**Issue:** `resolvedServerUrl` returns `''` when `serverBase` or `username` is empty (line 219). There is no client-side guard in `submit()` — the empty string is passed straight to `connectWebDav()`, which sends it to the backend. The backend will return a 422, but the error message ("Invalid server URL") gives no hint about which field is missing. More critically, in the edit path (line 309), `resolvedServerUrl.value || undefined` converts the empty string to `undefined`, which is then omitted from the PUT body — the backend interprets this as "keep the current URL", silently suppressing what should be a user error.
|
||||
|
||||
# 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,
|
||||
**Fix:** Add pre-submit validation:
|
||||
```javascript
|
||||
async function submit() {
|
||||
connectError.value = ''
|
||||
const url = resolvedServerUrl.value
|
||||
if (!url) {
|
||||
connectError.value = props.provider?.key === 'nextcloud'
|
||||
? 'Enter the Nextcloud server URL and username first.'
|
||||
: 'Server URL is required.'
|
||||
return
|
||||
}
|
||||
saving.value = true
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-03: `SettingsCloudTab` uses `connectionFor(provider.key)` — only shows the first connection per provider
|
||||
### WR-02: Silent `catch {}` in `CloudCredentialModal` config-load leaves fields blank with no feedback
|
||||
|
||||
**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.
|
||||
**File:** `frontend/src/components/cloud/CloudCredentialModal.vue:282-283`
|
||||
|
||||
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.
|
||||
**Issue:** The `catch` block when loading existing connection config is completely empty. If `api.getConnectionConfig()` fails (network error, 503, decryption failure on the backend), the modal silently opens with all fields empty. The user has no indication that pre-population failed and may inadvertently submit a PUT with blank `serverUrl` and `username`, potentially resetting a working connection.
|
||||
|
||||
**Fix:**
|
||||
```javascript
|
||||
} catch {
|
||||
connectError.value = 'Could not load connection settings. You may re-enter them below.'
|
||||
} finally {
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-03: `handleDisconnect` / `handleDisconnectAll` swallow errors silently — no user feedback
|
||||
|
||||
**File:** `frontend/src/components/settings/SettingsCloudTab.vue:322-328, 331-336`
|
||||
|
||||
**Issue:** Both disconnect handlers catch all errors and discard them with `// Error handled by store`. But the store does not surface disconnect errors into any UI state visible to `SettingsCloudTab`. If the DELETE call fails (network outage, 500), the ConfirmBlock disappears and the connection appears to still be listed, with the user receiving no explanation. They may click "Remove" again, producing duplicate requests.
|
||||
|
||||
**Fix:** Expose the error in the component:
|
||||
```javascript
|
||||
const disconnectError = ref('')
|
||||
async function handleDisconnect(id) {
|
||||
disconnectError.value = ''
|
||||
if (!id) return
|
||||
try {
|
||||
await store.disconnect(id)
|
||||
} catch (e) {
|
||||
disconnectError.value = e.message || 'Failed to remove connection. Please try again.'
|
||||
}
|
||||
confirmRemoveId.value = null
|
||||
}
|
||||
```
|
||||
And render `disconnectError` in the template near the ConfirmBlock.
|
||||
|
||||
---
|
||||
|
||||
### WR-04: `renameError` is set but never rendered in the template
|
||||
|
||||
**File:** `frontend/src/components/settings/SettingsCloudTab.vue:253, 357-359`
|
||||
|
||||
**Issue:** `renameError` is defined on line 253 and set on error in `submitRename` (line 357-358), but there is no `v-if="renameError"` binding in the template. A rename failure produces no visible feedback — the input stays open and the user has no idea why the save did not work.
|
||||
|
||||
**Fix:** Add an error span adjacent to the rename input:
|
||||
```html
|
||||
<input v-model="renameValue" ... />
|
||||
<span v-if="renamingId === conn.id && renameError" class="text-xs text-red-600 ml-2">
|
||||
{{ renameError }}
|
||||
</span>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### WR-05: `_get_active_connection` raises `MultipleResultsFound` when user has two active same-provider connections
|
||||
|
||||
**File:** `backend/api/cloud/connections.py:148-162`
|
||||
|
||||
**Issue:** `scalar_one_or_none()` on line 156 raises `sqlalchemy.exc.MultipleResultsFound` if a user has two `ACTIVE` connections for the same provider. Phase 12 explicitly permits multiple same-provider connections. The legacy `GET /folders/{provider}/{folder_id}` endpoint (line 638) calls `_get_active_connection` keyed on `provider`, so any user with two Google Drive accounts will get a 500 Internal Server Error when browsing via the legacy endpoint.
|
||||
|
||||
The backward-compat route is preserved for existing clients; it now silently fails for users who added a second account under Phase 12's multi-connection feature.
|
||||
|
||||
**Fix:** Either return a 410 Gone on the legacy endpoint with a migration notice, or change to `.first()` with a documented "returns first active connection" semantic. Given it is marked a compatibility route, 410 is cleaner:
|
||||
```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,
|
||||
@router.get("/folders/{provider}/{folder_id:path}")
|
||||
async def list_cloud_folders(...):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_410_GONE,
|
||||
detail="This endpoint is deprecated. Use GET /api/cloud/connections/{id}/items instead.",
|
||||
)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 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`
|
||||
### IN-01: `package.json` uses floating `^` ranges for all dependencies
|
||||
|
||||
**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.
|
||||
**File:** `frontend/package.json:12-19`
|
||||
|
||||
**Fix:** Move `_DISPLAY_NAMES` to `backend/api/cloud/schemas.py` or a new `backend/api/cloud/constants.py` and import from both callers.
|
||||
**Issue:** All runtime and dev dependencies use `^` (caret) ranges rather than pinned exact versions. CLAUDE.md requires pinned exact versions for security-critical packages. While lock-file integrity provides some protection, floating ranges create a discrepancy between what is declared and what can be installed from a clean state.
|
||||
|
||||
**Fix:** Pin all versions to exact values (remove `^`) and commit `package-lock.json`.
|
||||
|
||||
---
|
||||
|
||||
### IN-02: `DefaultStorageRequest` model defined in both `connections.py` and `__init__.py`
|
||||
### IN-02: Dead assignment in `test_browse_connection_schedules_background_refresh_on_cached_items`
|
||||
|
||||
**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.
|
||||
**File:** `backend/tests/test_cloud.py:1265`
|
||||
|
||||
**Fix:** Remove `DefaultStorageRequest` from `connections.py`.
|
||||
**Issue:** Line 1265 sets `mock_adapter.get_capabilities = _uuid.__class__` (which is `<class 'uuid.UUID'>`, not a callable that returns capabilities). This is immediately overridden two lines later by `mock_adapter.get_capabilities = _fake_caps`. The first assignment is dead code and suggests copy-paste confusion.
|
||||
|
||||
**Fix:** Remove the dead assignment on line 1265.
|
||||
|
||||
---
|
||||
|
||||
### IN-03: `_master_key()` function defined twice
|
||||
### IN-03: `CloudFolderTreeItem` passes `folder.id` as `parentRef` — must be provider-native item ID, not DB UUID
|
||||
|
||||
**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.
|
||||
**File:** `frontend/src/components/cloud/CloudFolderTreeItem.vue:49`
|
||||
|
||||
**Fix:** Move to `backend/api/cloud/connections.py` (or a shared helper) and import in `browse.py`.
|
||||
**Issue:** `loadChildren()` passes `props.folder.id` as the `parentRef` to `getCloudFoldersByConnectionId`. The backend browse endpoint interprets `parent_ref` as a provider-native path/ID string. If `CloudItemOut.id` in the API response is the durable DB UUID of the `CloudItem` row (not the `provider_item_id`), then this call sends the wrong identifier to the provider and browse will return an empty list or a 404 at the provider level. The correct field is `provider_item_id` (or whichever field in `CloudItemOut` carries the provider-native reference). Verify the schema and use the correct field.
|
||||
|
||||
**Fix:** Confirm the `CloudItemOut` schema field for the provider-native item reference and use it:
|
||||
```javascript
|
||||
async function loadChildren() {
|
||||
const data = await api.getCloudFoldersByConnectionId(
|
||||
props.connectionId,
|
||||
props.folder.provider_item_id ?? props.folder.id // prefer provider_item_id
|
||||
)
|
||||
return (data.items ?? []).filter(i => i.is_dir)
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 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_
|
||||
_Reviewed: 2026-06-21T00:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
|
||||
Reference in New Issue
Block a user