|
| 1 | +# ADR-0007: First-party SQL-safety kernel (de-vendor `crystaldba/postgres-mcp`) |
| 2 | + |
| 3 | +- **Status:** accepted |
| 4 | +- **Date:** 2026-07-08 |
| 5 | +- **Supersedes:** [ADR-0001](0001-build-approach.md) (the hard-fork / vendor decision) |
| 6 | + |
| 7 | +## Context |
| 8 | + |
| 9 | +[ADR-0001](0001-build-approach.md) chose to hard-fork MCPg from |
| 10 | +`crystaldba/postgres-mcp` and **vendor** its `sql/` subpackage — the |
| 11 | +`pglast`-AST SQL-safety allowlist validator plus the async connection |
| 12 | +pool / driver — near-verbatim under `src/mcpg/_vendor/sql/` (MIT, pinned |
| 13 | +commit `07eb329`). That was the right call to ship fast, but it left two |
| 14 | +standing costs: |
| 15 | + |
| 16 | +1. **~1.3k lines of security-critical code outside every quality gate.** |
| 17 | + The vendored kernel — including the SQL allowlist itself — was excluded |
| 18 | + from the coverage gate, `mypy --strict`, `ruff`, and `bandit` |
| 19 | + (`pyproject.toml` carve-outs). |
| 20 | +2. **Manual re-syncs.** Upstream changes had to be re-applied by hand with |
| 21 | + local modifications tracked in a vendor README. |
| 22 | + |
| 23 | +It was also the last third-party runtime *code* MCPg shipped. Roadmap 18.1 |
| 24 | +set out to own it. |
| 25 | + |
| 26 | +## Decision |
| 27 | + |
| 28 | +Replace the vendored kernel with a **first-party** `src/mcpg/sql/` package, |
| 29 | +re-authored from the MIT-licensed original (attribution retained in |
| 30 | +`NOTICE`). Recon showed three of the vendored modules (`bind_params.py`, |
| 31 | +`extension_utils.py`, `index.py`) had no consumer outside `_vendor/`, so |
| 32 | +they were **deleted, not rebuilt**. The kernel is re-architected to |
| 33 | +**separate policy from mechanism**: |
| 34 | + |
| 35 | +- `sql/allowlist.py` — the permitted statement / AST-node / function / |
| 36 | + extension sets, as **data** (the single auditable decision surface). |
| 37 | +- `sql/safety.py` — `SafeSqlDriver`: the `pglast` parse + AST-walker + |
| 38 | + read-only execute path. Reads policy from `allowlist.py`; cannot widen it. |
| 39 | +- `sql/driver.py` — `SqlDriver` / `DbConnPool` / `obfuscate_password` |
| 40 | + (pool + execution + credential redaction; no policy). |
| 41 | + |
| 42 | +The public seam (`SqlDriver`, `SafeSqlDriver`, `DbConnPool`, |
| 43 | +`obfuscate_password`) is unchanged, so the ~74 consuming modules only |
| 44 | +changed an import path. |
| 45 | + |
| 46 | +**Faithful re-author, not a redesign.** The security *behaviour* is pinned |
| 47 | +identical to the vendored validator: |
| 48 | + |
| 49 | +- A **differential parity harness** ran a safe/unsafe/malformed corpus |
| 50 | + through both validators — **0 divergence**. |
| 51 | +- The 760-LOC adversarial suite (ported) passes 100% against `mcpg.sql`. |
| 52 | +- A **fuzz pass** confirms the validator only ever returns a clean verdict |
| 53 | + (accept / `ValueError`) — never a crash / hang — on adversarial input. |
| 54 | +- A dedicated security-review gate (allowlist audit, `/security-review`, |
| 55 | + threat model) passed — see |
| 56 | + [`../reviews/devendor-sql-kernel-security-review.md`](../reviews/devendor-sql-kernel-security-review.md). |
| 57 | + |
| 58 | +## Consequences |
| 59 | + |
| 60 | +- The SQL-safety kernel is now inside the coverage gate (90%), |
| 61 | + `mypy --strict`, `ruff`, and `bandit` — a net security improvement over |
| 62 | + the vendored state. |
| 63 | +- `src/mcpg/_vendor/` and `tests/vendor/` are deleted; the five |
| 64 | + `pyproject.toml` `_vendor` carve-outs are removed; the module map and |
| 65 | + docs (`architecture.md`, `CLAUDE.md`, `SECURITY.md`, `NOTICE`) are |
| 66 | + first-party. |
| 67 | +- MCPg ships **no vendored runtime code**. (`pglast`, `psycopg`, and |
| 68 | + `psycopg-pool` remain upstream *library* dependencies — not vendored |
| 69 | + source.) |
| 70 | +- Attribution to `crystaldba/postgres-mcp` (MIT) is retained in `NOTICE` |
| 71 | + as the design lineage; the in-query `/* crystaldba */` marker is kept |
| 72 | + for now (a cosmetic rename is a possible follow-up). |
| 73 | + |
| 74 | +## Alternatives considered |
| 75 | + |
| 76 | +- **Keep vendoring.** Rejected — perpetuates the gate carve-outs and the |
| 77 | + manual re-sync burden; ADR-0001's "ship fast" rationale no longer applies. |
| 78 | +- **Redesign the validator (stricter/simpler allowlist).** Deferred — a |
| 79 | + behaviour change would need its own threat model + review. This ADR is a |
| 80 | + faithful re-author; any policy tightening is a separate, reviewed change |
| 81 | + (see the pre-existing observations in the security-review note). |
0 commit comments