Summary

Severity: Critical
Area: Security / SQL
Affected files: agentkit/tools/builtin/sql_readonly.py:61-64 (_EXISTING_BOUND_REGEX), :289-305 (_apply_row_limit)
Suggested branch: fix/sql-row-limit-cap
Suggested PR title: fix(sql): enforce a real server-side row cap

This is finding AUDIT-02 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: Depends on the same area as AUDIT-01; can be fixed independently.

AUDIT-02 — Critical — LIMIT ALL / huge limits bypass the row cap

Files

• agentkit/tools/builtin/sql_readonly.py:61-64 (_EXISTING_BOUND_REGEX)
• agentkit/tools/builtin/sql_readonly.py:289-305 (_apply_row_limit)

Current code — agentkit/tools/builtin/sql_readonly.py:61-64 and :289-305

#: A clause that already bounds the statement's own output. Such statements are executed
#: untouched so a caller-supplied bound is never widened.
_EXISTING_BOUND_REGEX = re.compile(
    r"(\blimit\b|\bfetch\s+first\b)",
    re.IGNORECASE,
)

def _apply_row_limit(sql: str, row_limit: int) -> str:
    """Bound the statement's output in SQL, preferring an appended LIMIT.
    ...
    """
    tail = _top_level_tail(_mask_literals(sql))
    if _EXISTING_BOUND_REGEX.search(tail):
        return sql
    if _LOCKING_CLAUSE_REGEX.search(tail):
        return f"SELECT * FROM ({sql}) AS agentkit_limited_query LIMIT {int(row_limit)}"
    return f"{sql} LIMIT {int(row_limit)}"

Why this is a bug

Any top-level LIMIT makes the statement pass through untouched, so LIMIT ALL (PostgreSQL's "no limit") completely defeats the row cap. The executed statement then has no server-side bound at all; the client-side fetchmany(row_limit) only limits what is displayed, not what the server computes.

Reproduced

validate_sql_query("SELECT * FROM runs LIMIT ALL")        -> ACCEPTED
_apply_row_limit("SELECT * FROM runs LIMIT ALL", 50)       -> "SELECT * FROM runs LIMIT ALL"   (no cap)
_apply_row_limit("SELECT * FROM runs LIMIT 1000000000",50) -> "SELECT * FROM runs LIMIT 1000000000"

Pass cases

• LIMIT ALL (and LIMIT NULL, FETCH FIRST ROWS ONLY without a count) is replaced by the configured row limit.
• A caller-supplied limit larger than the configured maximum is clamped down to the configured maximum.
• A caller-supplied limit smaller than the configured maximum is left alone.
• The effective limit is enforced server-side (assert the SQL handed to conn.execute() contains a bounded LIMIT), not only by the client-side fetchmany.
• statement_timeout still bounds expensive uncapped scans (pg_sleep, cross join) — an existing timeout mechanism must not be weakened by the LIMIT rewrite.
• Regression tests for LIMIT, FETCH FIRST n ROWS ONLY, LIMIT n OFFSET m, LIMIT inside a subquery (must not be treated as the outer bound), and FOR UPDATE (subquery-wrap path).

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).