diff --git a/README.md b/README.md index c6dadddb9..8bf5e384f 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 ``` @@ -792,6 +792,17 @@ Options: skillspector baseline [-o FILE] [--no-llm] [--reason TEXT] ``` +Report files and new baselines use private permissions (0600). Existing baselines +retain their access metadata when safely replaced; updates requiring in-place +writes are refused. All file output uses atomic replacement. The 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 9ed418988..3bce84c8d 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 @@ -157,8 +158,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 @@ -203,8 +206,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.""" options["timeout"] = timeout @@ -367,6 +375,13 @@ def _print(*args: object, **kwargs: object) -> None: else: print(" ".join(str(a) for a in args), file=file) # type: ignore[arg-type] + 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") @@ -391,8 +406,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 "" ) @@ -503,13 +517,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]") @@ -524,7 +535,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 df319d186..1705df77f 100644 --- a/docs/SUPPRESSION.md +++ b/docs/SUPPRESSION.md @@ -55,6 +55,20 @@ 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 to a **new file outside the scan target**, +then replace an existing baseline only after the command exits with code 0. +Redirecting directly over the old file truncates it before the scan starts, +even when scanning fails. On Windows PowerShell 5.1, use `cmd /c` for the +redirection or `Out-File -Encoding utf8`; its default `>` encoding is UTF-16LE, +which the baseline loader does not accept. Check `$LASTEXITCODE` before replacing +the old baseline. File output +requires POSIX no-follow directory operations and fails before analysis when +unsupported. New baselines use mode 0600; existing baselines retain access metadata +when safely replaced. Updates requiring in-place writes are refused. 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 @@ -124,18 +138,21 @@ Only identical exact fingerprints share an entry. A baseline generated by an affected version may omit these occurrences; regenerate it to include them. If the complete baseline exceeds the loader's size or record limits, generation fails before replacing the output file instead of writing a partial baseline. -After validation, generation normally writes a complete temporary file beside the +After validation, generation writes a complete temporary file beside the destination and atomically replaces the output. A failed write or replacement preserves the existing baseline. Existing destinations must be writable regular files; symlinks and special files are rejected. Replacement requires a writable parent directory. On POSIX, new files grant access only to their owner; existing ordinary permission bits and ownership are preserved, including group-write access. -When a non-owner has write access to an existing file, or the owner cannot -assign the file's group because it is not a member, generation updates its -validated descriptor in place and preserves its ACLs. This shared-file fallback -serializes cooperating writers, but readers can see a partial write and an I/O -failure or interruption can leave a partial baseline. Use an owner-managed output -when atomic replacement is required. +Access ACLs are copied through validated descriptors before any content is written. +A non-owner update, or an update that cannot preserve the destination's group, +fails without changing the existing baseline. Use a new owner-managed output +file or `--output -` redirected to a new file in those cases. Do not redirect +over the shared baseline: that rewrites the inode in place and can change other +hard-linked files. Generation does not fall back to in-place +writes, which would lose the atomic replacement and hard-link safety guarantees. +All path operations use the same no-follow parent descriptor so a directory swap +cannot redirect the write. Every v2 entry must be a mapping with a 64-hex-character `sha256:` hash and a non-empty `reason`. `rule_id` and `file` are informational fields for reviewers. diff --git a/src/skillspector/cli.py b/src/skillspector/cli.py index 6004a2715..c86520896 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 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 ( @@ -83,6 +84,7 @@ dump_baseline, effective_findings, load_baseline, + serialize_baseline, source_content_key, ) from skillspector.terminal_text import visible_terminal_text as _visible_terminal_text @@ -373,7 +375,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: @@ -690,6 +692,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 @@ -720,7 +729,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) @@ -3276,7 +3285,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) @@ -3304,7 +3313,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) @@ -3337,7 +3346,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, markup=False, emoji=False) @@ -3418,7 +3427,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[ @@ -3455,12 +3464,16 @@ def baseline( _exit_on_unsupported_llm_controls(no_llm) 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) completeness_value = result.get("analysis_completeness") completeness = completeness_value if isinstance(completeness_value, dict) else {} @@ -3491,10 +3504,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(serialize_baseline(data, json_output=True) + "\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 new file mode 100644 index 000000000..8e44d0a76 --- /dev/null +++ b/src/skillspector/file_output.py @@ -0,0 +1,142 @@ +# 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 errno +import os +from collections.abc import Iterator +from contextlib import contextmanager +from pathlib import Path +from secrets import token_hex +from stat import S_ISREG, filemode + +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 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 to a new file outside the scan target, and replace " + "an existing report only after the command succeeds." + ) + + +@contextmanager +def open_output_parent(path: str | Path) -> Iterator[tuple[Path, int]]: + """Keep a no-follow parent descriptor open for all operations on an output.""" + 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 = None + try: + 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) + directory_fd = next_fd + 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 open output directory for {str(path)!r}: {detail}." + ) from exc + yield absolute, directory_fd + finally: + if directory_fd is not None: + os.close(directory_fd) + + +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. + """ + temporary_name = None + try: + with open_output_parent(path) as (absolute, directory_fd): + try: + 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( + f"Refusing to overwrite non-regular output {str(path)!r} " + f"(type {filemode(existing.st_mode)[0]!r})." + ) + descriptor = os.open( + absolute.name, + os.O_WRONLY | os.O_NONBLOCK | os.O_NOFOLLOW, + dir_fd=directory_fd, + ) + try: + if not S_ISREG(os.fstat(descriptor).st_mode): + raise ValueError( + f"Refusing to overwrite non-regular output {str(path)!r}." + ) + finally: + os.close(descriptor) + 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 + 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 diff --git a/src/skillspector/suppression.py b/src/skillspector/suppression.py index 5069e58ca..b2eabcaa6 100644 --- a/src/skillspector/suppression.py +++ b/src/skillspector/suppression.py @@ -63,15 +63,16 @@ import posixpath import re import sys -import tempfile from collections.abc import Mapping from dataclasses import dataclass, field from pathlib import Path +from secrets import token_hex from stat import S_IMODE, S_ISREG from typing import Any import yaml +from skillspector.file_output import open_output_parent from skillspector.logging_config import get_logger from skillspector.models import Finding @@ -84,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") @@ -726,43 +728,10 @@ def _preserve_baseline_acl(source: int, destination: int) -> None: os.setxattr(destination, "system.posix_acl_access", acl) -def _write_baseline_in_place( - descriptor: int, encoded: bytes, p: Path, opened: os.stat_result -) -> bool: - """Rewrite a validated inode in place, keeping its owner, group, mode and ACLs. - - Returns False, without writing, when *p* no longer names *opened*. - """ - import fcntl - - fcntl.flock(descriptor, fcntl.LOCK_EX) - current = p.lstat() - if (current.st_dev, current.st_ino) != (opened.st_dev, opened.st_ino): - return False - remaining = memoryview(encoded) - while remaining: - written = os.write(descriptor, remaining) - if written <= 0: - raise OSError(errno.EIO, "Could not write shared baseline", str(p)) - remaining = remaining[written:] - os.ftruncate(descriptor, len(encoded)) - os.fsync(descriptor) - return True - - -def dump_baseline(data: dict[str, object], path: str | Path) -> None: - """Validate and write a regular baseline (``.json`` -> JSON). - - On POSIX, new files have owner-only permissions. Replacements preserve - ownership and ordinary permission bits; the old file must be writable. - Non-owner writers, and owners that cannot assign the destination's group, - update the validated descriptor in place, preserving its permissions and - ACLs without requiring chown. That shared-file path is not atomic for - readers or crash-safe. Symlinks and special files are rejected. - """ +def serialize_baseline(data: dict[str, object], *, json_output: bool = False) -> str: + """Validate baseline output and retain Unicode round trips through its loader.""" baseline_from_dict(data) - p = Path(path) - if p.suffix.lower() == ".json": + if json_output: # PyYAML does not combine JSON's escaped UTF-16 surrogate pairs. Emit # astral characters directly, escaping only genuine lone surrogates. content = ( @@ -789,94 +758,123 @@ def dump_baseline(data: dict[str, object], path: str | Path) -> None: # report fits. Reject it before overwriting an existing, usable baseline. encoded = content.encode("utf-8") if len(encoded) > MAX_BASELINE_BYTES: - raise ValueError(f"Baseline file exceeds byte limit ({MAX_BASELINE_BYTES}): {p}") + raise ValueError(f"Baseline file exceeds byte limit ({MAX_BASELINE_BYTES})") yaml.load(content, Loader=_BoundedBaselineLoader) - destination = None - access_descriptor = None - # A cooperating writer can atomically replace the path between validating - # and opening or locking it. Validate the replacement from the start, but - # only a bounded number of times; a path that keeps changing fails closed. - for attempt in range(1, _BASELINE_DESTINATION_ATTEMPTS + 1): - final_attempt = attempt == _BASELINE_DESTINATION_ATTEMPTS - try: - destination = p.lstat() - except FileNotFoundError: - destination = None - break - if not S_ISREG(destination.st_mode): - raise ValueError(f"Baseline output must be a regular file: {p}") - if os.name == "posix" and os.geteuid() == 0 and not destination.st_mode & 0o222: - raise PermissionError(errno.EACCES, "Baseline output is not writable", str(p)) - # The descriptor check handles ACL grants that mode bits omit. Root - # still observes the explicit read-only mode guard above. - flags = os.O_WRONLY | getattr(os, "O_NONBLOCK", 0) | getattr(os, "O_NOFOLLOW", 0) - descriptor = os.open(p, flags) - try: - opened = os.fstat(descriptor) - if not S_ISREG(opened.st_mode): - raise ValueError(f"Baseline output must be a regular file: {p}") - if (opened.st_dev, opened.st_ino) != (destination.st_dev, destination.st_ino): - if final_attempt: - raise ValueError(f"Baseline output changed while opening: {p}") - continue - destination = opened - if os.name == "posix" and os.geteuid() not in {0, destination.st_uid}: - # Replacing somebody else's writable file would require chown - # and would discard its ACLs. Serialize cooperating shared-file - # writers and keep this already validated inode instead. - if _write_baseline_in_place(descriptor, encoded, p, opened): - return - if final_attempt: - raise ValueError(f"Baseline output changed before writing: {p}") - continue - # Retain the validated inode's access metadata while competing - # atomic writers replace the path. There is no need to reopen it. - access_descriptor = os.dup(descriptor) - break - finally: - os.close(descriptor) + return content + - temporary_path = None +def dump_baseline(data: dict[str, object], path: str | Path) -> None: + """Publish an anchored baseline while preserving existing access metadata. + + New files are private. Existing writable files retain owner, group, mode, + and access ACLs when they can be atomically replaced. Shared-file updates + requiring in-place writes are refused, keeping the old inode untouched. + """ + p = Path(path) + try: + content = serialize_baseline(data, json_output=p.suffix.lower() == ".json") + except ValueError as exc: + raise ValueError(f"{exc}: {p}") from exc + encoded = content.encode("utf-8") try: - with tempfile.NamedTemporaryFile( - mode="wb", dir=p.parent, prefix=".skillspector-baseline.", suffix=".tmp", delete=False - ) as temporary: - temporary_path = Path(temporary.name) - _restrict_baseline_temporary(temporary.fileno()) - if destination is not None: - current = os.fstat(temporary.fileno()) - if (current.st_uid, current.st_gid) != (destination.st_uid, destination.st_gid): + with open_output_parent(p) as (absolute, directory_fd): + destination = None + access_descriptor = None + # A cooperating writer can atomically replace the path between validating + # and opening it. Validate the replacement from the start, but + # only a bounded number of times; a path that keeps changing fails closed. + for attempt in range(1, _BASELINE_DESTINATION_ATTEMPTS + 1): + final_attempt = attempt == _BASELINE_DESTINATION_ATTEMPTS + try: + destination = os.stat(absolute.name, dir_fd=directory_fd, follow_symlinks=False) + except FileNotFoundError: + destination = None + break + if not S_ISREG(destination.st_mode): + raise ValueError(f"Baseline output must be a regular file: {p}") + if os.name == "posix" and os.geteuid() == 0 and not destination.st_mode & 0o222: + raise PermissionError(errno.EACCES, "Baseline output is not writable", str(p)) + # The descriptor check handles ACL grants that mode bits omit. Root + # still observes the explicit read-only mode guard above. + flags = os.O_WRONLY | getattr(os, "O_NONBLOCK", 0) | getattr(os, "O_NOFOLLOW", 0) + descriptor = os.open(absolute.name, flags, dir_fd=directory_fd) + try: + opened = os.fstat(descriptor) + if not S_ISREG(opened.st_mode): + raise ValueError(f"Baseline output must be a regular file: {p}") + if (opened.st_dev, opened.st_ino) != (destination.st_dev, destination.st_ino): + if final_attempt: + raise ValueError(f"Baseline output changed while opening: {p}") + continue + destination = opened + if os.name == "posix" and os.geteuid() not in {0, destination.st_uid}: + raise PermissionError( + errno.EACCES, + "Cannot safely replace a baseline owned by another user; " + "write to a new file or use --output - redirected to a new file", + str(p), + ) + # Retain the validated inode's access metadata while competing + # atomic writers replace the path. There is no need to reopen it. + access_descriptor = os.dup(descriptor) + break + finally: + os.close(descriptor) + + temporary_name = None + try: + candidate = f".skillspector-baseline-{token_hex(16)}.tmp" + descriptor = 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(descriptor, "wb") as temporary: + _restrict_baseline_temporary(temporary.fileno()) + if destination is not None: + current = os.fstat(temporary.fileno()) + if (current.st_uid, current.st_gid) != ( + destination.st_uid, + destination.st_gid, + ): + try: + os.fchown( + temporary.fileno(), destination.st_uid, destination.st_gid + ) + except PermissionError as exc: + raise PermissionError( + errno.EACCES, + "Cannot safely preserve baseline ownership or group; " + "write to a new file or use --output - redirected to a new file", + str(p), + ) from exc + # Keep existing group writers/readers. Newly generated files + # remain private; replacing one does not revoke shared access. + mode = S_IMODE(destination.st_mode) & 0o777 + if os.name == "posix": + os.fchmod(temporary.fileno(), mode) + # Clearing inherited ACLs must not remove a restrictive or + # shared access ACL from the existing destination. + assert access_descriptor is not None + _preserve_baseline_acl(access_descriptor, temporary.fileno()) + # Configure all destination access metadata while the file is empty. + temporary.write(encoded) + temporary.flush() + os.fsync(temporary.fileno()) + os.replace( + temporary_name, absolute.name, src_dir_fd=directory_fd, dst_dir_fd=directory_fd + ) + temporary_name = None + finally: + if access_descriptor is not None: + os.close(access_descriptor) + if temporary_name is not None: try: - os.fchown(temporary.fileno(), destination.st_uid, destination.st_gid) - except PermissionError: - if access_descriptor is None: - raise - # A non-root owner cannot assign a group it is not a - # member of. Rewrite the validated inode, keeping its group. - if not _write_baseline_in_place(access_descriptor, encoded, p, destination): - raise ValueError( - f"Baseline output changed before writing: {p}" - ) from None - return - # Keep existing group writers/readers. Newly generated files - # remain private; replacing one does not revoke shared access. - mode = S_IMODE(destination.st_mode) & 0o777 - if os.name == "posix": - os.fchmod(temporary.fileno(), mode) - # Clearing inherited ACLs must not remove a restrictive or - # shared access ACL from the existing destination. - assert access_descriptor is not None - _preserve_baseline_acl(access_descriptor, temporary.fileno()) - else: - os.chmod(temporary_path, mode) - # Configure all destination access metadata while the file is empty. - temporary.write(encoded) - temporary.flush() - os.fsync(temporary.fileno()) - os.replace(temporary_path, p) - finally: - if access_descriptor is not None: - os.close(access_descriptor) - if temporary_path is not None: - temporary_path.unlink(missing_ok=True) + os.unlink(temporary_name, dir_fd=directory_fd) + except FileNotFoundError: + pass + except OSError as exc: + raise OSError(exc.errno, exc.strerror or str(exc), str(p)) from exc diff --git a/tests/test_batch_scan_security.py b/tests/test_batch_scan_security.py index baf6d319d..c01ea8ab5 100644 --- a/tests/test_batch_scan_security.py +++ b/tests/test_batch_scan_security.py @@ -746,6 +746,64 @@ def test_scan_forwards_verbose_logging(batch_skill, monkeypatch): 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", "language": "en"}, + "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") + + @pytest.mark.parametrize("impact", ["High", "none", None, "catastrophic"]) def test_compat_meta_repairs_soft_fields_before_validation(impact): batch = llm_analyzer_base.Batch(file_path="SKILL.md", content=_SAFE_TEXT) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 6e09b15f2..13c586523 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -7385,6 +7385,270 @@ def test_recursive_sarif_uses_real_encoded_directory_and_preserves_external_sour 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"]) +@pytest.mark.parametrize("writer", ["report", "baseline"]) +def test_report_output_cannot_redirect_to_external_file(tmp_path, monkeypatch, race, writer): + from skillspector import file_output + from skillspector.suppression import dump_baseline + + def write_output(path): + if writer == "baseline": + dump_baseline({"version": 2}, path) + else: + file_output.write_text_no_follow(path, "report") + + 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(ValueError, match="symlink or is not a directory"): + write_output(output) + else: + write_output(output) + written = (moved if race == "parent-after-open" else parent) / output.name + if writer == "baseline": + assert json.loads(written.read_text(encoding="utf-8")) == {"version": 2} + else: + assert written.read_text(encoding="utf-8") == "report" + expected_mode = ( + protected.stat().st_mode & 0o777 + if writer == "baseline" and race == "hard-link" + else 0o600 + ) + assert written.stat().st_mode & 0o777 == expected_mode + 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(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" + 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 "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()) + + +@pytest.mark.parametrize("reason", ["Accepted", "Accepted 🚀 𐐷\u2028--- with notes"]) +def test_baseline_stdout_works_without_secure_file_output(tmp_path, monkeypatch, reason): + 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": [_finding("P1", "instruction")], + "file_cache": {"SKILL.md": "instruction"}, + "risk_score": 0, + } + + monkeypatch.setattr(cli.graph, "invoke", invoke) + result = runner.invoke( + app, ["baseline", str(tmp_path), "--no-llm", "-o", "-", "--verbose", "--reason", reason] + ) + assert result.exit_code == 0, result.output + import yaml + + from skillspector.suppression import baseline_from_dict + + parsed = baseline_from_dict(yaml.safe_load(result.stdout)) + assert list(parsed.fingerprints.values()) == [reason] + assert json.loads(result.stdout)["fingerprints"][0]["reason"] == reason + 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", ["single", "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 == "single": + monkeypatch.setattr( + cli, "_scan_skill", lambda **kwargs: {"report_body": "report", "risk_score": 0} + ) + args = ["scan", str(skill), "--no-llm", "--format", "json"] + 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 + 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() + + +@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-*")) + + +@pytest.mark.skipif( + os.name != "posix" or os.geteuid() == 0, reason="requires unprivileged POSIX permissions" +) +def test_report_output_preserves_read_only_file(tmp_path): + from skillspector.file_output import write_text_no_follow + + output = tmp_path / "report.json" + output.write_text("preserve", encoding="utf-8") + output.chmod(0o444) + original = output.stat() + with pytest.raises(ValueError) as error: + write_text_no_follow(output, "replacement") + assert str(output) in str(error.value) + assert output.read_text(encoding="utf-8") == "preserve" + assert output.stat().st_ino == original.st_ino + assert output.stat().st_mode == original.st_mode + assert list(tmp_path.iterdir()) == [output] + + @pytest.mark.parametrize( "failure_status", [ diff --git a/tests/unit/test_suppression.py b/tests/unit/test_suppression.py index c631c34e8..83945a050 100644 --- a/tests/unit/test_suppression.py +++ b/tests/unit/test_suppression.py @@ -1161,9 +1161,7 @@ def fail(*args: object, **kwargs: object) -> None: raise error("injected I/O failure") if failure in {"write", "interrupt"}: - import tempfile - - original = tempfile.NamedTemporaryFile + original = os.fdopen def failing_temporary_file(*args: object, **kwargs: object): temporary = original(*args, **kwargs) @@ -1177,7 +1175,7 @@ def partial_write(content: bytes) -> None: temporary.write = partial_write return temporary - monkeypatch.setattr(tempfile, "NamedTemporaryFile", failing_temporary_file) + monkeypatch.setattr(os, "fdopen", failing_temporary_file) elif failure == "sync": monkeypatch.setattr(os, "fsync", fail) else: @@ -1207,7 +1205,7 @@ def test_dump_baseline_preserves_existing_permissions(tmp_path: Path, mode: int) @pytest.mark.skipif(os.name != "posix", reason="requires POSIX descriptors") -def test_dump_baseline_shared_writer_preserves_inode_and_group_access( +def test_dump_baseline_refuses_nonowner_update_without_changing_shared_file( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: output = tmp_path / "baseline.json" @@ -1221,9 +1219,11 @@ def no_chown(*args: object) -> None: pytest.fail("a shared writer must not require chown") monkeypatch.setattr(os, "fchown", no_chown) - dump_baseline({"version": 2, "rules": [{"id": "TM1", "reason": "shared"}]}, output) + with pytest.raises(PermissionError, match="owned by another user"): + dump_baseline({"version": 2, "rules": [{"id": "TM1", "reason": "shared"}]}, output) - assert load_baseline(output).rules[0].reason == "shared" + assert output.read_text(encoding="utf-8") == "old baseline" * 200 + assert list(tmp_path.iterdir()) == [output] assert (output.stat().st_uid, output.stat().st_gid, output.stat().st_ino) == ( old.st_uid, old.st_gid, @@ -1233,26 +1233,34 @@ def no_chown(*args: object) -> None: @pytest.mark.skipif(os.name != "posix", reason="requires POSIX descriptors") -def test_dump_baseline_owner_outside_destination_group_rewrites_in_place( +def test_dump_baseline_refuses_update_when_group_cannot_be_preserved( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: output = tmp_path / "baseline.yaml" output.write_text("old baseline", encoding="utf-8") output.chmod(0o664) - groups = [gid for gid in os.getgroups() if gid != tmp_path.stat().st_gid] - if not groups: - pytest.skip("requires a supplementary group distinct from the directory group") - os.chown(output, -1, groups[0]) old = output.stat() + original_fstat = os.fstat + + def temporary_group_differs(descriptor: int): + current = original_fstat(descriptor) + if current.st_ino != old.st_ino: + values = list(current) + values[5] = old.st_gid + 1 + return os.stat_result(values) + return current + + monkeypatch.setattr(os, "fstat", temporary_group_differs) def denied_chown(*args: object) -> None: # A non-root owner cannot assign a group it is not a member of. raise PermissionError(errno.EPERM, "Operation not permitted") monkeypatch.setattr(os, "fchown", denied_chown) - dump_baseline({"version": 2, "rules": [{"id": "TM1", "reason": "owner"}]}, output) + with pytest.raises(PermissionError, match="Cannot safely preserve baseline ownership or group"): + dump_baseline({"version": 2, "rules": [{"id": "TM1", "reason": "owner"}]}, output) - assert load_baseline(output).rules[0].reason == "owner" + assert output.read_text(encoding="utf-8") == "old baseline" assert (output.stat().st_gid, output.stat().st_ino) == (old.st_gid, old.st_ino) assert S_IMODE(output.stat().st_mode) == 0o664 assert list(tmp_path.iterdir()) == [output] @@ -1272,7 +1280,11 @@ def test_dump_baseline_revalidates_destination_replaced_while_opening( def swapped_open(path, flags, *args, **kwargs): # A cooperating writer atomically publishes once, between lstat and open. - if Path(path) == output and replacement.exists(): + if ( + Path(path).name == output.name + and kwargs.get("dir_fd") is not None + and replacement.exists() + ): os.replace(replacement, output) return original_open(path, flags, *args, **kwargs) @@ -1296,7 +1308,7 @@ def test_dump_baseline_rejects_destination_that_keeps_changing( def swapped_open(path, flags, *args, **kwargs): nonlocal swaps - if Path(path) == output: + if Path(path).name == output.name and kwargs.get("dir_fd") is not None: swaps += 1 replacement = tmp_path / f"replacement-{swaps}.yaml" replacement.write_text(f"replacement {swaps}", encoding="utf-8") @@ -1312,45 +1324,6 @@ def swapped_open(path, flags, *args, **kwargs): assert list(tmp_path.iterdir()) == [output] -@pytest.mark.skipif(os.name != "posix", reason="requires POSIX descriptors") -@pytest.mark.parametrize("persistent", [False, True]) -def test_dump_baseline_shared_writer_revalidates_destination_replaced_before_lock( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch, persistent: bool -) -> None: - import fcntl - - output = tmp_path / "baseline.yaml" - output.write_text("original", encoding="utf-8") - output.chmod(0o664) - monkeypatch.setattr(os, "geteuid", lambda: output.stat().st_uid + 1) - original_flock = fcntl.flock - replaced: list[int] = [] - - def replacing_flock(descriptor: int, operation: int) -> None: - # Another writer publishes after this one opened the path, before the lock. - if persistent or not replaced: - replacement = tmp_path / f"replacement-{len(replaced)}.yaml" - replacement.write_text("replacement", encoding="utf-8") - replacement.chmod(0o664) - os.replace(replacement, output) - replaced.append(output.stat().st_ino) - original_flock(descriptor, operation) - - monkeypatch.setattr(fcntl, "flock", replacing_flock) - data = {"version": 2, "rules": [{"id": "TM1", "reason": "shared"}]} - if persistent: - with pytest.raises(ValueError, match="changed before writing"): - dump_baseline(data, output) - assert len(replaced) == suppression_module._BASELINE_DESTINATION_ATTEMPTS - # Every validated inode was replaced before anything was written. - assert output.read_text(encoding="utf-8") == "replacement" - else: - dump_baseline(data, output) - assert load_baseline(output).rules[0].reason == "shared" - assert output.stat().st_ino == replaced[0] - assert list(tmp_path.iterdir()) == [output] - - @pytest.mark.skipif(os.name != "posix", reason="requires POSIX descriptors") @pytest.mark.parametrize("error", [errno.ENOTSUP, errno.EOPNOTSUPP]) def test_dump_baseline_accepts_filesystem_without_extended_acls( @@ -1392,13 +1365,29 @@ def test_dump_baseline_preserves_read_only_destination(tmp_path: Path) -> None: output.write_text("existing baseline", encoding="utf-8") output.chmod(0o444) - with pytest.raises(PermissionError): + with pytest.raises(PermissionError) as error: dump_baseline({"version": 2}, output) + assert str(output) in str(error.value) assert output.read_text(encoding="utf-8") == "existing baseline" assert list(tmp_path.iterdir()) == [output] +@pytest.mark.skipif( + os.name != "posix" or os.geteuid() == 0, reason="requires unprivileged POSIX permissions" +) +def test_dump_baseline_parent_permission_error_names_output(tmp_path: Path) -> None: + output = tmp_path / "baseline.yaml" + tmp_path.chmod(0o555) + try: + with pytest.raises(PermissionError) as error: + dump_baseline({"version": 2}, output) + assert str(output) in str(error.value) + assert not output.exists() + finally: + tmp_path.chmod(0o700) + + @pytest.mark.parametrize("target_exists", [True, False]) def test_dump_baseline_rejects_symlink_destination(tmp_path: Path, target_exists: bool) -> None: target = tmp_path / "target.yaml"