Summary

Severity: Critical
Area: Security / SQL
Affected files: agentkit/tools/builtin/sql_readonly.py:74-80 (engine), :46-49 (keyword regex), :102-147 (validation), :150-193 (literal masking)
Suggested branch: fix/sql-readonly-isolation
Suggested PR title: fix(sql): isolate the read-only SQL tool from the app superuser

This is finding AUDIT-01 of 18 from a consolidated audit of agentkit/. It is deliberately self-contained so it can be fixed on its own branch with its own PR.

Related findings: Closely related: AUDIT-02 (row cap), AUDIT-18 (deployment credentials). The AST check in AUDIT-01 is defence in depth — the dedicated read-only DB role is the primary control.

AUDIT-01 — Critical — SQL tool runs as the app DB superuser; validation is bypassable

Files

• agentkit/tools/builtin/sql_readonly.py:74-80 (engine construction)
• agentkit/tools/builtin/sql_readonly.py:46-49 (keyword regex)
• agentkit/tools/builtin/sql_readonly.py:102-147 (validation entry point)
• agentkit/tools/builtin/sql_readonly.py:150-193 (literal masking)

Current code — agentkit/tools/builtin/sql_readonly.py:74-80

def get_readonly_engine() -> AsyncEngine:
    """Return the shared read-only SQLAlchemy AsyncEngine, creating it on first use."""
    global _readonly_engine
    if _readonly_engine is None:
        settings = get_settings()
        _readonly_engine = create_async_engine(settings.DATABASE_URL, pool_pre_ping=True)
    return _readonly_engine

Current code — agentkit/tools/builtin/sql_readonly.py:150-193 (excerpt)

def _mask_literals(sql: str) -> str:
    """Replace string literals and quoted identifiers with placeholders.
    ...
    """
    masked: list[str] = []
    index = 0
    length = len(sql)

    while index < length:
        char = sql[index]

        if char == "'":
            masked.append(" ")
            index += 1
            while index < length:
                if sql[index] == "'":
                    # Doubled quote is an escaped quote inside the literal, not its end.
                    if index + 1 < length and sql[index + 1] == "'":
                        index += 2
                        continue
                    index += 1
                    break
                index += 1
            continue

        if char == '"':
            ...

Why this is a bug

1. The tool connects with Settings.DATABASE_URL, i.e. the application's own superuser (postgres/postgres in the default deployment). The "read-only" guarantee is enforced only by the keyword regex.
2. _mask_literals understands '…' and "…" but not PostgreSQL dollar-quoted strings ($$…$$, $tag$…$tag$) and not backslash escapes inside E'…'. The masked text is what the keyword scan (:142-145) and the multi-statement scan (:129-130) inspect, so both scans can be blinded.
3. Side-effecting and file-reading built-ins are not on the denylist, so they are accepted and executed with superuser rights.

Reproduced — all of the following are ACCEPTED by validate_sql_query() and executed:

SELECT pg_read_file('/etc/passwd')                  -> ACCEPTED   (reads server files)
SELECT setval('runs_id_seq', 1)                     -> ACCEPTED   (side effect, not read-only)
SELECT pg_terminate_backend(12345)                  -> ACCEPTED   (kills other backends)
SELECT query_to_xml('SELECT * FROM runs',true,true,'') -> ACCEPTED (dynamic SQL execution)
SELECT $$'$$ INTO x                                 -> ACCEPTED   ("INTO" hidden by dollar-quote)
SELECT E'\''; DROP TABLE runs; --'                  -> ACCEPTED   (keyword + ";" both hidden)

Caveat (already noted in the original audit, and correct): asyncpg refuses multi-statement prepared statements, so the DROP TABLE payload does not execute. The confirmed live issues are: superuser access, single-statement file/table reads, dynamic SQL via query_to_xml, non-transactional side effects, and reading the app's own runs/steps audit tables (other users' goals and session ids).

Pass cases

• validate_sql_query raises ValueError for: pg_read_file(...), pg_read_binary_file(...), setval(...), nextval(...), pg_terminate_backend(...), pg_cancel_backend(...), query_to_xml(...), lo_import(...), dblink_exec(...), COPY ... PROGRAM, lo_export(...), pg_logical_emit_message(...).
• validate_sql_query raises ValueError for SELECT $$'$$ INTO x and for SELECT E'\''; DROP TABLE runs; --' (dollar-quoted and E'' payloads must not blind the scans).
• A legitimate SELECT containing a dollar-quoted literal without a prohibited construct is still accepted (no false positives from the new masking).
• A legitimate SELECT whose string literal contains the word insert is still accepted (existing masking behaviour preserved).
• The SQL tool connects with a dedicated, non-superuser role: an integration test asserts the connection's role is not a superuser and that default_transaction_read_only is on for that session.
• The tool fails closed: if the dedicated read-only URL/role is missing or unreachable, the tool returns ToolResult(ok=False, error=…) and performs no query (no silent fallback to the application superuser).
• The application itself (migrations, seed, run/step persistence) still uses the superuser connection and is unaffected.
• A regression test asserts the app superuser URL is never used by sql_readonly.
• docs/ARCHITECTURE.md:115 ("DB role is read-only") becomes true, or is corrected (see AUDIT-16).

Definition of done (per AGENTS.md)

• New/updated unit and integration tests covering the pass cases above, all passing (pytest).
• ruff check ., ruff format --check . and mypy agentkit clean.
• Docstrings updated where behaviour changed; docs/PROGRESS.md updated (status, PR link, commit count, notes).
• A new ADR in docs/DECISIONS.md if an architectural decision was made.
• Branch pushed and PR opened (never commit directly to main).

Baseline at the time of writing: pytest is green on main (all tests pass; tests/real/* skipped unless AGENTKIT_REAL_INFRA=1).