diff --git a/.planning/phases/12-cloud-resource-foundation/12-05-REVIEW.md b/.planning/phases/12-cloud-resource-foundation/12-05-REVIEW.md new file mode 100644 index 0000000..cf043a0 --- /dev/null +++ b/.planning/phases/12-cloud-resource-foundation/12-05-REVIEW.md @@ -0,0 +1,322 @@ +--- +phase: 12-cloud-resource-foundation +reviewed: 2026-06-20T00:00:00Z +depth: standard +files_reviewed: 10 +files_reviewed_list: + - docker-compose.yml + - backend/tests/test_compose_migrations.py + - backend/tests/test_migration_0006.py + - backend/main.py + - frontend/package.json + - AGENTS.md + - README.md + - RUNBOOK.md + - SECURITY.md + - CLAUDE.md +findings: + critical: 3 + warning: 5 + info: 3 + total: 11 +status: issues_found +--- + +# Phase 12: Code Review Report + +**Reviewed:** 2026-06-20 +**Depth:** standard +**Files Reviewed:** 10 +**Status:** issues_found + +## Summary + +The Phase 12 gap-closure implementation adds a one-shot `migrate` service to docker-compose.yml, +seven static compose-invariant tests, and a 12-test integration suite for the 0005→0006 Alembic +migration. The compose hardening approach (read_only, cap_drop ALL, no-new-privileges, restart: no) +is correct in concept. However, several concrete defects were found: + +- Three critical issues: the `promtail` service mounts the Docker socket with no restrictions (full + container escape vector), the Grafana admin password has a hardcoded default with no boot-time + override enforcement, and the `test_docuvault_app_cannot_create_table` test silently skips rather + than failing when the role is absent — defeating the purpose of the DDL-privilege gate. +- Five warnings: the `migrate` service mounts the entire source tree as a writable volume (defeating + `read_only`), the `minio-init` service has no hardening at all, version-pinned `latest` tags on + three images, the integration test class relies on execution order without `@pytest.mark.order`, + and `celery-beat` is missing `CLOUD_CREDS_KEY` and JWT key env vars. +- Three info items: README/AGENTS.md version skew, the `backend` service unnecessarily receives + `DATABASE_MIGRATE_URL`, and the health endpoint leaks Python exception class names to callers. + +--- + +## Critical Issues + +### CR-01: promtail mounts Docker socket — full container escape + +**File:** `docker-compose.yml:259` +**Issue:** The `promtail` service bind-mounts `/var/run/docker.sock` into the container with no +read-only flag and no capability restriction. Docker socket access is equivalent to unrestricted +root on the host: any process inside promtail that is compromised (or any malicious log entry that +achieves code execution) can spawn privileged containers, exfiltrate secrets from all other +containers, or escape to the host entirely. +```yaml +volumes: + - /var/run/docker.sock:/var/run/docker.sock # no :ro, no hardening +``` +**Fix:** Mount the socket read-only and add container hardening: +```yaml +promtail: + image: grafana/promtail:2.9.10 # pin version (see WR-02) + volumes: + - ./docker/loki/promtail-config.yaml:/etc/promtail/config.yaml + - /var/lib/docker/containers:/var/lib/docker/containers:ro + - /var/run/docker.sock:/var/run/docker.sock:ro + cap_drop: + - ALL + security_opt: + - "no-new-privileges:true" + read_only: true + tmpfs: + - "/tmp:mode=1777" +``` +Even with `:ro` the socket still grants read access to container metadata (env vars, image names). +For production, prefer the promtail `file`-based log scraping mode and remove the socket entirely. + +--- + +### CR-02: Grafana admin password has hardcoded `changeme` default — no enforcement gate + +**File:** `docker-compose.yml:271` +**Issue:** `GF_SECURITY_ADMIN_PASSWORD` falls back to the literal string `changeme`: +```yaml +- GF_SECURITY_ADMIN_PASSWORD=${GRAFANA_ADMIN_PASSWORD:-changeme} +``` +If an operator deploys without setting `GRAFANA_ADMIN_PASSWORD` in `.env`, Grafana starts with a +well-known default credential. Grafana has full access to Loki logs, which contain correlation IDs, +path information, and potentially PII in error messages. There is no startup check that rejects +the default. The CLAUDE.md Security Protocol requires "no default credentials". + +**Fix:** Remove the default fallback — use a strict variable reference so Docker Compose fails at +startup if the variable is unset: +```yaml +- GF_SECURITY_ADMIN_PASSWORD=${GRAFANA_ADMIN_PASSWORD:?GRAFANA_ADMIN_PASSWORD must be set in .env} +``` +Apply the same pattern to `GF_SECURITY_ADMIN_USER`. Update `.env.example` accordingly. + +--- + +### CR-03: `test_docuvault_app_cannot_create_table` silently skips when the role is absent — gate is toothless + +**File:** `backend/tests/test_migration_0006.py:269-288` +**Issue:** The test that enforces "DML-only app role cannot run DDL" calls `pytest.skip()` when the +`docuvault_app` role does not exist in the integration database: +```python +if row is None: + pytest.skip("docuvault_app role does not exist in the integration database") +``` +A `SKIP` exit is indistinguishable from a passing test in the `pytest -q` summary the security gate +uses (12 skipped — correct behavior). If an integration environment is misconfigured and the role +was never created, the privilege-escalation gate silently passes instead of catching the gap. The +skip also means the test cannot be run in CI without the exact role present — which makes the gate +depend on an undocumented pre-condition rather than failing loudly. + +**Fix:** Change `pytest.skip` to `pytest.fail` so a missing role is treated as a configuration +error that must be resolved before the gate passes. If the role genuinely cannot exist in all +environments, add an explicit environment variable (`ASSERT_APP_ROLE_EXISTS=1`) that controls +whether the absent-role branch is a skip or a failure, defaulting to failure: +```python +if row is None: + msg = ( + "docuvault_app role does not exist in the integration database. " + "Create the role with USAGE-only privilege or set SKIP_ROLE_CHECK=1 to bypass." + ) + if os.environ.get("SKIP_ROLE_CHECK") == "1": + pytest.skip(msg) + pytest.fail(msg) +``` + +--- + +## Warnings + +### WR-01: `migrate` source-code volume defeats `read_only: true` + +**File:** `docker-compose.yml:107-108` +**Issue:** The migrate service mounts the entire backend source tree: +```yaml +volumes: + - ./backend:/app +``` +This bind-mount is writable by default. A compromised or buggy migration script running under this +service could modify source files on the host (`/app/alembic/`, `main.py`, etc.) even though the +container filesystem is otherwise read-only. The `read_only: true` flag only protects the container +layer, not the bind-mounted host path. The same issue exists for `backend`, `celery-worker`, and +`celery-beat`, but is most severe for `migrate` because Alembic runs as the DDL-capable role. + +**Fix:** For the `migrate` service, mount the backend directory read-only: +```yaml +volumes: + - ./backend:/app:ro + - /tmp # if alembic needs to write temp files, use a tmpfs +``` +This is already present for the postgres initdb volume at line 10 (`:ro`). Apply the same pattern +to the migrate service. Production deployments should bake the migration scripts into the image and +omit the bind-mount entirely. + +--- + +### WR-02: Three services use `latest` image tags — non-deterministic deployments + +**File:** `docker-compose.yml:19, 245, 254, 264` +**Issue:** `minio/minio:latest`, `grafana/loki:latest`, `grafana/promtail:latest`, and +`grafana/grafana:latest` all use floating `latest` tags. CLAUDE.md mandates pinned exact versions +for security-critical packages. A `latest` tag can silently introduce breaking changes or +vulnerabilities on the next `docker compose pull`. The SECURITY.md security gate checklist requires +`docker scout` CVE scanning, which only produces reproducible results against pinned versions. + +**Fix:** Pin each image to a specific digest or semver tag: +```yaml +minio: + image: minio/minio:RELEASE.2024-11-07T00-52-20Z +loki: + image: grafana/loki:3.3.2 +promtail: + image: grafana/promtail:3.3.2 +grafana: + image: grafana/grafana:11.4.0 +``` + +--- + +### WR-03: `minio-init` service has no container hardening + +**File:** `docker-compose.yml:40-87` +**Issue:** The `minio-init` service runs an inline shell script that holds both the MinIO root +credentials and the application access key/secret. Unlike every other service, it has no +`read_only`, `cap_drop`, `security_opt`, or `restart` policy. If this container is compromised or +the shell script is manipulated via a volume-mount attack, the attacker has full MinIO admin access +(root credentials are present). It also has no `restart: "no"`, meaning Docker's default restart +policy could cause it to re-run. + +**Fix:** +```yaml +minio-init: + image: minio/mc:latest + restart: "no" + cap_drop: + - ALL + security_opt: + - "no-new-privileges:true" + read_only: true + tmpfs: + - "/tmp:mode=1777" +``` + +--- + +### WR-04: Integration test class relies on implicit execution order without ordering enforcement + +**File:** `backend/tests/test_migration_0006.py:137-288` +**Issue:** `TestMigration0005To0006` contains 12 test methods that must run in definition order +(`test_upgrade_to_0005` before `test_0005_lacks_phase12_tables` before `test_upgrade_to_head` before +schema-assertion tests). pytest does not guarantee execution order within a class by default, +especially when collected by multiple workers (`pytest-xdist`) or when tests are re-ordered by +plugins. If `test_upgrade_to_head` runs before `test_upgrade_to_0005`, the "lacks_phase12_tables" +assertions will fail spuriously — or worse, pass because the DB was already at head from a prior +test run, hiding a real regression. + +The module-scoped `pg_conn` fixture also means any test that leaves the DB in a dirty state +(partial upgrade) will corrupt all subsequent tests in the module. + +**Fix:** Add `@pytest.mark.order` (via `pytest-ordering` plugin) or restructure as sequential +functions with explicit `depends` markers. Alternatively, use a single test function that sequences +the steps explicitly and rolls back via a fixture teardown: +```python +def test_full_0005_to_0006_upgrade_cycle(migrate_dsn, pg_conn): + _run_alembic("0005", dsn=migrate_dsn) + # assert 0005 state + _run_alembic("head", dsn=migrate_dsn) + # assert 0006 state +``` + +--- + +### WR-05: `celery-beat` missing `CLOUD_CREDS_KEY`, `JWT_PRIVATE_KEY`, `JWT_PUBLIC_KEY` env vars + +**File:** `docker-compose.yml:212-243` +**Issue:** The `celery-beat` service environment block (lines 212–223) is missing +`CLOUD_CREDS_KEY`, `JWT_PRIVATE_KEY`, and `JWT_PUBLIC_KEY`. These are present in `celery-worker` +(lines 171–186). If a scheduled Celery beat task ever needs to decrypt cloud credentials (e.g., a +future refresh task scheduled via beat rather than triggered from the browse endpoint) or verify a +JWT, it will raise a `KeyError` or produce an unintelligible `settings` validation error at runtime +rather than at startup. `celery-worker` already carries all three, indicating the intent is for all +Celery processes to have access to them. + +**Fix:** Add the missing variables to `celery-beat`: +```yaml +celery-beat: + environment: + - CLOUD_CREDS_KEY=${CLOUD_CREDS_KEY} + - JWT_PRIVATE_KEY=${JWT_PRIVATE_KEY} + - JWT_PUBLIC_KEY=${JWT_PUBLIC_KEY} +``` + +--- + +## Info + +### IN-01: README version (0.2.0) diverges from backend/main.py and AGENTS.md (0.2.1) + +**File:** `README.md:3` +**Issue:** README.md reports `Version 0.2.0 — Alpha` while `backend/main.py:247` and +`frontend/package.json` (version `0.2.1`) and `AGENTS.md` all reflect `0.2.1`. CLAUDE.md mandates +a version bump in both `backend/main.py` and `frontend/package.json` and notes these are the +"sources of truth". README was not updated after the Phase 12 gap-closure patch bump. + +**Fix:** Update `README.md:3` to `**Version 0.2.1 — Alpha**`. + +--- + +### IN-02: `backend` service unnecessarily receives `DATABASE_MIGRATE_URL` + +**File:** `docker-compose.yml:129` +**Issue:** The `backend` service environment block includes `DATABASE_MIGRATE_URL`. The application +service uses `DATABASE_URL` (DML-only role). Providing the DDL-capable migrate URL to the +application process means that if any code path in the app accidentally reads `DATABASE_MIGRATE_URL` +from the environment and uses it (e.g., a misconfigured Alembic `env.py` fallback), it would +operate with DDL privileges. The principle of least privilege requires the application service to +hold only the credentials it needs. + +**Fix:** Remove `DATABASE_MIGRATE_URL` from the `backend` (and `celery-worker`) environment blocks. +It is only needed by the `migrate` service. + +--- + +### IN-03: `/health` endpoint leaks Python exception class names in response body + +**File:** `backend/main.py:299, 308` +**Issue:** The health endpoint renders exceptions as `f"error: {type(e).__name__}: {e}"`: +```python +checks["postgres"] = f"error: {type(e).__name__}: {e}" +checks["minio"] = f"error: {type(e).__name__}: {e}" +``` +The inline comment at line 289 acknowledges this ("acceptable for an internal/dev endpoint in +Phase 1. Phase 2 will trim…"), but Phase 12 is complete and the endpoint is still exposing +exception internals. Exception messages from psycopg or the MinIO SDK may include connection +strings, hostnames, or partial credentials. The CLAUDE.md Security Protocol requires "no stack +traces in API responses". + +**Fix:** Trim the error payload to a safe, opaque string for any path that could be publicly +reachable: +```python +checks["postgres"] = "error" +checks["minio"] = "error" +``` +Log the full exception to structlog at WARNING level for observability without exposing it in the +response body. + +--- + +_Reviewed: 2026-06-20_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_