diff --git a/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-SECURITY.md b/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-SECURITY.md new file mode 100644 index 0000000..4312645 --- /dev/null +++ b/.planning/phases/06.2-close-v1-sharing-cloud-delete-csv-export-gaps/06.2-SECURITY.md @@ -0,0 +1,95 @@ +--- +phase: "06.2" +audited_at: "2026-05-31" +asvs_level: L1 +threats_total: 16 +threats_closed: 16 +threats_open: 0 +result: SECURED +--- + +# Security Audit — Phase 06.2 +## close-v1-sharing-cloud-delete-csv-export-gaps + +**Auditor:** gsd-security-auditor +**ASVS Level:** L1 +**block_on:** HIGH +**Threats Closed:** 16 / 16 +**Result: SECURED** + +--- + +## Threat Verification + +| Threat ID | Category | Disposition | Status | Evidence | +|-----------|----------|-------------|--------|----------| +| T-06.2-01-01 | Tampering | accept | CLOSED | All three xfail stubs promoted to real tests in Plans 02–04; no stub logic leaked into production code paths. Acceptance rationale sound: stub-only plan cannot add attack surface. | +| T-06.2-02-01 | Elevation of Privilege | mitigate | CLOSED | `backend/api/shares.py:264` — `if share is None or share.owner_id != current_user.id: raise HTTPException(status_code=404, ...)` in `update_share_permission`. Returns 404, not 403, preventing share ID enumeration. Pattern mirrors `revoke_share` at line 295. | +| T-06.2-02-02 | Tampering | mitigate | CLOSED | `backend/api/shares.py:54–58` — `SharePermissionPatch.validate_permission` field_validator enforces `v not in {"view", "edit"} → ValueError`. No arbitrary string can pass through to the ORM. | +| T-06.2-02-03 | Tampering | mitigate | CLOSED | `backend/api/shares.py:43–48` — `ShareCreate.validate_permission` applies the same `{"view", "edit"}` allowlist. Default `"view"` is server-enforced via Pydantic default; client cannot inject other values. | +| T-06.2-02-SC | Tampering | accept | CLOSED | Plan 02 SUMMARY confirms no new npm/pip packages installed. Acceptance rationale sound. | +| T-06.2-03-01 | Tampering | mitigate | CLOSED | `backend/api/documents.py:654` — `ok = await storage.delete_document(session, doc_id, skip_quota=is_cloud)` where `is_cloud = doc.storage_backend != "minio"`. `backend/services/storage.py:167` — `if not skip_quota:` gates the quota decrement block. Cloud doc deletes never underflow quota. | +| T-06.2-03-02 | Information Disclosure | mitigate | CLOSED | `backend/api/documents.py:642–652` — `except Exception as exc:` catches the provider error; `print(f"[cloud-delete] provider error: {exc}", file=sys.stderr)` logs to stderr only; JSON response body contains only the fixed string `"Cloud provider delete failed. You can remove from app only."` — `str(exc)` is never serialised into the response. | +| T-06.2-03-03 | Elevation of Privilege | accept | CLOSED | `backend/api/documents.py:629` — `if doc is None or doc.user_id != current_user.id: raise HTTPException(404, ...)` executes before the `remove_only` branch at line 637–638. Ownership is always asserted regardless of query param value. Acceptance rationale sound. | +| T-06.2-03-04 | Spoofing | mitigate | CLOSED | `backend/api/documents.py:638–641` — `if is_cloud and not remove_only:` block calls `get_storage_backend_for_document(doc, current_user, session)` which returns the cloud backend; `storage.delete_document()` is only reached after cloud routing is complete (or after `remove_only=true` skips the cloud call). MinIO is never called for cloud docs. | +| T-06.2-03-SC | Tampering | accept | CLOSED | Plan 03 SUMMARY confirms no new npm/pip packages installed. Acceptance rationale sound. | +| T-06.2-04-01 | Tampering | mitigate | CLOSED | `backend/api/audit.py:216` — `if not re.fullmatch(r"\d{4}-\d{2}-\d{2}", date): raise HTTPException(status_code=404, ...)` executes before `key = f"audit-logs/{date}.csv"` at line 220. Any non-date string (including path traversal sequences) is rejected with 404. | +| T-06.2-04-02 | Elevation of Privilege | mitigate | CLOSED | `backend/api/audit.py:170` — `list_daily_exports` uses `_admin: User = Depends(get_current_admin)`; line 205 — `download_daily_export` uses the same dependency. Both new endpoints require admin authentication. Regular users receive 403; unauthenticated requests receive 401. | +| T-06.2-04-03 | Information Disclosure | mitigate | CLOSED | `frontend/src/api/client.js:398–427` — `adminExportAuditLogCsv()` uses `fetch()` with `Authorization: Bearer ${authStore.accessToken}` header and `res.text()` → Blob → `.click()` pattern. `window.location.href` is absent from `frontend/src/components/admin/AuditLogTab.vue` (confirmed: no match). | +| T-06.2-04-04 | Information Disclosure | accept | CLOSED | User handles are already public within the platform (visible in sharing UI). Admin view of handles is consistent with existing admin privileges. Acceptance rationale sound; no mitigation code is required or expected. | +| T-06.2-04-05 | Denial of Service | mitigate | CLOSED | `backend/api/audit.py:198` — `items = await asyncio.to_thread(_list)` wraps the synchronous `list_objects()` iterator for `list_daily_exports`; line 231 — `csv_bytes = await asyncio.to_thread(_get)` wraps `get_object()` for `download_daily_export`. Both synchronous MinIO SDK calls are offloaded from the async event loop. | +| T-06.2-04-SC | Tampering | accept | CLOSED | Plan 04 SUMMARY confirms no new npm/pip packages installed. Acceptance rationale sound. | + +--- + +## Accepted Risk Log + +The following threats are accepted per plan-time decisions. No mitigation code is present or required. + +| Threat ID | Rationale | +|-----------|-----------| +| T-06.2-01-01 | Wave 0 stub plan cannot introduce production attack surface; stubs contain only `pytest.xfail()` calls and were verified to be fully promoted (no stubs remain) by Plans 02–04. | +| T-06.2-02-SC | No new packages installed in Plan 02. Supply-chain risk unchanged from baseline. | +| T-06.2-03-03 | Ownership check at `documents.py:629` unconditionally precedes the `remove_only` branch — privilege escalation via the query param is structurally impossible. | +| T-06.2-03-SC | No new packages installed in Plan 03. Supply-chain risk unchanged from baseline. | +| T-06.2-04-04 | User handles are public within the platform. Admin audit access to handles is consistent with the broader admin privilege model already in place. | +| T-06.2-04-SC | No new packages installed in Plan 04. Supply-chain risk unchanged from baseline. | + +--- + +## Unregistered Flags + +None. No `## Threat Flags` sections were present in any of the four SUMMARY.md files. No new attack surface was flagged by executors during implementation. + +--- + +## Verification Commands Run + +``` +grep -n "share.owner_id != current_user.id" backend/api/shares.py +# → lines 264, 295 (update_share_permission + revoke_share — both entry points covered) + +grep -n "field_validator" backend/api/shares.py +# → lines 43, 54 (both ShareCreate and SharePermissionPatch) + +grep -n "skip_quota" backend/services/storage.py +# → lines 143, 150, 167 (signature, docstring, guard) + +grep -n "cloud_delete_failed\|sys.stderr" backend/api/documents.py +# → lines 644, 649 (stderr log, fixed-string response) + +grep -n "re.fullmatch" backend/api/audit.py +# → line 216 + +grep -n "get_current_admin" backend/api/audit.py +# → lines 170, 205 (both new daily-export endpoints) + +grep -n "asyncio.to_thread" backend/api/audit.py +# → lines 198, 231 + +grep -n "adminExportAuditLogCsv\|adminListDailyExports\|adminDownloadDailyExport" frontend/src/api/client.js +# → lines 398, 435, 460 + +grep "window.location.href" frontend/src/components/admin/AuditLogTab.vue +# → (no output — absent) +```