From 6124d8440df55b579518877f432d87d45a33e85c Mon Sep 17 00:00:00 2001 From: Amar K P Date: Tue, 27 Oct 2026 23:22:01 +0530 Subject: [PATCH 1/4] fix(tools): enforce read_file byte cap on the actual read instead of a prior stat The cap was checked via stat().st_size and then enforced by a second open, so a file grown between the two calls was read in full. One bounded read of max_bytes + 1 closes the check-then-read window; an exact-cap file stays fully readable. Regression test simulates the race with a lying st_size. Closes #95 (ROADMAP-03) --- agentkit/tools/builtin/read_file.py | 16 ++++++---- tests/unit/test_tool_read_file.py | 47 +++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 6 deletions(-) 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/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") From 9dea7fdfff7a71786c201d7ba29c4bd499be7246 Mon Sep 17 00:00:00 2001 From: Amar K P Date: Tue, 27 Oct 2026 23:22:01 +0530 Subject: [PATCH 2/4] fix(tools): reject non-finite calculator results; name the exponent pair guard Float overflow (1e308 * 10 ** 3) previously rendered 'inf' into the prompt, and an overflowing float literal (1e999) silently parsed to inf at AST level and flowed through every operation. The result is now checked with math.isfinite at the exit, and non-finite literals are rejected at the operand boundary. The bare 1000/100 literals in the Pow guard become MAX_POW_BASE_MAGNITUDE / MAX_POW_PAIR_EXPONENT with unchanged values. Closes #96 (ROADMAP-04) --- agentkit/tools/builtin/calculator.py | 26 ++++++++++++++++++++++++-- tests/unit/test_tool_calculator.py | 23 +++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) 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/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" From 3701aa21f3d99b128d71e0f20f892d4605da8f55 Mon Sep 17 00:00:00 2001 From: Amar K P Date: Tue, 27 Oct 2026 23:22:01 +0530 Subject: [PATCH 3/4] fix(tools): escape and truncate SQL result cells before markdown output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cell values were inserted into the markdown table unescaped, so a value containing a pipe, newline or control character could forge table rows and smuggle prompt structure — stored prompt injection through database data. Escapes are applied per cell (backslash first), the rendered cell length is capped (MAX_CELL_CHARS), and column names get the same treatment. Closes #94 (ROADMAP-02) --- agentkit/tools/builtin/sql_readonly.py | 49 +++++++++++++++++-- tests/unit/test_tool_sql_readonly.py | 66 ++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 5 deletions(-) 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/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 From e5933f63271bcfd9a4516f204662533ca5a1d9a2 Mon Sep 17 00:00:00 2001 From: Amar K P Date: Tue, 27 Oct 2026 23:22:47 +0530 Subject: [PATCH 4/4] docs(progress): register the ROADMAP-02/03/04 tools-hardening batch --- docs/PROGRESS.md | 9 +++++++++ 1 file changed, 9 insertions(+) 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.