From 4a74e962c793b8e0acc6531c008a3d9326a58702 Mon Sep 17 00:00:00 2001 From: yashrajbasav Date: Wed, 7 Oct 2026 13:05:27 +0530 Subject: [PATCH 1/6] Write CLI reports through no-follow directory descriptors Signed-off-by: yashrajbasav --- src/skillspector/cli.py | 11 +++-- src/skillspector/file_output.py | 79 +++++++++++++++++++++++++++++++ src/skillspector/suppression.py | 5 +- tests/unit/test_cli.py | 83 +++++++++++++++++++++++++++++++++ 4 files changed, 171 insertions(+), 7 deletions(-) create mode 100644 src/skillspector/file_output.py diff --git a/src/skillspector/cli.py b/src/skillspector/cli.py index dd71792f7..ccdcec2b5 100644 --- a/src/skillspector/cli.py +++ b/src/skillspector/cli.py @@ -46,6 +46,7 @@ from skillspector import __version__, transitive from skillspector.cleanup import TempDirTracker, cleanup_result from skillspector.constants import RISK_THRESHOLD +from skillspector.file_output import write_text_no_follow from skillspector.graph_proxy import graph from skillspector.input_handler import validate_local_input_path from skillspector.inspection_ledger import ( @@ -359,7 +360,7 @@ def _write_result( """Write report_body to file or stdout. Uses sarif_report if report_body missing.""" report_body = _result_body(result) if output: - Path(output).write_text(report_body, encoding="utf-8") + write_text_no_follow(output, report_body) if format == FormatChoice.terminal: console.print(f"\n[green]Report saved to:[/green] {output}") else: @@ -693,7 +694,7 @@ def scan( ) report = json.dumps(result, indent=2) if output: - output.write_text(report, encoding="utf-8") + write_text_no_follow(output, report) console.print(f"Report saved to: {output}") else: print(report) @@ -3232,7 +3233,7 @@ def _scan_multi_skill( rendered = json.dumps(combined, indent=2) _ensure_recursive_output_bound(rendered) if output is not None: - Path(output).write_text(rendered, encoding="utf-8") + write_text_no_follow(output, rendered) progress_console.print(f"[green]Combined report saved to:[/green] {output}") else: sys.stdout.write(rendered) @@ -3260,7 +3261,7 @@ def _scan_multi_skill( rendered = json.dumps(merged_sarif, indent=2) _ensure_recursive_output_bound(rendered) if output is not None: - Path(output).write_text(rendered, encoding="utf-8") + write_text_no_follow(output, rendered) progress_console.print(f"[green]Combined report saved to:[/green] {output}") else: sys.stdout.write(rendered) @@ -3293,7 +3294,7 @@ def _scan_multi_skill( ) _ensure_recursive_output_bound(rendered) if output is not None: - Path(output).write_text(rendered, encoding="utf-8") + write_text_no_follow(output, rendered) progress_console.print(f"[green]Combined report saved to:[/green] {output}") elif format is FormatChoice.terminal: console.print(rendered) diff --git a/src/skillspector/file_output.py b/src/skillspector/file_output.py new file mode 100644 index 000000000..e669bf35e --- /dev/null +++ b/src/skillspector/file_output.py @@ -0,0 +1,79 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Write reports without following attacker-replaced paths.""" + +from __future__ import annotations + +import os +from pathlib import Path +from secrets import token_hex +from stat import S_ISREG + +from skillspector.input_handler import _normalize_root_owned_alias + +_SECURE_OUTPUT_SUPPORTED = ( + hasattr(os, "O_NOFOLLOW") + and hasattr(os, "O_DIRECTORY") + and {os.open, os.stat, os.rename, os.unlink} <= os.supports_dir_fd + and os.stat in os.supports_follow_symlinks +) + + +def write_text_no_follow(path: str | Path, text: str) -> None: + """Atomically replace a regular output through an anchored parent descriptor. + + Files are created privately, including replacements. A final-component swap + cannot redirect the write; a parent swap cannot change the opened directory. + Platforms without these guarantees must use explicitly managed stdout. + """ + if not _SECURE_OUTPUT_SUPPORTED: + raise ValueError("Safe file output is unsupported on this platform; use stdout redirection.") + absolute = _normalize_root_owned_alias(Path(path)) + flags = os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_PATH", os.O_RDONLY) + directory_fd = os.open(absolute.anchor, flags) + temporary_name = None + try: + for part in absolute.parts[1:-1]: + next_fd = os.open(part, flags, dir_fd=directory_fd) + os.close(directory_fd) + directory_fd = next_fd + try: + existing = os.stat(absolute.name, dir_fd=directory_fd, follow_symlinks=False) + except FileNotFoundError: + pass + else: + if not S_ISREG(existing.st_mode): + raise ValueError("Refusing to overwrite a non-regular output file.") + candidate = f".skillspector-output-{token_hex(16)}" + fd = os.open( + candidate, os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, + 0o600, dir_fd=directory_fd, + ) + temporary_name = candidate + with os.fdopen(fd, "w", encoding="utf-8") as stream: + stream.write(text) + os.replace( + temporary_name, absolute.name, + src_dir_fd=directory_fd, dst_dir_fd=directory_fd, + ) + temporary_name = None + finally: + if temporary_name is not None: + try: + os.unlink(temporary_name, dir_fd=directory_fd) + except FileNotFoundError: + pass + os.close(directory_fd) diff --git a/src/skillspector/suppression.py b/src/skillspector/suppression.py index 61809eb35..c4d331e26 100644 --- a/src/skillspector/suppression.py +++ b/src/skillspector/suppression.py @@ -69,6 +69,7 @@ import yaml +from skillspector.file_output import write_text_no_follow from skillspector.logging_config import get_logger from skillspector.models import Finding @@ -646,10 +647,10 @@ def dump_baseline(data: dict[str, object], path: str | Path) -> None: """Write a baseline mapping to *path* as YAML (``.json`` extension -> JSON).""" p = Path(path) if p.suffix.lower() == ".json": - p.write_text(json.dumps(data, indent=2), encoding="utf-8") + write_text_no_follow(p, json.dumps(data, indent=2)) else: header = ( "# SkillSpector baseline — findings listed here are suppressed on future scans.\n" "# Edit 'reason' fields and add glob 'rules' as needed. See docs/SUPPRESSION.md.\n" ) - p.write_text(header + yaml.safe_dump(data, sort_keys=False), encoding="utf-8") + write_text_no_follow(p, header + yaml.safe_dump(data, sort_keys=False)) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 820683d68..fd58074e2 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -7060,3 +7060,86 @@ def test_recursive_sarif_uses_real_encoded_directory_and_preserves_external_sour local = locations[1]["physicalLocation"]["artifactLocation"] resolved = urljoin(urljoin(bases["SCANROOT"]["uri"], bases["SKILLROOT"]["uri"]), local["uri"]) assert Path(unquote(urlsplit(resolved).path)) == skill.path / "scripts/helper.py" + + +@pytest.mark.skipif(os.name != "posix", reason="descriptor-relative output requires POSIX") +@pytest.mark.parametrize("race", ["leaf", "parent-before-open", "parent-after-open", "hard-link"]) +def test_report_output_cannot_redirect_to_external_file(tmp_path, monkeypatch, race): + from skillspector import file_output + + parent = tmp_path / "reports" + parent.mkdir() + moved = tmp_path / "original-reports" + outside = tmp_path / "outside" + outside.mkdir() + protected = outside / "report.json" + protected.write_text("protected", encoding="utf-8") + output = parent / protected.name + original_open = os.open + original_replace = os.replace + if race == "hard-link": + os.link(protected, output) + elif race == "leaf": + def replace_after_swap(src, dst, **kwargs): + output.symlink_to(protected) + return original_replace(src, dst, **kwargs) + monkeypatch.setattr(file_output.os, "replace", replace_after_swap) + else: + def open_after_swap(path, flags, *args, **kwargs): + if path == parent.name: + if race == "parent-after-open": + fd = original_open(path, flags, *args, **kwargs) + parent.rename(moved) + parent.symlink_to(outside, target_is_directory=True) + if race == "parent-after-open": + return fd + return original_open(path, flags, *args, **kwargs) + monkeypatch.setattr(file_output.os, "open", open_after_swap) + if race == "parent-before-open": + with pytest.raises(OSError): + file_output.write_text_no_follow(output, "report") + else: + file_output.write_text_no_follow(output, "report") + written = (moved if race == "parent-after-open" else parent) / output.name + assert written.read_text(encoding="utf-8") == "report" + assert written.stat().st_mode & 0o777 == 0o600 + assert not written.is_symlink() + assert protected.read_text(encoding="utf-8") == "protected" + + +@pytest.mark.skipif(os.name != "posix", reason="descriptor-relative output requires POSIX") +def test_report_output_rejects_symlinks_and_cleans_failed_replace(tmp_path, monkeypatch): + from skillspector import file_output + + protected = tmp_path / "protected" + protected.write_text("original", encoding="utf-8") + output = tmp_path / "report" + output.symlink_to(protected) + with pytest.raises(ValueError, match="non-regular"): + file_output.write_text_no_follow(output, "report") + output.unlink() + output.write_text("previous", encoding="utf-8") + def fail_replace(*args, **kwargs): + raise PermissionError("synthetic replacement failure") + monkeypatch.setattr(file_output.os, "replace", fail_replace) + with pytest.raises(PermissionError): + file_output.write_text_no_follow(output, "report") + assert output.read_text(encoding="utf-8") == "previous" + assert protected.read_text(encoding="utf-8") == "original" + assert not list(tmp_path.glob(".skillspector-output-*")) + + +def test_report_output_unsupported_platform_keeps_stdout(tmp_path, monkeypatch): + from skillspector import file_output + + monkeypatch.setattr(file_output, "_SECURE_OUTPUT_SUPPORTED", False) + skill = tmp_path / "SKILL.md" + skill.write_text("---\nname: safe\n---\nHello", encoding="utf-8") + monkeypatch.setattr(cli, "_scan_skill", lambda **kwargs: {"report_body": "report", "risk_score": 0}) + result = runner.invoke(app, ["scan", str(skill), "--no-llm", "--output", str(tmp_path / "report")]) + assert result.exit_code == 2 + assert "use stdout redirection" in result.output + assert not (tmp_path / "report").exists() + result = runner.invoke(app, ["scan", str(skill), "--no-llm", "--format", "json"]) + assert result.exit_code == 0 + assert result.stdout.strip() == "report" From dd1c2fb64c031d392f086f0e6aebb04197fae50d Mon Sep 17 00:00:00 2001 From: yashrajbasav Date: Wed, 7 Oct 2026 13:06:55 +0530 Subject: [PATCH 2/6] Format secure report writer and race checks Signed-off-by: yashrajbasav --- src/skillspector/file_output.py | 16 +++++++++++----- tests/unit/test_cli.py | 14 ++++++++++++-- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/skillspector/file_output.py b/src/skillspector/file_output.py index e669bf35e..0b3b7bb4d 100644 --- a/src/skillspector/file_output.py +++ b/src/skillspector/file_output.py @@ -40,7 +40,9 @@ def write_text_no_follow(path: str | Path, text: str) -> None: Platforms without these guarantees must use explicitly managed stdout. """ if not _SECURE_OUTPUT_SUPPORTED: - raise ValueError("Safe file output is unsupported on this platform; use stdout redirection.") + raise ValueError( + "Safe file output is unsupported on this platform; use stdout redirection." + ) absolute = _normalize_root_owned_alias(Path(path)) flags = os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_PATH", os.O_RDONLY) directory_fd = os.open(absolute.anchor, flags) @@ -59,15 +61,19 @@ def write_text_no_follow(path: str | Path, text: str) -> None: raise ValueError("Refusing to overwrite a non-regular output file.") candidate = f".skillspector-output-{token_hex(16)}" fd = os.open( - candidate, os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, - 0o600, dir_fd=directory_fd, + candidate, + os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW, + 0o600, + dir_fd=directory_fd, ) temporary_name = candidate with os.fdopen(fd, "w", encoding="utf-8") as stream: stream.write(text) os.replace( - temporary_name, absolute.name, - src_dir_fd=directory_fd, dst_dir_fd=directory_fd, + temporary_name, + absolute.name, + src_dir_fd=directory_fd, + dst_dir_fd=directory_fd, ) temporary_name = None finally: diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index fd58074e2..29e178b4d 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -7080,11 +7080,14 @@ def test_report_output_cannot_redirect_to_external_file(tmp_path, monkeypatch, r if race == "hard-link": os.link(protected, output) elif race == "leaf": + def replace_after_swap(src, dst, **kwargs): output.symlink_to(protected) return original_replace(src, dst, **kwargs) + monkeypatch.setattr(file_output.os, "replace", replace_after_swap) else: + def open_after_swap(path, flags, *args, **kwargs): if path == parent.name: if race == "parent-after-open": @@ -7094,6 +7097,7 @@ def open_after_swap(path, flags, *args, **kwargs): if race == "parent-after-open": return fd return original_open(path, flags, *args, **kwargs) + monkeypatch.setattr(file_output.os, "open", open_after_swap) if race == "parent-before-open": with pytest.raises(OSError): @@ -7119,8 +7123,10 @@ def test_report_output_rejects_symlinks_and_cleans_failed_replace(tmp_path, monk file_output.write_text_no_follow(output, "report") output.unlink() output.write_text("previous", encoding="utf-8") + def fail_replace(*args, **kwargs): raise PermissionError("synthetic replacement failure") + monkeypatch.setattr(file_output.os, "replace", fail_replace) with pytest.raises(PermissionError): file_output.write_text_no_follow(output, "report") @@ -7135,8 +7141,12 @@ def test_report_output_unsupported_platform_keeps_stdout(tmp_path, monkeypatch): monkeypatch.setattr(file_output, "_SECURE_OUTPUT_SUPPORTED", False) skill = tmp_path / "SKILL.md" skill.write_text("---\nname: safe\n---\nHello", encoding="utf-8") - monkeypatch.setattr(cli, "_scan_skill", lambda **kwargs: {"report_body": "report", "risk_score": 0}) - result = runner.invoke(app, ["scan", str(skill), "--no-llm", "--output", str(tmp_path / "report")]) + monkeypatch.setattr( + cli, "_scan_skill", lambda **kwargs: {"report_body": "report", "risk_score": 0} + ) + result = runner.invoke( + app, ["scan", str(skill), "--no-llm", "--output", str(tmp_path / "report")] + ) assert result.exit_code == 2 assert "use stdout redirection" in result.output assert not (tmp_path / "report").exists() From cb1855d8f6d7677b4f24d475ef6d0685aafade8a Mon Sep 17 00:00:00 2001 From: yashrajbasav Date: Fri, 9 Oct 2026 11:12:11 +0530 Subject: [PATCH 3/6] fix(output): validate support early and preserve stdout workflows Signed-off-by: yashrajbasav --- README.md | 11 ++- contrib/batch_scan/batch_scan.py | 39 +++++++--- docs/SUPPRESSION.md | 7 ++ src/skillspector/cli.py | 30 ++++++-- src/skillspector/file_output.py | 37 +++++++-- tests/test_batch_scan_security.py | 58 ++++++++++++++ tests/unit/test_cli.py | 123 +++++++++++++++++++++++++++++- 7 files changed, 273 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 40fe48c16..e3eeb1b7d 100644 --- a/README.md +++ b/README.md @@ -116,7 +116,7 @@ docker run --rm \ **Write a report to the host filesystem** by writing to the mounted directory: ```bash -docker run --rm \ +docker run --rm --user "$(id -u):$(id -g)" \ -v "$PWD:/scan" \ skillspector scan ./my-skill/ --no-llm --format json --output report.json ``` @@ -776,6 +776,15 @@ Options: skillspector baseline [-o FILE] [--no-llm] [--reason TEXT] ``` +Report and baseline files are replaced atomically with private permissions (0600), +including existing files. Their parent directory must be writable; symlinked +parents and non-regular destinations such as devices are refused. On macOS, +ancestor directories must also be readable. File output requires POSIX no-follow +directory operations and is unavailable on Windows. Unsupported file output fails +before analysis. Omit `--output` for scan/batch stdout, or use +`skillspector baseline -o -` for JSON on stdout, then redirect only to a +trusted destination. Run Docker with your user ID as above so you can read its files. + ## Integrating SkillSpector SkillSpector is built to be driven by other tools (CI pipelines, install gates, editor integrations). Its exit code and JSON output are a stable contract. diff --git a/contrib/batch_scan/batch_scan.py b/contrib/batch_scan/batch_scan.py index 50e82a65a..86c4cbc23 100644 --- a/contrib/batch_scan/batch_scan.py +++ b/contrib/batch_scan/batch_scan.py @@ -78,6 +78,7 @@ from types import SimpleNamespace from uuid import uuid4 +from skillspector.file_output import require_secure_file_output, write_text_no_follow from skillspector.logging_config import set_level from .api_pool import create_api_key_pool_from_env @@ -155,8 +156,10 @@ def _kill_worker_group(pid: int) -> None: try: subprocess.run( ["taskkill", "/PID", str(pid), "/T", "/F"], - stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, - timeout=5, check=False, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + timeout=5, + check=False, ) except (OSError, subprocess.TimeoutExpired): pass @@ -201,8 +204,13 @@ def _scan_skill_process( def _scan_skill_bounded( - skill_dir: Path, root: Path, *, api_pool=None, timeout: float = 90, - startup_timeout: float = 90, **options + skill_dir: Path, + root: Path, + *, + api_pool=None, + timeout: float = 90, + startup_timeout: float = 90, + **options, ) -> tuple[dict[str, object], str | None, str]: """Enforce the wall-clock limit on actual work, including local analysis.""" owner = uuid4().hex @@ -365,6 +373,13 @@ def _print(*args: object, **kwargs: object) -> None: ) args = parser.parse_args() + if args.output: + try: + require_secure_file_output() + except ValueError as exc: + _print(f"Error: {exc}", markup=False, file=sys.stderr) + sys.exit(2) + if args.verbose: set_level("DEBUG") @@ -389,8 +404,7 @@ def _print(*args: object, **kwargs: object) -> None: # -- Header -------------------------------------------------------------- pool_note = ( - f", [green]{api_pool.keys_configured} keys " - f"({api_pool.total_capacity} slots)[/green]" + f", [green]{api_pool.keys_configured} keys ({api_pool.total_capacity} slots)[/green]" if api_pool else "" ) @@ -501,13 +515,10 @@ def _print(*args: object, **kwargs: object) -> None: f"{snap['total_requests_served']} requests served", ] if snap.get("peak_active_requests", 0) > 0: - _parts.append( - f"peak {snap['peak_active_requests']}/{snap['total_capacity']} slots" - ) + _parts.append(f"peak {snap['peak_active_requests']}/{snap['total_capacity']} slots") if snap.get("rate_limits_hit", 0) > 0: _parts.append( - f"{snap['rate_limits_hit']} rate-limit(s), " - f"{snap['retry_successes']} retried" + f"{snap['rate_limits_hit']} rate-limit(s), {snap['retry_successes']} retried" ) _parts.append(f"{snap['keys_configured']} keys") _print(f"\n[dim]API Pool: {', '.join(_parts)}[/dim]") @@ -522,7 +533,11 @@ def _print(*args: object, **kwargs: object) -> None: report_body = format_markdown(results) if args.output: - args.output.write_text(report_body, encoding="utf-8") + try: + write_text_no_follow(args.output, report_body) + except (OSError, ValueError) as exc: + _print(f"Error: {exc}", markup=False, file=sys.stderr) + sys.exit(2) _print(f"\n[green]Batch report saved to:[/green] {display(args.output)}") else: if fmt == "terminal": diff --git a/docs/SUPPRESSION.md b/docs/SUPPRESSION.md index 9a7065ca2..5e13f0d60 100644 --- a/docs/SUPPRESSION.md +++ b/docs/SUPPRESSION.md @@ -45,6 +45,13 @@ prevents sensitive rule text from creating a finding against itself or entering regenerated fingerprints. Other baseline files and sibling YAML/JSON files remain in normal scan scope unless they are selected with `--baseline` or `-o`. +Use `skillspector baseline ./my-skill/ -o -` to emit JSON on stdout. This is +available on Windows too; redirect only to a trusted destination. File output +requires POSIX no-follow directory operations and fails before analysis when +unsupported. Baselines are replaced atomically with mode 0600, including existing +files. The parent must be writable, symlinked parents and non-regular files are +refused, and macOS also requires readable ancestor directories. + ## Baseline file format YAML or JSON (the `.json` extension selects JSON output when generating). Two diff --git a/src/skillspector/cli.py b/src/skillspector/cli.py index 29fa158f1..dbd8a73f7 100644 --- a/src/skillspector/cli.py +++ b/src/skillspector/cli.py @@ -46,7 +46,7 @@ from skillspector import __version__, transitive from skillspector.cleanup import TempDirTracker, cleanup_result from skillspector.constants import RISK_THRESHOLD -from skillspector.file_output import write_text_no_follow +from skillspector.file_output import require_secure_file_output, write_text_no_follow from skillspector.graph_proxy import graph from skillspector.input_handler import validate_local_input_path from skillspector.inspection_ledger import ( @@ -667,6 +667,13 @@ def scan( ) raise typer.Exit(code=2) + if output is not None: + try: + require_secure_file_output() + except ValueError as exc: + err_console.print(f"[red]Error:[/red] {exc}") + raise typer.Exit(code=2) from exc + if mcp_registry: if ( recursive @@ -3401,7 +3408,7 @@ def baseline( typer.Option( "--output", "-o", - help="Where to write the baseline file (YAML; .json extension writes JSON).", + help="Baseline file (YAML; .json writes JSON), or - for JSON on stdout.", ), ] = Path(".skillspector-baseline.yaml"), no_llm: Annotated[ @@ -3437,12 +3444,16 @@ def baseline( """ result = None try: + to_stdout = str(output) == "-" + if not to_stdout: + require_secure_file_output() if verbose: set_level("DEBUG") - console.print("[dim]Scanning to build baseline...[/dim]") + err_console.print("[dim]Scanning to build baseline...[/dim]") # output_format is irrelevant here; we consume findings, not report_body. state = _scan_state(input_path, FormatChoice.json, no_llm) - state["baseline_path"] = os.path.abspath(output.expanduser()) + if not to_stdout: + state["baseline_path"] = os.path.abspath(output.expanduser()) result = graph.invoke(state) # Fingerprint every occurrence the next scan checks. The reported # findings are deduplicated and keep only one occurrence's evidence. @@ -3456,10 +3467,13 @@ def baseline( file_cache=result.get("local_file_cache") or result.get("file_cache") or {}, scanner_version=__version__, ) - dump_baseline(data, output) - console.print( - f"[green]Wrote baseline with {len(findings)} suppressed finding(s) to:[/green] {output}" - ) + if to_stdout: + sys.stdout.write(json.dumps(data, indent=2) + "\n") + else: + dump_baseline(data, output) + console.print( + f"[green]Wrote baseline with {len(findings)} suppressed finding(s) to:[/green] {output}" + ) except typer.Exit: raise except (FileNotFoundError, ValueError) as e: diff --git a/src/skillspector/file_output.py b/src/skillspector/file_output.py index 0b3b7bb4d..bc51d3c69 100644 --- a/src/skillspector/file_output.py +++ b/src/skillspector/file_output.py @@ -17,10 +17,11 @@ from __future__ import annotations +import errno import os from pathlib import Path from secrets import token_hex -from stat import S_ISREG +from stat import S_ISREG, filemode from skillspector.input_handler import _normalize_root_owned_alias @@ -32,6 +33,16 @@ ) +def require_secure_file_output() -> None: + """Reject unsupported file writes before starting potentially costly scans.""" + if not _SECURE_OUTPUT_SUPPORTED: + raise ValueError( + "Safe file output is unsupported on this platform. " + "For scan or batch reports, omit --output; for baselines, use --output -. " + "Redirect stdout only to a destination you trust." + ) + + def write_text_no_follow(path: str | Path, text: str) -> None: """Atomically replace a regular output through an anchored parent descriptor. @@ -39,15 +50,13 @@ def write_text_no_follow(path: str | Path, text: str) -> None: cannot redirect the write; a parent swap cannot change the opened directory. Platforms without these guarantees must use explicitly managed stdout. """ - if not _SECURE_OUTPUT_SUPPORTED: - raise ValueError( - "Safe file output is unsupported on this platform; use stdout redirection." - ) + require_secure_file_output() absolute = _normalize_root_owned_alias(Path(path)) flags = os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_PATH", os.O_RDONLY) - directory_fd = os.open(absolute.anchor, flags) + directory_fd = None temporary_name = None try: + directory_fd = os.open(absolute.anchor, flags) for part in absolute.parts[1:-1]: next_fd = os.open(part, flags, dir_fd=directory_fd) os.close(directory_fd) @@ -58,7 +67,10 @@ def write_text_no_follow(path: str | Path, text: str) -> None: pass else: if not S_ISREG(existing.st_mode): - raise ValueError("Refusing to overwrite a non-regular output file.") + raise ValueError( + f"Refusing to overwrite non-regular output {str(path)!r} " + f"(type {filemode(existing.st_mode)[0]!r})." + ) candidate = f".skillspector-output-{token_hex(16)}" fd = os.open( candidate, @@ -76,10 +88,19 @@ def write_text_no_follow(path: str | Path, text: str) -> None: dst_dir_fd=directory_fd, ) temporary_name = None + except OSError as exc: + if exc.errno in {errno.ELOOP, errno.ENOTDIR}: + detail = "an output directory is a symlink or is not a directory" + elif isinstance(exc, FileNotFoundError): + detail = "the output directory does not exist" + else: + detail = exc.strerror or str(exc) + raise ValueError(f"Could not write output {str(path)!r}: {detail}.") from exc finally: if temporary_name is not None: try: os.unlink(temporary_name, dir_fd=directory_fd) except FileNotFoundError: pass - os.close(directory_fd) + if directory_fd is not None: + os.close(directory_fd) diff --git a/tests/test_batch_scan_security.py b/tests/test_batch_scan_security.py index b1c7577f0..726e38326 100644 --- a/tests/test_batch_scan_security.py +++ b/tests/test_batch_scan_security.py @@ -541,3 +541,61 @@ def test_scan_forwards_verbose_logging(batch_skill, monkeypatch): skill, skill.parent, use_llm=False, lang="en", require_llm=False, verbose=True ) assert levels == ["DEBUG"] + + +@pytest.mark.parametrize("kind", ["symlink", "hard-link", "unsupported"]) +def test_batch_report_uses_safe_output(tmp_path, monkeypatch, kind): + from skillspector import file_output + + skill = tmp_path / "skill" + skill.mkdir() + (skill / "SKILL.md").write_text("# Safe skill", encoding="utf-8") + protected = tmp_path / "protected" + protected.write_text("preserve", encoding="utf-8") + output = tmp_path / "report.json" + if kind == "unsupported": + monkeypatch.setattr(file_output, "_SECURE_OUTPUT_SUPPORTED", False) + elif kind == "symlink": + output.symlink_to(protected) + else: + os.link(protected, output) + calls = [] + + def scan(*args, **kwargs): + calls.append(True) + return ( + { + "skill": {"name": "safe"}, + "risk_assessment": {"score": 0, "severity": "LOW"}, + "issues": [], + }, + None, + "safe", + ) + + monkeypatch.setattr(batch_scan, "_scan_skill_bounded", scan) + monkeypatch.setattr(batch_scan, "create_api_key_pool_from_env", lambda: None) + monkeypatch.setattr( + sys, + "argv", + [ + "batch_scan", + str(tmp_path), + "--no-llm", + "--workers", + "1", + "-f", + "json", + "-o", + str(output), + ], + ) + if kind == "hard-link": + batch_scan.main() + assert output.read_text(encoding="utf-8") != "preserve" + else: + with pytest.raises(SystemExit) as error: + batch_scan.main() + assert error.value.code == 2 + assert protected.read_text(encoding="utf-8") == "preserve" + assert bool(calls) == (kind != "unsupported") diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 29e178b4d..fd5927838 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -7100,7 +7100,7 @@ def open_after_swap(path, flags, *args, **kwargs): monkeypatch.setattr(file_output.os, "open", open_after_swap) if race == "parent-before-open": - with pytest.raises(OSError): + with pytest.raises(ValueError, match="symlink or is not a directory"): file_output.write_text_no_follow(output, "report") else: file_output.write_text_no_follow(output, "report") @@ -7128,7 +7128,7 @@ def fail_replace(*args, **kwargs): raise PermissionError("synthetic replacement failure") monkeypatch.setattr(file_output.os, "replace", fail_replace) - with pytest.raises(PermissionError): + with pytest.raises(ValueError, match="synthetic replacement failure"): file_output.write_text_no_follow(output, "report") assert output.read_text(encoding="utf-8") == "previous" assert protected.read_text(encoding="utf-8") == "original" @@ -7148,8 +7148,125 @@ def test_report_output_unsupported_platform_keeps_stdout(tmp_path, monkeypatch): app, ["scan", str(skill), "--no-llm", "--output", str(tmp_path / "report")] ) assert result.exit_code == 2 - assert "use stdout redirection" in result.output + assert "unsupported on this platform" in " ".join(result.output.split()) assert not (tmp_path / "report").exists() result = runner.invoke(app, ["scan", str(skill), "--no-llm", "--format", "json"]) assert result.exit_code == 0 assert result.stdout.strip() == "report" + + +@pytest.mark.parametrize( + "mode", ["single", "recursive", "registry", "baseline-default", "baseline-json"] +) +def test_unsupported_output_stops_before_analysis(tmp_path, monkeypatch, mode): + from skillspector import file_output + + monkeypatch.setattr(file_output, "_SECURE_OUTPUT_SUPPORTED", False) + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("COLUMNS", "79") + + def forbidden(*args, **kwargs): + pytest.fail("analysis must not run for unsupported output") + + monkeypatch.setattr(cli, "_scan_skill", forbidden) + monkeypatch.setattr(cli, "detect_skills", forbidden) + monkeypatch.setattr(cli, "scan_registry", forbidden) + monkeypatch.setattr(cli.graph, "invoke", forbidden) + args = ["baseline" if mode.startswith("baseline") else "scan", str(tmp_path), "--no-llm"] + if mode == "recursive": + args += ["--recursive"] + if mode == "registry": + args += ["--mcp-registry", "--format", "json"] + if mode != "baseline-default": + args += ["--output", "out.json"] + result = runner.invoke(app, args) + assert result.exit_code == 2 + assert "unsupported on this platform" in " ".join(result.output.split()) + assert not list(tmp_path.iterdir()) + + +def test_baseline_stdout_works_without_secure_file_output(tmp_path, monkeypatch): + from skillspector import file_output + + monkeypatch.setattr(file_output, "_SECURE_OUTPUT_SUPPORTED", False) + monkeypatch.chdir(tmp_path) + states = [] + + def invoke(state): + states.append(state) + return {"active_findings": [], "file_cache": {}, "risk_score": 0} + + monkeypatch.setattr(cli.graph, "invoke", invoke) + result = runner.invoke(app, ["baseline", str(tmp_path), "--no-llm", "-o", "-", "--verbose"]) + assert result.exit_code == 0, result.output + assert json.loads(result.stdout)["fingerprints"] == [] + assert "baseline_path" not in states[0] + assert not list(tmp_path.iterdir()) + + +@pytest.mark.skipif(os.name != "posix", reason="descriptor-relative output requires POSIX") +@pytest.mark.parametrize( + "mode", ["baseline-default", "baseline-json", "registry", "json", "sarif", "markdown"] +) +def test_all_cli_writers_reject_symlink_destinations(tmp_path, monkeypatch, mode): + monkeypatch.chdir(tmp_path) + protected = tmp_path / "protected" + protected.write_text("preserve", encoding="utf-8") + output = tmp_path / ( + ".skillspector-baseline.yaml" if mode == "baseline-default" else "out.json" + ) + output.symlink_to(protected.name) + skill = tmp_path / "skill" + skill.mkdir() + (skill / "SKILL.md").write_text("# Safe skill", encoding="utf-8") + if mode.startswith("baseline"): + monkeypatch.setattr( + cli.graph, + "invoke", + lambda state: {"active_findings": [], "file_cache": {}, "risk_score": 0}, + ) + args = ["baseline", str(skill), "--no-llm"] + elif mode == "registry": + monkeypatch.setattr( + cli, "scan_registry", lambda *args, **kwargs: {"findings": [], "risk_score": 0} + ) + args = ["scan", "registry.json", "--mcp-registry", "--format", "json"] + else: + monkeypatch.setattr( + cli, + "detect_skills", + lambda root: MultiSkillDetectionResult( + is_multi_skill=True, + has_root_skill=False, + skills=[SkillDirectory(path=skill, name="skill", relative_path="skill")], + ), + ) + monkeypatch.setattr( + cli, "_scan_skill", lambda *args, **kwargs: _bounded_recursive_result("safe") + ) + args = ["scan", str(tmp_path), "--recursive", "--no-llm", "--format", mode] + if mode != "baseline-default": + args += ["--output", str(output)] + result = runner.invoke(app, args) + assert result.exit_code == 2, result.output + assert "non-regular" in result.output + assert protected.read_text(encoding="utf-8") == "preserve" + assert output.is_symlink() + + +@pytest.mark.skipif(os.name != "posix", reason="descriptor-relative output requires POSIX") +@pytest.mark.parametrize("kind", ["missing", "symlink", "file", "device"]) +def test_output_errors_name_the_requested_path(tmp_path, kind): + from skillspector.file_output import write_text_no_follow + + output = tmp_path / "parent" / "report.json" + if kind == "symlink": + (tmp_path / "parent").symlink_to(tmp_path, target_is_directory=True) + elif kind == "file": + (tmp_path / "parent").write_text("not a directory", encoding="utf-8") + elif kind == "device": + output = Path(os.devnull) + with pytest.raises(ValueError) as error: + write_text_no_follow(output, "report") + assert str(output) in str(error.value) + assert not list(tmp_path.glob(".skillspector-output-*")) From 55f760078c5bce3760c3edf59922eb7c07244b21 Mon Sep 17 00:00:00 2001 From: yashrajbasav Date: Fri, 9 Oct 2026 11:15:30 +0530 Subject: [PATCH 4/6] test(batch): supply language in output fixture Signed-off-by: yashrajbasav --- tests/test_batch_scan_security.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_batch_scan_security.py b/tests/test_batch_scan_security.py index 726e38326..5e652c2fc 100644 --- a/tests/test_batch_scan_security.py +++ b/tests/test_batch_scan_security.py @@ -565,7 +565,7 @@ def scan(*args, **kwargs): calls.append(True) return ( { - "skill": {"name": "safe"}, + "skill": {"name": "safe", "language": "en"}, "risk_assessment": {"score": 0, "severity": "LOW"}, "issues": [], }, From a54a401d423b7ca20eeef95e9f132c017291bd55 Mon Sep 17 00:00:00 2001 From: yashrajbasav Date: Fri, 9 Oct 2026 13:49:21 +0530 Subject: [PATCH 5/6] test: retain baseline regular-file error contract Signed-off-by: yashrajbasav --- tests/unit/test_cli.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 185deb708..ac90b20a3 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -7371,7 +7371,8 @@ def test_all_cli_writers_reject_symlink_destinations(tmp_path, monkeypatch, mode args += ["--output", str(output)] result = runner.invoke(app, args) assert result.exit_code == 2, result.output - assert "non-regular" in result.output + expected_error = "must be a regular file" if mode.startswith("baseline") else "non-regular" + assert expected_error in result.output assert protected.read_text(encoding="utf-8") == "preserve" assert output.is_symlink() From 791f919ea9d3601b8f6a678f46ed6467f045ae7a Mon Sep 17 00:00:00 2001 From: yashrajbasav Date: Fri, 9 Oct 2026 13:52:39 +0530 Subject: [PATCH 6/6] fix: tolerate bounded concurrent baseline replacements Signed-off-by: yashrajbasav --- src/skillspector/suppression.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/skillspector/suppression.py b/src/skillspector/suppression.py index 915649e66..7b46e09c3 100644 --- a/src/skillspector/suppression.py +++ b/src/skillspector/suppression.py @@ -85,7 +85,8 @@ MAX_BASELINE_RECORDS = 10_000 MAX_BASELINE_SCALAR_CHARS = 64 * 1024 # Validation passes for an output path that concurrent writers replace. -_BASELINE_DESTINATION_ATTEMPTS = 2 +# Allow a small burst of cooperating atomic writers while bounding hostile swaps. +_BASELINE_DESTINATION_ATTEMPTS = 8 _FINGERPRINT_SCHEMA = "skillspector-finding-fingerprint-v2" _FINGERPRINT_RE = re.compile(r"sha256:[0-9a-f]{64}\Z") _SOURCE_IDENTITY_RE = re.compile(r"external/[0-9a-f]{64}\Z")