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

* Reject res:// path traversal in script + filesystem write tools (#347)

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

* Cache res_root in McpPathValidator (simplify pass)

`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

* Address Copilot review on PR #369

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

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: dsarno

plugin/addons/godot_ai/handlers/filesystem_handler.gd

@@ -7,11 +7,9 @@ extends RefCounted
 func read_file(params: Dictionary) -> Dictionary:
 	var path: String = params.get("path", "")
 
-	if path.is_empty():
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: path")
-
-	if not path.begins_with("res://"):
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res://")
+	var path_err := McpPathValidator.validate_resource_path(path)
+	if not path_err.is_empty():
+		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, path_err)
 
 	if not FileAccess.file_exists(path):
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "File not found: %s" % path)
@@ -37,11 +35,9 @@ func write_file(params: Dictionary) -> Dictionary:
 	var path: String = params.get("path", "")
 	var content: String = params.get("content", "")
 
-	if path.is_empty():
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: path")
-
-	if not path.begins_with("res://"):
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res://")
+	var path_err := McpPathValidator.validate_resource_path(path)
+	if not path_err.is_empty():
+		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, path_err)
 
 	# Ensure parent directory exists
 	var dir_path := path.get_base_dir()
@@ -87,8 +83,9 @@ func reimport(params: Dictionary) -> Dictionary:
 
 	for path_variant in paths:
 		var path: String = str(path_variant)
-		if not path.begins_with("res://"):
-			not_found.append("%s (must start with res://)" % path)
+		var path_err := McpPathValidator.validate_resource_path(path)
+		if not path_err.is_empty():
+			not_found.append("%s (%s)" % [path, path_err])
 			continue
 		if not FileAccess.file_exists(path):
 			not_found.append("%s (file does not exist)" % path)

plugin/addons/godot_ai/handlers/script_handler.gd

@@ -25,11 +25,9 @@ func create_script(params: Dictionary) -> Dictionary:
 	var path: String = params.get("path", "")
 	var content: String = params.get("content", "")
 
-	if path.is_empty():
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: path")
-
-	if not path.begins_with("res://"):
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res://")
+	var path_err := McpPathValidator.validate_resource_path(path)
+	if not path_err.is_empty():
+		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, path_err)
 
 	if not path.ends_with(".gd"):
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must end with .gd")
@@ -116,11 +114,9 @@ func _finish_create_script_deferred(request_id: String, path: String, data: Dict
 func read_script(params: Dictionary) -> Dictionary:
 	var path: String = params.get("path", "")
 
-	if path.is_empty():
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: path")
-
-	if not path.begins_with("res://"):
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res://")
+	var path_err := McpPathValidator.validate_resource_path(path)
+	if not path_err.is_empty():
+		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, path_err)
 
 	if not FileAccess.file_exists(path):
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "File not found: %s" % path)
@@ -148,14 +144,13 @@ func patch_script(params: Dictionary) -> Dictionary:
 	var new_text: String = params.get("new_text", "")
 	var replace_all: bool = params.get("replace_all", false)
 
-	if path.is_empty():
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: path")
+	var path_err := McpPathValidator.validate_resource_path(path)
+	if not path_err.is_empty():
+		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, path_err)
 	if not "old_text" in params:
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: old_text")
 	if not "new_text" in params:
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: new_text")
-	if not path.begins_with("res://"):
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res://")
 	if not path.ends_with(".gd"):
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must end with .gd (use filesystem_write_text for other text files)")
 	if old_text.is_empty():
@@ -283,11 +278,9 @@ func detach_script(params: Dictionary) -> Dictionary:
 func find_symbols(params: Dictionary) -> Dictionary:
 	var path: String = params.get("path", "")
 
-	if path.is_empty():
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: path")
-
-	if not path.begins_with("res://"):
-		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res://")
+	var path_err := McpPathValidator.validate_resource_path(path)
+	if not path_err.is_empty():
+		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, path_err)
 
 	if not FileAccess.file_exists(path):
 		return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "File not found: %s" % path)

plugin/addons/godot_ai/utils/path_validator.gd

@@ -0,0 +1,53 @@
+@tool
+class_name McpPathValidator
+extends RefCounted
+
+## Validates `res://`-rooted paths against directory-traversal escape.
+##
+## Issue #347 (audit-v2 #3): handlers were accepting `res://../etc/passwd.gd`
+## because the only check was `path.begins_with("res://")`. LLM-driven path
+## generation (prompt injection, agent typos, untrusted issue/PR text in
+## context) can produce traversal payloads for the write tools that produce
+## arbitrary disk content (`script_create`, `filesystem_write_text`,
+## `patch_script`) and for the matching reads (info disclosure surface).
+##
+## Layered checks:
+##   1. non-empty
+##   2. begins with `res://`
+##   3. no `..` substring (cheap, catches every common traversal payload)
+##   4. globalize → simplify → verify still under the project root
+##      (defence-in-depth against URL-encoded or otherwise sneaky shapes
+##      that simplify_path collapses but the substring check might miss)
+
+
+# Cached project root. `ProjectSettings.globalize_path("res://")` is stable
+# across the editor's lifetime — caching avoids redundant resolution on every
+# call. Matters most for `reimport`, which loops the validator over each path
+# in a batch. Lazy-init on first call so static-load timing can't see a
+# half-initialised ProjectSettings.
+static var _cached_res_root: String = ""
+
+
+static func _res_root() -> String:
+	if _cached_res_root.is_empty():
+		_cached_res_root = ProjectSettings.globalize_path("res://").simplify_path()
+	return _cached_res_root
+
+
+## Returns "" when the path is a safe `res://`-rooted reference inside the
+## project root. Returns a human-readable error message otherwise; callers
+## wrap it with `McpErrorCodes.make(INVALID_PARAMS, ...)`.
+static func validate_resource_path(path: String) -> String:
+	if path.is_empty():
+		return "Missing required param: path"
+	if not path.begins_with("res://"):
+		return "Path must start with res://"
+	if ".." in path:
+		return "Path must not contain '..' (path traversal not allowed)"
+	var globalized := ProjectSettings.globalize_path(path).simplify_path()
+	var res_root := _res_root()
+	# Append a separator so `/proj_evil/...` can't pretend to be inside
+	# `/proj` via prefix match. `globalized == res_root` covers `path == "res://"`.
+	if globalized != res_root and not globalized.begins_with(res_root + "/"):
+		return "Path must resolve under res:// root"
+	return ""

plugin/addons/godot_ai/utils/path_validator.gd.uid

@@ -0,0 +1 @@
+uid://blxntmd65ljyu

test_project/tests/test_filesystem.gd

@@ -59,6 +59,13 @@ func test_read_file_not_found() -> void:
 	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
 
 
+func test_read_file_rejects_traversal_path() -> void:
+	## Issue #347: traversal in read_file is the file-disclosure primitive.
+	var result := _handler.read_file({"path": "res://../etc/passwd"})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "..")
+
+
 # ----- write_file -----
 
 func test_write_file_basic() -> void:
@@ -105,6 +112,23 @@ func test_write_file_invalid_prefix() -> void:
 	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
 
 
+func test_write_file_rejects_traversal_path() -> void:
+	## Issue #347: the actual arbitrary-disk-write primitive.
+	## Use a synthetic target so a Unix host's pre-existing /etc/* doesn't
+	## false-positive the disk-state assertion below. If a regression let
+	## the write through, the file would land one dir above the project at
+	## `<project_parent>/__mcp_traversal_test_target__`, which never
+	## exists in a clean tree.
+	var traversal_path := "res://../__mcp_traversal_test_target__.txt"
+	var result := _handler.write_file({
+		"path": traversal_path,
+		"content": "owned\n",
+	})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "..")
+	assert_false(FileAccess.file_exists(traversal_path), "traversal must not write to disk")
+
+
 # ----- reimport -----
 
 func test_reimport_missing_paths() -> void:
@@ -139,3 +163,12 @@ func test_reimport_invalid_prefix() -> void:
 	assert_has_key(result, "data")
 	assert_eq(result.data.reimported_count, 0)
 	assert_eq(result.data.not_found_count, 1)
+
+
+func test_reimport_rejects_traversal_path() -> void:
+	## Issue #347: per-path validation in the loop must catch traversal too.
+	var result := _handler.reimport({"paths": ["res://../etc/passwd"]})
+	assert_has_key(result, "data")
+	assert_eq(result.data.reimported_count, 0)
+	assert_eq(result.data.not_found_count, 1)
+	assert_contains(result.data.not_found[0], "..")

test_project/tests/test_path_validator.gd

@@ -0,0 +1,96 @@
+@tool
+extends McpTestSuite
+
+## Tests for McpPathValidator — the resource-path traversal guard shared by
+## script_handler and filesystem_handler. Issue #347 (audit-v2 #3): paths
+## like `res://../etc/passwd.gd` were passing the bare prefix check.
+
+
+func suite_name() -> String:
+	return "path_validator"
+
+
+# ----- happy path -----
+
+func test_valid_simple_path_returns_empty() -> void:
+	assert_eq(McpPathValidator.validate_resource_path("res://main.tscn"), "")
+
+
+func test_valid_nested_path_returns_empty() -> void:
+	assert_eq(McpPathValidator.validate_resource_path("res://addons/godot_ai/plugin.gd"), "")
+
+
+func test_valid_root_path_returns_empty() -> void:
+	## "res://" itself has no traversal and resolves exactly to the project
+	## root, so the validator must not reject it on the boundary check.
+	assert_eq(McpPathValidator.validate_resource_path("res://"), "")
+
+
+# ----- empty + prefix -----
+
+func test_empty_path_rejected() -> void:
+	var err := McpPathValidator.validate_resource_path("")
+	assert_false(err.is_empty(), "empty path must report an error")
+	assert_contains(err, "Missing required param")
+
+
+func test_missing_prefix_rejected() -> void:
+	var err := McpPathValidator.validate_resource_path("/tmp/foo.gd")
+	assert_false(err.is_empty(), "absolute path without res:// must be rejected")
+	assert_contains(err, "res://")
+
+
+func test_user_prefix_rejected() -> void:
+	## user:// is a valid Godot scheme but it's outside the project — agents
+	## must not be able to write to user:// via the same handlers (they have
+	## different lifecycle and permission semantics).
+	var err := McpPathValidator.validate_resource_path("user://save.dat")
+	assert_false(err.is_empty(), "user:// path must be rejected")
+	assert_contains(err, "res://")
+
+
+# ----- traversal regressions (the actual security guard) -----
+
+func test_rejects_dotdot_at_root() -> void:
+	## The exact attack shape called out in issue #347.
+	var err := McpPathValidator.validate_resource_path("res://../etc/passwd.gd")
+	assert_false(err.is_empty(), "res://../etc/passwd.gd must be rejected")
+	assert_contains(err, "..")
+
+
+func test_rejects_dotdot_nested() -> void:
+	var err := McpPathValidator.validate_resource_path("res://addons/../../etc/passwd")
+	assert_false(err.is_empty(), "nested traversal must be rejected")
+	assert_contains(err, "..")
+
+
+func test_rejects_deep_dotdot_chain() -> void:
+	## Defence in depth: even if a payload chains through legitimate-looking
+	## subdirectories first, the substring check fires.
+	var err := McpPathValidator.validate_resource_path("res://addons/godot_ai/../../../etc/passwd.gd")
+	assert_false(err.is_empty(), "deep traversal chain must be rejected")
+
+
+func test_rejects_dotdot_in_filename() -> void:
+	## Per the audit's fix shape: reject any path containing `..`. A filename
+	## like `my..backup.json` is unusual enough that we accept the false-
+	## positive cost in exchange for a simpler, shorter security boundary.
+	var err := McpPathValidator.validate_resource_path("res://data/my..backup.json")
+	assert_false(err.is_empty(), "literal '..' anywhere in path must be rejected")
+
+
+# ----- boundary check (defence in depth past the substring guard) -----
+
+func test_well_formed_nested_path_passes_boundary_check() -> void:
+	## Sanity: a path with no `..` substring still has to clear the
+	## globalize_path → simplify_path → boundary check. This pins the safe
+	## path so a regression in the boundary comparison (e.g. trailing-slash
+	## handling) couldn't silently reject legitimate paths.
+	##
+	## Direct traversal payloads can't reach the boundary check — they're
+	## caught by the `..` substring rejection above — so there's no
+	## non-`..` traversal payload to assert rejection on. The boundary
+	## check exists as defence-in-depth for any future encoding-bypass
+	## that smuggles a `..` past the substring guard.
+	var safe := McpPathValidator.validate_resource_path("res://addons/godot_ai")
+	assert_eq(safe, "", "well-formed nested path must validate")

test_project/tests/test_path_validator.gd.uid

@@ -0,0 +1 @@
+uid://03e3vouyp30v

test_project/tests/test_script.gd

@@ -106,6 +106,22 @@ func test_create_script_wrong_extension() -> void:
 	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
 
 
+func test_create_script_rejects_traversal_path() -> void:
+	## Issue #347: `res://../etc/passwd.gd` previously passed the prefix check.
+	## Use a synthetic target so a host with a pre-existing
+	## `<project_parent>/etc/passwd.gd` couldn't false-positive the disk
+	## assertion. The synthetic name never exists in a clean tree.
+	var traversal_path := "res://../__mcp_traversal_test_target__.gd"
+	var result := _handler.create_script({
+		"path": traversal_path,
+		"content": "extends Node\n",
+	})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "..")
+	## Defence: confirm the file was NOT written outside the project.
+	assert_false(FileAccess.file_exists(traversal_path), "traversal must not write to disk")
+
+
 # ----- patch_script -----
 
 func test_patch_script_basic() -> void:
@@ -222,6 +238,18 @@ func test_patch_script_invalid_prefix() -> void:
 	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
 
 
+func test_patch_script_rejects_traversal_path() -> void:
+	## Issue #347 regression: traversal must be caught before the file is
+	## opened for read or write.
+	var result := _handler.patch_script({
+		"path": "res://../etc/passwd.gd",
+		"old_text": "x",
+		"new_text": "y",
+	})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "..")
+
+
 # ----- read_script -----
 
 func test_read_script_basic() -> void:
@@ -248,6 +276,13 @@ func test_read_script_not_found() -> void:
 	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
 
 
+func test_read_script_rejects_traversal_path() -> void:
+	## Issue #347: read_script must not become a file-disclosure primitive.
+	var result := _handler.read_script({"path": "res://../etc/passwd.gd"})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "..")
+
+
 # ----- attach_script -----
 
 func test_attach_script_basic() -> void:
@@ -378,3 +413,10 @@ func test_find_symbols_invalid_prefix() -> void:
 func test_find_symbols_not_found() -> void:
 	var result := _handler.find_symbols({"path": "res://nonexistent_script.gd"})
 	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+
+
+func test_find_symbols_rejects_traversal_path() -> void:
+	## Issue #347: find_symbols also reads file content; same disclosure surface.
+	var result := _handler.find_symbols({"path": "res://../etc/passwd.gd"})
+	assert_is_error(result, McpErrorCodes.INVALID_PARAMS)
+	assert_contains(result.error.message, "..")