[Core] Reject path traversal via embedded ".." segments in path_in_repo - #4884
Open
bodapatisaikrishna wants to merge 1 commit into
Open
[Core] Reject path traversal via embedded ".." segments in path_in_repo#4884bodapatisaikrishna wants to merge 1 commit into
bodapatisaikrishna wants to merge 1 commit into
Conversation
_validate_path_in_repo() only rejected a ".." segment when it was the first path component (exact "..", or a "../" prefix). A "../" later in the path, e.g. "a/../../etc/passwd", still resolves outside the repo root but was left unchanged. Walk the full path and track depth relative to the repo root, raising as soon as it would go negative, instead of only checking the first component. A ".." that resolves back into the repo without escaping (e.g. "a/../file.txt") is still allowed, matching current behavior.
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.
Summary
_validate_path_in_repo()(used by everyCommitOperationAdd/Copy/Delete, soupload_file,upload_folder,create_commit,delete_file,delete_folder, ...)only rejects a
..segment when it's the first path component:A
..later in the path still escapes the repo root once resolved, and slips throughunchanged:
This fixes it by walking the full path and tracking depth relative to the repo root,
raising as soon as it would go negative (net traversal above the root) — instead of
only checking the very first component. A
..that resolves back into the repowithout escaping (e.g.
"a/../file.txt") is still allowed, matching current behavior.I don't know how the server itself handles a raw
..in a commit path, so I can't saywhether this is exploitable end-to-end today — this closes a gap in the client-side
guard either way, and the function's own comment says its purpose is exactly this
("prevent a server-side issue").
Test plan
Added cases to
TestCommitOperationPathInRepointests/test_commit_api.py:invalid_valuesgets"a/../../file.txt"and"a/b/../../../file.txt";valid_valuesgets
"a/../file.txt"and"a/b/../../c/file.txt"to confirm non-escaping..isstill allowed unchanged.
ruff check/ruff format/ty checkclean on the changed files.Found this by reading
_commit_api.pyand testing_validate_path_in_repodirectlywith a few traversal patterns, not from a reported issue.
Note
Medium Risk
Security-hardening of path validation used by all commit/upload/delete APIs; behavior change only rejects previously accepted malicious paths, with low regression risk for normal paths covered by new tests.
Overview
Tightens client-side
path_in_repovalidation in_validate_path_in_repo()so commit paths cannot escape the repo root via..in the middle of a path (e.g.a/../../file.txt), not only when the path starts with../.After the existing leading-slash and prefix checks, the validator now walks every path segment and tracks depth from the repo root; it raises
ValueErrorif a..would drive depth below zero. Paths where..stays inside the repo (e.g.a/../file.txt) are still accepted unchanged.Tests in
TestCommitOperationPathInRepoadd invalid traversal examples and valid in-repo..cases for add/delete operations.Reviewed by Cursor Bugbot for commit a1cbea3. Bugbot is set up for automated code reviews on this repo. Configure here.