From 7447f9c097e327288854ce3f23be538b75ee067c Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Tue, 27 Oct 2026 21:24:35 +0000 Subject: [PATCH] =?UTF-8?q?feat:=20implement=20issue=20#1361=20=E2=80=94?= =?UTF-8?q?=20SonarCloud:=20code=20security=20review=20(pythonsecurity)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .coverage | Bin 0 -> 53248 bytes .../feature-ideation/validate-signals.py | 58 ++++++++++++++---- .../feature-ideation/test_validate_signals.py | 46 ++++++++++++++ 3 files changed, 93 insertions(+), 11 deletions(-) create mode 100644 .coverage diff --git a/.coverage b/.coverage new file mode 100644 index 0000000000000000000000000000000000000000..ad610822708bc906f9a8025df3319375118bc191 GIT binary patch literal 53248 zcmeI)?Qh#e90zbaNs~HF<)UedN;UO(fx2pWSj3Alc)<#*U}9Psw19@R%t%9u3xA9%rQ_8JLoVoc(1fW1OYyxOY?qz(xNg5UYy`IZ@s^~w7Sezm)<`2ROx4Gs5Z1Cs>+!+#y270d~=a3j3I+E8AXM1?NN zl)u9%+N4`@T3roVec*Ad4@_Zu-vhC7KE zMXeUI>;8}tshMmX>dk{=rWj%vju>dn4kA)XpKEUug{3mEO3SVc_cg!AxSR$E%>`k! zde^Foq2h`>7_d3Pl0IwdWcU7jD+b^2YPTl(a_4fU)Y2cu55LbEizMH=i}0(`(wyp(4UGoozvilhr;1#ghj zi{b)b)gLSLgAD=@fB*y_009U<00Izz00bZaf$o1H{ttIW71UKz{#!PSL;7&-EiC009U< z00Izz00bZa0SG_<0uX?}J^~kF38i?UCJ-jwbb#kwx;c^2a_M#mt19K4e{$X)cfH*iioNA9*K(Ar z{QiGM(O2}F`&0~vAOHafKmY;|fB*y_009U<00Iz*1U^ahPO4A7Mh`y^c$Vw`*lMDe z*PfjBvt9q={(n*9Ap`*kKmY;|fB*y_009U<00IygLxH#&SEj}H|Em6vLO<9b009U< z00Izz00bZa0SG_<0uUHOfp~mclmGvJThV{fZ;xRWQ6&gK00Izz00bZa0SG_<0uX?} zK^I`Lgr=R|{;jdLw)WGvKVP_6zjx=>or{let^fP*qwoLx@A9LU)P$;?jwQbSRXLSB z{^Pv%`%3KY4-5C-PX6`X{5|E;-PEM~|Nm`8-`4*==pZOR1Rwwb2tWV=5P$##AOHaf zKmY=fzyy76pvETXvjHt8zW-NdqGJd_00Izz00bZa0SG_<0uX=z1RyYe0^<9BT>p<> zyP`%AfB*y_009U<00Izz00bZafk6R$|39dKFa#g~0SG_<0uX=z1Rwwb2tZ&w1^x$E CPuGF~ literal 0 HcmV?d00001 diff --git a/.github/scripts/feature-ideation/validate-signals.py b/.github/scripts/feature-ideation/validate-signals.py index e0854d180..d28b79b29 100755 --- a/.github/scripts/feature-ideation/validate-signals.py +++ b/.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: diff --git a/test/feature-ideation/test_validate_signals.py b/test/feature-ideation/test_validate_signals.py index 939306958..6d6f70d3e 100644 --- a/test/feature-ideation/test_validate_signals.py +++ b/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