fix(security): enforce --allowed-paths sandbox on file read tools

[kovtcharov-amd]

Why this matters

GAIA documents --allowed-paths as a security sandbox for agent file tools, but it only ever restricted writes. read_file (and three sibling read tools) in FileSearchToolsMixin called open() directly with no allowlist check, so an agent could read any file the process owner can access — credentials, SSH keys, .env — regardless of the configured sandbox. write_file/edit_file correctly blocked the same paths, and the sibling FileIOToolsMixin already gates its reads, so reads were always meant to be confined; this one mixin just never applied the check.

This was reported by an external researcher (CWE-862 / missing authorization) with a working PoC against a shipped version. After this change the PoC no longer reproduces: a read outside allowed_paths returns Access denied and no file content reaches the agent/LLM.

The fix routes every content-reading tool in the mixin — read_file, search_file_content, get_file_info, analyze_data_file — through a shared _read_access_error helper that mirrors the write-side PathValidator enforcement. It no-ops when no validator is attached, so hosts without a sandbox are unaffected. Affected agents: ChatAgent, DocumentQAAgent, FileIOAgent.

Scope note: pure enumeration tools (search_file, search_directory, browse_directory) are intentionally left unchanged — their documented purpose is discovering files anywhere for the user, and gating them to the allowlist would break that UX. They disclose paths/metadata, not file contents; whether discovery should also honor the sandbox is a separate product decision, not this confidentiality fix.

Test plan

• pytest tests/unit/test_file_tools.py — 84 pass, incl. new TestReadToolsSandbox (in-sandbox read succeeds; out-of-sandbox read_file/search_file_content/get_file_info/analyze_data_file denied with no content leak; no-validator host still reads)
• pytest tests/unit/test_security_edge_cases.py test_file_write_guardrails.py test_hub_security.py — 237 pass, 6 skipped
• black --check / isort --check clean on changed files
• Researcher PoC re-run against patched code → bypass no longer reproduces (read returns "Access denied", write still blocked)

[github-actions]

Verdict: Approve

This closes a real confidentiality hole: the file read tools (read_file and three siblings) previously ignored the --allowed-paths sandbox that already gated writes, so an agent could read any file the process owner could — credentials, SSH keys, .env — outside the configured sandbox. The fix routes every content-reading tool through one shared allowlist check that no-ops when no sandbox is attached, and it mirrors the enforcement the sibling FileIOToolsMixin already applies. Clean scope, good regression tests, and the read gate is deliberately checked before the existence probe so out-of-sandbox paths can't be used as a file-existence oracle. Safe to merge.

One non-blocking observation for a follow-up (not this PR): search_file_content gates the top directory but then recurses and reads each file, so a symlinked file sitting inside the sandbox but pointing outside it could still be read. It's a narrow residual, matches the existing posture of the write/enumeration paths, and is out of scope for this confidentiality fix — flagging only so it's on the radar.

🔍 Technical details

Verified

• _read_access_error (file_tools.py:64) calls validator.is_path_allowed(path) with the default prompt_user=True — this exactly matches the sibling FileIOToolsMixin read gate (file_io_tools.py:54), so interactive-prompt vs. non-interactive auto-deny behavior stays consistent across mixins. Good parity.
• is_path_allowed (security.py:338) resolves via os.path.realpath before the allowlist prefix check, so passing the raw/unresolved file_path/str(fp) at each call site is fine — resolution happens inside the validator.
• Check ordering is correct at every site: read_file gates before os.path.exists (file_tools.py:590, avoids the existence-oracle); analyze_data_file gates after the indexed-file fallback (file_tools.py:1903) so legitimately-indexed docs inside the sandbox still resolve while the resolved path is still enforced.
• No-validator hosts short-circuit to None, preserving backward compat — covered by test_read_file_no_validator_still_reads.

🟢 Minor — residual symlink read in search_file_content (file_tools.py:813, :913)
The gate validates directory, then directory.rglob("*") enumerates and search_file opens each match. rglob won't recurse into symlinked directories, but a symlinked file inside the sandbox (safe/link.txt -> /etc/passwd) is yielded, passes is_file() (follows the link), and gets read — its content isn't re-checked against the allowlist. Requires a pre-existing symlink inside the sandbox, so it's narrow, and the write/enumeration paths share the same shape; genuinely a separate hardening item, not a regression this PR introduces. If you want to close it here, re-checking each resolved file path inside search_file would do it — otherwise fine to defer.

Strengths

• Tight, well-scoped fix — one shared helper, four call sites, zero deletions; the write path and no-sandbox hosts are untouched.
• The scope note (why enumeration tools are intentionally left alone) is exactly the right call and clearly documented.
• Regression tests assert the stronger property — not just status == "error" but that the secret content never appears anywhere in the result ("ghp_SECRETTOKEN_12345" not in str(result)), which is what actually matters for a confidentiality fix.

[github-actions]

🟡 search_file_content creates a directory-existence oracle that the read_file fix explicitly guards against.

In read_file the sandbox check runs before os.path.exists() with a comment explaining exactly why: "Checked before the existence probe so paths outside the sandbox can't be used as a file-existence oracle." But in search_file_content, the directory.exists() probe runs first and returns "Directory not found", then the sandbox check fires. This lets an agent probe any path on the filesystem — "Directory not found" vs "Access denied" tells it whether that directory exists, even if the directory is entirely outside the allowed paths.

No content leaks, but the fix inconsistency means the oracle protection it documents is only partial. analyze_data_file has the same ordering issue (existence → extension check → sandbox), though the code comment there justifies the post-existence order for the RAG fallback case.

Suggested fix for search_file_content: move the _read_access_error check to run on the unresolved directory argument before the .resolve() + .exists() calls, mirroring the read_file pattern.

🔍 Technical details

search_file_content in src/gaia/agents/tools/file_tools.py (~line 802):

directory = Path(directory).resolve()

if not directory.exists():           # ← oracle: leaks existence for out-of-sandbox paths
    return {"status": "error", "error": f"Directory not found: {directory}"}

denied = self._read_access_error(str(directory))   # ← sandbox check too late

Fix: check the sandbox on the raw argument before resolving/probing, or at minimum before the existence check:

denied = self._read_access_error(directory)   # raw arg, before resolve
if denied:
    return denied
directory = Path(directory).resolve()
if not directory.exists():
    ...

read_file (~line 584) already does this correctly and even has a comment documenting the reason. get_file_info also does it correctly.

[kovtcharov-amd]

Thanks for the review. Addressed in 2c23ec4:

• Directory-existence oracle in search_file_content — fixed. Moved the _read_access_error check ahead of resolve()/exists() so it mirrors read_file/get_file_info and gates before the existence probe. Added a regression test asserting a nonexistent out-of-sandbox dir returns Access denied, not Directory not found.
• Symlink-file read residual in search_file_content — agreed it's a narrow, separate hardening item (needs a pre-existing symlink inside the sandbox, and the write/enumeration paths share the shape). Deferring to a follow-up rather than expanding scope here, per your note.

Unrelated CI note: the only red check is Unit Tests (macOS smoke), which failed on test_daemon_relay.py::test_connection_stopped_after_running_raises_again (StopFailedError: sidecar pid survived the tree-kill) — a flaky daemon process-kill test unrelated to this change. The three real Unit Test jobs (py3.10/3.11/3.12) and Security Tests all pass.

[github-actions]

🟡 In analyze_data_file, the sandbox check is placed after the extension check, so an out-of-sandbox file with an unsupported extension (e.g. /secret/notes.txt) returns "Unsupported file type" instead of "Access denied" — bypassing the sandbox entirely and confirming the file exists at that path. The other three tools (read_file, search_file_content, get_file_info) all check the sandbox before any existence or type probe, so this is an inconsistency in coverage.

🔍 Technical details

file_tools.py around line 1890–1905:

fp = Path(file_path)        # ← 1. fp.exists()? → if True, skip indexed lookup
                            # ← 2. extension check → early return if wrong extension
                            # ← 3. sandbox check   ← NEW, but only reached when extension is OK

For /secret/data.txt that exists on disk:

• Step 1 succeeds (file is there), indexed-file fallback skipped.
• Step 2: .txt not in {".csv", ".tsv", ".xlsx", ".xls"} → returns "Unsupported file type: .txt" and exits.
• Step 3: never reached — the sandbox check is a dead letter for any out-of-sandbox file with an unsupported extension.

The caller (LLM) learns: "a file exists at /secret/notes.txt". File content is not leaked, but this is a file-existence oracle and a stated sandbox bypass.

Fix: swap the extension check and the sandbox check so the sandbox runs first:

# Enforce sandbox on the resolved path (after indexed-file fallback, before extension check)
denied = self._read_access_error(str(fp))
if denied:
    denied.update({"has_errors": True, "operation": "analyze_data_file"})
    return denied

supported_extensions = {".csv", ".tsv", ".xlsx", ".xls"}
if fp.suffix.lower() not in supported_extensions:
    ...

The comment's rationale ("checked after the indexed-file fallback so legitimately indexed documents inside the sandbox still resolve") remains correct even after this swap — the fix only moves the sandbox check before the extension guard, not before the indexed-file fallback.