fix(file tools) implement directory restrictions to file tools by jholt96 · Pull Request #1496 · kagent-dev/kagent

jholt96 · 2026-03-12T14:52:15Z

This limits the file tools to either the current working directory or the skills repo

write_file & edit_file - restricted to just working directory

read_file - restricted to working directory & skills directory

implements: #1495

Copilot

Pull request overview

This PR addresses #1495 by restricting filesystem access in the file operation tools so they can only operate within the session working directory (and, for reads, the skills directory), reducing the risk of unintended host filesystem access and secret leakage.

Changes:

Added centralized path validation to block path traversal / access outside configured allowed root(s).
Updated OpenAI and ADK tool wrappers to pass allowed roots into the underlying file operations.
Added unit tests covering allowed/blocked read/write/edit behavior, including traversal attempts.

Reviewed changes

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

File	Description
`python/packages/kagent-skills/src/kagent/skills/shell.py`	Adds `_validate_path` and threads `allowed_root` through file helpers to enforce directory restrictions.
`python/packages/kagent-openai/src/kagent/openai/tools/_tools.py`	Applies directory restrictions for OpenAI tool wrappers by passing `allowed_root` to file helpers.
`python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py`	Applies directory restrictions for ADK tool wrappers by passing `allowed_root` to file helpers.
`python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py`	Adds unit tests to validate traversal is blocked and in-root access works.

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

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-12T14:57:48Z

python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py

+    with pytest.raises(PermissionError, match="outside the allowed director"):
+        write_file_content(outside_path, "malicious", allowed_root=tmp_path)
+    assert not outside_path.exists()


The match pattern uses the misspelling "director"; consider using a correctly-spelled pattern that also matches both singular/plural (e.g., directory/directories) to keep the assertion clear and robust.

Copilot · 2026-03-12T14:57:49Z

python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py

+        with pytest.raises(PermissionError, match="outside the allowed director"):
+            edit_file_content(outside_file, "original", "hacked", allowed_root=tmp_path)
+        # File must not have been modified
+        assert outside_file.read_text() == "original"


The match pattern uses the misspelling "director"; consider using a correctly-spelled pattern that also matches both singular/plural (e.g., directory/directories) to keep the assertion clear and robust.

Copilot · 2026-03-12T14:57:49Z

python/packages/kagent-openai/src/kagent/openai/tools/_tools.py


-        return read_file_content(path, offset, limit)
-    except (FileNotFoundError, IsADirectoryError, OSError) as e:
+        return read_file_content(path, offset, limit, allowed_root=[working_dir, Path("/skills")])


read_file hardcodes Path("/skills") as an allowed root. Since this module supports custom skills_directory via get_skill_tool(s), this can unintentionally block reading skill files by absolute path when skills are mounted elsewhere (and the session symlink creation fallback in initialize_session_path mentions accessing skills via absolute path). Consider deriving the allowed skills root from the configured skills_directory (e.g., by closing over it in a factory) rather than assuming /skills.

Suggested change

return read_file_content(path, offset, limit, allowed_root=[working_dir, Path("/skills")])

allowed_roots = [working_dir]

skills_directory = getattr(wrapper.context, "skills_directory", None)

if skills_directory is not None:

allowed_roots.append(Path(skills_directory))

else:

# Fallback to the default skills mount point for backwards compatibility.

allowed_roots.append(Path("/skills"))

return read_file_content(path, offset, limit, allowed_root=allowed_roots)

Copilot · 2026-03-12T14:57:49Z

python/packages/kagent-adk/src/kagent/adk/tools/file_tools.py

            working_dir = get_session_path(session_id=tool_context.session.id)
            path = Path(file_path_str)
            if not path.is_absolute():
                path = working_dir / path
            path = path.resolve()

-            return read_file_content(path, offset, limit)
-        except (FileNotFoundError, IsADirectoryError, IOError) as e:
+            return read_file_content(path, offset, limit, allowed_root=[working_dir, Path("/skills")])
+        except (FileNotFoundError, IsADirectoryError, PermissionError, IOError) as e:
            return f"Error reading file {file_path_str}: {e}"


ReadFileTool hardcodes Path("/skills") as an allowed root. However, ADK supports configuring KAGENT_SKILLS_FOLDER (custom skills directory), and session initialization explicitly notes a fallback where skills may need to be accessed via their absolute path if the symlink cannot be created. Consider using the configured skills directory (or capturing it via plugin/tool initialization) instead of always assuming /skills.

Copilot · 2026-03-12T14:57:50Z

python/packages/kagent-skills/src/kagent/skills/shell.py

+def write_file_content(file_path: Path, content: str, allowed_root: Path | None = None) -> str:
    """Writes content to a file, creating parent directories if needed."""
+    file_path = _validate_path(file_path, allowed_root)
+


write_file_content only type-hints allowed_root as Path | None, but it is passed directly to _validate_path which supports Path | list[Path] | None and read_file_content already exposes list support. Align the signature/type hints (and naming, e.g. allowed_roots) to avoid an inconsistent public API and enable reuse when multiple allowed roots are needed.

Copilot · 2026-03-12T14:57:50Z

python/packages/kagent-skills/src/kagent/skills/shell.py

 def edit_file_content(
    file_path: Path,
    old_string: str,
    new_string: str,
    replace_all: bool = False,
+    allowed_root: Path | None = None,
 ) -> str:
    """Performs an exact string replacement in a file."""
    if old_string == new_string:
        raise ValueError("old_string and new_string must be different")

+    file_path = _validate_path(file_path, allowed_root)
+


edit_file_content only type-hints allowed_root as Path | None, but _validate_path and read_file_content support multiple allowed roots. Consider aligning the signature/type hints (and parameter naming) across the file tools for consistency and to avoid callers working around the mismatch.

python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py

Copilot · 2026-03-12T14:57:51Z

python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py

+        with pytest.raises(PermissionError, match="outside the allowed director"):
+            read_file_content(outside_file, allowed_root=tmp_path)


The match pattern uses the misspelling "director"; consider using a correctly-spelled pattern that also matches both singular/plural (e.g., directory/directories) to keep the assertion clear and robust.

Copilot · 2026-03-12T14:57:51Z

python/packages/kagent-skills/src/kagent/tests/unittests/test_skill_execution.py

+        with pytest.raises(PermissionError, match="outside the allowed director"):
+            read_file_content(
+                tmp_path / "subdir" / "../../secret.txt",
+                allowed_root=tmp_path,
+            )


The match pattern uses the misspelling "director"; consider using a correctly-spelled pattern that also matches both singular/plural (e.g., directory/directories) to keep the assertion clear and robust.

This limits the file tools to either the current working directory or the skills repo write_file & edit_file - restricted to just working directory read_file - restricted to working directory & skills directory Signed-off-by: Josh Holt <jholt96@live.com>

use skills dir instead of hardcoded path Signed-off-by: Josh Holt <jholt96@live.com>

Signed-off-by: Josh Holt <jholt96@live.com>

supreme-gg-gg

Agreed we should restrict directory, one nit

supreme-gg-gg · 2026-03-12T21:31:40Z

python/packages/kagent-skills/src/kagent/skills/shell.py

-    command: str,
-    working_dir: Path,
-) -> str:
+async def execute_command(command: str, working_dir: Path, skills_dir: Path) -> str:


Can you also update the usage of execute_command in test_skill_execution.py and kagent/openai/tools/_tools.py to pass in the skills_dir argument or make the skills dir argument optional?

just pushed changed for this! lmk if there is anything else. I made the openai have a default of /skills and try to pull in that env

Signed-off-by: Josh Holt <jholt96@live.com>

jholt96 requested review from EItanya, peterj and yuval-k as code owners March 12, 2026 14:52

Copilot AI review requested due to automatic review settings March 12, 2026 14:52

Copilot started reviewing on behalf of jholt96 March 12, 2026 14:52 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

jholt96 added 2 commits March 12, 2026 14:34

fix(skills dir): skills dir

773a54d

use skills dir instead of hardcoded path Signed-off-by: Josh Holt <jholt96@live.com>

jholt96 force-pushed the file-tool-restrictions branch from 5ebab86 to 773a54d Compare March 12, 2026 18:34

jholt96 added 2 commits March 12, 2026 14:45

fix(openai tools): fix hardcoded /skills in openai tool code

2d1b7af

Signed-off-by: Josh Holt <jholt96@live.com>

formatting

8fe0b87

Signed-off-by: Josh Holt <jholt96@live.com>

supreme-gg-gg reviewed Mar 12, 2026

View reviewed changes

fix(skills dir): ensure skills_dir is passed and has default value

e34e54e

Signed-off-by: Josh Holt <jholt96@live.com>

supreme-gg-gg approved these changes Mar 13, 2026

View reviewed changes

EItanya approved these changes Mar 15, 2026

View reviewed changes

EItanya merged commit e8c15bd into kagent-dev:main Mar 15, 2026
23 checks passed

-        return read_file_content(path, offset, limit, allowed_root=[working_dir, Path("/skills")])
+        allowed_roots = [working_dir]
+        skills_directory = getattr(wrapper.context, "skills_directory", None)
+        if skills_directory is not None:
+            allowed_roots.append(Path(skills_directory))
+        else:
+            # Fallback to the default skills mount point for backwards compatibility.
+            allowed_roots.append(Path("/skills"))
+        return read_file_content(path, offset, limit, allowed_root=allowed_roots)

		with pytest.raises(PermissionError, match="outside the allowed director"):
		read_file_content(outside_file, allowed_root=tmp_path)

Conversation

jholt96 commented Mar 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

supreme-gg-gg left a comment

Choose a reason for hiding this comment

Uh oh!

supreme-gg-gg Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

jholt96 Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants