From ebbb3e5dc4bfddcd5de0cb14ad03d640dc855fed Mon Sep 17 00:00:00 2001 From: dovvnloading Date: Tue, 21 Jul 2026 22:10:41 -0400 Subject: [PATCH] Reject Windows drive-lettered and colon-bearing Gitlink paths _normalize_repo_path's absolute-path guard (audit finding B1) checks PurePosixPath.is_absolute(), which doesn't recognize a Windows drive letter as absolute (no leading '/'). So a drive-lettered path slipped past it - not a real escape (_safe_local_target's resolve()+containment check still catches a genuine cross-drive path), but silent aliasing: Windows pathlib treats a same-drive "C:" path segment as a no-op drive-relative reference rather than a literal folder, so an LLM-proposed "C:/config/app.txt" resolved to the exact same target as the plain "config/app.txt" - not rejected as "absolute-looking input" the way this guard's own contract says it should be. Reject any ':' in a path segment loudly instead of letting it collapse silently - covers same-drive and cross-drive letters, the slash-less drive-relative form ("C:app.txt"), and an NTFS Alternate Data Stream marker as a bonus (ADS addressing doesn't actually work through this path-join code path, but rejecting it is free and consistent with the rest of this guard's defensive stance). Updated the one existing test that had pinned the old "silently contained" behavior to instead assert rejection, and added coverage for every variant found above. Full suite 1710 passed. Co-Authored-By: Claude Sonnet 5 --- .../graphlink_plugins/gitlink/agent.py | 13 +++++ .../tests/test_gitlink_path_safety.py | 47 ++++++++++++++++--- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/graphlink_app/graphlink_plugins/gitlink/agent.py b/graphlink_app/graphlink_plugins/gitlink/agent.py index 7107635..7d83111 100644 --- a/graphlink_app/graphlink_plugins/gitlink/agent.py +++ b/graphlink_app/graphlink_plugins/gitlink/agent.py @@ -86,6 +86,19 @@ def _normalize_repo_path(path_text): normalized = PurePosixPath(raw_path) if normalized.is_absolute() or ".." in normalized.parts: raise RuntimeError("Repository paths must stay inside the selected repository.") + # A Windows drive letter ("C:/config/app.txt", "C:app.txt") or an NTFS + # Alternate Data Stream ("file.txt:hidden") isn't "absolute" by POSIX's + # is_absolute() (no leading '/'), so it slipped past the check above - + # not as a real escape (_safe_local_target's resolve()+containment check + # still catches a genuine cross-drive path), but as silent aliasing: + # Windows pathlib treats a same-drive "C:" path segment as a no-op + # drive-relative reference rather than a literal folder, so + # "C:/config/app.txt" resolved to the exact same target as the plain + # "config/app.txt" - not rejected as "absolute-looking input" the way + # this guard's own contract says it should be. Reject any ':' in a path + # segment loudly instead of letting it collapse silently. + if any(":" in part for part in normalized.parts): + raise RuntimeError("Repository paths must stay inside the selected repository.") return normalized.as_posix() diff --git a/graphlink_app/tests/test_gitlink_path_safety.py b/graphlink_app/tests/test_gitlink_path_safety.py index e0bd866..7bf7886 100644 --- a/graphlink_app/tests/test_gitlink_path_safety.py +++ b/graphlink_app/tests/test_gitlink_path_safety.py @@ -61,6 +61,41 @@ def test_unc_style_path_is_rejected(self): with pytest.raises(RuntimeError): _normalize_repo_path("//server/share/evil.txt") + def test_windows_drive_letter_path_is_rejected(self): + # Bug-scan finding: a drive-lettered path isn't "absolute" by POSIX's + # is_absolute() (no leading '/'), so it slipped past the check above. + # Not a real escape - _safe_local_target's containment check still + # holds - but silent ALIASING: Windows pathlib treats a same-drive + # "C:" path segment as a no-op drive-relative reference rather than a + # literal folder, so this used to resolve to the exact same target as + # the plain "config/app.txt", contrary to this guard's own contract + # of rejecting absolute-looking input. + with pytest.raises(RuntimeError): + _normalize_repo_path("C:/config/app.txt") + + def test_windows_drive_relative_path_with_no_slash_is_rejected(self): + # "C:app.txt" (no slash) is the same drive-relative hazard as above, + # via a different textual shape - PurePosixPath keeps it as one part + # ("C:app.txt") rather than splitting off "C:", but the substring + # check still catches it. + with pytest.raises(RuntimeError): + _normalize_repo_path("C:app.txt") + + def test_cross_drive_path_is_rejected(self): + with pytest.raises(RuntimeError): + _normalize_repo_path("D:/config/app.txt") + + def test_alternate_data_stream_style_path_is_rejected(self): + # An NTFS Alternate Data Stream marker - same defensive "reject any + # colon" guard, though ADS addressing doesn't actually work through + # this path-join code path. + with pytest.raises(RuntimeError): + _normalize_repo_path("file.txt:hidden_stream") + + def test_a_colon_deeper_in_the_path_is_still_rejected(self): + with pytest.raises(RuntimeError): + _normalize_repo_path("src/weird:name.py") + class TestSafeLocalTarget: def test_normal_path_resolves_under_root(self, tmp_path): @@ -71,12 +106,12 @@ def test_parent_traversal_is_rejected(self, tmp_path): with pytest.raises(RuntimeError): _safe_local_target(tmp_path, "../../outside.py") - def test_windows_drive_letter_injection_stays_contained(self, tmp_path): - # A path that looks like a Windows absolute path (drive letter) must not - # be able to escape the repo root once re-joined onto it. - target = _safe_local_target(tmp_path, "C:/Windows/System32/evil.txt") - resolved_root = tmp_path.resolve() - assert target == resolved_root or resolved_root in target.parents + def test_windows_drive_letter_path_is_rejected_at_normalization(self, tmp_path): + # _safe_local_target normalizes first, and normalization now rejects + # a drive-lettered path outright (see TestNormalizeRepoPath) instead + # of silently aliasing it onto some path under the repo root. + with pytest.raises(RuntimeError): + _safe_local_target(tmp_path, "C:/Windows/System32/evil.txt") def test_unc_style_path_is_rejected_at_normalization(self, tmp_path): # _safe_local_target normalizes first, and normalization now rejects