feat: implement issue #1361 — SonarCloud: code security review (pythonsecurity)

.github/scripts/feature-ideation/validate-signals.py

@@ -94,18 +94,47 @@ def _allowed_roots() -> list[str]:
     return [r for r in roots if os.path.dirname(r) != r]
 
 
+def _rebuild_from_root(root: str, rel: str) -> Path:
+    """Re-derive ``root``/``rel`` from directory listings, starting at ``root``.
+
+    Each component of ``rel`` is matched by name against the real entries of
+    the directory reached so far, and the result is built only from those
+    entries — never from the CLI string itself. A confined argument therefore
+    cannot carry anything onto the file-read sink that the trusted root does
+    not actually contain (SonarCloud pythonsecurity:S8707 traced the argv taint
+    straight through a plain ``commonpath`` guard). ``rel`` comes from a
+    ``realpath``, so it holds no ``..`` or symlink components.
+
+    Raises ``FileNotFoundError`` when a component does not exist.
+    """
+    current = Path(root)
+    if rel == os.curdir:
+        return current
+    for part in Path(rel).parts:
+        wanted = os.path.normcase(part)
+        with os.scandir(current) as entries:
+            match = next(
+                (e for e in entries if os.path.normcase(e.name) == wanted), None
+            )
+        if match is None:
+            raise FileNotFoundError(os.path.join(root, rel))
+        current = Path(match.path)
+    return current
+
+
 def _confine_path(raw: str | os.PathLike[str]) -> Path | None:
     """Resolve ``raw`` and return it only if it lies inside an allowed root.
 
     Guards against path traversal (``../``, absolute paths, symlinks) via the
     CLI arguments — SonarCloud pythonsecurity:S8707. Returns ``None`` when the
-    resolved path escapes every allowed root.
+    resolved path escapes every allowed root, and raises ``FileNotFoundError``
+    when it is inside one but does not exist.
     """
     candidate = os.path.realpath(raw)
     for root in _allowed_roots():
         try:
             if os.path.commonpath([root, candidate]) == root:
-                return Path(candidate)
+                return _rebuild_from_root(root, os.path.relpath(candidate, root))
         except ValueError:
             # On Windows, os.path.commonpath raises ValueError when paths are
             # on different drives (e.g., C:\ and D:\). Treat this as outside.
@@ -123,26 +152,33 @@ def main(argv: list[str]) -> int:
     raw_schema = argv[2] if len(argv) == 3 else os.path.join(
         _REPO_ROOT, ".github", "schemas", "signals.schema.json"
     )
-    signals_path = _confine_path(argv[1])
+    try:
+        signals_path = _confine_path(argv[1])
+    except FileNotFoundError as exc:
+        sys.stderr.write(f"[validate-signals] not found: {exc}\n")
+        return 2
+    except OSError as exc:
+        sys.stderr.write(f"[validate-signals] cannot read {argv[1]}: {exc}\n")
+        return 2
     if signals_path is None:
         sys.stderr.write(
             f"[validate-signals] path is outside the allowed directories: {argv[1]}\n"
         )
         return 2
-    schema_path = _confine_path(raw_schema)
+    try:
+        schema_path = _confine_path(raw_schema)
+    except FileNotFoundError as exc:
+        sys.stderr.write(f"[validate-signals] schema not found: {exc}\n")
+        return 2
+    except OSError as exc:
+        sys.stderr.write(f"[validate-signals] cannot read schema {raw_schema}: {exc}\n")
+        return 2
     if schema_path is None:
         sys.stderr.write(
             f"[validate-signals] schema path is outside the allowed directories: {raw_schema}\n"
         )
         return 2
 
-    if not signals_path.exists():
-        sys.stderr.write(f"[validate-signals] not found: {signals_path}\n")
-        return 2
-    if not schema_path.exists():
-        sys.stderr.write(f"[validate-signals] schema not found: {schema_path}\n")
-        return 2
-
     try:
         signals = json.loads(signals_path.read_text(encoding="utf-8"))
     except OSError as exc:

test/feature-ideation/test_validate_signals.py

@@ -142,6 +142,52 @@ def test_schema_outside_root_is_rejected(self, confined, capsys):
         assert rc == 2
         assert "outside the allowed directories" in capsys.readouterr().err
 
+    def test_missing_signals_inside_root_reports_not_found(self, confined, capsys):
+        """A missing file inside an allowed root keeps the ``not found`` exit 2."""
+        rc = module.main(["validate-signals.py", "missing.json", str(self.SCHEMA)])
+        assert rc == 2
+        assert "not found" in capsys.readouterr().err
+
+    def test_missing_schema_inside_root_reports_not_found(self, confined, capsys):
+        """A missing schema inside an allowed root keeps the ``schema not found`` exit 2."""
+        (confined / "signals.json").write_text(
+            self.FIXTURE.read_text(encoding="utf-8"), encoding="utf-8"
+        )
+        rc = module.main(["validate-signals.py", "signals.json", "nope/schema.json"])
+        assert rc == 2
+        assert "schema not found" in capsys.readouterr().err
+
+    def test_confined_path_is_rebuilt_from_directory_entries(self, confined, monkeypatch):
+        """The returned path is assembled from scandir entries under the root."""
+        nested = confined / "a" / "b"
+        nested.mkdir(parents=True)
+        target = nested / "signals.json"
+        target.write_text("{}", encoding="utf-8")
+        seen = []
+        real_scandir = module.os.scandir
+
+        def spy(path):
+            seen.append(str(path))
+            return real_scandir(path)
+
+        monkeypatch.setattr(module.os, "scandir", spy)
+        result = module._confine_path("a/b/../b/signals.json")
+        monkeypatch.undo()
+        assert result == target.resolve()
+        assert seen == [str(confined.resolve()), str(confined.resolve() / "a"),
+                        str(confined.resolve() / "a" / "b")]
+
+    def test_confined_root_itself_is_returned(self, confined):
+        """An argument naming an allowed root resolves to that root."""
+        assert module._confine_path(".") == confined.resolve()
+
+    def test_confined_missing_component_raises_not_found(self, confined):
+        """A missing file or intermediate directory inside a root raises FileNotFoundError."""
+        with pytest.raises(FileNotFoundError):
+            module._confine_path("missing.json")
+        with pytest.raises(FileNotFoundError):
+            module._confine_path("no/such/dir/signals.json")
+
     def test_default_allowed_roots_exclude_system_temp(self, tmp_path, monkeypatch):
         """Default roots cover CWD, $RUNNER_TEMP and the repo, not the system temp dir."""
         import os