diff --git a/launchpad/scripts/security_audit_classifier.py b/launchpad/scripts/security_audit_classifier.py index 2ea9a7150cc..e2684483d90 100644 --- a/launchpad/scripts/security_audit_classifier.py +++ b/launchpad/scripts/security_audit_classifier.py @@ -20,10 +20,21 @@ blind to exactly the files #62 exists to watch. """ +import re import subprocess -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Optional, Set +#: Matches a Windows drive letter root ("C:/", "d:/") after backslash-to-slash +#: normalization. PurePosixPath.is_absolute() only recognizes a leading "/" -- +#: it has no concept of a drive letter, so "C:/Users/x/buzz/foo.py" reads as +#: NOT absolute to it and would otherwise fall through to the plain membership +#: check below, almost certainly returning the wrong-direction "fork-added" +#: guess this function exists to avoid. This repo already treats Windows-style +#: paths as a real input class (the backslash normalization two lines below +#: predates this check), so the absolute-path guard must catch this form too. +_WINDOWS_DRIVE_ROOT = re.compile(r"^[A-Za-z]:/") + UPSTREAM_URL = "https://github.com/block/buzz.git" UPSTREAM_REF = "main" @@ -64,4 +75,13 @@ def classify(path: str, upstream_paths: Optional[Set[str]]) -> str: """'fork-added', 'inherited', or 'indeterminate' when upstream_paths is None.""" if upstream_paths is None: return "indeterminate" - return "inherited" if path.replace("\\", "/") in upstream_paths else "fork-added" + normalized = path.replace("\\", "/") + if normalized.startswith("./"): + normalized = normalized[2:] + # git ls-tree's output (what upstream_paths is built from) is always relative. + # An absolute path can't be safely compared against it without knowing the + # repo root, and guessing fork-added for it is exactly the wrong-direction + # guess this module's docstring warns against — indeterminate is honest. + if PurePosixPath(normalized).is_absolute() or _WINDOWS_DRIVE_ROOT.match(normalized): + return "indeterminate" + return "inherited" if normalized in upstream_paths else "fork-added" diff --git a/launchpad/scripts/test_security_audit_classifier.py b/launchpad/scripts/test_security_audit_classifier.py index 63c45fb9427..c1b554abd06 100644 --- a/launchpad/scripts/test_security_audit_classifier.py +++ b/launchpad/scripts/test_security_audit_classifier.py @@ -60,6 +60,53 @@ def test_windows_style_separators_are_normalized(self): "inherited", ) + def test_dot_slash_prefixed_path_is_normalized(self): + self.assertEqual( + classify("./.github/workflows/ci.yml", _SYNTHETIC_UPSTREAM_PATHS), + "inherited", + ) + + def test_absolute_path_is_indeterminate_not_a_guess(self): + self.assertEqual( + classify( + "/home/serina/Launchpad/buzz/.github/workflows/ci.yml", + _SYNTHETIC_UPSTREAM_PATHS, + ), + "indeterminate", + ) + + def test_absolute_path_to_a_fork_only_file_is_still_indeterminate(self): + # A caller bug that computes absolute paths should never resolve to a + # guess in either direction, not just for genuinely-inherited files. + self.assertEqual( + classify("/home/serina/Launchpad/buzz/launchpad/deploy/README.md", _SYNTHETIC_UPSTREAM_PATHS), + "indeterminate", + ) + + def test_windows_absolute_path_is_indeterminate_not_a_guess(self): + # PurePosixPath.is_absolute() has no concept of a drive letter, so + # "C:/Users/.../ci.yml" is NOT absolute to it -- without a dedicated + # check this falls through to the plain membership test and returns + # the wrong-direction "fork-added" guess for a genuinely inherited + # file, exactly the failure mode the POSIX-absolute check exists to + # prevent for forward-slash-rooted paths. + self.assertEqual( + classify( + "C:\\Users\\serina\\Launchpad\\buzz\\.github\\workflows\\ci.yml", + _SYNTHETIC_UPSTREAM_PATHS, + ), + "indeterminate", + ) + + def test_windows_absolute_path_to_a_fork_only_file_is_still_indeterminate(self): + self.assertEqual( + classify( + "C:\\Users\\serina\\Launchpad\\buzz\\launchpad\\deploy\\README.md", + _SYNTHETIC_UPSTREAM_PATHS, + ), + "indeterminate", + ) + class FetchUpstreamPathsTest(unittest.TestCase): def test_returns_none_on_called_process_error(self):