fix(finqa_env): prevent path traversal via agent-supplied company/table names

[OfficialAbhinavSingh]

Summary

The FinQA env's MCP tools (get_descriptions, get_table_info, sql_query) build a filesystem path from the agent-supplied company_name/table_name and guard it only with an existence check (os.path.isdir/os.path.isfile), not a containment check. An agent can pass .. components over the MCP boundary to enumerate directories and read arbitrary .json files outside the data directory (e.g. withheld answer data for other episodes → reward hacking, or any .json mounted into the container). This adds a _resolve_within() helper that rejects any path escaping companies_path, applied to all three tools.

Type of Change

• Bug fix

Alignment Checklist

• I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
• I have checked .claude/docs/INVARIANTS.md and no invariants are violated
• I have run lint (ruff format --check, ruff check, usort check) and the finqa tests — all pass

RFC Status

• Not required (bug fix, no API change)

Details

envs/finqa_env/server/tools.py — the sinks:

company_path = os.path.join(self.companies_path, company_name)      # get_descriptions / get_table_info
table_path   = os.path.join(self.companies_path, company_name, f"{cleaned_table_name}.json")  # sql_query

company_name/table_name come straight from the MCP CallToolAction arguments, and the only validation is os.path.isdir/isfile — an existence check, not containment. .. traversal therefore escapes companies_path.

Fix: a small _resolve_within(*parts) helper resolves the joined path with os.path.realpath and returns None unless it stays within companies_path (via os.path.commonpath). Each tool now rejects a None result. Legitimate company/table access is unchanged; the fix runs before any file read. Blast radius is confined to the env's own container/process, but it breaks the intended "companies are a closed set from get_available_companies()" boundary.

Test Plan

New self-contained test class TestToolsPathTraversal in tests/envs/test_finqa_environment.py (synthetic input_companies/ in tmp_path + a secret.json outside it; needs only pandas, not the downloaded dataset, so it runs in CI — unlike the dataset-gated TestTools).

Verified red → green against a synthetic layout:

Before the fix (vulnerable):

get_descriptions('..')                         -> ["secret"]                      # enumerated parent dir
sql_query('..', 'secret', 'SELECT password …') -> [{"password":"leaked-secret"}]  # read file outside root

After the fix (blocked, legit access intact):

legit get_descriptions('acme')                 -> ok (no error)
legit sql_query('acme','revenue', …)           -> ok (no error)
get_descriptions('..')                         -> Error (no leak)
sql_query('..','secret', …)                    -> Error: Table file not found …   (no leak)
sql_query('acme','../../secret', …)            -> Error (no leak)
get_table_info('../..','whatever')             -> Error

Full file: 67 passed, 16 skipped (skips = dataset-gated integration tests). ruff format --check, ruff check, usort check all clean on the changed files.

Claude Code Review

Self-reviewed the diff for scope and the containment logic (realpath + commonpath, ValueError guard for cross-drive on Windows, applied uniformly to all three sinks). No new dependencies; no public API change; docstring follows the HF doc-builder format.

---

Note

Medium Risk
Security-sensitive fix at the MCP agent boundary for filesystem reads; behavior change is limited to rejecting malicious paths, with dedicated regression tests.

Overview
Closes a path-traversal hole (CWE-22) in FinQA MCP tools where company_name and table_name were joined under input_companies/ with only existence checks, so .. could read JSON outside the data directory.

Adds _resolve_within() in FinQATools: realpath + commonpath against companies_path, returning None on escape (with a Windows cross-drive guard). get_descriptions, get_table_info, and sql_query now resolve paths through this helper before any directory listing or file read; sql_query uses a generic “table not found” error when resolution fails.

Adds TestToolsPathTraversal with a synthetic layout (secret.json outside input_companies/) so CI verifies legitimate access and blocks traversal via company or table names—no downloaded dataset required.

^{Reviewed by Cursor Bugbot for commit 4050b9b. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.