Reject file keys that escape the workspace in inject_files - #1476
Open
Tech Guy (lukiod) wants to merge 1 commit into
Open
Reject file keys that escape the workspace in inject_files#1476Tech Guy (lukiod) wants to merge 1 commit into
Tech Guy (lukiod) wants to merge 1 commit into
Conversation
FBWorkspace.inject_files joined keys supplied by the caller onto workspace_path with no containment check, so a key like "../escape.txt" (or an absolute path) wrote or deleted outside the workspace once resolved. Resolve the workspace root once, resolve each candidate path against it, and reject anything not relative to the root before either the write or delete branch runs. Fixes microsoft#1473
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
1 participant
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.
Fixes #1473
FBWorkspace.inject_filesjoined the caller's file keys directly ontoworkspace_pathand wrote/deleted the result with no containmentcheck, so a key such as
"../escape.txt"(or an absolute path)resolved to a location outside the workspace.
Resolves the workspace root once, resolves each candidate path against
it, and raises
ValueErrorbefore either the write or the deletebranch runs if the resolved path is not inside the workspace root.
Added
test_inject_files_rejects_paths_outside_workspaceto theexisting
test/utils/test_ws.py(rather than a new top-level testfile) since that is already this class's real home. It covers ordinary
and nested relative files still working, a
../key, an absolute-pathkey, and the
DEL_KEYdelete branch against a file that alreadyexists outside the workspace.
Verified: the new test fails with
AssertionError: ValueError not raisedagainst the code onmain(reverted the fix locally, kept thetest, reran it) and passes with the fix applied.
test/utils/test_ws.py(2 tests, including the pre-existing checkpoint-roundtrip test) passes
in full:
2 passed in 0.04s.ruff checkon both changed files isclean relative to
main— the only pre-existing lint items intest_ws.py(unittest-style assertions,os.readlink) are unrelatedand already present before this change, left untouched.
Not in scope here:
FBWorkspace.remove_fileshas the identicalself.workspace_path / file_namepattern with no containment check.Flagging it since it's the same root cause, but leaving it out of this
PR since the issue is scoped to
inject_files— happy to send afollow-up if that's wanted.
📚 Documentation preview 📚: https://RDAgent--1476.org.readthedocs.build/en/1476/