diff --git a/plugin/addons/godot_ai/handlers/audio_handler.gd b/plugin/addons/godot_ai/handlers/audio_handler.gd index 387a7ae8..1e3428fd 100644 --- a/plugin/addons/godot_ai/handlers/audio_handler.gd +++ b/plugin/addons/godot_ai/handlers/audio_handler.gd @@ -101,6 +101,10 @@ func set_stream(params: Dictionary) -> Dictionary: if stream_path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: stream_path") + var stream_path_err = McpPathValidator.loadable_error(stream_path, "stream_path") + if stream_path_err != null: + return stream_path_err + var resolved := _resolve_player(player_path) if resolved.has("error"): return resolved @@ -259,8 +263,9 @@ func list_streams(params: Dictionary) -> Dictionary: var root: String = params.get("root", "res://") var include_duration: bool = bool(params.get("include_duration", true)) - if not root.begins_with("res://"): - return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "root must start with res://") + var root_err = McpPathValidator.path_error(root, "root") + if root_err != null: + return root_err var efs := EditorInterface.get_resource_filesystem() if efs == null: diff --git a/plugin/addons/godot_ai/handlers/autoload_handler.gd b/plugin/addons/godot_ai/handlers/autoload_handler.gd index 4469ba81..22e77645 100644 --- a/plugin/addons/godot_ai/handlers/autoload_handler.gd +++ b/plugin/addons/godot_ai/handlers/autoload_handler.gd @@ -33,8 +33,9 @@ func add_autoload(params: Dictionary) -> Dictionary: return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: name") if path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: path") - if not path.begins_with("res://"): - return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "Path must start with res:// (got: %s)" % path) + var path_err = McpPathValidator.path_error(path, "path") + if path_err != null: + return path_err if not FileAccess.file_exists(path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "File not found: %s" % path) diff --git a/plugin/addons/godot_ai/handlers/curve_handler.gd b/plugin/addons/godot_ai/handlers/curve_handler.gd index 64e4ea8b..b4b71b81 100644 --- a/plugin/addons/godot_ai/handlers/curve_handler.gd +++ b/plugin/addons/godot_ai/handlers/curve_handler.gd @@ -38,6 +38,9 @@ func set_points(params: Dictionary) -> Dictionary: var node: Node = null var curve_created := false if has_file_target: + var rpath_err = McpPathValidator.loadable_error(resource_path, "resource_path") + if rpath_err != null: + return rpath_err if not ResourceLoader.exists(resource_path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Resource not found: %s" % resource_path) # ResourceLoader.load() returns Godot's cached Resource. Duplicate diff --git a/plugin/addons/godot_ai/handlers/filesystem_handler.gd b/plugin/addons/godot_ai/handlers/filesystem_handler.gd index 0c366252..483798b2 100644 --- a/plugin/addons/godot_ai/handlers/filesystem_handler.gd +++ b/plugin/addons/godot_ai/handlers/filesystem_handler.gd @@ -9,9 +9,9 @@ const ErrorCodes := preload("res://addons/godot_ai/utils/error_codes.gd") func read_file(params: Dictionary) -> Dictionary: var path: String = params.get("path", "") - var path_err := McpPathValidator.validate_resource_path(path) - if not path_err.is_empty(): - return ErrorCodes.make(ErrorCodes.INVALID_PARAMS, path_err) + var path_err = McpPathValidator.path_error(path, "path") + if path_err != null: + return path_err if not FileAccess.file_exists(path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "File not found: %s" % path) @@ -37,9 +37,9 @@ func write_file(params: Dictionary) -> Dictionary: var path: String = params.get("path", "") var content: String = params.get("content", "") - var path_err := McpPathValidator.validate_resource_path(path) - if not path_err.is_empty(): - return ErrorCodes.make(ErrorCodes.INVALID_PARAMS, path_err) + var path_err = McpPathValidator.path_error(path, "path", true) + if path_err != null: + return path_err # Ensure parent directory exists var dir_path := path.get_base_dir() diff --git a/plugin/addons/godot_ai/handlers/material_handler.gd b/plugin/addons/godot_ai/handlers/material_handler.gd index 9e761dfd..3086f09e 100644 --- a/plugin/addons/godot_ai/handlers/material_handler.gd +++ b/plugin/addons/godot_ai/handlers/material_handler.gd @@ -40,7 +40,7 @@ func create_material(params: Dictionary) -> Dictionary: var shader_path: String = params.get("shader_path", "") var overwrite: bool = params.get("overwrite", false) - var err := _validate_material_path(path, "path") + var err := _validate_material_path(path, "path", true) if err != null: return err @@ -65,8 +65,11 @@ func create_material(params: Dictionary) -> Dictionary: if shader_path.is_empty(): return ErrorCodes.make( ErrorCodes.INVALID_PARAMS, - "ShaderMaterial requires shader_path (res:// path to a .gdshader)" + "ShaderMaterial requires shader_path (res:// / uid:// / user:// path to a .gdshader)" ) + var shader_path_err = McpPathValidator.loadable_error(shader_path, "shader_path") + if shader_path_err != null: + return shader_path_err if not ResourceLoader.exists(shader_path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Shader not found: %s" % shader_path) var shader_res := ResourceLoader.load(shader_path) @@ -111,7 +114,7 @@ func create_material(params: Dictionary) -> Dictionary: # ============================================================================ func set_param(params: Dictionary) -> Dictionary: - var load_result := _load_material_from_path(params.get("path", "")) + var load_result := _load_material_from_path(params.get("path", ""), true) if load_result.has("error"): return load_result var mat: Material = load_result.material @@ -169,7 +172,7 @@ func set_param(params: Dictionary) -> Dictionary: # ============================================================================ func set_shader_param(params: Dictionary) -> Dictionary: - var load_result := _load_material_from_path(params.get("path", "")) + var load_result := _load_material_from_path(params.get("path", ""), true) if load_result.has("error"): return load_result var mat: Material = load_result.material @@ -297,8 +300,9 @@ func list_materials(params: Dictionary) -> Dictionary: var root: String = params.get("root", "res://") var type_filter: String = params.get("type", "") - if not root.begins_with("res://"): - return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "root must start with res://") + var root_err = McpPathValidator.path_error(root, "root") + if root_err != null: + return root_err var efs := EditorInterface.get_resource_filesystem() if efs == null: @@ -366,6 +370,9 @@ func assign_material(params: Dictionary) -> Dictionary: var mat: Material = null var material_created := false if not resource_path.is_empty(): + var rpath_err = McpPathValidator.loadable_error(resource_path, "resource_path") + if rpath_err != null: + return rpath_err if not ResourceLoader.exists(resource_path): if create_if_missing: # We'd need to create a new file here — refuse; callers should @@ -463,7 +470,7 @@ func apply_to_node(params: Dictionary) -> Dictionary: var save_to: String = params.get("save_to", "") var saved := false if not save_to.is_empty(): - var save_err_validation := _validate_material_path(save_to, "save_to") + var save_err_validation := _validate_material_path(save_to, "save_to", true) if save_err_validation != null: return save_err_validation var dir_path := save_to.get_base_dir() @@ -564,7 +571,7 @@ func apply_preset(params: Dictionary) -> Dictionary: "Material already exists at %s (pass overwrite=true to replace)" % path ) - var path_err := _validate_material_path(path, "path") + var path_err := _validate_material_path(path, "path", true) if path_err != null: return path_err @@ -665,11 +672,12 @@ static func _reverse_type_map() -> Dictionary: return out -static func _validate_material_path(path: String, param_name: String) -> Variant: +static func _validate_material_path(path: String, param_name: String, for_write: bool = false) -> Variant: if path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: %s" % param_name) - if not path.begins_with("res://"): - return ErrorCodes.make(ErrorCodes.INVALID_PARAMS, "%s must start with res:// (got %s)" % [param_name, path]) + var path_err := McpPathValidator.validate_resource_path(path, for_write) + if not path_err.is_empty(): + return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "%s: %s" % [param_name, path_err]) var has_suffix := false for s in _SUPPORTED_SUFFIXES: if path.ends_with(s): @@ -683,8 +691,8 @@ static func _validate_material_path(path: String, param_name: String) -> Variant return null -func _load_material_from_path(path: String) -> Dictionary: - var err := _validate_material_path(path, "path") +func _load_material_from_path(path: String, for_write: bool = false) -> Dictionary: + var err := _validate_material_path(path, "path", for_write) if err != null: return err if not ResourceLoader.exists(path): diff --git a/plugin/addons/godot_ai/handlers/material_values.gd b/plugin/addons/godot_ai/handlers/material_values.gd index cdb99a80..56a20402 100644 --- a/plugin/addons/godot_ai/handlers/material_values.gd +++ b/plugin/addons/godot_ai/handlers/material_values.gd @@ -153,9 +153,10 @@ static func parse_gradient(value: Variant) -> Variant: return grad -## Load a Texture2D from a res:// path. Returns null on failure. +## Load a Texture2D from a res:// / uid:// / user:// path (validate_loadable_path). +## Returns null on failure (including a path that fails confinement / traversal). static func load_texture(path: String) -> Texture2D: - if path.is_empty(): + if not McpPathValidator.validate_loadable_path(path).is_empty(): return null if not ResourceLoader.exists(path): return null diff --git a/plugin/addons/godot_ai/handlers/node_handler.gd b/plugin/addons/godot_ai/handlers/node_handler.gd index c462c641..eda5c131 100644 --- a/plugin/addons/godot_ai/handlers/node_handler.gd +++ b/plugin/addons/godot_ai/handlers/node_handler.gd @@ -39,8 +39,9 @@ func create_node(params: Dictionary) -> Dictionary: # scene instance (foldout icon, the .tscn stores a reference instead of # an exploded subtree). Descendants remain owned by their sub-scene; # setting their owner to our scene_root would break the instance link. - if not scene_path.begins_with("res://"): - return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "scene_path must start with res://") + var scene_path_err = McpPathValidator.loadable_error(scene_path, "scene_path") + if scene_path_err != null: + return scene_path_err if not ResourceLoader.exists(scene_path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Scene not found: %s" % scene_path) var packed_scene = ResourceLoader.load(scene_path) @@ -222,6 +223,9 @@ func set_property(params: Dictionary) -> Dictionary: if value == "": value = null else: + var value_path_err = McpPathValidator.loadable_error(value, "value") + if value_path_err != null: + return value_path_err if not ResourceLoader.exists(value): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Resource not found: %s" % value) var loaded := ResourceLoader.load(value) diff --git a/plugin/addons/godot_ai/handlers/particle_handler.gd b/plugin/addons/godot_ai/handlers/particle_handler.gd index 18414374..323a6243 100644 --- a/plugin/addons/godot_ai/handlers/particle_handler.gd +++ b/plugin/addons/godot_ai/handlers/particle_handler.gd @@ -341,6 +341,9 @@ func _set_draw_pass_gpu_3d(node: GPUParticles3D, node_path: String, pass_idx: in if int(node.draw_passes) >= pass_idx: existing_mesh = node.get(property_name) as Mesh if not mesh_path.is_empty(): + var mesh_path_err = McpPathValidator.loadable_error(mesh_path, "mesh_path") + if mesh_path_err != null: + return mesh_path_err if not ResourceLoader.exists(mesh_path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Mesh not found: %s" % mesh_path) var loaded := ResourceLoader.load(mesh_path) @@ -357,6 +360,9 @@ func _set_draw_pass_gpu_3d(node: GPUParticles3D, node_path: String, pass_idx: in var material: Material = null if not material_path.is_empty(): + var material_path_err = McpPathValidator.loadable_error(material_path, "material_path") + if material_path_err != null: + return material_path_err if not ResourceLoader.exists(material_path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Material not found: %s" % material_path) var loaded_mat := ResourceLoader.load(material_path) @@ -407,6 +413,9 @@ func _set_draw_pass_cpu_3d(node: CPUParticles3D, node_path: String, mesh_path: S var mesh: Mesh = node.mesh var old_mesh: Mesh = mesh if not mesh_path.is_empty(): + var mesh_path_err = McpPathValidator.loadable_error(mesh_path, "mesh_path") + if mesh_path_err != null: + return mesh_path_err if not ResourceLoader.exists(mesh_path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Mesh not found: %s" % mesh_path) var loaded := ResourceLoader.load(mesh_path) @@ -417,6 +426,9 @@ func _set_draw_pass_cpu_3d(node: CPUParticles3D, node_path: String, mesh_path: S var material: Material = null var old_material: Material = node.material_override if not material_path.is_empty(): + var material_path_err = McpPathValidator.loadable_error(material_path, "material_path") + if material_path_err != null: + return material_path_err if not ResourceLoader.exists(material_path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Material not found: %s" % material_path) var loaded_mat := ResourceLoader.load(material_path) @@ -447,6 +459,9 @@ func _set_draw_pass_cpu_3d(node: CPUParticles3D, node_path: String, mesh_path: S func _set_draw_pass_2d(node: Node, node_path: String, texture_path: String) -> Dictionary: if texture_path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "2D particles require texture param") + var texture_path_err = McpPathValidator.loadable_error(texture_path, "texture_path") + if texture_path_err != null: + return texture_path_err if not ResourceLoader.exists(texture_path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Texture not found: %s" % texture_path) var tex := ResourceLoader.load(texture_path) diff --git a/plugin/addons/godot_ai/handlers/resource_handler.gd b/plugin/addons/godot_ai/handlers/resource_handler.gd index 65489f72..eb3b5f2b 100644 --- a/plugin/addons/godot_ai/handlers/resource_handler.gd +++ b/plugin/addons/godot_ai/handlers/resource_handler.gd @@ -64,8 +64,9 @@ func load_resource(params: Dictionary) -> Dictionary: if path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: path") - if not path.begins_with("res://"): - return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "Path must start with res://") + var path_err = McpPathValidator.loadable_error(path, "path") + if path_err != null: + return path_err if not ResourceLoader.exists(path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Resource not found: %s" % path) @@ -112,6 +113,10 @@ func assign_resource(params: Dictionary) -> Dictionary: if resource_path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: resource_path") + var rpath_err = McpPathValidator.loadable_error(resource_path, "resource_path") + if rpath_err != null: + return rpath_err + var _resolved := McpNodeValidator.resolve_or_error(node_path, "node_path") if _resolved.has("error"): return _resolved @@ -248,6 +253,9 @@ static func _apply_resource_properties(res: Resource, properties: Dictionary) -> if v == "": v = null else: + var vpath_err = McpPathValidator.loadable_error(v, "property '%s'" % key) + if vpath_err != null: + return vpath_err var loaded := ResourceLoader.load(v) if loaded == null: return ErrorCodes.make( diff --git a/plugin/addons/godot_ai/handlers/scene_handler.gd b/plugin/addons/godot_ai/handlers/scene_handler.gd index 35804676..f2514bcc 100644 --- a/plugin/addons/godot_ai/handlers/scene_handler.gd +++ b/plugin/addons/godot_ai/handlers/scene_handler.gd @@ -90,8 +90,9 @@ func create_scene(params: Dictionary) -> Dictionary: if path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: path") - if not path.begins_with("res://"): - return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "Path must start with res://") + var path_err = McpPathValidator.path_error(path, "path", true) + if path_err != null: + return path_err if not path.ends_with(".tscn") and not path.ends_with(".scn"): path += ".tscn" @@ -148,6 +149,10 @@ func open_scene(params: Dictionary) -> Dictionary: if path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: path") + var path_err = McpPathValidator.loadable_error(path, "path") + if path_err != null: + return path_err + if not ResourceLoader.exists(path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Scene not found: %s" % path) @@ -202,8 +207,9 @@ func save_scene_as(params: Dictionary) -> Dictionary: if path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: path") - if not path.begins_with("res://"): - return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "Path must start with res://") + var path_err = McpPathValidator.path_error(path, "path", true) + if path_err != null: + return path_err if not path.ends_with(".tscn") and not path.ends_with(".scn"): path += ".tscn" diff --git a/plugin/addons/godot_ai/handlers/script_handler.gd b/plugin/addons/godot_ai/handlers/script_handler.gd index 956b5b5c..1a4aaece 100644 --- a/plugin/addons/godot_ai/handlers/script_handler.gd +++ b/plugin/addons/godot_ai/handlers/script_handler.gd @@ -27,9 +27,9 @@ func create_script(params: Dictionary) -> Dictionary: var path: String = params.get("path", "") var content: String = params.get("content", "") - var path_err := McpPathValidator.validate_resource_path(path) - if not path_err.is_empty(): - return ErrorCodes.make(ErrorCodes.INVALID_PARAMS, path_err) + var path_err = McpPathValidator.path_error(path, "path", true) + if path_err != null: + return path_err if not path.ends_with(".gd"): return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "Path must end with .gd") @@ -141,9 +141,9 @@ static func _finish_create_script_deferred( func read_script(params: Dictionary) -> Dictionary: var path: String = params.get("path", "") - var path_err := McpPathValidator.validate_resource_path(path) - if not path_err.is_empty(): - return ErrorCodes.make(ErrorCodes.INVALID_PARAMS, path_err) + var path_err = McpPathValidator.path_error(path, "path") + if path_err != null: + return path_err if not FileAccess.file_exists(path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "File not found: %s" % path) @@ -171,9 +171,9 @@ func patch_script(params: Dictionary) -> Dictionary: var new_text: String = params.get("new_text", "") var replace_all: bool = params.get("replace_all", false) - var path_err := McpPathValidator.validate_resource_path(path) - if not path_err.is_empty(): - return ErrorCodes.make(ErrorCodes.INVALID_PARAMS, path_err) + var path_err = McpPathValidator.path_error(path, "path", true) + if path_err != null: + return path_err if not "old_text" in params: return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: old_text") if not "new_text" in params: @@ -241,6 +241,10 @@ func attach_script(params: Dictionary) -> Dictionary: if script_path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: script_path") + var spath_err = McpPathValidator.loadable_error(script_path, "script_path") + if spath_err != null: + return spath_err + var _resolved := McpNodeValidator.resolve_or_error(node_path, "node_path") if _resolved.has("error"): return _resolved @@ -304,9 +308,9 @@ func detach_script(params: Dictionary) -> Dictionary: func find_symbols(params: Dictionary) -> Dictionary: var path: String = params.get("path", "") - var path_err := McpPathValidator.validate_resource_path(path) - if not path_err.is_empty(): - return ErrorCodes.make(ErrorCodes.INVALID_PARAMS, path_err) + var path_err = McpPathValidator.path_error(path, "path") + if path_err != null: + return path_err if not FileAccess.file_exists(path): return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "File not found: %s" % path) diff --git a/plugin/addons/godot_ai/handlers/theme_handler.gd b/plugin/addons/godot_ai/handlers/theme_handler.gd index 16913786..979fe0cf 100644 --- a/plugin/addons/godot_ai/handlers/theme_handler.gd +++ b/plugin/addons/godot_ai/handlers/theme_handler.gd @@ -28,7 +28,7 @@ func create_theme(params: Dictionary) -> Dictionary: var path: String = params.get("path", "") var overwrite: bool = params.get("overwrite", false) - var err := _validate_res_path(path, ".tres", "path") + var err := _validate_res_path(path, ".tres", "path", true) if err != null: return err @@ -430,7 +430,7 @@ func apply_theme(params: Dictionary) -> Dictionary: func _load_theme_from_params(params: Dictionary) -> Dictionary: var theme_path: String = params.get("theme_path", "") - var err := _validate_res_path(theme_path, ".tres") + var err := _validate_res_path(theme_path, ".tres", "theme_path", true) if err != null: return err if not ResourceLoader.exists(theme_path): @@ -441,14 +441,12 @@ func _load_theme_from_params(params: Dictionary) -> Dictionary: return {"theme": theme, "path": theme_path} -static func _validate_res_path(path: String, required_suffix: String, param_name: String = "theme_path") -> Variant: +static func _validate_res_path(path: String, required_suffix: String, param_name: String = "theme_path", for_write: bool = false) -> Variant: if path.is_empty(): return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: %s" % param_name) - if not path.begins_with("res://"): - return ErrorCodes.make( - ErrorCodes.VALUE_OUT_OF_RANGE, - "%s must start with res:// (got %s)" % [param_name, path] - ) + var path_err := McpPathValidator.validate_resource_path(path, for_write) + if not path_err.is_empty(): + return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "%s: %s" % [param_name, path_err]) if not path.ends_with(required_suffix): return ErrorCodes.make( ErrorCodes.VALUE_OUT_OF_RANGE, diff --git a/plugin/addons/godot_ai/handlers/ui_handler.gd b/plugin/addons/godot_ai/handlers/ui_handler.gd index 05d2244f..5d8db9b0 100644 --- a/plugin/addons/godot_ai/handlers/ui_handler.gd +++ b/plugin/addons/godot_ai/handlers/ui_handler.gd @@ -213,7 +213,7 @@ func set_text(params: Dictionary) -> Dictionary: ## Params: ## tree - Dictionary describing the root node. Required fields: "type". ## Optional: "name", "properties" (dict), "anchor_preset", -## "anchor_margin", "theme" (res:// path), "children" (array). +## "anchor_margin", "theme" (res://, uid:// or user:// path), "children" (array). ## parent_path - Parent scene path. Empty or "/" = scene root. ## ## Validation is done before any scene mutation: class names, property @@ -295,13 +295,14 @@ func _build_subtree(spec: Dictionary) -> Dictionary: node.free() return apply_err - # Theme (res:// path -> Resource). + # Theme (res:// / uid:// / user:// path -> Resource). 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: node.free() - return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "theme must be a res:// path") + return theme_path_err if not ResourceLoader.exists(theme_path): node.free() return ErrorCodes.make(ErrorCodes.RESOURCE_NOT_FOUND, "Theme not found: %s" % theme_path) @@ -398,9 +399,12 @@ func _apply_property(node: Node, prop: String, value: Variant) -> Variant: var info: Dictionary = _THEME_OVERRIDE_MAP[prefix] var coerce_type: int = info.coerce_type - # For stylebox overrides, load from a res:// path. + # For stylebox overrides, load from a res:// / uid:// / user:// 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: + return style_path_err var res := ResourceLoader.load(value) if res == null or not res is StyleBox: return ErrorCodes.make( @@ -411,7 +415,7 @@ func _apply_property(node: Node, prop: String, value: Variant) -> Variant: else: return ErrorCodes.make( ErrorCodes.INVALID_PARAMS, - "theme_override_styles/ expects a res:// path to a StyleBox" + "theme_override_styles/ expects a res:// / uid:// / user:// path to a StyleBox" ) else: var coercion := _coerce_for_type(value, coerce_type) diff --git a/plugin/addons/godot_ai/utils/path_validator.gd b/plugin/addons/godot_ai/utils/path_validator.gd index 43e49c4a..afc1564f 100644 --- a/plugin/addons/godot_ai/utils/path_validator.gd +++ b/plugin/addons/godot_ai/utils/path_validator.gd @@ -13,21 +13,34 @@ const ErrorCodes := preload("res://addons/godot_ai/utils/error_codes.gd") ## 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. +## Two entry points: +## * `validate_resource_path` — for paths that name a `res://` disk file the +## plugin will read or (with `for_write`) write. This is the strict one. +## * `validate_loadable_path` — for paths handed to `ResourceLoader`, which +## also accepts `uid://` (an opaque resource-DB id that cannot express +## traversal) and `user://` (the per-project user data sandbox). Load +## handlers must use this so `uid://` references copied out of `.tscn` +## ExtResource / `.uid` sidecars and `user://` runtime assets keep loading. +## +## Error wrapping: callers should use `path_error` / `loadable_error`, which +## return a ready `ErrorCodes.make(VALUE_OUT_OF_RANGE, …)` dict (or null). A +## bad path is a value-domain error, and funneling every site through one +## wrapper keeps the error code consistent across all handlers. +## +## Known limitation: containment is lexical (`globalize_path` + `simplify_path` +## prefix match). It does NOT resolve symlinks — GDScript exposes no realpath. +## A symlink *inside* the project that points outside it can therefore defeat +## the under-root check. This matches the engine's own `res://` resolution and +## is accepted; the loopback trust boundary is the primary control. + + +# Cached project / user roots. `globalize_path` 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 var _cached_user_root: String = "" static func _res_root() -> String: @@ -36,20 +49,123 @@ static func _res_root() -> String: return _cached_res_root +static func _user_root() -> String: + if _cached_user_root.is_empty(): + _cached_user_root = ProjectSettings.globalize_path("user://").simplify_path() + return _cached_user_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 `ErrorCodes.make(INVALID_PARAMS, ...)`. -static func validate_resource_path(path: String) -> String: +## project root. Returns a human-readable error message otherwise. +## Prefer `path_error` over calling this directly — it wraps the message in the +## canonical error code. +## +## Pass `for_write = true` for any handler that creates/overwrites the file +## (write_file, create_script, patch_script, ResourceSaver-backed saves, +## scene saves). Write callers additionally refuse the project manifest and +## startup override, plus the `.godot/` metadata dir. Reads default to +## `for_write = false`, which permits inspecting those files. +static func validate_resource_path(path: String, for_write: bool = false) -> String: if path.is_empty(): return "Missing required param: path" + ## Guard the sentinel: on builds where String.chr(0) yields "" (some engines + ## 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. + var nul := String.chr(0) + if not nul.is_empty() and path.contains(nul): + return "Path must not contain null bytes" if not path.begins_with("res://"): return "Path must start with res://" + var confine_err := _confine_under(path, _res_root(), "res://") + if not confine_err.is_empty(): + return confine_err + if for_write: + return _reject_sensitive_write(path) + return "" + + +## Returns "" when `path` is safe to hand to `ResourceLoader.load` / `.exists`. +## Accepts, in addition to confined `res://` paths: +## * `uid://` — an opaque 64-bit resource id; it cannot express a path +## and the engine only ever resolves it to a resource already in the +## project, so there is nothing to confine. +## * `user://…` — the per-project user data dir, confined under its root the +## same way `res://` is (so `user://../…` can't escape the sandbox). +static func validate_loadable_path(path: String) -> String: + if path.is_empty(): + return "Missing required param: path" + ## Guard the sentinel: on builds where String.chr(0) yields "" (some engines + ## 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. + var nul := String.chr(0) + if not nul.is_empty() and path.contains(nul): + return "Path must not contain null bytes" + if path.begins_with("uid://"): + return "" + if path.begins_with("user://"): + return _confine_under(path, _user_root(), "user://") + if path.begins_with("res://"): + return _confine_under(path, _res_root(), "res://") + return "Path must start with res://, uid://, or user://" + + +## Shared traversal + under-root containment. `root` must already be simplified. +static func _confine_under(path: String, root: String, label: String) -> String: 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" + # Append a separator so `/proj_evil/...` can't pretend to be inside `/proj` + # via prefix match. `globalized == root` covers the bare `res://` / `user://`. + if globalized != root and not globalized.begins_with(root + "/"): + return "Path must resolve under %s root" % label + return "" + + +## Refuse writes that would clobber project-critical files. The path is already +## confirmed `res://`-rooted and traversal-free by the caller. +## +## Comparisons are case-folded: macOS (APFS) and Windows (NTFS) are +## case-insensitive by default, so `res://Project.godot` resolves to the real +## `project.godot` and must be refused too. +## +## `.import` sidecars are deliberately NOT blocked — editing an asset's import +## options then re-importing is a legitimate, recoverable workflow (the file is +## source-controlled). The blocked set is the startup-execution surface only: +## the manifest, its `override.cfg` shadow, and the `.godot/` cache dir. +static func _reject_sensitive_write(path: String) -> String: + var file_lower := path.get_file().to_lower() + if file_lower == "project.godot": + return "Refusing to write res://project.godot (project manifest)" + if file_lower == "override.cfg": + return "Refusing to write res://override.cfg (startup config override)" + # Reject the `.godot/` editor-metadata dir at any depth. Split drops empty + # segments so a trailing slash can't hide a segment from the check. + for segment in path.trim_prefix("res://").split("/", false): + if segment.to_lower() == ".godot": + return "Refusing to write under res://.godot/ (editor metadata)" return "" + + +## Validate a write/read `res://` path and return a ready error dict, or null +## when the path is fine. The single wrapper every handler should use so the +## error code (VALUE_OUT_OF_RANGE — a bad path is a value-domain error) stays +## consistent. `param_name` is prefixed onto the message for context. +static func path_error(path: String, param_name: String = "path", for_write: bool = false) -> Variant: + if path.is_empty(): + return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: %s" % param_name) + var err := validate_resource_path(path, for_write) + if err.is_empty(): + return null + return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "%s: %s" % [param_name, err]) + + +## Same as `path_error` but for paths handed to `ResourceLoader` (allows +## `uid://` / `user://`). Returns a ready error dict or null. An empty path is +## reported as MISSING_REQUIRED_PARAM rather than a value error. +static func loadable_error(path: String, param_name: String = "path") -> Variant: + if path.is_empty(): + return ErrorCodes.make(ErrorCodes.MISSING_REQUIRED_PARAM, "Missing required param: %s" % param_name) + var err := validate_loadable_path(path) + if err.is_empty(): + return null + return ErrorCodes.make(ErrorCodes.VALUE_OUT_OF_RANGE, "%s: %s" % [param_name, err]) diff --git a/plugin/addons/godot_ai/utils/resource_io.gd b/plugin/addons/godot_ai/utils/resource_io.gd index cc4762cb..00263c4d 100644 --- a/plugin/addons/godot_ai/utils/resource_io.gd +++ b/plugin/addons/godot_ai/utils/resource_io.gd @@ -71,8 +71,9 @@ static func save_to_disk( extra_fields: Dictionary = {}, pause_target: McpConnection = null, ) -> Dictionary: - if not resource_path.begins_with("res://"): - return ErrorCodes.make(ErrorCodes.INVALID_PARAMS, "resource_path must start with res://") + var path_err = McpPathValidator.path_error(resource_path, "resource_path", true) + if path_err != null: + return path_err var existed_before := FileAccess.file_exists(resource_path) if existed_before and not overwrite: diff --git a/test_project/tests/test_audio.gd b/test_project/tests/test_audio.gd index 1e7a7a44..e71ba386 100644 --- a/test_project/tests/test_audio.gd +++ b/test_project/tests/test_audio.gd @@ -9,9 +9,9 @@ const AudioHandler := preload("res://addons/godot_ai/handlers/audio_handler.gd") ## stream assignment, playback properties, editor-preview play/stop, and ## audio asset listing. ## -## A silent AudioStreamWAV fixture is generated at runtime (written to -## user://) rather than committing a binary file. Cleaned up by -## suite_teardown. +## A silent AudioStreamWAV fixture is generated at runtime (written to a +## user:// .tres path, outside the project tree) rather than committing a +## binary file. Cleaned up by suite_teardown. ## ## NOTE: GDScript tests must not call save_scene, scene_create, scene_open, ## quit_editor, or reload_plugin (see CLAUDE.md Known Issues). @@ -41,10 +41,9 @@ func suite_teardown() -> void: _fixture_path = "" -# Build a tiny silent 16-bit mono WAV stream and save it to user://. -# user:// paths are valid res:// surrogates for ResourceLoader but live -# outside the imported asset pipeline, so they don't pollute the project -# or require a reimport cycle. +# Build a tiny silent 16-bit mono WAV stream saved to a user:// path. user:// +# is outside the project tree (no repo pollution, no EditorFileSystem index to +# keep in sync), and set_stream accepts it via validate_loadable_path. func _make_fixture() -> String: var stream := AudioStreamWAV.new() stream.format = AudioStreamWAV.FORMAT_16_BITS @@ -55,7 +54,7 @@ func _make_fixture() -> String: silence.resize(4410) silence.fill(0) stream.data = silence - var path := "user://audio_fixture.wav.tres" + var path := "user://test_audio_fixture.wav.tres" var err := ResourceSaver.save(stream, path) if err != OK: return "" diff --git a/test_project/tests/test_filesystem.gd b/test_project/tests/test_filesystem.gd index 69715ccd..db2738fa 100644 --- a/test_project/tests/test_filesystem.gd +++ b/test_project/tests/test_filesystem.gd @@ -48,7 +48,7 @@ func test_read_file_basic() -> void: func test_read_file_missing_path() -> void: var result := _handler.read_file({}) - assert_is_error(result, ErrorCodes.INVALID_PARAMS) + assert_is_error(result, ErrorCodes.MISSING_REQUIRED_PARAM) func test_read_file_invalid_prefix() -> void: @@ -106,7 +106,7 @@ func test_write_file_overwrite_omits_cleanup_hint() -> void: func test_write_file_missing_path() -> void: var result := _handler.write_file({"content": "hello"}) - assert_is_error(result, ErrorCodes.INVALID_PARAMS) + assert_is_error(result, ErrorCodes.MISSING_REQUIRED_PARAM) func test_write_file_invalid_prefix() -> void: diff --git a/test_project/tests/test_path_validator.gd b/test_project/tests/test_path_validator.gd index 7aff39ec..6c4d539c 100644 --- a/test_project/tests/test_path_validator.gd +++ b/test_project/tests/test_path_validator.gd @@ -94,3 +94,105 @@ func test_well_formed_nested_path_passes_boundary_check() -> void: ## 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") + + +# ----- null byte (truncation trap, audit GH-4) ----- + +func test_rejects_embedded_null_byte() -> void: + ## A NUL can truncate a C string, so the path written could differ from the + ## one validated/reported. Reject any path containing one. + ## + ## Some Godot builds (e.g. 4.3) strip embedded nulls from String, so the + ## payload can't be constructed there — the validator's check is simply a + ## harmless no-op on those builds (a String that can't hold a null can't + ## smuggle one past the guard). Skip rather than assert 4.6-only behavior. + var nul := String.chr(0) + if nul.is_empty() or not ("res://a" + nul + "b").contains(nul): + skip("this Godot build does not retain embedded null bytes in String") + return + # No "..": the ONLY reason to reject this path is the embedded null. + var err := McpPathValidator.validate_resource_path("res://safe" + nul + "name.gd") + assert_false(err.is_empty(), "path with an embedded null byte must be rejected") + assert_contains(err, "null") + + +# ----- write blocklist: project-critical files (audit GH-3) ----- +# +# These pass every structural check (res://-rooted, no traversal, under root) +# but overwriting them corrupts the project. Blocked for writes, allowed for +# reads (inspecting config is legitimate). + +func test_write_rejects_project_godot() -> void: + var err := McpPathValidator.validate_resource_path("res://project.godot", true) + assert_false(err.is_empty(), "writing res://project.godot must be rejected") + assert_contains(err, "project.godot") + + +func test_write_rejects_godot_metadata_dir() -> void: + var err := McpPathValidator.validate_resource_path("res://.godot/uid_cache.bin", true) + assert_false(err.is_empty(), "writing under res://.godot/ must be rejected") + assert_contains(err, ".godot") + + +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), "") + + +func test_write_allows_normal_resource_path() -> void: + ## The blocklist must not catch ordinary writes. + assert_eq(McpPathValidator.validate_resource_path("res://scenes/level.tscn", true), "") + assert_eq(McpPathValidator.validate_resource_path("res://data/config.json", true), "") + + +func test_read_allows_project_critical_files() -> void: + ## for_write defaults to false — reading project config / import data is + ## legitimate and must not be blocked. + assert_eq(McpPathValidator.validate_resource_path("res://project.godot"), "") + assert_eq(McpPathValidator.validate_resource_path("res://.godot/uid_cache.bin"), "") + assert_eq(McpPathValidator.validate_resource_path("res://icon.svg.import"), "") + + +func test_write_still_rejects_traversal() -> void: + ## The structural traversal check fires regardless of for_write. + assert_false(McpPathValidator.validate_resource_path("res://../etc/passwd", true).is_empty()) + + +func test_write_rejects_override_cfg() -> void: + ## override.cfg is applied over project.godot at startup — same takeover + ## surface as the manifest, so writes must be refused too. + var err := McpPathValidator.validate_resource_path("res://override.cfg", true) + assert_false(err.is_empty(), "writing res://override.cfg must be rejected") + assert_contains(err, "override.cfg") + + +func test_write_blocklist_is_case_insensitive() -> void: + ## macOS/Windows default filesystems are case-insensitive, so a case-variant + ## spelling resolves to the same protected file and must be refused. + assert_false(McpPathValidator.validate_resource_path("res://Project.godot", true).is_empty()) + assert_false(McpPathValidator.validate_resource_path("res://.GODOT/uid_cache.bin", true).is_empty()) + + +func test_loadable_accepts_uid() -> void: + ## uid:// is an opaque resource id ResourceLoader resolves to an in-project + ## resource — it cannot express traversal, so load handlers must accept it. + assert_eq(McpPathValidator.validate_loadable_path("uid://b8x3k7q2vn1ya"), "") + + +func test_loadable_accepts_user() -> void: + ## user:// runtime assets were always loadable and must remain so. + assert_eq(McpPathValidator.validate_loadable_path("user://recording.wav.tres"), "") + + +func test_loadable_rejects_user_traversal() -> void: + ## ...but a user:// path still can't escape the user data sandbox. + assert_false(McpPathValidator.validate_loadable_path("user://../../etc/passwd").is_empty()) + + +func test_loadable_still_rejects_res_traversal() -> void: + assert_false(McpPathValidator.validate_loadable_path("res://../evil.gd").is_empty()) + + +func test_loadable_rejects_unknown_scheme() -> void: + assert_false(McpPathValidator.validate_loadable_path("/etc/passwd").is_empty()) diff --git a/test_project/tests/test_scene.gd b/test_project/tests/test_scene.gd index b786b616..a50c8e70 100644 --- a/test_project/tests/test_scene.gd +++ b/test_project/tests/test_scene.gd @@ -171,6 +171,19 @@ func test_create_scene_invalid_path_prefix() -> void: assert_is_error(result, ErrorCodes.VALUE_OUT_OF_RANGE) +func test_create_scene_rejects_traversal() -> void: + ## create_scene now routes through McpPathValidator — a traversal payload + ## that escapes the project root must be rejected (audit GH-1). + var result := _handler.create_scene({"path": "res://../evil.tscn"}) + assert_is_error(result, ErrorCodes.VALUE_OUT_OF_RANGE) + + +func test_create_scene_rejects_project_godot_overwrite() -> void: + ## Write blocklist (audit GH-3): refuse clobbering the project manifest. + var result := _handler.create_scene({"path": "res://project.godot"}) + assert_is_error(result, ErrorCodes.VALUE_OUT_OF_RANGE) + + # ----- open_scene (validation only — opening scenes triggers UI that blocks test runner) ----- func test_open_scene_missing_path() -> void: diff --git a/test_project/tests/test_script.gd b/test_project/tests/test_script.gd index 0d921c6a..863e76d1 100644 --- a/test_project/tests/test_script.gd +++ b/test_project/tests/test_script.gd @@ -120,7 +120,7 @@ func test_create_script_overwrite_omits_cleanup_hint() -> void: func test_create_script_missing_path() -> void: var result := _handler.create_script({"content": "extends Node\n"}) - assert_is_error(result, ErrorCodes.INVALID_PARAMS) + assert_is_error(result, ErrorCodes.MISSING_REQUIRED_PARAM) func test_create_script_invalid_prefix() -> void: @@ -290,7 +290,7 @@ func test_read_script_basic() -> void: func test_read_script_missing_path() -> void: var result := _handler.read_script({}) - assert_is_error(result, ErrorCodes.INVALID_PARAMS) + assert_is_error(result, ErrorCodes.MISSING_REQUIRED_PARAM) func test_read_script_invalid_prefix() -> void: @@ -432,7 +432,7 @@ func test_find_symbols_exports() -> void: func test_find_symbols_missing_path() -> void: var result := _handler.find_symbols({}) - assert_is_error(result, ErrorCodes.INVALID_PARAMS) + assert_is_error(result, ErrorCodes.MISSING_REQUIRED_PARAM) func test_find_symbols_invalid_prefix() -> void: diff --git a/tests/unit/test_path_traversal_guard.py b/tests/unit/test_path_traversal_guard.py index 1c3a7bc5..b05bcd10 100644 --- a/tests/unit/test_path_traversal_guard.py +++ b/tests/unit/test_path_traversal_guard.py @@ -79,15 +79,24 @@ def test_script_handler_uses_path_validator_at_every_entry_point() -> None: 'script_handler.gd must not contain bare `begins_with("res://")` ' "checks — replace with McpPathValidator.validate_resource_path." ) - # Each listed entry point (issue #347) calls the validator. - for func_name in ("create_script", "read_script", "patch_script", "find_symbols"): + # Each listed entry point (issue #347 + attach_script) calls the validator. + # attach_script is included so a regression where it stops validating its + # path is caught (Copilot review on #546). + for func_name in ( + "create_script", + "read_script", + "patch_script", + "attach_script", + "find_symbols", + ): assert f"func {func_name}" in source, f"{func_name} missing from script_handler" - # The validator helper is referenced — surface area covers all four entry - # points; counting calls catches a partial revert. - validator_calls = source.count("McpPathValidator.validate_resource_path") - assert validator_calls >= 4, ( - f"script_handler.gd should call McpPathValidator.validate_resource_path " - f"at least 4 times (create_script, read_script, patch_script, find_symbols); " + # Handlers delegate via path_error / loadable_error / validate_resource_path + # — count any McpPathValidator. reference so the wrapper refactor still pins + # that every one of the five entry points goes through the validator. + validator_calls = source.count("McpPathValidator.") + assert validator_calls >= 5, ( + f"script_handler.gd should delegate to McpPathValidator at least 5 times " + f"(create_script, read_script, patch_script, attach_script, find_symbols); " f"found {validator_calls}" ) @@ -100,8 +109,8 @@ def test_filesystem_handler_uses_path_validator_at_every_entry_point() -> None: ) for func_name in ("read_file", "write_file", "reimport"): assert f"func {func_name}" in source - validator_calls = source.count("McpPathValidator.validate_resource_path") + validator_calls = source.count("McpPathValidator.") assert validator_calls >= 3, ( - f"filesystem_handler.gd should call McpPathValidator.validate_resource_path " - f"at least 3 times (read_file, write_file, reimport); found {validator_calls}" + f"filesystem_handler.gd should delegate to McpPathValidator at least 3 " + f"times (read_file, write_file, reimport); found {validator_calls}" )