fix: validate directory hrefs per segment so sighted mode recurses again - #83
Open
LuvW-Q wants to merge 1 commit into
Open
fix: validate directory hrefs per segment so sighted mode recurses again#83LuvW-Q wants to merge 1 commit into
LuvW-Q wants to merge 1 commit into
Conversation
Commit 5f2a8ba (the GHSA-hr3m-4qwq-3mgc fix) hardened add_folder's recursion branch by passing the whole directory href to _is_safe_path_segment. That gate is a single-segment allowlist (^[A-Za-z0-9._\-+@]+\Z), while a directory href is a multi-segment path ('/.git/objects/'), so the match always fails: every subdirectory of a directory listing is skipped with "Skipping unsafe directory listing entry", the rebuilt repo has no objects/refs, `git clone` fails with "repository does not exist", and copy_useful_files then crashes with FileNotFoundError. Any listing-enabled (sighted) target fails on 1.1.8. Resolve the directory URL with urljoin (listings may emit absolute hrefs), require it to stay inside the origin and the .git/ crawl anchor, split it into segments, and validate each segment with the existing _is_safe_path_segment gate before recursing — the same per-segment approach the file branch already uses. Traversal ('..'), NUL, and cross-origin entries are still rejected; an absolute href like '/etc/' is now additionally confined to the .git/ anchor. Add regression tests: benign subdirectory entries must be recursed into ('objects/', '/.git/objects/', 'refs/heads/'), and files listed in a subdirectory's own listing must be queued anchored under temp_dst. Fixes WangYihang#82
WangYihang
pushed a commit
that referenced
this pull request
Sep 8, 2026
…recursion Builds on #83 (merged into this branch, so its commit and tests are kept). add_folder resolved directory and file entries separately — the file branch with urljoin, the directory branch by string concatenation plus a check meant for a single path segment. Two implementations of one operation drifted, and that drift was issue #82. #83 makes the two branches more alike; this makes them one. _resolve_href takes an href and returns the URL to fetch together with the path components to write, or None. Deriving both from one resolution is what stops them disagreeing again. It applies four checks in order: same origin, under the .git/ anchor, decodes to at least one segment, and every segment passes _is_safe_path_segment. Hrefs are now unquoted before validation, so a percent-encoded separator is judged as what it decodes to rather than as literal text. Fixes three findings the new suites pinned: - C5 (off-tree crawl): the file branch checked the origin but not the anchor, so a listing could steer the crawl at any same-origin path — /private/ CREDENTIALS was fetched into the operator's output. The anchor check now applies to both kinds of entry. - C6 (infinite listing): no visited set and no depth cap. A server answering every URL with a one-subdirectory listing drove the crawl until the path outgrew the filesystem limit — 131 requests, then an unhandled OSError. A visited-URL set stops a listing that links to itself (the URL never grows); _MAX_CRAWL_DEPTH stops one that grows the URL each step. - Listing self-links (Apache's ?C=N;O=D, in-page anchors) resolved back to the listing URL and were queued as a file, costing a request per listing. Verified against a real evil_server with local GitHacker (Docker is unavailable here, so the containerised leg is still unrun): before after C5_offtree_crawl FAIL PASS (7 requests, no /private/) C6_infinite_listing FAIL, 131 req PASS (38 requests) A1 / C1 / C4 / A7 PASS PASS (no regression) Against the absolute-href listing server, GitHacker went from skipping every subdirectory and recovering 0 files to "1 / 1 were exploited successfully" with 26 files — issue #82, end to end. Eight xfail(strict) markers flipped to XPASS and were removed, which is the mechanism working as intended. The `....//` expectation moved from ".git/....//" to ".git/..../" because urljoin normalises the doubled slash that string concatenation used to leave. The runaway-detector meta-test no longer depends on the crawler being broken: it drives the scripted server directly. Two legacy test helpers build a GitHacker with __new__ and needed the new instance attributes added by hand — the coupling the conftest fixture avoids. 292 passed, 16 xfailed. ruff check and format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fxyt81cRHQ4ewH5Gqrtb45
WangYihang
pushed a commit
that referenced
this pull request
Sep 8, 2026
…recursion Builds on #83 (merged into this branch, so its commit and tests are kept). add_folder resolved directory and file entries separately — the file branch with urljoin, the directory branch by string concatenation plus a check meant for a single path segment. Two implementations of one operation drifted, and that drift was issue #82. #83 makes the two branches more alike; this makes them one. _resolve_href takes an href and returns the URL to fetch together with the path components to write, or None. Deriving both from one resolution is what stops them disagreeing again. It applies four checks in order: same origin, under the .git/ anchor, decodes to at least one segment, and every segment passes _is_safe_path_segment. Hrefs are now unquoted before validation, so a percent-encoded separator is judged as what it decodes to rather than as literal text. Fixes three findings the new suites pinned: - C5 (off-tree crawl): the file branch checked the origin but not the anchor, so a listing could steer the crawl at any same-origin path — /private/ CREDENTIALS was fetched into the operator's output. The anchor check now applies to both kinds of entry. - C6 (infinite listing): no visited set and no depth cap. A server answering every URL with a one-subdirectory listing drove the crawl until the path outgrew the filesystem limit — 131 requests, then an unhandled OSError. A visited-URL set stops a listing that links to itself (the URL never grows); _MAX_CRAWL_DEPTH stops one that grows the URL each step. - Listing self-links (Apache's ?C=N;O=D, in-page anchors) resolved back to the listing URL and were queued as a file, costing a request per listing. Verified against a real evil_server with local GitHacker (Docker is unavailable here, so the containerised leg is still unrun): before after C5_offtree_crawl FAIL PASS (7 requests, no /private/) C6_infinite_listing FAIL, 131 req PASS (38 requests) A1 / C1 / C4 / A7 PASS PASS (no regression) Against the absolute-href listing server, GitHacker went from skipping every subdirectory and recovering 0 files to "1 / 1 were exploited successfully" with 26 files — issue #82, end to end. Eight xfail(strict) markers flipped to XPASS and were removed, which is the mechanism working as intended. The `....//` expectation moved from ".git/....//" to ".git/..../" because urljoin normalises the doubled slash that string concatenation used to leave. The runaway-detector meta-test no longer depends on the crawler being broken: it drives the scripted server directly. Two legacy test helpers build a GitHacker with __new__ and needed the new instance attributes added by hand — the coupling the conftest fixture avoids. 292 passed, 16 xfailed. ruff check and format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fxyt81cRHQ4ewH5Gqrtb45
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.
Summary
Fixes #82.
1.1.8's sighted (directory-listing) mode skips every subdirectory of the listing:
The temp repo ends up with only top-level files, so
git clonefails andcopy_useful_files()crashes withFileNotFoundError. Every listing-enabled target fails on 1.1.8.Root cause: 5f2a8ba (the GHSA-hr3m-4qwq-3mgc fix) passes the whole directory href to
_is_safe_path_segment, but that gate is a single-segment allowlist (^[A-Za-z0-9._\-+@]+\Z) while a directory href is a multi-segment path (/.git/objects/) — the match always fails.Fix (
githacker/__main__.py,add_folder): resolve the directory href withurljoin(listings may emit absolute hrefs), require it to stay inside the origin and the.git/crawl anchor (a same-origin absolute href like/etc/must not be followed), split it into segments, and validate each segment with the existing_is_safe_path_segment— the same per-segment approach the file branch already uses. Traversal (..), NUL, cross-origin, and off-anchor entries are all still rejected.Tests
Added two regression tests in
tests/test_add_folder_traversal.py:test_add_folder_recurses_into_benign_subdirectories— benign dir entries (objects/,/.git/objects/,refs/heads/) must issue a second fetch (recursed into), pinned by request count.test_add_folder_recursion_queues_files_from_sublisting— files listed in a subdirectory's own listing are queued anchored undertemp_dst, no.., no escape.The existing 22-payload traversal corpus and all other security tests pass unchanged:
Verified RED on 1.1.8 (benign recursion tests fail; traversal tests keep passing), GREEN with the fix.