Skip to content

fix(file-io): scope delete, move and frontmatter_delete to allowed paths - #729

Open
RerankerGuo wants to merge 1 commit into
agentscope-ai:mainfrom
RerankerGuo:fix/scope-delete-move-frontmatter-delete
Open

RerankerGuo wants to merge 1 commit into
agentscope-ai:mainfrom
RerankerGuo:fix/scope-delete-move-frontmatter-delete

Conversation

@RerankerGuo

Copy link
Copy Markdown
Contributor

Summary

_allowed_paths is the request-scoped permission constraint that hosts inject (via injected_job_kwargs) to bound what an agent session may touch. ReadStep, EditStep, WriteStep, FrontmatterReadStep and FrontmatterUpdateStep enforce it, but three mutating file I/O steps silently ignored it:

  • delete_step unlinked files and shutil.rmtreed whole folders anywhere in the workspace.
  • move_step copied any file to any destination and unlinked the source.
  • frontmatter_delete_step removed frontmatter keys from any Markdown file.

A session scoped to a single note (for example AutoMemoryStep update rounds injecting _allowed_paths: [note_path]) could therefore still delete other notes or entire folders, move files in or out of the scope, or tamper with other notes' frontmatter — which is exactly the hole the FrontmatterUpdateStep docstring says the constraint exists to close.

Fix

  • DeleteStep._delete: fail with no permission to delete this path when the resolved target is outside the scope (before any unlink/rmtree). Directory scope semantics are unchanged: deleting a folder inside the allowed directory works, deleting an ancestor of the scope does not.
  • MoveStep._move: check both endpoints — the source (which gets unlinked) and the destination (which gets written) — and fail closed with no permission to move this file before the copy.
  • FrontmatterDeleteStep: mirror FrontmatterUpdateStep (same placement, same no permission to update this file message).

Without an injected constraint (_allowed_paths is None) the steps keep their unrestricted behavior, so existing jobs are unaffected; invalid constraints already fail closed in _check_path_permission.

Tests

Four regression tests in tests/unit/test_path_permission.py (all fail on main before the fix):

  • delete: exact-note scope allows deleting the scoped note, rejects siblings and paths outside the scope;
  • delete with directory scope: rejects the scope's ancestor folder and sibling folders, allows deleting the scoped folder itself;
  • move: rename inside a directory scope succeeds; moving across the scope boundary is denied in both directions and leaves both files untouched;
  • frontmatter_delete: rejects removing keys from another note, allows the scoped note.

The full tests/unit suite passes except the pre-existing test_index_update_loop_retries_source_changed_during_initial_scan, which fails identically on unmodified main on this machine (timing-sensitive watcher waits; unrelated to this change).

DeleteStep, MoveStep and FrontmatterDeleteStep mutated any workspace
path without consulting the request-scoped _allowed_paths constraint,
so a scope-limited agent session could still unlink or rmtree paths
outside its scope, move files in or out of it, or drop frontmatter
keys from other notes. Enforce the same fail-closed check the other
file I/O steps already apply: delete checks the target, move checks
both the source and the destination, and frontmatter_delete mirrors
frontmatter_update. Without an injected constraint the steps keep
their unrestricted behavior.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant