harden(handlers): route every path-taking handler through McpPathValidator by dsarno · Pull Request #546 · hi-godot/godot-ai

dsarno · 2026-06-09T23:44:58Z

Wave-1 advisory GHSA-p5x8-v25q-qw69 (findings GH-1…GH-4) plus the full code-review follow-up.

Routes every path-taking handler through McpPathValidator — the strong traversal validator from #347 was wired into only 2 of ~12 handlers; the rest used a bare begins_with("res://") (which doesn't reject ..) or no check at all.

Validator (`utils/path_validator.gd`)

validate_resource_path(path, for_write=false) — non-empty → null-byte reject → res:// prefix → .. reject → globalize/simplify under-root containment.
validate_loadable_path(path) — for ResourceLoader-backed loads: accepts confined res://, uid:// (opaque resource id, can't express traversal), and user:// (confined under the user-data root). Restores uid:///user:// loads that the strict validator would have broken.
for_write blocklist — write callers additionally refuse res://project.godot and its startup shadow res://override.cfg, and the res://.godot/ metadata dir. Comparisons are case-folded (macOS/Windows are case-insensitive). .import sidecars are allowed to write — editing import options + reimporting is a legitimate, recoverable workflow. Reads permit inspecting all of these.
Shared path_error / loadable_error wrappers return a ready error dict so every handler reports the same code (MISSING_REQUIRED_PARAM for empty, VALUE_OUT_OF_RANGE for a bad path).
Null-byte check guards the String.chr(0) sentinel so builds that normalize embedded nulls away aren't affected.

Handlers

Writers validate with for_write=true (closes the traversal-write primitive); load/read sites validate before ResourceLoader.load/exists (closes the unvalidated-load surface incl. the @tool-script-execution risk from open_scene/create_node). Material/theme thread for_write so pure reads (get_material, apply_theme) don't run the write blocklist.

Error-code contract change

Path errors on filesystem/script handlers move from INVALID_PARAMS to MISSING_REQUIRED_PARAM (empty) / VALUE_OUT_OF_RANGE (invalid), unifying with the rest. Noted for release notes.

Tests (live Godot 4.6.3, all suites 0 failures)

Validator unit tests: null-byte reject, write blocklist (project.godot / override.cfg / .godot/, case-insensitive), .import-allowed-for-write, reads-allow-those, traversal-still-rejected, uid:///user:// accepted, user:// traversal + unknown scheme rejected. Scene traversal/manifest-overwrite rejection. End-to-end smoke (real uid:// loads, blocklist via live tools) + stormtest (3356 calls, all reloads survived). test_error_code_distribution + test_path_traversal_guard green.

Closes the GH-1…GH-4 advisory items. 🤖 Generated with Claude Code

codecov · 2026-06-10T00:05:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…dator The strong traversal validator from #347 was wired into only filesystem and script handlers; ~12 other path-taking handlers used a bare begins_with("res://") check (which does not reject "..") or no check at all (relying solely on ResourceLoader.exists/load). This unifies all of them on McpPathValidator and extends the validator with two checks. Validator (utils/path_validator.gd): - Reject embedded null bytes (truncation trap — the path written could differ from the one validated). - New for_write flag: write callers additionally refuse res://project.godot, the res://.godot/ metadata dir, and .import sidecars (overwriting these corrupts the project / import cache). Reads still permit inspecting them. Signature is backward-compatible (for_write defaults to false). Writers now validate with for_write=true (closes the traversal-write primitive): resource_io.save_to_disk (backs resource/environment/texture/curve saves), scene create_scene/save_scene_as, material/theme save helpers, filesystem write_text, script create/patch. Load/read sites now validate (closes the unvalidated-load surface, incl. the @tool-script-execution risk from open_scene/create_node): resource load/assign + nested object-property loads, scene open_scene, script attach_script, node create_node scene_path + set_property resource values, audio set_stream + list root, material assign/shader/list, ui theme + stylebox, autoload path, curve set_points, particle mesh/ material/texture, material_values.load_texture. Path-validation rejections report VALUE_OUT_OF_RANGE (the code these handlers already used for "must start with res://"), keeping the INVALID_PARAMS catch-all count under the audit-v2 #21 ceiling enforced by test_error_code_distribution. The four pre-existing validator sites that already wrapped errors as INVALID_PARAMS (filesystem, script create/read/patch/find, resource_io save, material _validate_material_path) are left unchanged. set_stream and the other load handlers now reject user:// (consistent with the validator's existing test_user_prefix_rejected policy); the audio test fixture moves from user:// to a res:// .tres registered via EditorFileSystem.update_file. Tests (run against live Godot 4.6.3): validator unit tests for null-byte + write blocklist + read-allows; scene traversal/manifest-overwrite rejection. All affected suites pass (path_validator/scene/resource/node/material/theme/ curve/particle/ui/audio/autoload/filesystem/script). test_error_code_distribution passes. Parse check clean. Addresses advisory GHSA-p5x8-v25q-qw69 (GH-1, GH-2, GH-3, GH-4). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… shared helper Follow-up to the McpPathValidator unification, fixing the code-review findings. Correctness: - Restore uid:// and user:// support on load handlers. ResourceLoader accepts both (uid:// is an opaque resource id that can't express traversal; user:// is the user data sandbox), but routing every load through the strict res:// validator rejected them — a regression for uid:// refs copied from .tscn/.uid and for user:// runtime assets. New validate_loadable_path() accepts res:// (confined), uid:// (as-is), and user:// (confined under the user root), and load sites now use it: set_property/assign_resource/resource property dicts, open_scene, create_node, attach_script, set_stream, particle mesh/material/ texture, material assign/shader, ui theme/stylebox, curve, material_values. - Case-fold the write blocklist. macOS (APFS) and Windows (NTFS) are case-insensitive by default, so res://Project.godot resolved to the real project.godot and slipped past a case-sensitive compare. - Add override.cfg to the write blocklist (applied over project.godot at startup — same takeover surface as the manifest). - Reads no longer run the write blocklist: _validate_material_path / _validate_res_path / _load_material_from_path / _load_theme_from_params take for_write, passed true only by the create/save callers. get_material and apply_theme (pure reads) no longer return a spurious "Refusing to write". - Stop blocking .import writes — editing import config then reimporting is a legitimate, recoverable workflow; the blocklist is the startup-execution surface (manifest, override.cfg, .godot/) only. Cleanup / altitude: - Single error-code choke point: McpPathValidator.path_error / loadable_error return a ready error dict (MISSING_REQUIRED_PARAM for empty, VALUE_OUT_OF_RANGE for invalid) so every handler reports the same code for the same failure. filesystem/script move off their old INVALID_PARAMS wrapping onto this. - curve set_points validates the load path (the save layer, resource_io.save_to_disk, remains the authoritative write-confinement check) instead of double-running the write validator. - Drop the redundant is_empty() pre-check in material_values.load_texture. - Document the lexical-containment (no symlink resolution) limitation in the validator — GDScript has no realpath; the loopback boundary is the control. Tests (live Godot 4.6.3, all suites 0 failures): uid:// / user:// acceptance, user:// traversal + unknown-scheme rejection, case-insensitive blocklist, override.cfg block, .import now allowed; audio fixture moved back to user:// (no repo/EFS pollution). test_path_traversal_guard counts any McpPathValidator delegation; missing-path tests now assert MISSING_REQUIRED_PARAM. Addresses review findings 1-10 on GHSA-p5x8-v25q-qw69 (path confinement). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR hardens the Godot editor plugin’s MCP handlers by routing all path-taking operations through McpPathValidator, expanding validation to cover more handler entry points and adding stricter checks for write operations.

Changes:

Extend McpPathValidator with loadable-path validation (res://, uid://, user://), null-byte rejection, and a for_write mode that blocks writes to project-critical locations.
Replace ad-hoc begins_with("res://") checks across many handlers with McpPathValidator.path_error(...) / loadable_error(...).
Update and add tests to cover traversal rejection, write blocklist behavior, and revised handler error behavior.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/unit/test_path_traversal_guard.py	Updates structural regression assertions for handler validation delegation.
test_project/tests/test_script.gd	Adjusts expected error codes for missing-path script handler tests.
test_project/tests/test_scene.gd	Adds traversal + manifest-overwrite rejection tests for scene creation.
test_project/tests/test_path_validator.gd	Adds validator coverage for null bytes, write blocklist, and loadable schemes.
test_project/tests/test_filesystem.gd	Adjusts expected error codes for missing-path filesystem handler tests.
test_project/tests/test_audio.gd	Updates audio fixture naming/docs alongside loadable-path validation behavior.
plugin/addons/godot_ai/utils/resource_io.gd	Routes save paths through `McpPathValidator.path_error(..., for_write=true)`.
plugin/addons/godot_ai/utils/path_validator.gd	Implements `for_write`, null-byte checks, and loadable-path validation helpers.
plugin/addons/godot_ai/handlers/ui_handler.gd	Validates theme/stylebox paths via `loadable_error` and broader scheme support.
plugin/addons/godot_ai/handlers/theme_handler.gd	Threads `for_write` through theme path validation where saves occur.
plugin/addons/godot_ai/handlers/script_handler.gd	Uses `path_error`/`loadable_error` for all script path entry points.
plugin/addons/godot_ai/handlers/scene_handler.gd	Validates create/save via `path_error(..., for_write=true)` and open via `loadable_error`.
plugin/addons/godot_ai/handlers/resource_handler.gd	Validates resource load/assign/property resource-string loads via `loadable_error`.
plugin/addons/godot_ai/handlers/particle_handler.gd	Validates mesh/material/texture paths before `ResourceLoader` operations.
plugin/addons/godot_ai/handlers/node_handler.gd	Validates scene instancing path + resource-valued properties via `loadable_error`.
plugin/addons/godot_ai/handlers/material_values.gd	Validates texture loads via `validate_loadable_path`.
plugin/addons/godot_ai/handlers/material_handler.gd	Threads `for_write` through material-path validation; validates shader/material paths.
plugin/addons/godot_ai/handlers/filesystem_handler.gd	Uses `path_error` for read/write entry points (write uses `for_write=true`).
plugin/addons/godot_ai/handlers/curve_handler.gd	Validates curve resource paths before load.
plugin/addons/godot_ai/handlers/autoload_handler.gd	Replaces prefix check with `path_error` for autoload paths.
plugin/addons/godot_ai/handlers/audio_handler.gd	Validates `set_stream` path via `loadable_error` and listing root via `path_error`.

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

+	if path.contains(String.chr(0)):
+		return "Path must not contain null bytes"


+	if path.contains(String.chr(0)):
+		return "Path must not contain null bytes"


 			# For stylebox overrides, load from a res:// path.
 			if coerce_type == TYPE_OBJECT:
-				if value is String and value.begins_with("res://"):
+				if value is String and (value.begins_with("res://") or value.begins_with("uid://") or value.begins_with("user://")):
+					var style_path_err = McpPathValidator.loadable_error(value, "stylebox")
+					if style_path_err != null:


+## Load a Texture2D from a res:// path. Returns null on failure (including a
+## path that fails res:// confinement / traversal validation).
 static func load_texture(path: String) -> Texture2D:
-	if path.is_empty():
+	if not McpPathValidator.validate_loadable_path(path).is_empty():
 		return null


+        f"script_handler.gd should delegate to McpPathValidator at least 4 times "
+        f"(create_script, read_script, patch_script, attach_script, find_symbols); "
        f"found {validator_calls}"
    )


+func test_write_allows_import_sidecar() -> void:
+	## .import sidecars are source-controlled import config; editing them then
+	## reimporting is a legitimate, recoverable workflow, so writes are allowed.
+	assert_eq(McpPathValidator.validate_resource_path("res://icon.svg.import", true), "")


- Guard the null-byte check against an empty String.chr(0) sentinel in both validate_resource_path and validate_loadable_path. On builds that normalize embedded nulls away (e.g. 4.3), contains("") would be true and reject every path; a String that can't hold a null can't smuggle one, so skip the check. - Update stale "res:// path" comments in ui_handler (stylebox override) and material_values.load_texture — both now accept uid:// / user:// via validate_loadable_path. - Tighten test_path_traversal_guard: assert attach_script is present and require >=5 McpPathValidator delegations in script_handler, so a regression where attach_script stops validating its path is caught. Live Godot 4.6.3: path_validator/filesystem/script/ui/material/resource/scene/ audio suites all pass; test_path_traversal_guard green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

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

 	if spec.has("theme"):
 		var theme_path: String = str(spec.get("theme", ""))
 		if not theme_path.is_empty():
-			if not theme_path.begins_with("res://"):
+			var theme_path_err = McpPathValidator.loadable_error(theme_path, "theme")
+			if theme_path_err != null:


+				if value is String and (value.begins_with("res://") or value.begins_with("uid://") or value.begins_with("user://")):
+					var style_path_err = McpPathValidator.loadable_error(value, "stylebox")
+					if style_path_err != null:
+						return style_path_err


…501) The 5-entry func list exceeded the 100-char line limit after adding attach_script. Pure formatting; the assertion is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… support Copilot re-review: now that the theme / stylebox / shader load paths accept uid:// and user:// via loadable_error, the surrounding comments, the build_layout docstring, the stylebox fallback error ("expects a res:// path"), and the shader_path missing-arg error still said res:// only. Text-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

dsarno force-pushed the harden/unify-path-validation branch from 11ea117 to 51b29ef Compare June 9, 2026 23:56

dsarno force-pushed the harden/unify-path-validation branch from 51b29ef to a235656 Compare June 10, 2026 00:07

dsarno requested a review from Copilot June 10, 2026 05:57

Copilot started reviewing on behalf of dsarno June 10, 2026 05:57 View session

Copilot AI reviewed Jun 10, 2026

View reviewed changes

dsarno requested a review from Copilot June 10, 2026 07:50

Copilot started reviewing on behalf of dsarno June 10, 2026 07:50 View session

Copilot AI reviewed Jun 10, 2026

View reviewed changes

dsarno and others added 2 commits June 10, 2026 01:00

style: wrap long func-name tuple in test_path_traversal_guard (ruff E…

1242f3c

…501) The 5-entry func list exceeded the 100-char line limit after adding attach_script. Pure formatting; the assertion is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

dsarno merged commit cff4c3f into main Jun 10, 2026
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

harden(handlers): route every path-taking handler through McpPathValidator#546

harden(handlers): route every path-taking handler through McpPathValidator#546
dsarno merged 5 commits into
mainfrom
harden/unify-path-validation

dsarno commented Jun 9, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jun 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if path.contains(String.chr(0)):
		return "Path must not contain null bytes"

Conversation

dsarno commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validator (utils/path_validator.gd)

Handlers

Error-code contract change

Tests (live Godot 4.6.3, all suites 0 failures)

Uh oh!

codecov Bot commented Jun 10, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dsarno commented Jun 9, 2026 •

edited

Loading

Validator (`utils/path_validator.gd`)