14 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 12-cloud-resource-foundation | 2026-06-21T00:00:00Z | standard | 14 |
|
|
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:
# 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:
# 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:
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:
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:
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:
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:
} 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:
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:
<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:
@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:
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