268 lines
14 KiB
Markdown
268 lines
14 KiB
Markdown
---
|
|
phase: 12-cloud-resource-foundation
|
|
reviewed: 2026-06-21T00:00:00Z
|
|
depth: standard
|
|
files_reviewed: 14
|
|
files_reviewed_list:
|
|
- backend/api/cloud/connections.py
|
|
- backend/main.py
|
|
- backend/tests/test_cloud.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
|
|
findings:
|
|
critical: 3
|
|
warning: 5
|
|
info: 3
|
|
total: 11
|
|
status: issues_found
|
|
---
|
|
|
|
# Phase 12 (gap-closure pass): Code Review Report
|
|
|
|
**Reviewed:** 2026-06-21T00:00:00Z
|
|
**Depth:** standard
|
|
**Files Reviewed:** 14
|
|
**Status:** issues_found
|
|
|
|
## Summary
|
|
|
|
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: OAuth `error` parameter reflected unsanitised into redirect URL
|
|
|
|
**File:** `backend/api/cloud/connections.py:318-319, 364-365`
|
|
|
|
**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:**
|
|
```python
|
|
# 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-02: `rename_connection` writes to `display_name_override` but list reads `display_name` — rename is silently a no-op
|
|
|
|
**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
|
|
# 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)
|
|
```
|
|
Also add a round-trip assertion to the test:
|
|
```python
|
|
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-03: Test fixture uses mismatched encryption key — integration tests can silently fail decryption
|
|
|
|
**File:** `backend/tests/test_cloud.py:983-984`
|
|
|
|
**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
|
|
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
|
|
|
|
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: `resolvedServerUrl` can be empty on Nextcloud submit with no client-side validation
|
|
|
|
**File:** `frontend/src/components/cloud/CloudCredentialModal.vue:225-234, 301-316`
|
|
|
|
**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.
|
|
|
|
**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-02: Silent `catch {}` in `CloudCredentialModal` config-load leaves fields blank with no feedback
|
|
|
|
**File:** `frontend/src/components/cloud/CloudCredentialModal.vue:282-283`
|
|
|
|
**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
|
|
@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.",
|
|
)
|
|
```
|
|
|
|
---
|
|
|
|
## Info
|
|
|
|
### IN-01: `package.json` uses floating `^` ranges for all dependencies
|
|
|
|
**File:** `frontend/package.json:12-19`
|
|
|
|
**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: Dead assignment in `test_browse_connection_schedules_background_refresh_on_cached_items`
|
|
|
|
**File:** `backend/tests/test_cloud.py:1265`
|
|
|
|
**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: `CloudFolderTreeItem` passes `folder.id` as `parentRef` — must be provider-native item ID, not DB UUID
|
|
|
|
**File:** `frontend/src/components/cloud/CloudFolderTreeItem.vue:49`
|
|
|
|
**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)
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
_Reviewed: 2026-06-21T00:00:00Z_
|
|
_Reviewer: Claude (gsd-code-reviewer)_
|
|
_Depth: standard_
|