diff --git a/migrations/core/013_guardrail_audit_error_detail.sql b/migrations/core/013_guardrail_audit_error_detail.sql new file mode 100644 index 00000000..f099a37c --- /dev/null +++ b/migrations/core/013_guardrail_audit_error_detail.sql @@ -0,0 +1,11 @@ +-- Store the RAW driver/DB error text server-side, keyed by audit_id. The caller only ever receives a +-- generic, value-free classified message (raw schema / column / value names must never cross the +-- assistant boundary), so the operator needs somewhere to see the real error when debugging a failed +-- query — that is this column. Populated ONLY on an operational failure (the executor's stderr); +-- NULL for guardrail refusals (their reasons are already clean). Portable (SQLite + Postgres). +-- +-- SENSITIVITY: a driver error can embed data VALUES (e.g. a Postgres unique-violation +-- `DETAIL: Key (email)=(...)`), so this column inherits the datasource's sensitivity classification. +-- It is strictly better than the prior behavior (which returned that text to the caller), but for a +-- PHI/PII deployment it must fall under the audit table's access-control + retention/redaction policy. +ALTER TABLE guardrail_audit ADD COLUMN error_detail TEXT; diff --git a/packages/agami-core/src/contracts.py b/packages/agami-core/src/contracts.py index 7c765be7..06a74168 100644 --- a/packages/agami-core/src/contracts.py +++ b/packages/agami-core/src/contracts.py @@ -217,3 +217,4 @@ class GuardrailAuditRecord(_Contract): execution_ms: int | None = None correlation_id: str | None = None source: str | None = None + error_detail: str | None = None # RAW driver error — server-side audit only, NEVER in the Envelope diff --git a/packages/agami-core/src/execute_sql.py b/packages/agami-core/src/execute_sql.py index b95e61dd..38ee9547 100644 --- a/packages/agami-core/src/execute_sql.py +++ b/packages/agami-core/src/execute_sql.py @@ -1347,19 +1347,25 @@ def execute_guarded( ) -> ExecResult: """The un-bypassable guarded envelope — the single execution chokepoint (REQ-002/REQ-014). - In fixed order: read-only / dangerous-SQL guard (the hard security gate — NOT bypassable via - ``no_safety``, which skips only the semantic-model pass, never write/RCE/DoS protection) -> - semantic-model safety pass (fan/chasm pre-flight + scope + PII + ``default_filters`` rewrite) -> - resolve the datasource -> ``executor.execute(vetted_sql, …)``. The executor only ever receives - SQL both guards have passed. Raises ``GuardRefused`` carrying the typed ``Refusal`` on a refusal, - and ``ExecutorError`` on a connect/run failure — so the subprocess ``main`` and the in-process MCP - handler apply the same guard and build the same envelope. The row cap rides the request-scoped - ``_max_rows_override`` ContextVar the caller sets.""" + In fixed order: read-only / dangerous-SQL guard -> recon / metadata deny-list (both hard security + gates — NOT bypassable via ``no_safety``, which skips only the semantic-model pass, never + write/RCE/DoS protection) -> semantic-model safety pass (fan/chasm pre-flight + scope + PII + + ``default_filters`` rewrite) -> resolve the datasource -> ``executor.execute(vetted_sql, …)``. The + executor only ever receives SQL every guard has passed. Raises ``GuardRefused`` carrying the typed + ``Refusal`` on a refusal, and ``ExecutorError`` on a connect/run failure — so the subprocess + ``main`` and the in-process MCP handler apply the same guards and build the same envelope. The row + cap rides the request-scoped ``_max_rows_override`` ContextVar the caller sets.""" import sql_guard verdict = sql_guard.check_read_only(sql) if verdict is not None: raise GuardRefused(_refusal_from_verdict("permission", verdict), code=1) + # Recon / metadata deny-list — refuse server-fingerprinting + system-catalog introspection + # (version(), current_user, information_schema, pg_* relations) as a distinct `recon` refusal. + # A hard gate, at the chokepoint so BOTH surfaces get it, and NOT bypassable via no_safety. + recon = sql_guard.check_no_recon(sql) + if recon is not None: + raise GuardRefused(_refusal_from_verdict("recon", recon), code=1) if not no_safety: sql, refusal = _model_safety(sql, profile, area) if refusal is not None: @@ -1424,11 +1430,11 @@ def main() -> int: profile = args.profile or _resolve_default_profile() - # Route through the single guarded envelope with the built-in executor: guard -> model-safety -> - # resolve -> connect-and-run, returning native rows we then serialize to stdout as CSV (the - # subprocess wire). The guard is the hard security gate for EVERY caller (both MCP servers, the - # agami-query skill, cron), NOT bypassable via --no-safety (which skips only the semantic-model - # pass, never write/RCE/DoS protection). + # Route through the single guarded envelope with the built-in executor: guard -> recon deny-list + # -> model-safety -> resolve -> connect-and-run, returning native rows we then serialize to stdout + # as CSV (the subprocess wire). The guard is the hard security gate for EVERY caller (both MCP + # servers, the agami-query skill, cron), NOT bypassable via --no-safety (which skips only the + # semantic-model pass, never write/RCE/DoS protection). try: result = execute_guarded( sql, profile, args.area, executor=BUILTIN_EXECUTOR, no_safety=args.no_safety diff --git a/packages/agami-core/src/guardrail.py b/packages/agami-core/src/guardrail.py index a0408862..6ef30801 100644 --- a/packages/agami-core/src/guardrail.py +++ b/packages/agami-core/src/guardrail.py @@ -50,11 +50,16 @@ # data-protection / governance — emitted by those gates "sensitive_columns", "preflight_refused", - # operational / execution failures — from the executor + DB driver + # operational / execution failures — from the executor + DB driver (classified from stderr; + # `permission` reuses the safety kind above). The rich column/table/network kinds mirror + # db_error_classifier.md. "timeout", "dsn", + "network", "driver_missing", "auth", + "column_not_found", + "table_not_found", "syntax", "other", ) diff --git a/packages/agami-core/src/model_store.py b/packages/agami-core/src/model_store.py index 20eef090..d3cf43d4 100644 --- a/packages/agami-core/src/model_store.py +++ b/packages/agami-core/src/model_store.py @@ -350,24 +350,49 @@ def record_tool_call(self, record: Any) -> None: ) self._store.commit() + _AUDIT_COLS = ( + "audit_id, ts, datasource, status, refusal_kind, sql, row_count, execution_ms, " + "correlation_id, source" + ) + def record_guardrail_audit(self, record: Any) -> None: - self._store.execute( - "INSERT INTO guardrail_audit (audit_id, ts, datasource, status, refusal_kind, sql, " - "row_count, execution_ms, correlation_id, source) " - "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", - ( - record.audit_id, - record.ts, - record.datasource, - record.status, - record.refusal_kind, - record.sql, - record.row_count, - record.execution_ms, - record.correlation_id, - record.source, - ), + base = ( + record.audit_id, + record.ts, + record.datasource, + record.status, + record.refusal_kind, + record.sql, + record.row_count, + record.execution_ms, + record.correlation_id, + record.source, ) + try: + self._store.execute( + f"INSERT INTO guardrail_audit ({self._AUDIT_COLS}, error_detail) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", + (*base, record.error_detail), + ) + except Exception as exc: + # ONLY the pre-013 schema (the error_detail column isn't there yet — new code against an + # un-migrated DB) is retried WITHOUT the raw detail. Any OTHER failure must propagate: a + # blanket retry would both mask a real DB error and silently drop error_detail (Copilot + # review). Match the missing-column signature across drivers (sqlite "has no column named", + # Postgres "does not exist" / UndefinedColumn). + msg = str(exc).lower() + missing_error_detail = "error_detail" in msg and ( + "no column" in msg or "does not exist" in msg or "undefined" in msg + ) + if not missing_error_detail: + raise # a genuine DB failure — surface it to the best-effort caller, don't mask it + # Roll back first so a Postgres aborted-transaction doesn't fail the retry. + self._store.rollback() + self._store.execute( + f"INSERT INTO guardrail_audit ({self._AUDIT_COLS}) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", + base, + ) self._store.commit() diff --git a/packages/agami-core/src/sql_guard.py b/packages/agami-core/src/sql_guard.py index df32c693..aa8129e9 100644 --- a/packages/agami-core/src/sql_guard.py +++ b/packages/agami-core/src/sql_guard.py @@ -15,6 +15,11 @@ `check_read_only(sql)` returns `None` when the SQL is a single safe read-only statement, else a safety `Verdict` the shared executor maps to a refusal (`kind="permission"`). The rejection ladder itself lives in `_read_only_reason`. + +`check_no_recon(sql)` is the companion metadata/recon gate — it returns a `Verdict` +(`kind="recon"`) for a query that fingerprints the server or reads the system catalog +(`version()`, `current_user`, `information_schema`, `pg_*` relations). Both run at the +same chokepoint over the same neutralized SQL. """ from __future__ import annotations @@ -233,6 +238,101 @@ def _neutralize(sql: str) -> str: ) +# --------------------------------------------------------------------------- +# Recon / metadata deny-list. A query can pass read-only + object-scope and STILL fingerprint the +# server (`version()`, `current_user`, `inet_server_addr()`), enumerate or dump the system catalog +# (`information_schema`, `pg_catalog`, any `pg_*` relation — including `pg_stats` sampled VALUES and +# `pg_authid` password hashes), read catalog DDL (`pg_get_viewdef`/`pg_get_functiondef`), or probe +# object existence (`has_table_privilege`, `to_regclass`) — all leaking DB internals across the LLM +# boundary. The read-only role can't revoke catalog access on most engines (Q1), so this is the +# INTENDED backstop and denies it app-side, as a DISTINCT `recon` refusal. Matched over the SAME +# neutralized SQL as `check_read_only` (no second parser); the sets/prefixes below are the source of +# truth — adding an engine's recon builtin is a one-line edit. +# +# FP discipline (regex, not AST — so exact-name matching is the residual): the paren set requires a +# trailing `(`, so bare `version`/`database`/`user` COLUMNS pass; the niladic + relation sets use a +# `.`-lookbehind so a qualified column (`t.current_user`, `t.pg_class`) passes; schema names require a +# trailing `.`. The `pg_`/`stl_`/… prefixes match RESERVED namespaces (Postgres reserves the `pg_` +# object-name prefix; Redshift reserves `stl_/stv_/svl_/svv_`), so the FP surface is a bare, unqualified +# column literally prefixed `pg_`/`stl_` — rare, documented, pinned in the FP corpus. Bare +# `user`/`schema`/`database` (Postgres niladic synonyms) are DELIBERATELY omitted from the niladic set — +# far too common as intended column names; only their `()`-call form is denied (a known, minor +# username-fingerprint residual). +_RECON_PAREN_FNS = frozenset( + { + "version", # server version string (pg / snowflake) + "current_database", + "current_schemas", # pg (plural, takes a bool) — the niladic current_schema is below + "database", + "schema", + "user", # MySQL user() / system_user() — the CALL form only + "connection_id", + "system_user", + "current_account", # snowflake + "current_region", + "current_version", + "current_warehouse", + "inet_server_addr", # server/client network fingerprint (pg) + "inet_server_port", + "inet_client_addr", + "inet_client_port", + } +) +# Function FAMILIES matched by prefix/pattern (each still requires the trailing `(`): every `pg_*` +# call (introspection + DDL-dump like pg_get_viewdef / size/topology like pg_relation_size — the +# dangerous subset is already denied earlier by check_read_only as `permission`), the +# has_*_privilege family (object-existence probing), and to_reg* (name→OID resolution / enumeration). +_RECON_CALL_FAMILIES = (r"pg_\w+", r"has_\w+_privilege", r"to_reg\w+") +_RECON_NILADIC = frozenset( + {"current_user", "session_user", "current_catalog", "current_schema", "current_role"} +) +_RECON_SCHEMAS = frozenset( + {"information_schema", "pg_catalog", "account_usage", "mysql", "performance_schema", "sys"} +) +# High-value catalog relations named explicitly for readability; the `pg_` prefix below is the actual +# catch-all (it also covers pg_stats / pg_authid / pg_statistic / pg_stat_statements / pg_type / … ). +_RECON_BARE_RELATIONS = frozenset( + { + "pg_tables", + "pg_class", + "pg_stat_activity", + "pg_roles", + "pg_user", + "pg_shadow", + "pg_authid", + "pg_stats", + } +) +# Reserved relation-name namespaces (unshadowable-by-convention): Postgres `pg_`, Redshift system tables. +_RECON_RELATION_PREFIXES = ("pg_", "stl_", "stv_", "svl_", "svv_") + + +def _recon_group(names: frozenset[str]) -> str: + # Longest-first so an alternation prefers the more specific name (regex alternation is greedy per + # position but this also keeps `current_schemas` from being shadowed by `current_schema`). + return "|".join(sorted(names, key=len, reverse=True)) + + +_RECON_PAREN_RE = re.compile( + rf"\b({_recon_group(_RECON_PAREN_FNS)}|{'|'.join(_RECON_CALL_FAMILIES)})\s*\(", re.IGNORECASE +) +# `(? str | None: """Return None if `sql` is a single safe read-only statement, else a reason string. @@ -314,3 +414,46 @@ def check_read_only(sql: str | None) -> Verdict | None: "Send a single read-only SELECT / WITH...SELECT — no DML, DDL, transaction/session " "control, or multiple statements.", ) + + +def check_no_recon(sql: str | None) -> Verdict | None: + """Return ``None`` if ``sql`` calls no metadata/recon function and reads no system catalog, else a + safety ``Verdict`` the shared executor maps to a refusal (``kind=recon``). + + Runs AFTER :func:`check_read_only` at the shared executor chokepoint (and as a fail-fast pre-check + in the tool layer). Detection is regex over the SAME neutralized SQL — comments + literals blanked, + quoted identifiers unwrapped — so a recon token hidden in a string/comment can't smuggle past, and + (symmetrically) a legitimate mention inside a literal/comment doesn't false-trip. See the + ``_RECON_*`` sets for the deny-list and its false-positive discipline. + """ + if not sql or not sql.strip(): + return None # empty — check_read_only owns that rejection + try: + stripped = _neutralize(sql) + except _GuardReject: + # Dialect-ambiguous form we can't neutralize → fail CLOSED. In the normal flow check_read_only + # (same neutralizer) already refused this input; failing closed here keeps the recon gate safe + # even for a hypothetical caller that invoked it standalone or reordered the two checks. + return safety_verdict( + "recon", + "the query uses a form that can't be safely analyzed for metadata/recon access", + "Send a single, unambiguous read-only SELECT over your declared tables and columns.", + ) + + m = ( + _RECON_PAREN_RE.search(stripped) + or _RECON_NILADIC_RE.search(stripped) + or _RECON_SCHEMA_RE.search(stripped) + or _RECON_RELATION_RE.search(stripped) + or _RECON_SYSVAR_RE.search(stripped) + ) + if m is None: + return None + hit = m.group(0).strip(" .(") # the matched token, without the trailing `(` / `.` anchor + return safety_verdict( + "recon", + f"metadata/recon access is not allowed (`{hit}`) — server/version fingerprinting and " + "system-catalog / information_schema introspection are blocked", + "Query only your declared tables and columns; drop any server-metadata function or " + "system-catalog / information_schema reference.", + ) diff --git a/packages/agami-core/src/store.py b/packages/agami-core/src/store.py index 24168f03..c8199fd5 100644 --- a/packages/agami-core/src/store.py +++ b/packages/agami-core/src/store.py @@ -124,6 +124,9 @@ def query(self, sql: str, params: tuple = ()) -> list[dict[str, Any]]: def commit(self) -> None: self.conn.commit() + def rollback(self) -> None: + self.conn.rollback() + def close(self) -> None: self.conn.close() diff --git a/packages/agami-core/src/tools.py b/packages/agami-core/src/tools.py index f1e28b71..f8869df2 100644 --- a/packages/agami-core/src/tools.py +++ b/packages/agami-core/src/tools.py @@ -202,6 +202,15 @@ def check_read_only(sql: str) -> Verdict | None: return sql_guard.check_read_only(sql) +def check_no_recon(sql: str) -> Verdict | None: + """Return None if the SQL calls no recon/metadata function and reads no system catalog, else a + safety Verdict (`recon`). Thin fail-fast wrapper over the SAME `sql_guard` gate the executor + enforces — blocking here avoids spawning the executor subprocess for a recon query.""" + import sql_guard + + return sql_guard.check_no_recon(sql) + + # --------------------------------------------------------------------------- # Tool implementations # --------------------------------------------------------------------------- @@ -866,6 +875,125 @@ def _classify_exit(code: int) -> str: }.get(code, "other") +# Value-free, user-facing messages per error kind. These NEVER echo the raw driver text (schema / +# column / value names) — that goes only to the server-side audit trail. The kinds mirror the shared +# taxonomy in plugins/agami/shared/db_error_classifier.md (whose remediations these paraphrase without +# the object/host placeholders). +_ERROR_KINDS: dict[str, tuple[str, str]] = { + "auth": ( + "Authentication to the database failed.", + "Check the datasource credentials and retry.", + ), + "dsn": ( + "The datasource host or path could not be resolved.", + "Check the datasource connection settings.", + ), + "network": ( + "Network error reaching the database.", + "Confirm the database is reachable (VPN / firewall / port) and retry.", + ), + "driver_missing": ( + "The database driver is not installed on the server.", + "Install the driver for this datasource on the server.", + ), + "permission": ( + "The database user lacks SELECT permission on a referenced object.", + "Grant the read-only role SELECT on the referenced tables, then retry.", + ), + "column_not_found": ( + "The query referenced a column that does not exist.", + "Re-check the query against the current schema; re-introspect if the model has drifted.", + ), + "table_not_found": ( + "The query referenced a table that does not exist.", + "Re-check the query against the current schema; re-introspect if the model has drifted.", + ), + "syntax": ( + "The generated SQL had a syntax error.", + "Re-run the query — generation usually self-corrects.", + ), + "timeout": ( + "The query was canceled (it timed out).", + "Add a filter, a LIMIT, or a date range to reduce the scan, then retry.", + ), + "other": ("The query failed; the details were logged for the operator.", ""), +} + + +def _classify_db_error(stderr: str | None, returncode: int) -> tuple[str, str, str]: + """Classify an executor operational failure into a ``(kind, reason, remediation)``. + + The RICH ``kind`` (mirroring ``db_error_classifier.md``) drives the assistant's handling; the + ``reason`` / ``remediation`` are FIXED, value-free strings from ``_ERROR_KINDS`` — they never echo + the raw driver text, which is captured separately for the server-side audit trail only. Detection + layers the exit-code prior with stderr-substring refinement.""" + text = (stderr or "").lower() + + def has(*needles: str) -> bool: + return any(n in text for n in needles) + + if returncode == 3 or has("no module named", "modulenotfounderror", "command not found"): + kind = "driver_missing" + elif has( + "permission denied", + "insufficient_privileges", + "command denied", + "insufficient_access_or_readonly", + ): + kind = "permission" + elif has( + "canceling statement", + "statement_timeout", + "query was canceled", + "querycanceled", + "lost connection during query", + ): + kind = "timeout" + elif has( + "no such column", "unknown column", "undefinedcolumn", "invalid identifier", "invalid_field" + ) or ("column" in text and has("does not exist", "not found")): + kind = "column_not_found" + elif has("no such table", "undefinedtable") or ( + has("relation", "table", "object") and has("does not exist", "doesn't exist", "not found") + ): + # `object` covers Snowflake `Object '' does not exist` (the name is between the words, + # so it's not a contiguous substring) — checked before `syntax` so it wins over Snowflake's + # `SQL compilation error` prefix. + kind = "table_not_found" + elif has("syntax error", "syntaxerror", "compilation error", "error in your sql syntax"): + kind = "syntax" + elif has( + "could not translate host", + "name or service not known", + "getaddrinfo", + "unknown mysql server host", + "can't connect", + "no such file or directory", + ): + kind = "dsn" + elif has( + "connection refused", + "timed out", + "connection reset", + "could not connect", + "wrong_version_number", + ): + kind = "network" + elif has( + "password authentication failed", + "no pg_hba", + "incorrect username or password", + "access denied", + ): + kind = "auth" + else: + # No substring matched — fall back to the exit-code prior (2→dsn, 3→driver_missing, + # 4→auth, 5→syntax, else→other). + kind = _classify_exit(returncode) + reason, remediation = _ERROR_KINDS.get(kind, _ERROR_KINDS["other"]) + return kind, reason, remediation + + def _executor_truncated(stderr: str | None) -> bool: """True if execute_sql flagged a bounded-fetch truncation (ACE-038/044). The executor emits a non-error `{"truncated": {"row_cap": N}}` line on stderr alongside any other notices; scan for it.""" @@ -963,12 +1091,16 @@ def _finalize_execution( def _run_in_process( sql: str, profile: str, area: str | None, max_rows: int | None, executor: Any -) -> tuple[list, list, bool] | Refusal: +) -> tuple[list, list, bool] | tuple[Refusal, str | None]: """Run through the in-process executor behind the shared guarded envelope (no subprocess, no CSV - round-trip). Returns ``(columns, data_rows, truncated)`` on success, or the typed ``Refusal`` on a - guard refusal / execution failure — the SAME refusal the subprocess path produces (``GuardRefused`` - now carries the typed ``Refusal``, incl. the model-safety detail), so the two paths build an - identical refused Envelope. + round-trip). Returns ``(columns, data_rows, truncated)`` on success, or ``(Refusal, error_detail)`` + on a guard refusal / execution failure — the SAME refusal the subprocess path produces. + + A guard refusal (read-only / recon / model-safety) carries its own clean typed ``Refusal`` and no + ``error_detail``. An execution/config failure (``ExecutorError``) is classified into a VALUE-FREE + ``Refusal`` via ``_classify_db_error`` — mirroring the subprocess ``_refusal_from_stderr`` — with + the raw driver text returned SEPARATELY as ``error_detail`` for the server-side audit trail only, + never in the refusal reason (which would leak schema / column / value names, or a DSN). Rows are textualized to match the subprocess CSV wire (``None`` → ``""``, else ``str``) so the two paths return observably identical JSON. Native-typed rows are a deliberately deferred decision @@ -982,14 +1114,20 @@ def _run_in_process( try: result = execute_sql.execute_guarded(sql, profile, area, executor=executor) except execute_sql.GuardRefused as refusal: - return refusal.refusal # typed Refusal (read-only OR model-safety) — identical to subprocess + return refusal.refusal, None # typed Refusal (read-only / recon / model-safety) — already clean except execute_sql.ExecutorError as exc: - return Refusal(_classify_exit(exc.code), reason=exc.msg) + # Sanitize like the subprocess wire: a value-free classified refusal out, the raw driver text + # to the audit trail only. Parity with `_refusal_from_stderr` closes the in-process leak. + kind, reason, remediation = _classify_db_error(exc.msg, exc.code) + return ( + Refusal(kind=kind, reason=reason, remediation=remediation), + (exc.msg or "").strip()[:500] or None, + ) except SystemExit as exc: # Defence-in-depth: a residual/future sys.exit deep in a driver becomes a fail-closed refusal # rather than taking down the host process. code = exc.code if isinstance(exc.code, int) else 2 - return Refusal(_classify_exit(code), reason="Datasource configuration error.") + return Refusal(_classify_exit(code), reason="Datasource configuration error."), None finally: execute_sql._max_rows_override.reset(cap_token) @@ -1002,10 +1140,15 @@ def _run_in_process( return columns, data_rows, truncated -def _refusal_from_stderr(stderr: str | None, returncode: int) -> Refusal: - """Build a Refusal from the executor's stderr. A guardrail refusal is a `{"refusal": {...}}` - line (safety / model gates); anything else is an execution/config failure classified by exit - code. Raw stderr is used only as the reason for the latter — never as a guardrail reason.""" +def _refusal_from_stderr(stderr: str | None, returncode: int) -> tuple[Refusal, str | None]: + """Build a ``(Refusal, error_detail)`` from the executor's stderr. + + A guardrail refusal is a ``{"refusal": {...}}`` line (safety / model gates) — its reason is + already clean → ``(Refusal(...), None)``. Anything else is an execution/config failure: it is + classified into a value-free message (``_classify_db_error``) and the RAW stderr is returned + SEPARATELY as ``error_detail`` for the server-side audit trail ONLY. Raw driver text (schema / + column / value names) never crosses the boundary in the ``Refusal`` — the previous behavior of + surfacing stderr as the reason leaked exactly that.""" for line in (stderr or "").splitlines(): line = line.strip() if line.startswith("{") and '"refusal"' in line: @@ -1014,15 +1157,17 @@ def _refusal_from_stderr(stderr: str | None, returncode: int) -> Refusal: except ValueError: continue if isinstance(r, dict) and r.get("kind"): - return Refusal( - kind=r["kind"], - reason=r.get("reason", ""), - remediation=r.get("remediation", ""), + return ( + Refusal( + kind=r["kind"], + reason=r.get("reason", ""), + remediation=r.get("remediation", ""), + ), + None, ) - return Refusal( - kind=_classify_exit(returncode), - reason=(stderr or "").strip() or "execute_sql.py failed", - ) + kind, reason, remediation = _classify_db_error(stderr, returncode) + error_detail = (stderr or "").strip()[:500] or None + return Refusal(kind=kind, reason=reason, remediation=remediation), error_detail def _envelope_json(env: Envelope) -> str: @@ -1035,6 +1180,7 @@ def _finish( *, sql: str | None = None, execution_ms: int | None = None, + error_detail: str | None = None, ) -> str: """Record the guardrail audit row keyed by ``env.audit_id``, then return the Envelope JSON. @@ -1056,6 +1202,7 @@ def _finish( "execution_ms": execution_ms, "correlation_id": args.get("correlation_id"), "source": "mcp_server", + "error_detail": error_detail, # raw driver text, server-side only } ) except Exception as exc: @@ -1084,9 +1231,7 @@ def tool_execute_sql(args: dict[str, Any]) -> str: sql = args.get("sql") if not isinstance(sql, str) or not sql.strip(): return _finish( - Envelope.refused( - Refusal("other", "Pass a non-empty `sql` string."), audit_id=audit_id - ), + Envelope.refused(Refusal("other", "Pass a non-empty `sql` string."), audit_id=audit_id), args, ) @@ -1100,6 +1245,14 @@ def tool_execute_sql(args: dict[str, Any]) -> str: sql=sql, ) + recon = check_no_recon(sql) + if recon is not None: + return _finish( + Envelope.refused(Refusal("recon", recon.detail, recon.remediation), audit_id=audit_id), + args, + sql=sql, + ) + profile = resolve_profile(args.get("datasource")) max_rows = args.get("max_rows") try: @@ -1118,12 +1271,14 @@ def tool_execute_sql(args: dict[str, Any]) -> str: started = time.monotonic() outcome = _run_in_process(sql, profile, area, max_rows, _INJECTED_EXECUTOR) execution_ms = int((time.monotonic() - started) * 1000) - if isinstance(outcome, Refusal): # guard refusal / execution error + if isinstance(outcome[0], Refusal): # guard refusal / execution error → (Refusal, error_detail) + refusal, error_detail = outcome return _finish( - Envelope.refused(outcome, audit_id=audit_id), + Envelope.refused(refusal, audit_id=audit_id), args, sql=sql, execution_ms=execution_ms, + error_detail=error_detail, ) columns, data_rows, truncated = outcome return _finalize_execution( @@ -1160,13 +1315,13 @@ def tool_execute_sql(args: dict[str, Any]) -> str: execution_ms = int((time.monotonic() - started) * 1000) if proc.returncode != 0: + refusal, error_detail = _refusal_from_stderr(proc.stderr, proc.returncode) return _finish( - Envelope.refused( - _refusal_from_stderr(proc.stderr, proc.returncode), audit_id=audit_id - ), + Envelope.refused(refusal, audit_id=audit_id), args, sql=sql, execution_ms=execution_ms, + error_detail=error_detail, ) # Parse the RFC-4180 CSV emitted on stdout. diff --git a/plugins/agami/lib/execute_sql.py b/plugins/agami/lib/execute_sql.py index b95e61dd..38ee9547 100644 --- a/plugins/agami/lib/execute_sql.py +++ b/plugins/agami/lib/execute_sql.py @@ -1347,19 +1347,25 @@ def execute_guarded( ) -> ExecResult: """The un-bypassable guarded envelope — the single execution chokepoint (REQ-002/REQ-014). - In fixed order: read-only / dangerous-SQL guard (the hard security gate — NOT bypassable via - ``no_safety``, which skips only the semantic-model pass, never write/RCE/DoS protection) -> - semantic-model safety pass (fan/chasm pre-flight + scope + PII + ``default_filters`` rewrite) -> - resolve the datasource -> ``executor.execute(vetted_sql, …)``. The executor only ever receives - SQL both guards have passed. Raises ``GuardRefused`` carrying the typed ``Refusal`` on a refusal, - and ``ExecutorError`` on a connect/run failure — so the subprocess ``main`` and the in-process MCP - handler apply the same guard and build the same envelope. The row cap rides the request-scoped - ``_max_rows_override`` ContextVar the caller sets.""" + In fixed order: read-only / dangerous-SQL guard -> recon / metadata deny-list (both hard security + gates — NOT bypassable via ``no_safety``, which skips only the semantic-model pass, never + write/RCE/DoS protection) -> semantic-model safety pass (fan/chasm pre-flight + scope + PII + + ``default_filters`` rewrite) -> resolve the datasource -> ``executor.execute(vetted_sql, …)``. The + executor only ever receives SQL every guard has passed. Raises ``GuardRefused`` carrying the typed + ``Refusal`` on a refusal, and ``ExecutorError`` on a connect/run failure — so the subprocess + ``main`` and the in-process MCP handler apply the same guards and build the same envelope. The row + cap rides the request-scoped ``_max_rows_override`` ContextVar the caller sets.""" import sql_guard verdict = sql_guard.check_read_only(sql) if verdict is not None: raise GuardRefused(_refusal_from_verdict("permission", verdict), code=1) + # Recon / metadata deny-list — refuse server-fingerprinting + system-catalog introspection + # (version(), current_user, information_schema, pg_* relations) as a distinct `recon` refusal. + # A hard gate, at the chokepoint so BOTH surfaces get it, and NOT bypassable via no_safety. + recon = sql_guard.check_no_recon(sql) + if recon is not None: + raise GuardRefused(_refusal_from_verdict("recon", recon), code=1) if not no_safety: sql, refusal = _model_safety(sql, profile, area) if refusal is not None: @@ -1424,11 +1430,11 @@ def main() -> int: profile = args.profile or _resolve_default_profile() - # Route through the single guarded envelope with the built-in executor: guard -> model-safety -> - # resolve -> connect-and-run, returning native rows we then serialize to stdout as CSV (the - # subprocess wire). The guard is the hard security gate for EVERY caller (both MCP servers, the - # agami-query skill, cron), NOT bypassable via --no-safety (which skips only the semantic-model - # pass, never write/RCE/DoS protection). + # Route through the single guarded envelope with the built-in executor: guard -> recon deny-list + # -> model-safety -> resolve -> connect-and-run, returning native rows we then serialize to stdout + # as CSV (the subprocess wire). The guard is the hard security gate for EVERY caller (both MCP + # servers, the agami-query skill, cron), NOT bypassable via --no-safety (which skips only the + # semantic-model pass, never write/RCE/DoS protection). try: result = execute_guarded( sql, profile, args.area, executor=BUILTIN_EXECUTOR, no_safety=args.no_safety diff --git a/plugins/agami/lib/guardrail.py b/plugins/agami/lib/guardrail.py index a0408862..6ef30801 100644 --- a/plugins/agami/lib/guardrail.py +++ b/plugins/agami/lib/guardrail.py @@ -50,11 +50,16 @@ # data-protection / governance — emitted by those gates "sensitive_columns", "preflight_refused", - # operational / execution failures — from the executor + DB driver + # operational / execution failures — from the executor + DB driver (classified from stderr; + # `permission` reuses the safety kind above). The rich column/table/network kinds mirror + # db_error_classifier.md. "timeout", "dsn", + "network", "driver_missing", "auth", + "column_not_found", + "table_not_found", "syntax", "other", ) diff --git a/plugins/agami/lib/sql_guard.py b/plugins/agami/lib/sql_guard.py index df32c693..aa8129e9 100644 --- a/plugins/agami/lib/sql_guard.py +++ b/plugins/agami/lib/sql_guard.py @@ -15,6 +15,11 @@ `check_read_only(sql)` returns `None` when the SQL is a single safe read-only statement, else a safety `Verdict` the shared executor maps to a refusal (`kind="permission"`). The rejection ladder itself lives in `_read_only_reason`. + +`check_no_recon(sql)` is the companion metadata/recon gate — it returns a `Verdict` +(`kind="recon"`) for a query that fingerprints the server or reads the system catalog +(`version()`, `current_user`, `information_schema`, `pg_*` relations). Both run at the +same chokepoint over the same neutralized SQL. """ from __future__ import annotations @@ -233,6 +238,101 @@ def _neutralize(sql: str) -> str: ) +# --------------------------------------------------------------------------- +# Recon / metadata deny-list. A query can pass read-only + object-scope and STILL fingerprint the +# server (`version()`, `current_user`, `inet_server_addr()`), enumerate or dump the system catalog +# (`information_schema`, `pg_catalog`, any `pg_*` relation — including `pg_stats` sampled VALUES and +# `pg_authid` password hashes), read catalog DDL (`pg_get_viewdef`/`pg_get_functiondef`), or probe +# object existence (`has_table_privilege`, `to_regclass`) — all leaking DB internals across the LLM +# boundary. The read-only role can't revoke catalog access on most engines (Q1), so this is the +# INTENDED backstop and denies it app-side, as a DISTINCT `recon` refusal. Matched over the SAME +# neutralized SQL as `check_read_only` (no second parser); the sets/prefixes below are the source of +# truth — adding an engine's recon builtin is a one-line edit. +# +# FP discipline (regex, not AST — so exact-name matching is the residual): the paren set requires a +# trailing `(`, so bare `version`/`database`/`user` COLUMNS pass; the niladic + relation sets use a +# `.`-lookbehind so a qualified column (`t.current_user`, `t.pg_class`) passes; schema names require a +# trailing `.`. The `pg_`/`stl_`/… prefixes match RESERVED namespaces (Postgres reserves the `pg_` +# object-name prefix; Redshift reserves `stl_/stv_/svl_/svv_`), so the FP surface is a bare, unqualified +# column literally prefixed `pg_`/`stl_` — rare, documented, pinned in the FP corpus. Bare +# `user`/`schema`/`database` (Postgres niladic synonyms) are DELIBERATELY omitted from the niladic set — +# far too common as intended column names; only their `()`-call form is denied (a known, minor +# username-fingerprint residual). +_RECON_PAREN_FNS = frozenset( + { + "version", # server version string (pg / snowflake) + "current_database", + "current_schemas", # pg (plural, takes a bool) — the niladic current_schema is below + "database", + "schema", + "user", # MySQL user() / system_user() — the CALL form only + "connection_id", + "system_user", + "current_account", # snowflake + "current_region", + "current_version", + "current_warehouse", + "inet_server_addr", # server/client network fingerprint (pg) + "inet_server_port", + "inet_client_addr", + "inet_client_port", + } +) +# Function FAMILIES matched by prefix/pattern (each still requires the trailing `(`): every `pg_*` +# call (introspection + DDL-dump like pg_get_viewdef / size/topology like pg_relation_size — the +# dangerous subset is already denied earlier by check_read_only as `permission`), the +# has_*_privilege family (object-existence probing), and to_reg* (name→OID resolution / enumeration). +_RECON_CALL_FAMILIES = (r"pg_\w+", r"has_\w+_privilege", r"to_reg\w+") +_RECON_NILADIC = frozenset( + {"current_user", "session_user", "current_catalog", "current_schema", "current_role"} +) +_RECON_SCHEMAS = frozenset( + {"information_schema", "pg_catalog", "account_usage", "mysql", "performance_schema", "sys"} +) +# High-value catalog relations named explicitly for readability; the `pg_` prefix below is the actual +# catch-all (it also covers pg_stats / pg_authid / pg_statistic / pg_stat_statements / pg_type / … ). +_RECON_BARE_RELATIONS = frozenset( + { + "pg_tables", + "pg_class", + "pg_stat_activity", + "pg_roles", + "pg_user", + "pg_shadow", + "pg_authid", + "pg_stats", + } +) +# Reserved relation-name namespaces (unshadowable-by-convention): Postgres `pg_`, Redshift system tables. +_RECON_RELATION_PREFIXES = ("pg_", "stl_", "stv_", "svl_", "svv_") + + +def _recon_group(names: frozenset[str]) -> str: + # Longest-first so an alternation prefers the more specific name (regex alternation is greedy per + # position but this also keeps `current_schemas` from being shadowed by `current_schema`). + return "|".join(sorted(names, key=len, reverse=True)) + + +_RECON_PAREN_RE = re.compile( + rf"\b({_recon_group(_RECON_PAREN_FNS)}|{'|'.join(_RECON_CALL_FAMILIES)})\s*\(", re.IGNORECASE +) +# `(? str | None: """Return None if `sql` is a single safe read-only statement, else a reason string. @@ -314,3 +414,46 @@ def check_read_only(sql: str | None) -> Verdict | None: "Send a single read-only SELECT / WITH...SELECT — no DML, DDL, transaction/session " "control, or multiple statements.", ) + + +def check_no_recon(sql: str | None) -> Verdict | None: + """Return ``None`` if ``sql`` calls no metadata/recon function and reads no system catalog, else a + safety ``Verdict`` the shared executor maps to a refusal (``kind=recon``). + + Runs AFTER :func:`check_read_only` at the shared executor chokepoint (and as a fail-fast pre-check + in the tool layer). Detection is regex over the SAME neutralized SQL — comments + literals blanked, + quoted identifiers unwrapped — so a recon token hidden in a string/comment can't smuggle past, and + (symmetrically) a legitimate mention inside a literal/comment doesn't false-trip. See the + ``_RECON_*`` sets for the deny-list and its false-positive discipline. + """ + if not sql or not sql.strip(): + return None # empty — check_read_only owns that rejection + try: + stripped = _neutralize(sql) + except _GuardReject: + # Dialect-ambiguous form we can't neutralize → fail CLOSED. In the normal flow check_read_only + # (same neutralizer) already refused this input; failing closed here keeps the recon gate safe + # even for a hypothetical caller that invoked it standalone or reordered the two checks. + return safety_verdict( + "recon", + "the query uses a form that can't be safely analyzed for metadata/recon access", + "Send a single, unambiguous read-only SELECT over your declared tables and columns.", + ) + + m = ( + _RECON_PAREN_RE.search(stripped) + or _RECON_NILADIC_RE.search(stripped) + or _RECON_SCHEMA_RE.search(stripped) + or _RECON_RELATION_RE.search(stripped) + or _RECON_SYSVAR_RE.search(stripped) + ) + if m is None: + return None + hit = m.group(0).strip(" .(") # the matched token, without the trailing `(` / `.` anchor + return safety_verdict( + "recon", + f"metadata/recon access is not allowed (`{hit}`) — server/version fingerprinting and " + "system-catalog / information_schema introspection are blocked", + "Query only your declared tables and columns; drop any server-metadata function or " + "system-catalog / information_schema reference.", + ) diff --git a/tests/test_ah012_executor_seam.py b/tests/test_ah012_executor_seam.py index 197f18bf..4a63c414 100644 --- a/tests/test_ah012_executor_seam.py +++ b/tests/test_ah012_executor_seam.py @@ -259,7 +259,10 @@ def execute(self, vetted_sql, creds, *, profile): out = json.loads(tools.tool_execute_sql({"sql": "SELECT 1", "datasource": "acme"})) assert out["refusal"]["kind"] == tools._classify_exit(4) - assert "connect failed" in out["refusal"]["reason"] # ExecutorError msg rides the refusal reason + # Sanitized (ACE-039): the raw driver text never rides the reason — a value-free classified + # message does; the raw goes to the server-side audit trail only. Parity with the subprocess wire. + assert "connect failed" not in out["refusal"]["reason"] and "refused" not in out["refusal"]["reason"] + assert out["refusal"]["reason"] # a non-empty, value-free reason def test_set_injected_executor_rejects_a_bad_shape(): @@ -273,10 +276,11 @@ class _NotAnExecutor: assert tools._INJECTED_EXECUTOR is None # rejected, nothing stored -def test_injected_executor_credential_error_surfaces_detailed_remediation(monkeypatch): - # Parity with the subprocess path: a bad-profile ExecutorError carries its detailed message, so - # the in-process tool envelope surfaces the SAME remediation the CLI stderr would (not a generic - # string). This is why _load_credentials/_parse_dsn raise instead of sys.exit. +def test_injected_executor_credential_error_is_sanitized_not_leaked(monkeypatch): + # Parity with the subprocess path (ACE-039): a bad-profile ExecutorError is classified into a + # VALUE-FREE refusal reason — the raw message (which can name a DSN / host / env var) never rides + # the reason; it goes to the server-side audit trail only. This is why _load_credentials/_parse_dsn + # raise ExecutorError (so the classifier can sanitize) rather than surfacing the raw text. import tools monkeypatch.setattr(tools, "resolve_profile", lambda ds: "acme") @@ -292,7 +296,9 @@ def _bad(profile): out = json.loads(tools.tool_execute_sql({"sql": "SELECT 1", "datasource": "acme"})) - assert "DATASOURCE_URL" in out["refusal"]["reason"] # detailed, not the generic net string + assert out["status"] == "refused" + assert "DATASOURCE_URL" not in out["refusal"]["reason"] # value-free, not the raw config text + assert out["refusal"]["reason"] # a non-empty classified reason def test_injected_executor_systemexit_is_caught_not_fatal(monkeypatch): diff --git a/tests/test_contracts.py b/tests/test_contracts.py index a8426f4f..7dae4312 100644 --- a/tests/test_contracts.py +++ b/tests/test_contracts.py @@ -126,6 +126,7 @@ def test_guardrail_audit_record_roundtrip(): "execution_ms": None, "correlation_id": "turn-1", "source": "mcp_server", + "error_detail": None, } assert _roundtrip(GuardrailAuditRecord, sample) == sample diff --git a/tests/test_execute_sql_envelope.py b/tests/test_execute_sql_envelope.py index e0c62df4..ca3868c1 100644 --- a/tests/test_execute_sql_envelope.py +++ b/tests/test_execute_sql_envelope.py @@ -95,10 +95,55 @@ def test_resource_limit_refusal_discards_partial_stdout(monkeypatch): assert env["audit_id"] -def test_bare_operational_failure_is_classified_by_exit_code(monkeypatch): - # Exit 5 = SQL execution error with no {"refusal"} line → classified as 'syntax' by exit code. - env = _run(monkeypatch, _Proc(5, stderr='relation "x" does not exist')) +@pytest.mark.parametrize( + "raw,returncode,expected_kind,secret", + [ + ("permission denied for table salaries", 5, "permission", "salaries"), + ('relation "hr.employees" does not exist', 5, "table_not_found", "hr.employees"), + ('column "ssn" does not exist', 5, "column_not_found", "ssn"), + ('syntax error at or near "SELCT"', 5, "syntax", "SELCT"), + ("canceling statement due to statement timeout", 5, "timeout", None), + # opaque exit-5 → the exit-code prior (syntax); the raw (incl. a path) must not leak + ("driver panic 0xDEADBEEF at /var/lib/pg/secret", 5, "syntax", "/var/lib/pg/secret"), + # the HIGH-LEAK kinds — raw stderr carries a hostname / username + ('could not translate host name "internal-db.corp"', 4, "dsn", "internal-db.corp"), + ("connection refused", 4, "network", None), + ('password authentication failed for user "admin_svc"', 4, "auth", "admin_svc"), + # dialect variants — same refined kind from different driver wording + ("(1054, \"Unknown column 'ssn' in 'field list'\")", 5, "column_not_found", "ssn"), + ("no such column: ssn", 5, "column_not_found", "ssn"), + ("no such table: employees", 5, "table_not_found", "employees"), + # Snowflake: 'object does not exist' must win over its 'compilation error' prefix + ( + "SQL compilation error: Object 'DB.SCHEMA.SECRETS' does not exist or not authorized", + 5, + "table_not_found", + "DB.SCHEMA.SECRETS", + ), + ], +) +def test_operational_error_is_classified_and_sanitized( + monkeypatch, raw, returncode, expected_kind, secret +): + # An execution error must NOT leak raw driver text (schema / column / value / host / user names) + # into the response — it is classified into a generic, value-free message. (The raw goes only to + # the audit trail; see tests/test_guardrail_audit.py::test_operational_error_puts_raw_in_audit_not_in_envelope.) + env = _run(monkeypatch, _Proc(returncode, stderr=raw)) assert env["status"] == "refused" - assert env["refusal"]["kind"] == "syntax" # _classify_exit(5) - # Operational stderr surfaces as the reason (proper sanitization is handled separately). - assert 'relation "x"' in env["refusal"]["reason"] + assert env["refusal"]["kind"] == expected_kind, env + assert "data" not in env + if secret is not None: + assert secret not in json.dumps(env), ( + f"raw token {secret!r} leaked into the envelope: {env}" + ) + + +@pytest.mark.parametrize( + "returncode,expected_kind", + [(2, "dsn"), (3, "driver_missing"), (4, "auth"), (5, "syntax"), (99, "other")], +) +def test_operational_error_exit_code_prior(monkeypatch, returncode, expected_kind): + # With no recognizable stderr text, classification falls back to the exit-code prior. + env = _run(monkeypatch, _Proc(returncode, stderr="")) + assert env["status"] == "refused" + assert env["refusal"]["kind"] == expected_kind, env diff --git a/tests/test_guardrail.py b/tests/test_guardrail.py index cda807c6..b7c2edf9 100644 --- a/tests/test_guardrail.py +++ b/tests/test_guardrail.py @@ -75,7 +75,7 @@ def test_action_and_class_sets_match_the_contract(): def test_every_emitted_refusal_kind_is_documented(): # `Refusal.kind` is an open str, so this pins the kinds the code ACTUALLY emits (executor gates + - # the tools layer + _classify_exit) against the documented REFUSAL_KINDS set, so the vocabulary + # the tools layer + _classify_db_error) against the documented REFUSAL_KINDS set, so the vocabulary # can never silently drift from reality. Keep in sync with the emit sites if you add a kind. emitted = { # execute_sql._model_safety + main + tools.tool_execute_sql pre-check (guardrail refusals) @@ -83,15 +83,21 @@ def test_every_emitted_refusal_kind_is_documented(): "table_out_of_scope", "select_star", "column_out_of_scope", + "unscopable_sql", + "resource_limit", + "recon", "model_unavailable", "preflight_refused", "sensitive_columns", - # tools.tool_execute_sql + _classify_exit (operational / execution failures) + # tools.tool_execute_sql + _classify_db_error (operational / execution failures) "other", "timeout", "dsn", + "network", "driver_missing", "auth", + "column_not_found", + "table_not_found", "syntax", } assert emitted <= set(REFUSAL_KINDS), emitted - set(REFUSAL_KINDS) diff --git a/tests/test_guardrail_audit.py b/tests/test_guardrail_audit.py index 81a2f318..bd583f1f 100644 --- a/tests/test_guardrail_audit.py +++ b/tests/test_guardrail_audit.py @@ -66,6 +66,9 @@ def test_refused_query_writes_a_refused_audit_row(db): assert row["datasource"] == "sales" assert row["correlation_id"] == "turn-1" assert row["row_count"] is None + assert ( + row["error_detail"] is None + ) # a guardrail refusal has a clean reason — no raw detail stored def test_ok_query_writes_an_ok_audit_row(db, monkeypatch): @@ -84,6 +87,133 @@ def test_ok_query_writes_an_ok_audit_row(db, monkeypatch): assert row["row_count"] == 1 +def test_operational_error_puts_raw_in_audit_not_in_envelope(db, monkeypatch): + # A DB execution error: the RAW driver text (schema / column / value names) must be captured in + # the audit row's error_detail (for the operator) but NEVER surface in the Envelope refusal. + class _ErrProc: + returncode = 5 + stdout = "" + stderr = "permission denied for table salaries" + + monkeypatch.setattr(tools.subprocess, "run", lambda *a, **k: _ErrProc()) + + resp = json.loads(tools.tool_execute_sql({"sql": "SELECT x FROM t", "datasource": "sales"})) + assert resp["status"] == "refused" + assert resp["refusal"]["kind"] == "permission" + assert "salaries" not in json.dumps( + resp + ) # the raw object name is absent from the whole response + + (row,) = _audit_rows(db) + assert row["audit_id"] == resp["audit_id"] + assert row["refusal_kind"] == "permission" + assert "salaries" in (row["error_detail"] or "") # but IS captured server-side for the operator + + +def test_recon_query_writes_a_recon_audit_row(db): + # A recon query is refused before any subprocess — the recon refusal audits with kind=recon and, + # being a guardrail refusal, stores no raw error_detail. + resp = json.loads(tools.tool_execute_sql({"sql": "SELECT current_user", "datasource": "sales"})) + assert resp["status"] == "refused" and resp["refusal"]["kind"] == "recon" + + (row,) = _audit_rows(db) + assert row["audit_id"] == resp["audit_id"] + assert row["refusal_kind"] == "recon" + assert row["error_detail"] is None + + +def test_jsonl_fallback_carries_error_detail_on_operational_failure(tmp_path, monkeypatch): + # The default OSS deployment (no AGAMI_DB_URL) writes audit to the local jsonl — an operational + # failure's RAW text must land there (and only there), not in the response. + monkeypatch.delenv("AGAMI_DB_URL", raising=False) + log = tmp_path / "guardrail_audit.jsonl" + monkeypatch.setattr(tools, "GUARDRAIL_AUDIT_LOG", log) + + class _ErrProc: + returncode = 5 + stdout = "" + stderr = "permission denied for table salaries" + + monkeypatch.setattr(tools.subprocess, "run", lambda *a, **k: _ErrProc()) + + resp = json.loads(tools.tool_execute_sql({"sql": "SELECT x FROM t", "datasource": "sales"})) + assert resp["status"] == "refused" and "salaries" not in json.dumps(resp) + + rec = json.loads(log.read_text().splitlines()[0]) + assert "salaries" in (rec.get("error_detail") or "") + + +def test_audit_insert_degrades_on_pre_013_schema(tmp_path): + # New code against an un-migrated DB (guardrail_audit WITHOUT error_detail): the audit row must + # SURVIVE (minus the raw detail), not be silently dropped by the best-effort recorder. + from contracts import GuardrailAuditRecord # noqa: PLC0415 + from model_store import DbActivitySink # noqa: PLC0415 + + s = Store.connect("sqlite://" + str(tmp_path / "old.db")) + s.execute( + "CREATE TABLE guardrail_audit (audit_id TEXT PRIMARY KEY, ts TEXT NOT NULL, datasource TEXT, " + "status TEXT NOT NULL, refusal_kind TEXT, sql TEXT, row_count INTEGER, execution_ms INTEGER, " + "correlation_id TEXT, source TEXT)" # the pre-013 10-column schema + ) + s.commit() + + rec = GuardrailAuditRecord( + audit_id="a1", + ts="2026-07-12T00:00:00Z", + status="refused", + refusal_kind="syntax", + error_detail="permission denied for table salaries", + ) + DbActivitySink(s).record_guardrail_audit(rec) # must NOT raise and must NOT drop the row + + rows = s.query("SELECT * FROM guardrail_audit") + s.close() + assert len(rows) == 1 and rows[0]["audit_id"] == "a1" and rows[0]["refusal_kind"] == "syntax" + assert "error_detail" not in rows[0] # column absent; row still written without the raw detail + + +def test_audit_insert_does_not_retry_on_a_non_schema_error(): + # Copilot review: the pre-013 fallback fires ONLY for the missing error_detail column. Any OTHER + # insert failure must propagate on the FIRST attempt — a blind retry would mask a real DB error + # (e.g. a deadlock / constraint violation) AND silently drop error_detail. Pin: one execute call, + # no rollback, the original error re-raised. + from model_store import DbActivitySink # noqa: PLC0415 + + class _FakeStore: + def __init__(self): + self.execute_calls = 0 + self.rolled_back = False + + def execute(self, *_a, **_k): + self.execute_calls += 1 + raise RuntimeError("deadlock detected") # NOT a missing-column signature + + def rollback(self): + self.rolled_back = True + + def commit(self): + pass + + class _Rec: + audit_id = "a" + ts = "t" + datasource = None + status = "ok" + refusal_kind = None + sql = None + row_count = None + execution_ms = None + correlation_id = None + source = "x" + error_detail = "raw driver text" + + store = _FakeStore() + with pytest.raises(RuntimeError, match="deadlock"): + DbActivitySink(store).record_guardrail_audit(_Rec()) + assert store.execute_calls == 1 # first insert only — NO fallback retry on a non-schema error + assert store.rolled_back is False # rollback belongs to the pre-013 path, never reached here + + def test_jsonl_fallback_when_no_datastore(tmp_path, monkeypatch): # No AGAMI_DB_URL → the audit row lands in the local jsonl instead of the DB. monkeypatch.delenv("AGAMI_DB_URL", raising=False) diff --git a/tests/test_sql_guard.py b/tests/test_sql_guard.py index cb5daa37..3548310d 100644 --- a/tests/test_sql_guard.py +++ b/tests/test_sql_guard.py @@ -13,11 +13,13 @@ every day MUST pass. Over-tightening the deny-list silently degrades every query, so this corpus is the primary safety net. -Bare `pg_catalog` / `information_schema` / environment-introspection blocking is a -deferred follow-up (see `test_known_deferred_gaps_currently_pass` for the pinned -current behavior). +The recon / metadata deny-list (`sql_guard.check_no_recon`) is the companion gate — +`version()`, `current_user`, `information_schema`, `pg_*` catalog relations are refused +(`recon`). Its corpus below pins both the must-refuse vectors (no false negatives / +info-leaks) and the must-pass analytics look-alikes (no false positives). -`sql_guard.check_read_only` returns None (safe) or a short reason string (rejected). +`sql_guard.check_read_only` returns None (safe) or a safety Verdict (rejected); +`sql_guard.check_no_recon` returns None or a `recon` safety Verdict. """ from __future__ import annotations @@ -29,7 +31,8 @@ from typing import Any import pytest -from sql_guard import _MAX_SQL_CHARS, check_read_only +import sql_guard +from sql_guard import _MAX_SQL_CHARS, check_no_recon, check_read_only # --------------------------------------------------------------------------- # Accept — valid single read-only statements @@ -577,28 +580,181 @@ def test_false_positive_guard_legitimate_analytics_sql(sql: str) -> None: # --------------------------------------------------------------------------- -# Deferred-scope pins — behaviors intentionally NOT hardened in this pass, so a -# future change that adds them is a conscious decision (and updates this test). +# Recon / metadata deny-list — check_no_recon. A false NEGATIVE here is a +# shipped info-leak (server fingerprint / schema enumeration across the LLM boundary); +# a false POSITIVE breaks a legitimate analytics query. Both are pinned exhaustively. +# (These queries pass check_read_only — recon is a separate, distinct `recon` gate.) # --------------------------------------------------------------------------- +_RECON_REFUSE = [ + # paren fns / pg + "SELECT version()", + "SELECT current_database()", + "SELECT current_schemas(true)", + # paren fns / mysql + "SELECT database()", + "SELECT connection_id()", + "SELECT system_user()", + # paren fns / snowflake + "SELECT current_account()", + "SELECT current_warehouse()", + "SELECT current_version()", + # niladic (no-paren special values) + "SELECT current_user", + "SELECT session_user", + "SELECT current_catalog", + "SELECT current_schema", + "SELECT current_role", + "SELECT id FROM t WHERE owner = current_user", + # schema-qualified catalog access + "SELECT * FROM information_schema.tables", + "SELECT column_name FROM information_schema.columns", + "SELECT * FROM pg_catalog.pg_class", + "SELECT * FROM account_usage.query_history", + "SELECT * FROM mysql.user", + # bare catalog relations + "SELECT * FROM pg_tables", + "SELECT relname FROM pg_class", + "SELECT * FROM pg_stat_activity", + "SELECT usename FROM pg_user", + "SELECT * FROM pg_settings", + "SELECT * FROM pg_roles", + # catalog DATA / password / stats relations — caught by the pg_ prefix, NOT the explicit list + "SELECT rolname, rolpassword FROM pg_authid", + "SELECT most_common_vals, histogram_bounds FROM pg_stats WHERE tablename = 'salaries'", + "SELECT query FROM pg_stat_statements", + "SELECT * FROM pg_statistic", + "SELECT typname FROM pg_type", + "SELECT * FROM pg_constraint", + "SELECT * FROM pg_stat_user_tables", + "SELECT srvname FROM pg_foreign_server", + # catalog-DDL-dump / object-existence-probe / topology FUNCTIONS + "SELECT pg_get_viewdef('v')", + "SELECT pg_get_functiondef('f'::regprocedure)", + "SELECT has_table_privilege('secret_table', 'SELECT')", + "SELECT to_regclass('secret_table')", + "SELECT inet_server_addr()", + "SELECT inet_client_addr()", + "SELECT pg_relation_size('t')", + "SELECT pg_backend_pid()", + # redshift system tables (all four reserved prefixes) + pg_-prefixed redshift catalog helpers + "SELECT * FROM stl_query", + "SELECT * FROM svv_table_info", + "SELECT * FROM stv_sessions", + "SELECT * FROM svl_statementtext", + "SELECT * FROM pg_table_def", + "SELECT * FROM pg_user_info", + # mysql secondary recon schemas + "SELECT * FROM performance_schema.threads", + "SELECT * FROM sys.session", + # mysql system variables + "SELECT @@version", + "SELECT @@hostname", + "SELECT @@datadir", + "SELECT @@basedir", + # quoted-identifier bypass — _neutralize unwraps the quotes, so the recon token re-forms + 'SELECT "version"()', + 'SELECT "pg_class" FROM t', + # bypass attempts — neutralization must still catch them + "SELECT /*c*/ version()", + "SELECT VERSION()", + "SELECT version ()", + "select * from Information_Schema.Tables", + "SELECT id FROM t UNION SELECT version()", + "SELECT * FROM (SELECT * FROM pg_tables) s", +] + + +@pytest.mark.parametrize("sql", _RECON_REFUSE) +def test_recon_refused(sql: str) -> None: + v = check_no_recon(sql) + assert v is not None, f"RECON NOT BLOCKED (false negative / info-leak): {sql!r}" + assert v.rule == "recon", v + + +_RECON_PASS = [ + # bare recon token as a column / alias — only the ()-call form is recon + "SELECT version FROM releases", + "SELECT version AS v FROM app_releases", + "SELECT current_database FROM config", + # suffix on a niladic/paren token — the word boundary protects these (NOT pg_-prefixed) + "SELECT current_user_id FROM audit", + "SELECT session_user_count FROM stats", + "SELECT versions.id FROM versions", + # deliberately-allowed bare niladic synonyms (too common as column names) — only their ()-form is recon + "SELECT user FROM accounts", + "SELECT schema, name FROM migrations", + "SELECT database FROM connections", + # qualified column — the (? None: + assert check_no_recon(sql) is None, ( + f"FALSE POSITIVE — legit query blocked by recon gate: {sql!r}" + ) + @pytest.mark.parametrize( "sql", [ - # Bare `pg_catalog` / `information_schema` probing and environment-introspection - # keywords/functions are NOT blocked by this gate (schema-scoping is a deferred - # follow-up — see the plan). They currently PASS. When the follow-up lands, move - # these into a reject test. - "SELECT * FROM pg_tables", - "SELECT * FROM information_schema.tables", - "SELECT current_user", - "SELECT current_database()", + 'SELECT "current_user"', # bare quoted niladic — _neutralize unwraps the quotes + "SELECT x AS pg_tables FROM t", # alias deliberately named a reserved catalog relation + # A bare, unqualified user identifier literally prefixed `pg_` — the reserved-prefix rule + # refuses it (Postgres reserves the `pg_` object-name prefix). Qualified `t.pg_foo` passes. + "SELECT pg_tables_archived FROM meta", + "SELECT pg_class_history FROM audit_log", ], ) -def test_known_deferred_gaps_currently_pass(sql: str) -> None: - assert check_read_only(sql) is None, ( - f"Expected this deferred-scope query to pass the read-only gate for now: {sql!r}" - ) +def test_recon_accepted_residual_false_positives(sql: str) -> None: + # Regex without AST binding can't distinguish these from a real recon reference; all are rare + + # convention-violating and documented in sql_guard. Pinned so a future change that flips one is a + # conscious decision. + assert check_no_recon(sql) is not None, sql + + +def test_recon_sets_drive_the_regexes() -> None: + # A new engine builtin is a one-line SET edit — the compiled regexes derive from these sets (SC4). + # Loop over EVERY member so the corpus is self-updating: adding/removing a set entry is auto-tested, + # and no member can silently stop matching. + for name in sql_guard._RECON_PAREN_FNS: + assert check_no_recon(f"SELECT {name}()").rule == "recon", name + for name in sql_guard._RECON_NILADIC: + assert check_no_recon(f"SELECT {name}").rule == "recon", name + for name in sql_guard._RECON_BARE_RELATIONS: + assert check_no_recon(f"SELECT * FROM {name}").rule == "recon", name + for name in sql_guard._RECON_SCHEMAS: + assert check_no_recon(f"SELECT * FROM {name}.x").rule == "recon", name + # the pg_ reserved-prefix catch-all covers catalog relations NOT in the explicit list + for name in ("pg_statistic", "pg_authid", "pg_stats", "pg_stat_statements", "pg_type"): + assert check_no_recon(f"SELECT * FROM {name}").rule == "recon", name + + +def test_recon_fails_closed_on_unparseable_input() -> None: + # A form _neutralize can't disambiguate (a bare `--x` comment) → check_no_recon fails CLOSED (a + # recon verdict), not open. In the normal flow check_read_only refuses it first; this pins the + # defense-in-depth for a standalone / reordered caller. + v = check_no_recon("SELECT 1--x") + assert v is not None and v.rule == "recon", v # --------------------------------------------------------------------------- @@ -644,6 +800,23 @@ def test_executor_dangerous_function_blocked(tmp_path) -> None: assert "permission" in proc.stderr +def test_executor_recon_query_blocked(tmp_path) -> None: + # The recon gate fires at the shared chokepoint (execute_sql.py::main, which both transports + # call), refusing BEFORE credentials are loaded — proving both surfaces inherit it (SC3). + proc = _run_executor("SELECT current_user", tmp_path) + assert proc.returncode != 0, proc.stdout + envelope = None + for line in proc.stderr.splitlines(): + line = line.strip() + if line.startswith("{"): + try: + envelope = json.loads(line) + except ValueError: + continue + assert envelope is not None, f"no JSON refusal envelope on stderr; got: {proc.stderr!r}" + assert envelope["refusal"]["kind"] == "recon", envelope + + def test_executor_lets_valid_select_past_the_gate(tmp_path) -> None: """A valid SELECT is NOT rejected by the read-only gate — it proceeds to the credential step and fails there instead (proving the gate didn't block it)."""