Skip to content

fix(permission): resolve redirect targets against the cd-tracked cwd - #947

Open
Terminator666666 wants to merge 1 commit into
modelscope:mainfrom
Terminator666666:fix/redirect-cwd-tracking
Open

fix(permission): resolve redirect targets against the cd-tracked cwd#947
Terminator666666 wants to merge 1 commit into
modelscope:mainfrom
Terminator666666:fix/redirect-cwd-tracking

Conversation

@Terminator666666

Copy link
Copy Markdown

Change Summary

ShellPathValidator.check() tracks cd as it walks a compound command and passes the
resulting cwd down to _check_command(). _check_redirects() never got that treatment —
it resolves relative redirect targets against self._workspace_root regardless of where
the shell has actually moved.

So two halves of the same check disagree with each other:

cd work/sub && rm ../f        ->  allow   (correct)
cd work/sub && echo x > ../f  ->  deny    (wrong — this writes work/f)

Both commands touch the same file. Only one of them is judged correctly.

The mismatch goes both ways. Writes that stay inside the workspace get rejected, which is
the harmless direction. The other direction: a relative target can resolve into an allowed
directory when measured from the workspace root while the shell writes somewhere else. I
could only reproduce that with several allowed_dirs at differing depths, and it also
needs the real parent directory to already exist, so I doubt it amounts to much in
practice. Wrong either way.

The fix hands the tracked cwd to _check_redirects(). The call stays where it is, ahead
of the cd bookkeeping, because a shell resolves the redirect in cd foo > log against
the old cwd as well.

Related issue number

None.

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Run pre-commit install and pre-commit run --all-files before git commit, and passed lint check.
  • Documentation reflects the changes where applicable

Added TestRedirectCwdTracking in tests/permission/test_shell_validator.py — four cases
covering both directions plus one asserting that redirects and ordinary path arguments
agree on the cwd. Reverting shell_validator.py and keeping the tests fails three of them.

tests/permission/ is at 340 passed. Note that a plain pytest tests/permission/ gave me
30 failures on async tests before I touched anything; they need pytest-asyncio and pass
with --asyncio-mode=auto. Unrelated to this change, but it tripped me up while
establishing a baseline, so flagging it in case the CI config is worth a look.

Docs box left unchecked — the behaviour matches what the docstrings already describe, so
there was nothing to update.

ShellPathValidator tracks `cd` while walking a compound command and passes
the resulting cwd to _check_command(), but _check_redirects() ignored it and
always resolved relative redirect targets against the workspace root. The two
checks therefore disagreed about the cwd within one and the same command:

    cd work/sub && rm ../f        -> allow   (correct)
    cd work/sub && echo x > ../f  -> deny    (wrong, writes work/f)

The mismatch cuts both ways. Writes that stay inside the workspace are
rejected, and with several allowed_dirs at differing depths a target can
resolve into an allowed directory from the workspace root while the shell
writes outside of it.

Pass the tracked cwd into _check_redirects(). It stays ahead of the `cd`
handling on purpose, since a shell resolves the redirect in `cd foo > log`
against the old cwd as well.
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