diff --git a/agentkit/tools/builtin/calculator.py b/agentkit/tools/builtin/calculator.py index 828094b..782fcb3 100644 --- a/agentkit/tools/builtin/calculator.py +++ b/agentkit/tools/builtin/calculator.py @@ -1,12 +1,18 @@ """Safe arithmetic calculator tool powered by strict Python AST parsing.""" import ast +import math from agentkit.tools.registry import tool MAX_EXPRESSION_LEN = 1000 MAX_AST_NODES = 100 MAX_EXPONENT = 1000 +#: When the base magnitude grows too, an exponent well below MAX_EXPONENT can already +#: overflow a float (``1000.0 ** 100`` is ~1e300; ``1999.0 ** 101`` is inf), so pairs of an +#: oversized base and a still-large exponent are rejected before the operation runs. +MAX_POW_BASE_MAGNITUDE = 1000 +MAX_POW_PAIR_EXPONENT = 100 #: Rough ceiling on the bit length of an exponentiation result. 10**1000 is ~3322 bits and #: still cheap once, but the ReAct loop can repeat the call, so the result size itself is @@ -23,6 +29,11 @@ def _evaluate_node(node: ast.AST) -> int | float: # Explicitly reject boolean, string, None, complex, etc. if isinstance(node.value, bool) or not isinstance(node.value, (int, float)): raise ValueError(f"Disallowed operand type: {type(node.value).__name__}") + # A float literal that overflows Python's double range (e.g. ``1e999``) silently + # becomes inf at parse time; anchoring finiteness here keeps it out of every + # subsequent operation. + if isinstance(node.value, float) and not math.isfinite(node.value): + raise ValueError("Numeric literal is not finite (out of range)") return node.value if isinstance(node, ast.UnaryOp): @@ -56,7 +67,9 @@ def _evaluate_node(node: ast.AST) -> int | float: raise ValueError("Modulo by zero") return left % right if isinstance(node.op, ast.Pow): - if abs(right) > MAX_EXPONENT or (abs(left) > 1000 and right > 100): + if abs(right) > MAX_EXPONENT or ( + abs(left) > MAX_POW_BASE_MAGNITUDE and right > MAX_POW_PAIR_EXPONENT + ): raise ValueError(f"Exponent too large (maximum allowed is {MAX_EXPONENT})") # The exponent bound alone still permits enormous big integers (10**1000 is a @@ -108,7 +121,16 @@ def safe_calculate(expression: str) -> int | float: if len(nodes) > MAX_AST_NODES: raise ValueError(f"Expression too complex (exceeds {MAX_AST_NODES} AST nodes).") - return _evaluate_node(tree) + result = _evaluate_node(tree) + + # Any float operand is finite, but sums/products/divisions can still overflow into + # inf or collapse into nan. Intercept once at the exit so neither ever reaches the + # prompt or history. (The only finite value derivable from an inf intermediate is + # inf ** 0 == 1.0, which is the correct x**0 answer rather than a masked overflow.) + if isinstance(result, float) and not math.isfinite(result): + raise ValueError("Result is not a finite number") + + return result @tool diff --git a/agentkit/tools/builtin/read_file.py b/agentkit/tools/builtin/read_file.py index 459d77b..7716954 100644 --- a/agentkit/tools/builtin/read_file.py +++ b/agentkit/tools/builtin/read_file.py @@ -50,14 +50,18 @@ def read_sandboxed_file( if not target.is_file(): raise ValueError(f"Target is not a regular file: '{file_path}'") - file_size = target.stat().st_size - if file_size > max_bytes: - raise ValueError( - f"File '{file_path}' exceeds maximum size limit of {max_bytes} bytes (size: {file_size} bytes)." - ) + # The byte cap is enforced on the bytes actually read, not on a prior st_size: a file + # grown between a stat check and a second open would otherwise be read in full (the + # check-then-read race). One bounded read of max_bytes + 1 closes that window while + # making oversize still fail — and an exact-cap file stays fully readable. + with target.open("rb") as handle: + data = handle.read(max_bytes + 1) + + if len(data) > max_bytes: + raise ValueError(f"File '{file_path}' exceeds maximum size limit of {max_bytes} bytes.") try: - return target.read_text(encoding="utf-8") + return data.decode("utf-8") except UnicodeDecodeError as exc: raise ValueError(f"File '{file_path}' is not a valid UTF-8 text file.") from exc diff --git a/agentkit/tools/builtin/sql_readonly.py b/agentkit/tools/builtin/sql_readonly.py index 398fdc3..51840a5 100644 --- a/agentkit/tools/builtin/sql_readonly.py +++ b/agentkit/tools/builtin/sql_readonly.py @@ -36,6 +36,26 @@ DEFAULT_ROW_LIMIT = 50 DEFAULT_STATEMENT_TIMEOUT_S = 10.0 +#: Per-cell character ceiling for rendered table cells. The executor budget +#: (TOOL_OUTPUT_MAX_CHARS) bounds the whole table afterwards, but one wide column value +#: should not be able to consume most of that budget — or hide a large payload — behind +#: row truncation. +MAX_CELL_CHARS = 200 + +#: Row-breaking and invisible control characters are rewritten to visible escapes, and +#: every other C0 control character (plus DEL) is rendered as ``\\uXXXX`` so no control +#: byte reaches the prompt as structure. +_CONTROL_CHAR_ESCAPES: dict[int, str] = { + ord("\n"): "\\n", + ord("\r"): "\\r", + ord("\t"): "\\t", + **{ + code: f"\\u{code:04x}" + for code in (*range(0x20), 0x7F) + if code not in (ord("\n"), ord("\r"), ord("\t")) + }, +} + DISALLOWED_KEYWORDS = ( "insert", "update", @@ -402,17 +422,36 @@ def _dollar_quote_end(sql: str, start: int) -> int | None: return len(sql) if closing == -1 else closing + len(tag) +def _escape_cell_text(raw_text: str) -> str: + """Escape and bound a single table cell so it cannot alter the markdown structure. + + A pipe would forge additional columns, a newline would forge additional rows, and + invisible control characters could smuggle prompt structure — the classic stored + prompt injection through attacker-influenced database values. Backslash is escaped + first so it cannot escape its own escape sequences. Finiteness of the rendered + length (not the raw value) is the cap, so the displayed cell is bounded no matter + how the escapes inflated the text. + """ + escaped = raw_text.replace("\\", "\\\\").replace("|", "\\|") + escaped = escaped.translate(_CONTROL_CHAR_ESCAPES) + if len(escaped) > MAX_CELL_CHARS: + escaped = escaped[:MAX_CELL_CHARS] + "[Truncated]" + return escaped + + +def _format_cell(value: Any) -> str: + """Render one SQL result value as a safe markdown table cell.""" + return "NULL" if value is None else _escape_cell_text(str(value)) + + def format_rows_as_markdown(columns: Sequence[str], rows: Sequence[Any]) -> str: """Format tabular SQL results into clean Markdown table string.""" if not rows: return "No rows returned." - header = "| " + " | ".join(columns) + " |" + header = "| " + " | ".join(_format_cell(col) for col in columns) + " |" divider = "| " + " | ".join(["---"] * len(columns)) + " |" - data_lines = [ - "| " + " | ".join(str(val) if val is not None else "NULL" for val in row) + " |" - for row in rows - ] + data_lines = ["| " + " | ".join(_format_cell(val) for val in row) + " |" for row in rows] return "\n".join([header, divider, *data_lines]) diff --git a/docs/PROGRESS.md b/docs/PROGRESS.md index f5d20ea..91a52d1 100644 --- a/docs/PROGRESS.md +++ b/docs/PROGRESS.md @@ -76,6 +76,9 @@ This document tracks execution progress across all backlog tasks, including pull | FU-85 | AUDIT | Real-Infra Provisioning Failures | `test/real-infra-provisioning-failures` | #90 | 1 | Done | Provisioning errors fail the live SQL-isolation tier instead of skipping it | | FU-88 | AUDIT | Audit History Consistency | `docs/audit-history-consistency` | #90 | 1 | Done | Canonical 12-layer/14-commit wording; stale "AST validation" row corrected and guarded | | FU-89 | AUDIT | uv Lockfile Policy | `chore/repo-uv-lock` | #90 | 1 | Done | `uv.lock` ignored; pip-based CI does not consume a uv lock | +| ROADMAP-02 | ROADMAP | SQL Markdown Cell Escaping | `fix/roadmap-02-03-04-tools-hardening` | #120 | 1 | Done | Cells escaped (`\|`, `\n`, `\uXXXX` controls) and length-capped before markdown output; closes #94 | +| ROADMAP-03 | ROADMAP | read_file Read-After-Stat Race | `fix/roadmap-02-03-04-tools-hardening` | #120 | 1 | Done | Byte cap enforced on a bounded `read(max_bytes + 1)` in one open; closes #95 | +| ROADMAP-04 | ROADMAP | Calculator Non-Finite Result Guard | `fix/roadmap-02-03-04-tools-hardening` | #120 | 1 | Done | `math.isfinite` result check, inf literals rejected, pow pair guard named; closes #96 | --- @@ -505,3 +508,9 @@ This document tracks execution progress across all backlog tasks, including pull - **#85 (test reliability)** — `tests/real/test_sql_isolation_real.py::readonly_engine` no longer converts `provision_readonly_role()` failures into `pytest.skip`. The upstream `real_database_url` fixture already applies migrations and fails loudly when PostgreSQL is unreachable, so a provisioning error is a defect and now turns CI red instead of leaving the isolation tier green-by-skip. - **#88 (docs)** — `docs/AUDIT.md` records the canonical "linear 12-layer stack, merged as 14 commits (12 layer commits plus two progress-bookkeeping commits)" wording instead of "nine branches", and the historical M4-T04 row describes the lexer plus dedicated read-only role instead of "Strict SELECT AST validation". `tests/unit/test_docs_accuracy.py` guards both claims. - **#89 (repo hygiene)** — `.gitignore` ignores `uv.lock` with a comment explaining that all three CI jobs install from `pyproject.toml` with pip, so a lockfile is not part of the reproducible build. Adopting uv properly (commit the lock, switch CI to `uv sync --locked`) remains a separate decision. + +### PR #120: ROADMAP-02 / ROADMAP-03 / ROADMAP-04 Tools-Hardening Batch (P0 quick fixes) +- **Status**: Done +- **PR**: `fix/roadmap-02-03-04-tools-hardening` — PR #120 (three `fix(tools)` commits, squash-merged; docs rows in this commit) +- **Commits**: 3 (+1 docs) +- **Notes**: Closed the first three `ROADMAP` issues filed from the 2026-10-27 assessment. **#94** — `format_rows_as_markdown` now routes every cell (and column name) through `_format_cell`/`_escape_cell_text`: backslash first, then `|` → `\|`, newline/CR/tab → `\n`/`\r`/`\t`, every other C0/DEL control character → `\uXXXX`, and a rendered-length cap `MAX_CELL_CHARS` (200) with a `[Truncated]` marker, so a database value can no longer forge table rows, columns or prompt structure (stored prompt injection). **#95** — `read_sandboxed_file` enforces the byte cap on the bytes actually read (`read(max_bytes + 1)` from a single open) instead of a `stat().st_size` check + second open; oversize keeps the same user-visible phrase, an exact-cap file stays fully readable, and a regression test stands in for the race by lying about `st_size` (the old code returned the full oversized file). **#96** — `safe_calculate` rejects non-finite float results (`1e308 * 10 ** 3` no longer renders `inf` into the prompt) and non-finite float literals (`1e999` parses to inf) at the operand boundary; the `Pow` guard's bare `1000`/`100` literals became `MAX_POW_BASE_MAGNITUDE`/`MAX_POW_PAIR_EXPONENT` with unchanged values. 11 tests added (`test_tool_sql_readonly.py`, `test_tool_read_file.py`, `test_tool_calculator.py`); suite moved from 483 to 494 passing. diff --git a/tests/unit/test_tool_calculator.py b/tests/unit/test_tool_calculator.py index 5bc8f39..1cf7ba7 100644 --- a/tests/unit/test_tool_calculator.py +++ b/tests/unit/test_tool_calculator.py @@ -93,6 +93,29 @@ def test_dos_protection_large_exponents() -> None: safe_calculate("10 ** 10000") +def test_non_finite_results_rejected() -> None: + with pytest.raises(ValueError, match="not a finite number"): + safe_calculate("1e308 * 10 ** 3") + + with pytest.raises(ValueError, match="not a finite number"): + safe_calculate("1e308 * 2") + + +def test_overflowing_float_literal_rejected() -> None: + # A float literal beyond double range silently parses to inf at AST level. + with pytest.raises(ValueError, match="not finite"): + safe_calculate("1e999") + + with pytest.raises(ValueError, match="not finite"): + safe_calculate("-1e999") + + +def test_large_integer_results_still_allowed() -> None: + assert safe_calculate("2 ** 1000") == 2**1000 + assert safe_calculate("10 ** 100") == 10**100 + assert safe_calculate("1000 ** 100") == 1000**100 + + def test_calculator_tool_function() -> None: assert calculator("10 + 20") == "30" assert calculator("7 / 2") == "3.5" diff --git a/tests/unit/test_tool_read_file.py b/tests/unit/test_tool_read_file.py index 7b496a2..d867785 100644 --- a/tests/unit/test_tool_read_file.py +++ b/tests/unit/test_tool_read_file.py @@ -1,5 +1,6 @@ """Unit and security tests for sandboxed file reader tool.""" +import os from pathlib import Path import pytest @@ -90,6 +91,52 @@ def test_max_bytes_limit_enforced(tmp_path: Path) -> None: read_sandboxed_file("large.txt", base_dir=tmp_path, max_bytes=100) +def test_exact_cap_file_is_readable(tmp_path: Path) -> None: + doc = tmp_path / "exact.txt" + doc.write_text("A" * 40, encoding="utf-8") + + assert read_sandboxed_file("exact.txt", base_dir=tmp_path, max_bytes=40) == "A" * 40 + + +def test_size_cap_enforced_on_the_read_not_on_stat( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A file whose stat lies small is still capped by the bytes actually read. + + Regression: the cap used to consult ``stat().st_size`` and then re-open the file, so a + file grown between the two calls bypassed the limit completely. Lying about the size is + an observable stand-in for that race; the bounded read must reject it regardless. + """ + doc = tmp_path / "growing.txt" + doc.write_text("A" * 50, encoding="utf-8") + + real_stat = Path.stat + + def lying_stat(self: Path, *, follow_symlinks: bool = True) -> "os.stat_result": + st = real_stat(self, follow_symlinks=follow_symlinks) + # Report st_size = 10 while the real content is 50 bytes. + return os.stat_result( + ( + st.st_mode, + st.st_ino, + st.st_dev, + st.st_nlink, + st.st_uid, + st.st_gid, + 10, + st.st_atime, + st.st_mtime, + st.st_ctime, + ) + ) + + monkeypatch.setattr(Path, "stat", lying_stat) + + # The old stat-driven cap would have passed this file through and returned all 50 bytes. + with pytest.raises(ValueError, match="exceeds maximum size limit"): + read_sandboxed_file("growing.txt", base_dir=tmp_path, max_bytes=40) + + def test_binary_file_rejected(tmp_path: Path) -> None: doc = tmp_path / "binary.bin" doc.write_bytes(b"\x80\x81\xff\xfe\x00\x01") diff --git a/tests/unit/test_tool_sql_readonly.py b/tests/unit/test_tool_sql_readonly.py index 52ce3e2..bc332ca 100644 --- a/tests/unit/test_tool_sql_readonly.py +++ b/tests/unit/test_tool_sql_readonly.py @@ -7,7 +7,9 @@ from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine from agentkit.tools.builtin.sql_readonly import ( + MAX_CELL_CHARS, execute_sql_query, + format_rows_as_markdown, sql_readonly, validate_sql_query, ) @@ -158,3 +160,67 @@ async def test_sql_tool_registry_integration( bad_result = await registry.execute(bad_call) assert bad_result.ok is False assert "Only SELECT" in (bad_result.error or "") + + +def test_markdown_escapes_pipe_and_backslash_per_cell() -> None: + """A pipe in a cell value cannot forge additional columns, and rows stay intact.""" + output = format_rows_as_markdown(["customer"], [("Al|ice",), ("a\\b",), ("Bob",)]) + + assert output.splitlines() == [ + "| customer |", + "| --- |", + "| Al\\|ice |", + "| a\\\\b |", + "| Bob |", + ] + + +def test_markdown_escapes_newlines_and_control_characters() -> None: + """A newline in a cell value cannot forge additional rows.""" + output = format_rows_as_markdown(["text"], [("line1\nline2\r\nline3\x07end\x7f",)]) + + assert output.splitlines() == [ + "| text |", + "| --- |", + "| line1\\nline2\\r\\nline3\\u0007end\\u007f |", + ] + + +def test_markdown_null_and_unchanged_basics() -> None: + output = format_rows_as_markdown(["a", "b"], [(None, 150.0)]) + + assert output.splitlines() == ["| a | b |", "| --- | --- |", "| NULL | 150.0 |"] + + +def test_markdown_truncates_oversized_cell() -> None: + output = format_rows_as_markdown(["c"], [("x" * 5_000,)]) + lines = output.splitlines() + + # One data row, one bounded cell, marker appended inside the cell. + assert len(lines) == 3 + assert lines[-1].endswith("[Truncated] |") + assert len(lines[-1]) <= MAX_CELL_CHARS + len("[Truncated]") + 4 + + +def test_markdown_escapes_column_names_too() -> None: + output = format_rows_as_markdown(["name|extra"], [("v",)]) + + assert output.splitlines()[0] == "| name\\|extra |" + + +@pytest.mark.asyncio +async def test_execute_sql_query_escapes_adversarial_cells(demo_db_engine: AsyncEngine) -> None: + async with demo_db_engine.begin() as conn: + await conn.execute( + text("INSERT INTO orders (customer, amount) VALUES ('Al|ice\nEvil', 7.5)") + ) + + output = await execute_sql_query( + "SELECT customer, amount FROM orders ORDER BY id", engine=demo_db_engine + ) + lines = [line for line in output.strip().split("\n") if line.strip()] + + # Header + divider + 4 data rows: the injected pipe and newline are escapes within + # one cell, not structure that adds a line or column. + assert len(lines) == 6 + assert "Al\\|ice\\nEvil" in output