[Audit v2 #3] Reject res:// path traversal in script + filesystem write tools by dsarno · Pull Request #369 · hi-godot/godot-ai

dsarno · 2026-05-05T15:07:55Z

Summary

Closes #347 — audit-v2 P1 finding #3 from umbrella #343.

script_handler.gd and filesystem_handler.gd validated agent-supplied
paths only with path.begins_with("res://"). That accepts payloads like
res://../etc/passwd.gd; whether Godot rejects them depends on platform/
version. Combined with LLM-driven path generation (prompt injection,
agent typos, untrusted issue/PR text in context), this is a real escape
vector for the only write tools that produce arbitrary disk content
(script_create, filesystem_write_text, patch_script) and for the
matching reads (info-disclosure surface).

Fix

Adds plugin/addons/godot_ai/utils/path_validator.gd →
McpPathValidator.validate_resource_path(path) with the four layers
called for in the issue's fix shape:

non-empty
path.begins_with("res://")
no .. substring (cheap defence-in-depth)
ProjectSettings.globalize_path(path).simplify_path() must still
resolve under globalize_path("res://").simplify_path() (boundary
verification with trailing-slash handling so /proj_evil/... can't
pretend to be inside /proj)

Migrates every prefix-check call site named in #347:

script_handler.gd lines 31 / 122 / 157 / 289 →
create_script, read_script, patch_script, find_symbols
filesystem_handler.gd lines 13 / 43 / 90 →
read_file, write_file, reimport (per-path inside the loop)

No bare begins_with("res://") remains in either handler.

Tests

test_project/tests/test_path_validator.gd — McpTestSuite covering
happy path, prefix variants (user:// rejected), and four traversal
shapes including the exact res://../etc/passwd.gd payload from the
audit body.
test_project/tests/test_script.gd / test_filesystem.gd — per-
handler regression tests asserting INVALID_PARAMS on the traversal
payload at every entry point. The two write tests also assert no
file landed on disk outside the project root.
tests/unit/test_path_traversal_guard.py — source-structure pytest
(5 tests) pinning the validator's existence, all four required check
layers, and the absence of bare begins_with("res://") patterns in
the affected handlers. Drift here would silently weaken the security
boundary without any single behavioural test catching it.

Test plan

ruff check src/ tests/ clean
ruff format applied to the new test file (other files have
pre-existing format drift on beta — not touched)
pytest -q — 772 passed (5 new)
script/ci-check-gdscript — all GDScript files OK (the new
validator + test suite parse cleanly)
Live editor test_run via MCP — not feasible from this sandboxed
CLI session (the running godot is --headless without the
GODOT_AI_ALLOW_HEADLESS=1 override, so the plugin returns
early). The Godot CI matrix (Linux + macOS + Windows) will
exercise the new path_validator suite and the new handler
regression tests on PR.

Scope

Per the audit prompt's "One audit finding == one PR" rule, only the
seven sites named in #347 are migrated. Other handlers
(scene_handler.gd, theme_handler.gd, resource_handler.gd,
autoload_handler.gd, material_handler.gd, audio_handler.gd,
ui_handler.gd, node_handler.gd) still carry the bare
begins_with("res://") pattern; if a future audit finding promotes
them, the validator is already there to delegate to.

Blocks #355 (audit-v2 #11) only inasmuch as #355 references finding #1
(WebSocket auth/Origin) — independent of this PR.

Part of #343.

https://claude.ai/code/session_01LYwesbn3B7LzLvcV2mPQTV

Generated by Claude Code

Audit v2 finding #3 (#343): script_handler.gd and filesystem_handler.gd validated agent-supplied paths only with `path.begins_with("res://")`, which accepts payloads like `res://../etc/passwd.gd`. LLM-driven path generation (prompt injection, agent typos, untrusted issue/PR text in context) makes this a real escape vector for the only write tools that produce arbitrary disk content. Adds `McpPathValidator.validate_resource_path` with four layered checks: non-empty, `res://` prefix, no `..` substring (cheap defence-in-depth), and globalize-simplify boundary verification against the project root. Migrates the seven prefix-check call sites named in #347: script_handler.gd (create_script, read_script, patch_script, find_symbols) and filesystem_handler.gd (read_file, write_file, reimport). Tests: - McpTestSuite for the validator (happy path, prefix, all four traversal shapes including the exact `res://../etc/passwd.gd` payload from the audit body). - Per-handler regression tests asserting INVALID_PARAMS on the same payload, plus disk-state assertions for the two write tools confirming no file was written outside the project root. - Source-structure pytest pinning the validator's existence, the four required check layers, and the absence of bare `begins_with("res://")` patterns in the affected handlers — drift here would silently weaken the security boundary without any single test failing on a single bad input. https://claude.ai/code/session_01LYwesbn3B7LzLvcV2mPQTV

codecov · 2026-05-05T15:10:55Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

`ProjectSettings.globalize_path("res://")` is stable across the editor's lifetime — re-resolving it on every validation call (especially inside `reimport`'s per-path loop, where a 100-path batch produced 100x the redundant globalize) is wasted work. Lazy-init a static `_cached_res_root` on first call so static-load timing can't see a half-initialised ProjectSettings. Subsequent calls hit the cache. https://claude.ai/code/session_01LYwesbn3B7LzLvcV2mPQTV

Copilot

Pull request overview

This PR addresses audit-v2 finding #3 / issue #347 by hardening res:// path validation to prevent directory traversal (e.g. res://../etc/passwd.gd) in the script and filesystem read/write tool handlers.

Changes:

Introduces a shared McpPathValidator.validate_resource_path() utility that performs layered res:// traversal/boundary validation.
Replaces begins_with("res://") checks in script_handler.gd and filesystem_handler.gd with the shared validator.
Adds Godot-side regression tests and a Python “source-structure” guard test to prevent future reintroduction of the insecure prefix-only checks.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`plugin/addons/godot_ai/utils/path_validator.gd`	Adds centralized `res://` traversal/boundary validator for handlers to reuse.
`plugin/addons/godot_ai/utils/path_validator.gd.uid`	Godot UID sidecar for the new validator script.
`plugin/addons/godot_ai/handlers/script_handler.gd`	Switches all relevant entry points to use `McpPathValidator` instead of prefix-only checks.
`plugin/addons/godot_ai/handlers/filesystem_handler.gd`	Switches read/write/reimport to use `McpPathValidator` (including per-path validation in the reimport loop).
`test_project/tests/test_path_validator.gd`	Adds McpTestSuite coverage for validator behavior (happy paths + traversal rejection).
`test_project/tests/test_path_validator.gd.uid`	Godot UID sidecar for the new validator test.
`test_project/tests/test_script.gd`	Adds traversal regression tests for script create/read/patch/find-symbols entry points.
`test_project/tests/test_filesystem.gd`	Adds traversal regression tests for filesystem read/write/reimport entry points.
`tests/unit/test_path_traversal_guard.py`	Adds pytest “structure guard” to pin validator presence/check layers and prevent reintroducing bare `begins_with("res://")` in the audited handlers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+func test_rejects_path_resolving_outside_root() -> void:
+	## Direct traversal is caught by the substring guard. This test exercises
+	## the globalize_path → simplify_path → boundary verification — if a
+	## future encoding bypass slips past the substring check, this layer
+	## still rejects.
+	## Sanity: validate_resource_path on something we know simplifies safely.


+func test_write_file_rejects_traversal_path() -> void:
+	## Issue #347: the actual arbitrary-disk-write primitive.
+	var result := _handler.write_file({
+		"path": "res://../etc/passwd",
+		"content": "owned\n",
+	})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "..")
+	assert_false(FileAccess.file_exists("res://../etc/passwd"), "traversal must not write to disk")


+    # 1) prefix check
+    assert 'begins_with("res://")' in source, "validator must check res:// prefix"
+    # 2) literal `..` substring rejection — the cheap defence-in-depth layer
+    assert '".." in path' in source, "validator must reject any path containing '..'"
+    # 3) globalize → simplify normalisation
+    assert "ProjectSettings.globalize_path" in source
+    assert "simplify_path()" in source
+    # 4) boundary verification against the project root


Three fixes from Copilot's review: 1. test_filesystem.gd / test_script.gd write-traversal tests asserted `FileAccess.file_exists("res://../etc/passwd")` is false. On Unix hosts /etc/passwd already exists, so a regression that let the write through wouldn't matter, but the assertion would *also* fail when Godot's FileAccess globalizes the traversal path through to /etc/* — false-positive failure even when the validator correctly rejected. Switch to a synthetic target name that won't exist in any clean tree. 2. test_path_validator.gd's last test was named `test_rejects_path_resolving_outside_root` but only validated a safe in-root path (the boundary check is unreachable for non-`..` paths, since globalize_path can't escape res:// without a `..` substring, which is rejected by the earlier substring check). Renamed to `test_well_formed_nested_path_passes_boundary_check` so the name matches what the test actually verifies, with a comment explaining why no rejection-side input is asserted on this layer. 3. test_path_traversal_guard.py's structural test claimed to pin "all four layers" but never asserted the non-empty (`is_empty()`) check. Added the missing assertion so the empty-path layer is also drift- protected. Also updated the docstring to drop the misleading "four layers" framing. https://claude.ai/code/session_01LYwesbn3B7LzLvcV2mPQTV

dsarno · 2026-05-05T15:15:30Z

Addressed all three Copilot review findings in bcd6b0d:

test_path_validator.gd boundary test misnamed → renamed to test_well_formed_nested_path_passes_boundary_check with a comment explaining why no rejection-side input exists for that layer (the substring guard catches every .. payload before the boundary check is reachable; the boundary check is defence-in-depth for any future encoding bypass).
test_filesystem.gd / test_script.gd write-traversal disk assertions could false-positive on Unix (where /etc/passwd already exists) → switched both to a synthetic target name (__mcp_traversal_test_target__) that won't exist in any clean tree, so the assertion now reliably means "we did not create this file".
test_path_traversal_guard.py missed the non-empty layer → added the is_empty() assertion and updated the docstring to drop the misleading "four layers" framing now that all five guards are pinned.

Generated by Claude Code

dsarno · 2026-05-05T15:23:53Z

CI status (commit bcd6b0d): 9/11 green, only Godot tests / macOS and Godot tests / Windows failed.

Linux Godot tests passed at 1m17s with the new validator + 18 new tests. The macOS+Windows failures look like the known camera.test_configure_current_sibling_unmark_single_undo flake tracked in #316 (4th recurrence on macOS, 11 prior fix attempts) — and my changes have no plausible mechanism to fail only on those two platforms (ProjectSettings.globalize_path is string substitution; simplify_path normalises \ → / and strips trailing slashes consistently across OSes).

I don't have GitHub Actions log access from this environment (the public API rate-limits unauth, the HTML page requires sign-in), so I can't pull the specific failure messages to confirm. Could you share the failing test name + assertion text from the macOS and Windows job logs? If it's #316 and friends, that's the audit-v2 prompt's "ONE retry acceptable" exception. If it's something my change actually triggered, the message will pinpoint the test and I'll fix the cause.

Generated by Claude Code

Three minimal accuracy fixes flagged by /update-docs after the post-audit smoke pass on 72b35d7: - CLAUDE.md: pytest count 757 -> 903 (current passing total). - docs/implementation-plan.md: header annotation extended to cover audit-v2 PRs #369-#390 and the post-landing smoke verdict; Phase 3 exit criteria test count 757 + 991+ -> 903 + 1225 (47 suites). - README.md: tool count line "Over 120 MCP tools" -> "Over 120 ops across ~39 MCP tools" (the surface collapsed to ~39 in PR #203's rollup refactor; the README was never updated). No code changes; satellite docs left alone per the rule (audit-v2 was security/reliability/structural cleanup, not new tool surface).

51 commits forward from main, headlined by two architecture audits: audit-v1 (issue #297, PRs #298-#315): scene-path ancestry guard, update/config data-loss safeguards, lifecycle reliability, characterization tests, plugin.gd extraction (McpPortResolver + McpServerLifecycleManager), state-model cleanup, McpUpdateManager extraction, Runtime Protocol deletion + DirectRuntime retype, narrowed meta-tool JSON coercion, self-update preload-alias hardening, locked FastMCP middleware order. audit-v2 (issue #343, PRs #369-#390): origin allowlist (DNS-rebinding guard, #345/#375), path-traversal guards on script_* / filesystem_* writes (#347/#369), errno.EADDRINUSE portability across all OSes (#348/#373), SessionRegistry RLock removal (#350/#370), Pydantic-validated WebSocket event payloads (#351/#378), sole-survivor auto-failover on active disconnect (#352/#379), 30s filesystem_changed watchdog during update reload (#353/#381), FAILED_MIXED self-update visibility via mixed_state in editor_state (#354/#382), 32/tick inbound packet-drain cap with spillover logging (#356/#383), error-code vocabulary enrichment (#365/#385: NODE_NOT_FOUND, PROPERTY_NOT_ON_CLASS, VALUE_OUT_OF_RANGE, MISSING_REQUIRED_PARAM cut INVALID_PARAMS sites 471 -> 97), resolve-or-error helper extraction (#364/#389: 38+ duplicate sites migrated), resource-form lint for meta-tool reads (#363/#386), LogViewer + PortPickerPanel extraction from mcp_dock.gd (#360/#390), LogViewer.tick() buffer-clear recovery (#392). Conflict resolutions (both intentional, both took beta's side): - plugin/addons/godot_ai/animation_handler.gd plus new submodules animation_presets.gd and animation_values.gd: beta's 4-domain split retained. Main's revert (#368) was CI-flake hygiene, not a structural rejection per its own commit message ("Empty commit to retrigger CI (flake on Godot tests / macOS)"). Beta's #367 is the canonical state the audit found wanted. - plugin/addons/godot_ai/clients/_toml_strategy.gd: beta's version retained. Beta handles inline comments after section headers (`[next_section] # note`) via _is_any_section_header(); main's later re-derived bare-key fix uses the simpler bracket check and would regress on commented headers. The four main-only Windows / TOML hotfixes (#302, #318, #319, #320) all landed on beta independently under different commit hashes and are content-equivalent or beta-improved; no cherry-pick was needed before this merge. Validation: - CI: green on beta tip 72b35d7 (and on PR #392 separately) across ubuntu-latest / macos-latest / windows-latest for Python tests, Godot tests, release-smoke, and game-capture-smoke. - Operator smoke (macOS, 2026-05-06): all 10 phases of the audit-v1+v2 post-landing runbook green. Report on issue #343. 47 GDScript suites + 903 Python tests passing. Phase 7 interactive self-update verified end-to-end with the local-self-update-smoke harness; plugin advanced 2.3.2 -> 2.3.3, no .ips, no _exit_tree leak, server stop/start clean. - Operator smoke (Windows 11, beta tip d5aa29f, 2026-05-06): Phase 7 self-update green (7/7; macOS-only .ips check correctly downgraded to SKIP). Path-traversal guard rejects backslash variant. Cursor client configure cycle round-trips cleanly. editor_state ping 109ms. Pre-existing pytest UnicodeDecodeError on Windows tracked separately as #397 (also on main, not introduced by this merge). # Conflicts: # test_project/tests/test_clients.gd

Copilot AI review requested due to automatic review settings May 5, 2026 15:07

Copilot started reviewing on behalf of dsarno May 5, 2026 15:08 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

dsarno merged commit 758d821 into beta May 5, 2026
20 of 22 checks passed

This was referenced May 5, 2026

[Audit v2 #3 — P1] script_create / filesystem_write_text accept ../traversal #347

Closed

Architecture audit v2: post-#297 soft spots — auth/origin gap, path-traversal, errno portability, and structural carry-overs #343

Closed

dsarno mentioned this pull request May 6, 2026

Merge beta into main: audit-v1 PRs #298-#315 + audit-v2 PRs #369-#390 #394

Merged

7 tasks

dsarno deleted the claude/audit-v2-347-path-traversal branch May 7, 2026 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Audit v2 #3] Reject res:// path traversal in script + filesystem write tools#369

[Audit v2 #3] Reject res:// path traversal in script + filesystem write tools#369
dsarno merged 3 commits into
betafrom
claude/audit-v2-347-path-traversal

dsarno commented May 5, 2026

Uh oh!

codecov Bot commented May 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

dsarno commented May 5, 2026

Uh oh!

dsarno commented May 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dsarno commented May 5, 2026

Summary

Fix

Tests

Test plan

Scope

Uh oh!

codecov Bot commented May 5, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

dsarno commented May 5, 2026

Uh oh!

dsarno commented May 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants