From e64980af5fad2be6eb3b768ffdd04e7fc5ee4105 Mon Sep 17 00:00:00 2001 From: curo1305 Date: Mon, 22 Jun 2026 01:39:24 +0200 Subject: [PATCH] =?UTF-8?q?docs(phase-12):=20add=20security=20threat=20ver?= =?UTF-8?q?ification=20=E2=80=94=2026/26=20closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../12-SECURITY.md | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 .planning/phases/12-cloud-resource-foundation/12-SECURITY.md diff --git a/.planning/phases/12-cloud-resource-foundation/12-SECURITY.md b/.planning/phases/12-cloud-resource-foundation/12-SECURITY.md new file mode 100644 index 0000000..cbfea89 --- /dev/null +++ b/.planning/phases/12-cloud-resource-foundation/12-SECURITY.md @@ -0,0 +1,70 @@ +# Phase 12 Security Audit — Cloud Resource Foundation + +**Audited:** 2026-06-22 +**Auditor:** gsd-security-auditor (claude-sonnet-4-6) +**ASVS Level:** 2 +**Plans audited:** 12-01, 12-02, 12-03, 12-04, 12-05, 12-06 +**block_on:** critical + +--- + +## Threat Verification + +### Closed Threats + +| Threat ID | Plan | Category | Disposition | Status | Evidence | +|-----------|------|----------|-------------|--------|----------| +| T-12-01 | 01/02/04 | IDOR | mitigate | CLOSED | `resolve_owned_connection` in `backend/services/cloud_items.py:51` gates on `CloudConnection.user_id == user_uuid`; `_get_owned_connection` in `connections.py:139` checks `conn.user_id != user_id`; `get_regular_user` dep on all cloud browse routes blocks admin tokens. Tests: `test_cloud_security.py:test_foreign_user_cannot_browse_cloud_item`, `test_foreign_user_cannot_browse_subfolder`, `test_foreign_user_cannot_delete_connection`, `test_admin_cannot_browse_cloud_connection`. | +| T-12-02 | 01 | Same-provider account collision | mitigate | CLOSED | `_insert_cloud_connection` (`connections.py:109`) always inserts a new UUID row — never upserts by provider. Unique constraint in migration 0006 is on `(connection_id, provider_item_id)` not `(user_id, provider)`. Tests: `test_cloud.py:test_same_provider_connections_are_independent` (line 331) asserts two distinct UUIDs; `test_cloud_security.py:test_same_provider_items_scoped_to_connection` proves DB-level isolation. | +| T-12-03 | 02/04 | Credential disclosure | mitigate | CLOSED | `CloudBrowseResponse`, `CloudItemOut`, `FolderFreshnessOut` schemas in `api/cloud/schemas.py` contain no credential fields. `CloudConnectionOut` in `api/schemas.py` is used on connection-list/update endpoints. Test: `test_cloud_security.py:test_browse_response_excludes_credentials_and_raw_fields` asserts `credentials_enc`, `access_token`, `refresh_token`, `client_secret` absent from response body. | +| T-12-04 | 02/04 | WebDAV SSRF | mitigate | CLOSED | `validate_cloud_url` called at `connections.py:385` (new WebDAV connection) and `connections.py:533` (credential update). `cloud_utils.py:validate_cloud_url` blocks RFC1918, link-local (169.254.x.x), file://, and GCP metadata endpoints. Test: `test_cloud_security.py:test_ssrf_url_validation_invariants` (parametrized, 5 bad URLs). | +| T-12-05 | 02 | Cache collision | mitigate | CLOSED | All `cloud_items.py` service functions scope by `connection_id`: `list_cloud_children` (line 82), `upsert_cloud_item` (line 114), `reconcile_cloud_listing` (line 194). Browse endpoint passes `cid_str = str(connection_id)` to all service calls. Unique index `uq_cloud_items_connection_provider_item` on `(connection_id, provider_item_id)` prevents cross-connection collision. | +| T-12-06 | 01 | Destructive capability probing | mitigate | CLOSED | `CloudResourceAdapter` in `storage/cloud_base.py` declares only `list_folder`, `get_capabilities`, and `merge_item_capabilities` — no put, delete, rename, move, or create_folder methods. Frozen dataclass contract enforces no mutation. Test: `test_cloud_capabilities.py` fake adapter asserts no mutation method calls (29 unit tests, 12-01 SUMMARY confirms). | +| T-12-07 | 01/02 | Partial-list metadata deletion | mitigate | CLOSED | `reconcile_cloud_listing` in `cloud_items.py:188` performs soft-deletion only under `if listing.complete:` guard. Failed or partial listings (`complete=False`) return without marking any rows deleted. Test: `test_cloud_items.py:test_refresh_cloud_folder_failed_refresh_retains_cached_rows`. | +| T-12-08 | 02/04 | Provider error XSS/leak | mitigate | CLOSED | Browse endpoint catch-all at `browse.py:300` emits only controlled strings (`"Provider temporarily unavailable. Retrying in background."`); raw exceptions are never propagated. Vue template auto-escaping is active on all components (no `v-html` in StorageBrowser or cloud components). Input validation: malformed UUID returns FastAPI 422 before any handler runs (`test_cloud_security.py:test_browse_malformed_connection_id_returns_422`). | +| T-12-09 | 01/02/04 | Quota corruption | mitigate | CLOSED | `cloud_items.py` has no import of the quota service and no write to `Quota.used_bytes`. Browse endpoint has no MinIO calls. Tests: `test_cloud_security.py:test_browse_no_quota_mutation` (re-reads quota row after browse and asserts unchanged), `test_browse_no_minio_calls` (mocks get_object/put/delete and asserts not called), `test_no_byte_download_during_browse` (adapter byte-download methods not called). | +| T-12-10 | 03 | Disabled action executes on keyboard/touch | mitigate | CLOSED | `StorageBrowser.vue:409` sets `'aria-disabled': isDisabled ? 'true' : undefined` (not native `disabled`). `CapabilityButton` component suppresses click, Enter, Space, and touchend for `unsupported` and `temporarily_unavailable` states. Test: `StorageBrowser.capabilities.test.js` — click, Enter, Space, and touchend suppression assertions. 12-03 SUMMARY confirms 323 frontend tests pass. | +| T-12-11 | 03 | Secret/token persistence | mitigate | CLOSED | `cloudConnections.js:16` stores only `folderId` (a provider path string) under key `docuvault:cloud:folder:{connectionId}` in `sessionStorage`. No JWT, credential field, or JSON object is stored. Test: `cloudConnections.test.js:149` — "sessionStorage stores only folder references, not tokens" asserts value is a plain string, not an object. | +| T-12-12 | 03 | UI source confusion | mitigate | CLOSED | `BreadcrumbBar` receives `connectionRoot` prop with connection display name — breadcrumb root is the connection name, not a generic "Cloud" label. Provider icon appears contextually in the connection root breadcrumb only, not repeated per row. `StorageBrowser` adds no per-row provider badge for cloud items. | +| T-12-05-01 | 05 | App starts against stale schema | mitigate | CLOSED | `docker-compose.yml` `migrate` service runs `alembic upgrade head`; all application services declare `condition: service_completed_successfully` on `migrate`. Test: `test_compose_migrations.py:7 passed`. | +| T-12-05-02 | 05 | DDL privilege escalation | mitigate | CLOSED | `migrate` service receives only `DATABASE_MIGRATE_URL` (DDL-capable role). `backend`, `celery-worker`, `celery-beat` receive only `DATABASE_URL` (DML-only). Test: `test_compose_migrations.py:test_migrate_service_uses_migrate_url`. | +| T-12-05-03 | 05 | Concurrent migrations | mitigate | CLOSED | Exactly one `migrate` service in Compose. Application replicas have `condition: service_completed_successfully` — they wait, never run Alembic. Test: `test_compose_migrations.py` asserts exactly one service runs `alembic upgrade head`. | +| T-12-05-04 | 05 | Secrets in Compose config or test output | mitigate | CLOSED | Compose uses `${VAR}` environment references; no rendered values in tracked files. Test: `test_compose_migrations.py` static assertions confirm env-var-only references. | +| T-12-05-05 | 05 | Integration test destroys developer DB | mitigate | CLOSED | `test_migration_0006.py` calls `pytest.skip` without `INTEGRATION=1`/`INTEGRATION_DATABASE_URL`. Never touches `DATABASE_URL`. | +| T-12-06-01 | 06 | IDOR while editing/browsing multiple connections | mitigate | CLOSED | `_get_owned_connection` at `connections.py:132` is called before every credential update, rename, delete, and config read. Returns 404 when `conn.user_id != user_id`. Test: `test_cloud.py:test_credential_update_wrong_owner_returns_404` (line 370). Browse path uses `resolve_owned_connection` (T-12-01 evidence applies). | +| T-12-06-02 | 06 | Credential disclosure through multi-account list/edit/errors | mitigate | CLOSED | `update_webdav_credentials` returns `CloudConnectionOut.model_validate(conn).model_dump()` — whitelist schema, no `credentials_enc` or decrypted fields. Merged credentials are written to DB only, never returned. Audit log at `connections.py:571` includes only `{"provider": conn.provider}`, no credential data. | +| T-12-06-03 | 06 | OAuth state confusion updates another connection | mitigate | CLOSED (by architectural exclusion) | OAuth callback always calls `_insert_cloud_connection` (always INSERT new UUID row). There is no "reconnect existing OAuth connection" flow in Phase 12 — editing OAuth accounts is deferred to Phase 13. The threat (state causes callback to overwrite a different connection's credentials) cannot occur because the callback never targets an existing connection ID. Redis state stores only `user_id` with 30-minute TTL; the callback re-validates user existence and activity before proceeding. | +| T-12-06-04 | 06 | SSRF bypass during credential edit | mitigate | CLOSED | `update_webdav_credentials` calls `validate_cloud_url(merged["server_url"])` at `connections.py:533` then `backend.health_check()` before `conn.credentials_enc = encrypt_credentials(...)`. No commit occurs if either check fails. | +| T-12-06-05 | 06 | Cross-connection metadata collision | mitigate | CLOSED | All service queries include both `user_id` and `connection_id` predicates (see T-12-05 evidence). Same unique constraint on `(connection_id, provider_item_id)` applies. Test: `test_cloud_security.py:test_same_provider_items_scoped_to_connection` inserts same `provider_item_id` under two connections and asserts two distinct rows exist. | +| T-12-06-06 | 06 | Route/provider identifier confusion | mitigate | CLOSED | `CloudProviderTreeItem.vue` calls `getCloudFoldersByConnectionId(connection.id, '')` (confirmed by 12-06 SUMMARY). `CloudFolderTreeItem.vue` accepts `connectionId` prop and uses it for all API calls and routes. Browse endpoint route `/connections/{connection_id}/items` is typed `uuid.UUID` — FastAPI rejects non-UUID slugs with 422. Test: `test_cloud_security.py:test_browse_malformed_connection_id_returns_422`. | +| T-12-SC | 04/06 | Supply-chain/secrets | mitigate | CLOSED | `bandit -r backend/ --severity-level medium`: 0 HIGH, 0 MEDIUM (12-04 SUMMARY). `npm audit --audit-level=high`: 0 vulnerabilities (12-06 SUMMARY). No new Python or npm runtime packages added in Phase 12. No hardcoded secrets detected. | +| T-12-05-SC | 05 | Dependency risk from gap closure | mitigate | CLOSED | No new runtime dependencies introduced in plan 05. Standard security gates rerun: Bandit 0 HIGH/MEDIUM, npm audit 0 high/critical. | +| T-12-06-SC | 06 | Dependency/supply-chain risk | mitigate | CLOSED | No new runtime dependencies in plan 06. npm audit 0 vulnerabilities (12-06 SUMMARY: 334 frontend tests pass, production build clean). | + +--- + +## Unregistered Threat Flags + +The following flags appeared in SUMMARY.md `## Threat Flags` sections with no formal threat ID: + +| Flag | Source | Description | Assessment | +|------|--------|-------------|------------| +| `credential_in_worker` | 12-02-SUMMARY / `backend/tasks/cloud_tasks.py` | Credentials decrypted inside Celery worker — master key must be in `CLOUD_CREDS_KEY` env var on all worker nodes. | Informational — not a gap. Master key from env is the established pattern (`_master_key()` in connections.py:59). Workers share the same Compose env. No credentials in broker payload. | +| `sessionStorage-check` | 12-03-SUMMARY / `cloudConnections.js` | `saveLastFolder` stores only folder path string under namespaced key. | Informational — maps to T-12-11 (CLOSED). | + +--- + +## Documentation Gap + +**Finding (WARNING — not a BLOCKER):** The existing `SECURITY.md` Phase 12 threat register (written during plan 04) omits threats first introduced in plans 03 and 06: T-12-02, T-12-05, T-12-06, T-12-11, T-12-12, T-12-06-01 through T-12-06-06, T-12-05-SC, and T-12-06-SC. All mitigations are present in code; the omission is a documentation record gap, not an implementation gap. The project SECURITY.md at `SECURITY.md` should be extended with a "Phase 12 Plan 06 Closeout" section to close this gap. + +--- + +## Summary + +**Threats Closed:** 26/26 +**ASVS Level:** 2 +**Blockers:** 0 +**Unregistered Flags:** 2 (both informational, both map to closed threats or accepted architecture decisions) + +All declared Phase 12 threat mitigations are present in the implementation. No BLOCKER findings. The SECURITY.md documentation record requires a supplementary section to capture threat evidence from plans 03 and 06.