fix(permission): resolve redirect targets against the cd-tracked cwd - #947
Open
Terminator666666 wants to merge 1 commit into
Open
fix(permission): resolve redirect targets against the cd-tracked cwd#947Terminator666666 wants to merge 1 commit into
Terminator666666 wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Summary
ShellPathValidator.check()trackscdas it walks a compound command and passes theresulting cwd down to
_check_command()._check_redirects()never got that treatment —it resolves relative redirect targets against
self._workspace_rootregardless of wherethe shell has actually moved.
So two halves of the same check disagree with each other:
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_dirsat differing depths, and it alsoneeds 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, aheadof the
cdbookkeeping, because a shell resolves the redirect incd foo > logagainstthe old cwd as well.
Related issue number
None.
Checklist
pre-commit installandpre-commit run --all-filesbefore git commit, and passed lint check.Added
TestRedirectCwdTrackingintests/permission/test_shell_validator.py— four casescovering both directions plus one asserting that redirects and ordinary path arguments
agree on the cwd. Reverting
shell_validator.pyand keeping the tests fails three of them.tests/permission/is at 340 passed. Note that a plainpytest tests/permission/gave me30 failures on async tests before I touched anything; they need
pytest-asyncioand passwith
--asyncio-mode=auto. Unrelated to this change, but it tripped me up whileestablishing 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.