diff --git a/src/skillsaw/context.py b/src/skillsaw/context.py index b26020c6..64584d0e 100644 --- a/src/skillsaw/context.py +++ b/src/skillsaw/context.py @@ -22,12 +22,9 @@ codex_declared_skill_dirs, codex_local_source_path, codex_manifest_is_contained, - safe_exists, - safe_is_dir, - safe_is_symlink, - safe_resolve, ) from .formats.promptfoo import is_promptfoo_config +from .paths import contained_resolve, safe_exists, safe_is_dir, safe_is_symlink, safe_resolve from .utils import read_json if TYPE_CHECKING: @@ -869,8 +866,7 @@ def _codex_catalog_files(self) -> List[Path]: return [] def _usable(path: Path) -> bool: - resolved = safe_resolve(path) - if resolved is None or not resolved.is_relative_to(root): + if contained_resolve(path, root) is None: return False if self.is_path_excluded(path): return False @@ -987,8 +983,7 @@ def _add(directory: Path) -> None: manifest_dir = directory / self.CODEX_PLUGIN_MANIFEST[0] if not (safe_exists(manifest_dir) or safe_is_symlink(manifest_dir)): return - manifest_dir_resolved = safe_resolve(manifest_dir) - if manifest_dir_resolved is None or not manifest_dir_resolved.is_relative_to(resolved): + if contained_resolve(manifest_dir, resolved) is None: return # A missing manifest is still a plugin — codex-plugin-json-valid # reports it. One that resolves elsewhere is not. @@ -1414,8 +1409,8 @@ def _discover_from_plugins_dir(self, plugins: List[Path], discovered_paths: Set[ if not ((item / ".claude-plugin").exists() or (item / "commands").exists()): continue - resolved_path = safe_resolve(item) - if resolved_path is None or not resolved_path.is_relative_to(self.root_path): + resolved_path = contained_resolve(item, self.root_path) + if resolved_path is None: continue if resolved_path not in discovered_paths: plugins.append(item) @@ -1618,11 +1613,10 @@ def _discover_skills(self) -> List[Path]: if (skills_dir / "SKILL.md").exists(): # A manifest may name one skill directly rather than a # collection; descending would step straight past it. - resolved = safe_resolve(skills_dir) - entrypoint = safe_resolve(skills_dir / "SKILL.md") - if resolved is None or not resolved.is_relative_to(plugin_root): + resolved = contained_resolve(skills_dir, plugin_root) + if resolved is None: continue - if entrypoint is None or not entrypoint.is_relative_to(plugin_root): + if contained_resolve(skills_dir / "SKILL.md", plugin_root) is None: continue if resolved not in discovered: skills.append(skills_dir) diff --git a/src/skillsaw/diagnostics.py b/src/skillsaw/diagnostics.py new file mode 100644 index 00000000..de19df3a --- /dev/null +++ b/src/skillsaw/diagnostics.py @@ -0,0 +1,104 @@ +"""Making untrusted values safe to echo into violation messages. + +Any rule that quotes a manifest or config value in a diagnostic routes +it through here, whatever ecosystem the rule belongs to. +""" + +from __future__ import annotations + +import re + +# C0, DEL, C1, and the Unicode bidi overrides — any of them can reorder +# or hide message text in a terminal or a rendered SARIF viewer. +_CONTROL_CHARS = re.compile(r"[\x00-\x1f\x7f-\x9f\u202a-\u202e\u2066-\u2069]") + +# Everything a message needs to locate the defect fits comfortably here; +# an adversarial multi-kilobyte value must not become a multi-kilobyte +# diagnostic in a CI artifact. +_MAX_DISPLAY = 500 + + +def _redact_userinfo(text: str) -> str: + """Strip credential-shaped userinfo before every ``@`` in *text*. + + Covers scheme-full ("https://u:tok@h/x"), scheme-relative, bare + ("u:tok@h/x"), and scp-style ("tok@github.com:o/r.git") spellings: + the segment between the last ``/``/whitespace and an ``@`` is + redacted when it carries a colon (credential shape) or when the text + after the ``@`` looks host-like (a dot or colon before the next + whitespace). Over-redacting an email-shaped value in a path field is + the safe direction. A linear right-to-left scan — no regex, so there + is nothing to backtrack, and no length cap for a long token to slip + past. + """ + if "@" not in text: + return text + out = [] + emitted = 0 + search_from = 0 + length = len(text) + while True: + at = text.find("@", search_from) + if at == -1: + out.append(text[emitted:]) + return "".join(out) + # The backward window is floored like the forward host cap below: + # without it, a delimiter-free prefix is rescanned for every ``@`` + # and the scan goes quadratic on adversarial input. When the + # clipped window holds no delimiter the credential may extend past + # it, so redact from the last emit point — never clamp ``start`` + # into the middle of a secret and emit its head. + floor = max(emitted, at - 512) + found = max(text.rfind(ch, floor, at) for ch in ("/", " ", "\t", "\n", "@")) + start = found + 1 if found != -1 else emitted + userinfo = text[start:at] + if not userinfo: + search_from = at + 1 + continue + # Host inspection is capped: a host longer than any real one with + # no whitespace is treated as host-like, which errs toward + # redaction — the safe direction. + head_limit = min(at + 1 + 512, length) + host_head = text[at + 1 : head_limit] + ws = next((i for i, ch in enumerate(host_head) if ch.isspace()), None) + if ws is not None: + host_head = host_head[:ws] + host_like = "." in host_head or ":" in host_head + else: + host_like = "." in host_head or ":" in host_head or head_limit < length + if ":" in userinfo or host_like: + out.append(text[emitted:start]) + out.append("[redacted]") + emitted = at + search_from = at + 1 + + +def safe_display(value: object) -> str: + """A manifest value made safe to echo into a violation message. + + Reports are uploaded as CI artifacts and ingested as SARIF, so an + author's pasted ``user:token@host`` URL must not ride along — the + userinfo is redacted, keeping the locator. Control characters are + replaced so a crafted value cannot smuggle terminal escapes through + the text formatter, and the result is length-bounded. + """ + raw = str(value) + truncated = len(raw) > _MAX_DISPLAY + # Truncate before scanning so the display cap bounds the *work*, not + # just the output — redaction over the full value is quadratic-ish in + # the worst case, and a multi-megabyte manifest value must not buy + # minutes of CPU for one diagnostic. + text = raw[:_MAX_DISPLAY] + if truncated: + # The cut can sever a credential ahead of its ``@`` — the window + # then holds a bare colon-bearing segment redaction would never + # match. Treat the cut like an ``@``: redact a colon-bearing tail + # segment. Over-redacting a truncated tail is the safe direction. + start = max(text.rfind(ch) for ch in ("/", " ", "\t", "\n", "@")) + 1 + if ":" in text[start:]: + text = text[:start] + "[redacted]" + text = _CONTROL_CHARS.sub("\N{REPLACEMENT CHARACTER}", text) + text = _redact_userinfo(text) + if truncated or len(text) > _MAX_DISPLAY: + text = text[:_MAX_DISPLAY] + "…" + return text diff --git a/src/skillsaw/docs/extractor.py b/src/skillsaw/docs/extractor.py index 1b97ff09..a8bcc263 100644 --- a/src/skillsaw/docs/extractor.py +++ b/src/skillsaw/docs/extractor.py @@ -12,8 +12,8 @@ codex_local_source_path, codex_plugin_name, is_remote_source, - safe_resolve, ) +from skillsaw.paths import contained_resolve, safe_resolve from skillsaw.utils import read_json from skillsaw.docs.models import ( AgentDoc, @@ -489,8 +489,8 @@ def _scoped_prose( return [] scoped = [] for block in container.find(block_cls): - resolved = safe_resolve(block.path) - if resolved is None or not resolved.is_relative_to(plugin_resolved): + resolved = contained_resolve(block.path, plugin_resolved) + if resolved is None: continue owner = next( (c for c in resolved.parents if c in prose_roots), diff --git a/src/skillsaw/formats/codex.py b/src/skillsaw/formats/codex.py index 71e434c3..23f04f14 100644 --- a/src/skillsaw/formats/codex.py +++ b/src/skillsaw/formats/codex.py @@ -14,6 +14,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional +from skillsaw.paths import contained_resolve, safe_is_dir, safe_is_file, safe_resolve from skillsaw.utils import read_json @@ -55,64 +56,6 @@ def is_remote_source(source: Any) -> bool: return isinstance(kind, str) and kind in REMOTE_SOURCE_TYPES -def safe_resolve(path: Path) -> Optional[Path]: - """``path.resolve()``, or ``None`` when the path cannot be resolved. - - Discovery runs while ``RepositoryContext`` is being constructed, before - any rule can report anything, and it resolves strings taken straight - out of a manifest. ``Path.resolve()`` raises ``ValueError`` on an - embedded NUL, ``OSError`` on an unreadable parent, and — on a symlink - loop — ``RuntimeError`` before Python 3.13 but ``OSError`` from 3.13 - on. This project supports 3.9 through 3.14, so all three have to be - caught; any of them would abort the whole lint instead of producing - the violation the manifest deserves. Returning ``None`` drops the - candidate from discovery and leaves the reporting to the rules. - """ - try: - return path.resolve() - except (OSError, ValueError, RuntimeError): - return None - - -def _safe_stat(path: Path, predicate: str) -> bool: - """``path.()``, or ``False`` when the path cannot be stat'd. - - ``safe_resolve`` is not enough on its own: ``Path.resolve()`` does not - stat, so it happily returns a path that the very next ``is_dir()`` - raises on. ``pathlib`` swallows only ``ENOENT``/``ENOTDIR``/``EBADF``/ - ``ELOOP`` on Python 3.9 through 3.12, so a manifest declaring a - 4000-character path raises ``ENAMETOOLONG`` there — from inside - ``RepositoryContext.__init__``, where the ``rule-execution-error`` - guard cannot reach it. The whole lint aborts with a traceback and - reports nothing at all, on a repository whose only defect is one - over-long string in a JSON file. - """ - try: - return bool(getattr(path, predicate)()) - except (OSError, ValueError): - return False - - -def safe_is_dir(path: Path) -> bool: - """``path.is_dir()``, or ``False`` when the path cannot be stat'd.""" - return _safe_stat(path, "is_dir") - - -def safe_is_file(path: Path) -> bool: - """``path.is_file()``, or ``False`` when the path cannot be stat'd.""" - return _safe_stat(path, "is_file") - - -def safe_exists(path: Path) -> bool: - """``path.exists()``, or ``False`` when the path cannot be stat'd.""" - return _safe_stat(path, "exists") - - -def safe_is_symlink(path: Path) -> bool: - """``path.is_symlink()``, or ``False`` when the path cannot be stat'd.""" - return _safe_stat(path, "is_symlink") - - def inline_documents(declared: Any, key: str) -> List[Dict[str, Any]]: """One document per inline object in a Codex manifest field. @@ -187,8 +130,8 @@ def codex_declared_paths(plugin_dir: Path, field: str, want_dir: bool) -> List[P for item in candidates: if not isinstance(item, str) or not item: continue - candidate = safe_resolve(plugin_dir / item) - if candidate is None or not candidate.is_relative_to(root): + candidate = contained_resolve(plugin_dir / item, root) + if candidate is None: continue # ``"skills": "./"`` points at the plugin root, which is a legal # place to keep a skill. A file-valued field naming the root is @@ -290,11 +233,9 @@ def codex_manifest_is_contained(plugin_dir: Path) -> bool: if root is None: return False manifest_dir = plugin_dir / CODEX_PLUGIN_MANIFEST[0] - resolved_dir = safe_resolve(manifest_dir) - if resolved_dir is None or not resolved_dir.is_relative_to(root): + if contained_resolve(manifest_dir, root) is None: return False manifest = plugin_dir.joinpath(*CODEX_PLUGIN_MANIFEST) - resolved = safe_resolve(manifest) - if resolved is None or not resolved.is_relative_to(root): + if contained_resolve(manifest, root) is None: return False return safe_is_file(manifest) diff --git a/src/skillsaw/lint_tree.py b/src/skillsaw/lint_tree.py index 7f21ac96..67795737 100644 --- a/src/skillsaw/lint_tree.py +++ b/src/skillsaw/lint_tree.py @@ -40,10 +40,8 @@ codex_inline_hooks, codex_inline_mcp_servers, codex_manifest_is_contained, - safe_is_dir, - safe_is_file, - safe_resolve, ) +from .paths import safe_is_dir, safe_is_file, safe_resolve from .formats.promptfoo import ( extract_file_refs, is_promptfoo_config, diff --git a/src/skillsaw/paths.py b/src/skillsaw/paths.py index ffe24f9d..85dd4cbb 100644 --- a/src/skillsaw/paths.py +++ b/src/skillsaw/paths.py @@ -1,16 +1,19 @@ -"""Pure path predicates shared across formats and rule packages. - -These answer questions about a path *string* — is it absolute, does it -escape its root — without touching the filesystem. They live here rather -than in a rule module because two independent rule packages need them, -and neither should have to import the other's private helpers to get at -a pure predicate. ``skillsaw.formats.codex.safe_resolve`` is the -filesystem-touching companion. +"""Path helpers shared across formats and rule packages. + +The pure predicates answer questions about a path *string* — is it +absolute, does it escape its root — without touching the filesystem. +The ``safe_*`` wrappers are their filesystem-touching companions: +``pathlib`` calls that never raise, so discovery and rules can probe +manifest-supplied paths without aborting the lint. They live here +rather than in a format or rule module because independent packages +need them, and none should have to import another's private helpers +to get at an ecosystem-neutral utility. """ from __future__ import annotations -from pathlib import PurePosixPath, PureWindowsPath +from pathlib import Path, PurePosixPath, PureWindowsPath +from typing import Optional def is_absolute_path(path: str) -> bool: @@ -39,3 +42,96 @@ def is_absolute_path(path: str) -> bool: def has_parent_traversal(path: str) -> bool: """True when the path contains a '..' component.""" return ".." in path.replace("\\", "/").split("/") + + +def safe_resolve(path: Path) -> Optional[Path]: + """``path.resolve()``, or ``None`` when the path cannot be resolved. + + Discovery runs while ``RepositoryContext`` is being constructed, before + any rule can report anything, and it resolves strings taken straight + out of a manifest. ``Path.resolve()`` raises ``ValueError`` on an + embedded NUL, ``OSError`` on an unreadable parent, and — on a symlink + loop — ``RuntimeError`` before Python 3.13 but ``OSError`` from 3.13 + on. This project supports 3.9 through 3.14, so all three have to be + caught; any of them would abort the whole lint instead of producing + the violation the manifest deserves. Returning ``None`` drops the + candidate from discovery and leaves the reporting to the rules. + """ + try: + return path.resolve() + except (OSError, ValueError, RuntimeError): + return None + + +def contained_resolve(path: Path, root: Path) -> Optional[Path]: + """``path`` resolved, when it stays inside *root* — else ``None``. + + The reject-a-symlink-escape idiom in one place: a resolution failure + and a path that resolves outside *root* both yield ``None``, so a + caller holding a resolved root can write ``if contained_resolve(p, + root) is None: reject``. + """ + resolved = safe_resolve(path) + if resolved is None or not resolved.is_relative_to(root): + return None + return resolved + + +def _safe_stat(path: Path, predicate: str) -> bool: + """``path.()``, or ``False`` when the path cannot be stat'd. + + ``safe_resolve`` is not enough on its own: ``Path.resolve()`` does not + stat, so it happily returns a path that the very next ``is_dir()`` + raises on. ``pathlib`` swallows only ``ENOENT``/``ENOTDIR``/``EBADF``/ + ``ELOOP`` on Python 3.9 through 3.12, so a manifest declaring a + 4000-character path raises ``ENAMETOOLONG`` there — from inside + ``RepositoryContext.__init__``, where the ``rule-execution-error`` + guard cannot reach it. The whole lint aborts with a traceback and + reports nothing at all, on a repository whose only defect is one + over-long string in a JSON file. + """ + try: + return bool(getattr(path, predicate)()) + except (OSError, ValueError): + return False + + +def safe_is_dir(path: Path) -> bool: + """``path.is_dir()``, or ``False`` when the path cannot be stat'd.""" + return _safe_stat(path, "is_dir") + + +def safe_is_file(path: Path) -> bool: + """``path.is_file()``, or ``False`` when the path cannot be stat'd.""" + return _safe_stat(path, "is_file") + + +def safe_exists(path: Path) -> bool: + """``path.exists()``, or ``False`` when the path cannot be stat'd.""" + return _safe_stat(path, "exists") + + +def safe_is_symlink(path: Path) -> bool: + """``path.is_symlink()``, or ``False`` when the path cannot be stat'd.""" + return _safe_stat(path, "is_symlink") + + +def escapes_root(value: str, root: Path) -> bool: + """Whether *value* resolves outside *root* once symlinks are followed. + + A path that does not exist yet cannot escape through a link, so an + unresolvable candidate is left to the caller's existence check. ``OSError`` + (a symlink loop, an unreadable parent) counts as an escape: the linter + cannot prove containment, and failing closed is the safe direction for a + check whose whole purpose is keeping discovery inside the root. + """ + try: + resolved_root = root.resolve() + candidate = (root / value).resolve() + except (OSError, ValueError, RuntimeError): + # OSError: an unreadable parent, or a symlink loop on 3.13+. + # RuntimeError: a symlink loop before 3.13. ValueError: an embedded + # NUL. Containment cannot be proven in any of these cases, and + # failing closed is the safe direction for a containment check. + return True + return candidate != resolved_root and not candidate.is_relative_to(resolved_root) diff --git a/src/skillsaw/rules/builtin/agentskills/_helpers.py b/src/skillsaw/rules/builtin/agentskills/_helpers.py index 6496a71c..0f3d882c 100644 --- a/src/skillsaw/rules/builtin/agentskills/_helpers.py +++ b/src/skillsaw/rules/builtin/agentskills/_helpers.py @@ -7,7 +7,7 @@ from typing import Optional, TYPE_CHECKING from skillsaw.context import SKILL_REPO_TYPES # noqa: F401 — re-export for package rules -from skillsaw.formats.codex import safe_exists, safe_resolve +from skillsaw.paths import contained_resolve, safe_exists if TYPE_CHECKING: # pragma: no cover - import cycle at runtime from skillsaw.context import RepositoryContext @@ -43,8 +43,7 @@ def contained_skill_file( root = context.codex_plugin_owning(skill_dir) if root is None: return candidate - resolved = safe_resolve(candidate) - if resolved is None or not resolved.is_relative_to(root): + if contained_resolve(candidate, root) is None: return None return candidate diff --git a/src/skillsaw/rules/builtin/agentskills/unreferenced_files.py b/src/skillsaw/rules/builtin/agentskills/unreferenced_files.py index b5251b05..3acab87e 100644 --- a/src/skillsaw/rules/builtin/agentskills/unreferenced_files.py +++ b/src/skillsaw/rules/builtin/agentskills/unreferenced_files.py @@ -108,7 +108,7 @@ from skillsaw.blocks import ContentBlock from skillsaw.utils import read_text -from skillsaw.formats.codex import safe_resolve +from skillsaw.paths import safe_resolve from ._helpers import SKILL_REPO_TYPES, contained_skill_file diff --git a/src/skillsaw/rules/builtin/codex/_helpers.py b/src/skillsaw/rules/builtin/codex/_helpers.py index 80fc2d01..de2f99c0 100644 --- a/src/skillsaw/rules/builtin/codex/_helpers.py +++ b/src/skillsaw/rules/builtin/codex/_helpers.py @@ -9,7 +9,8 @@ from typing import Optional from skillsaw.context import RepositoryType -from skillsaw.paths import has_parent_traversal, is_absolute_path +from skillsaw.diagnostics import safe_display +from skillsaw.paths import escapes_root, has_parent_traversal, is_absolute_path from skillsaw.rules.builtin.utils import read_text # A Codex marketplace repository contains the plugins it catalogs, so the @@ -28,102 +29,6 @@ KEBAB_CASE = re.compile(r"^[a-z0-9]+(-[a-z0-9]+)*\Z") -# C0, DEL, C1, and the Unicode bidi overrides — any of them can reorder -# or hide message text in a terminal or a rendered SARIF viewer. -_CONTROL_CHARS = re.compile(r"[\x00-\x1f\x7f-\x9f\u202a-\u202e\u2066-\u2069]") - -# Everything a message needs to locate the defect fits comfortably here; -# an adversarial multi-kilobyte value must not become a multi-kilobyte -# diagnostic in a CI artifact. -_MAX_DISPLAY = 500 - - -def _redact_userinfo(text: str) -> str: - """Strip credential-shaped userinfo before every ``@`` in *text*. - - Covers scheme-full ("https://u:tok@h/x"), scheme-relative, bare - ("u:tok@h/x"), and scp-style ("tok@github.com:o/r.git") spellings: - the segment between the last ``/``/whitespace and an ``@`` is - redacted when it carries a colon (credential shape) or when the text - after the ``@`` looks host-like (a dot or colon before the next - whitespace). Over-redacting an email-shaped value in a path field is - the safe direction. A linear right-to-left scan — no regex, so there - is nothing to backtrack, and no length cap for a long token to slip - past. - """ - if "@" not in text: - return text - out = [] - emitted = 0 - search_from = 0 - length = len(text) - while True: - at = text.find("@", search_from) - if at == -1: - out.append(text[emitted:]) - return "".join(out) - # The backward window is floored like the forward host cap below: - # without it, a delimiter-free prefix is rescanned for every ``@`` - # and the scan goes quadratic on adversarial input. When the - # clipped window holds no delimiter the credential may extend past - # it, so redact from the last emit point — never clamp ``start`` - # into the middle of a secret and emit its head. - floor = max(emitted, at - 512) - found = max(text.rfind(ch, floor, at) for ch in ("/", " ", "\t", "\n", "@")) - start = found + 1 if found != -1 else emitted - userinfo = text[start:at] - if not userinfo: - search_from = at + 1 - continue - # Host inspection is capped: a host longer than any real one with - # no whitespace is treated as host-like, which errs toward - # redaction — the safe direction. - head_limit = min(at + 1 + 512, length) - host_head = text[at + 1 : head_limit] - ws = next((i for i, ch in enumerate(host_head) if ch.isspace()), None) - if ws is not None: - host_head = host_head[:ws] - host_like = "." in host_head or ":" in host_head - else: - host_like = "." in host_head or ":" in host_head or head_limit < length - if ":" in userinfo or host_like: - out.append(text[emitted:start]) - out.append("[redacted]") - emitted = at - search_from = at + 1 - - -def safe_display(value: object) -> str: - """A manifest value made safe to echo into a violation message. - - Reports are uploaded as CI artifacts and ingested as SARIF, so an - author's pasted ``user:token@host`` URL must not ride along — the - userinfo is redacted, keeping the locator. Control characters are - replaced so a crafted value cannot smuggle terminal escapes through - the text formatter, and the result is length-bounded. - """ - raw = str(value) - truncated = len(raw) > _MAX_DISPLAY - # Truncate before scanning so the display cap bounds the *work*, not - # just the output — redaction over the full value is quadratic-ish in - # the worst case, and a multi-megabyte manifest value must not buy - # minutes of CPU for one diagnostic. - text = raw[:_MAX_DISPLAY] - if truncated: - # The cut can sever a credential ahead of its ``@`` — the window - # then holds a bare colon-bearing segment redaction would never - # match. Treat the cut like an ``@``: redact a colon-bearing tail - # segment. Over-redacting a truncated tail is the safe direction. - start = max(text.rfind(ch) for ch in ("/", " ", "\t", "\n", "@")) + 1 - if ":" in text[start:]: - text = text[:start] + "[redacted]" - text = _CONTROL_CHARS.sub("\N{REPLACEMENT CHARACTER}", text) - text = _redact_userinfo(text) - if truncated or len(text) > _MAX_DISPLAY: - text = text[:_MAX_DISPLAY] + "…" - return text - - def reject_nonfinite_json_number(value: str) -> None: """Reject JavaScript number extensions that strict JSON does not allow. @@ -180,24 +85,3 @@ def path_problem(value: str, root_label: str, root: Optional[Path] = None) -> Op f"path '{safe_display(value)}' resolves outside the {root_label} — check for a symlink" ) return None - - -def escapes_root(value: str, root: Path) -> bool: - """Whether *value* resolves outside *root* once symlinks are followed. - - A path that does not exist yet cannot escape through a link, so an - unresolvable candidate is left to the caller's existence check. ``OSError`` - (a symlink loop, an unreadable parent) counts as an escape: the linter - cannot prove containment, and failing closed is the safe direction for a - check whose whole purpose is keeping discovery inside the root. - """ - try: - resolved_root = root.resolve() - candidate = (root / value).resolve() - except (OSError, ValueError, RuntimeError): - # OSError: an unreadable parent, or a symlink loop on 3.13+. - # RuntimeError: a symlink loop before 3.13. ValueError: an embedded - # NUL. Containment cannot be proven in any of these cases, and - # failing closed is the safe direction for a containment check. - return True - return candidate != resolved_root and not candidate.is_relative_to(resolved_root) diff --git a/src/skillsaw/rules/builtin/codex/marketplace_json_valid.py b/src/skillsaw/rules/builtin/codex/marketplace_json_valid.py index 9279cdbb..70342adf 100644 --- a/src/skillsaw/rules/builtin/codex/marketplace_json_valid.py +++ b/src/skillsaw/rules/builtin/codex/marketplace_json_valid.py @@ -9,8 +9,9 @@ from skillsaw.rule import Rule, RuleViolation, Severity from skillsaw.context import RepositoryContext, codex_local_source_path -from skillsaw.formats.codex import safe_exists +from skillsaw.diagnostics import safe_display from skillsaw.lint_target import CodexMarketplaceConfigNode +from skillsaw.paths import safe_exists from skillsaw.rules.builtin.utils import read_json from ._helpers import ( @@ -18,7 +19,6 @@ KEBAB_CASE, nonfinite_constant_error, path_problem, - safe_display, ) # Required fields per documented source type. Unknown types warn rather than diff --git a/src/skillsaw/rules/builtin/codex/marketplace_registration.py b/src/skillsaw/rules/builtin/codex/marketplace_registration.py index 18bc7e0b..217a20b2 100644 --- a/src/skillsaw/rules/builtin/codex/marketplace_registration.py +++ b/src/skillsaw/rules/builtin/codex/marketplace_registration.py @@ -14,21 +14,20 @@ AutofixResult, AutofixConfidence, ) -from skillsaw.context import RepositoryContext, codex_local_source_path, safe_resolve +from skillsaw.context import RepositoryContext, codex_local_source_path +from skillsaw.diagnostics import safe_display from skillsaw.formats.codex import ( codex_plugin_name, is_remote_source, - safe_is_dir, - safe_is_file, ) from skillsaw.lint_target import CodexMarketplaceConfigNode, CodexPluginConfigNode +from skillsaw.paths import safe_is_dir, safe_is_file, safe_resolve from skillsaw.rules.builtin.utils import read_json, read_text from ._helpers import ( CODEX_MARKETPLACE_REPO_TYPES, KEBAB_CASE, reject_nonfinite_json_number, - safe_display, ) # What ``fix()`` writes for a newly registered plugin. Every entry in the diff --git a/src/skillsaw/rules/builtin/codex/openai_metadata.py b/src/skillsaw/rules/builtin/codex/openai_metadata.py index 0f9aa416..e4397a2d 100644 --- a/src/skillsaw/rules/builtin/codex/openai_metadata.py +++ b/src/skillsaw/rules/builtin/codex/openai_metadata.py @@ -8,10 +8,9 @@ from skillsaw.blocks import OpenAIMetadataBlock from skillsaw.context import RepositoryContext, SKILL_REPO_TYPES -from skillsaw.formats.codex import safe_is_file, safe_resolve -from skillsaw.paths import is_absolute_path +from skillsaw.diagnostics import safe_display +from skillsaw.paths import is_absolute_path, safe_is_file, safe_resolve from skillsaw.rule import Rule, RuleViolation, Severity -from skillsaw.rules.builtin.codex._helpers import safe_display from skillsaw.rules.builtin.utils import ( commented_item_line, commented_key_line, diff --git a/src/skillsaw/rules/builtin/codex/plugin_json_valid.py b/src/skillsaw/rules/builtin/codex/plugin_json_valid.py index 3daa3473..e7ce8c5f 100644 --- a/src/skillsaw/rules/builtin/codex/plugin_json_valid.py +++ b/src/skillsaw/rules/builtin/codex/plugin_json_valid.py @@ -7,8 +7,9 @@ from skillsaw.rule import Rule, RuleViolation, Severity from skillsaw.context import RepositoryContext -from skillsaw.formats.codex import safe_exists, safe_is_dir, safe_is_file +from skillsaw.diagnostics import safe_display from skillsaw.lint_target import CodexPluginConfigNode +from skillsaw.paths import safe_exists, safe_is_dir, safe_is_file from skillsaw.rules.builtin.utils import read_json from ._helpers import ( @@ -16,7 +17,6 @@ KEBAB_CASE, nonfinite_constant_error, path_problem, - safe_display, ) # Manifest fields that point at bundled components or assets. Every one of diff --git a/src/skillsaw/rules/builtin/codex/plugin_structure.py b/src/skillsaw/rules/builtin/codex/plugin_structure.py index 1b5f678c..68dcdd08 100644 --- a/src/skillsaw/rules/builtin/codex/plugin_structure.py +++ b/src/skillsaw/rules/builtin/codex/plugin_structure.py @@ -8,7 +8,9 @@ from skillsaw.context import RepositoryContext from skillsaw.lint_target import CodexPluginConfigNode -from ._helpers import safe_display, CODEX_PLUGIN_REPO_TYPES +from skillsaw.diagnostics import safe_display + +from ._helpers import CODEX_PLUGIN_REPO_TYPES class CodexPluginStructureRule(Rule): diff --git a/src/skillsaw/rules/builtin/hooks/json_valid.py b/src/skillsaw/rules/builtin/hooks/json_valid.py index f2c277a9..fa524025 100644 --- a/src/skillsaw/rules/builtin/hooks/json_valid.py +++ b/src/skillsaw/rules/builtin/hooks/json_valid.py @@ -6,7 +6,7 @@ from skillsaw.rule import Rule, RuleViolation, Severity from skillsaw.context import RepositoryContext -from skillsaw.rules.builtin.codex._helpers import safe_display +from skillsaw.diagnostics import safe_display from skillsaw.rules.builtin.content_analysis import HooksBlock # Valid hook event types diff --git a/src/skillsaw/rules/builtin/marketplace/json_valid.py b/src/skillsaw/rules/builtin/marketplace/json_valid.py index b8d9aa33..1c125fef 100644 --- a/src/skillsaw/rules/builtin/marketplace/json_valid.py +++ b/src/skillsaw/rules/builtin/marketplace/json_valid.py @@ -5,10 +5,9 @@ import re from typing import List -from skillsaw.paths import has_parent_traversal, is_absolute_path +from skillsaw.paths import has_parent_traversal, is_absolute_path, safe_is_dir, safe_resolve from skillsaw.rule import Rule, RuleViolation, Severity from skillsaw.context import RepositoryContext, RepositoryType -from skillsaw.formats.codex import safe_is_dir, safe_resolve from skillsaw.lint_target import MarketplaceConfigNode, PluginNode from skillsaw.rules.builtin.utils import read_json diff --git a/tests/codex/test_catalogs.py b/tests/codex/test_catalogs.py index 7d577eb9..e0ef4baf 100644 --- a/tests/codex/test_catalogs.py +++ b/tests/codex/test_catalogs.py @@ -143,7 +143,7 @@ def test_manifest_credentials_are_redacted_from_every_echo_site(self, tmp_path): # A pasted JWT is far longer than any redaction cap — the bound # itself was the escape hatch. long_secret = "eyJ" + "b" * 400 # notsecret - from skillsaw.rules.builtin.codex._helpers import safe_display + from skillsaw.diagnostics import safe_display assert long_secret not in safe_display(f"https://u:{long_secret}@h/p") diff --git a/tests/codex/test_containment.py b/tests/codex/test_containment.py index afb59858..ba215e7f 100644 --- a/tests/codex/test_containment.py +++ b/tests/codex/test_containment.py @@ -8,8 +8,8 @@ from skillsaw.docs.extractor import extract_docs from skillsaw.context import RepositoryContext, RepositoryType from skillsaw.blocks import HooksBlock, SkillRefBlock -from skillsaw.formats.codex import codex_declared_skill_dirs, safe_resolve -from skillsaw.rules.builtin.codex._helpers import escapes_root +from skillsaw.formats.codex import codex_declared_skill_dirs +from skillsaw.paths import escapes_root, safe_resolve from skillsaw.rules.builtin.codex import CodexMarketplaceJsonValidRule, CodexPluginJsonValidRule from skillsaw.rules.builtin.plugins.json_required import PluginJsonRequiredRule diff --git a/tests/codex/test_docs_output_safety.py b/tests/codex/test_docs_output_safety.py index ef00cb73..623b3270 100644 --- a/tests/codex/test_docs_output_safety.py +++ b/tests/codex/test_docs_output_safety.py @@ -326,7 +326,7 @@ def test_redaction_work_is_bounded_on_adversarial_values(self): """ import time - from skillsaw.rules.builtin.codex._helpers import safe_display + from skillsaw.diagnostics import safe_display def cost(n: int) -> float: best = float("inf") @@ -348,7 +348,7 @@ def cost(n: int) -> float: def test_truncation_does_not_leak_a_severed_credential(self): """A credential whose ``@`` falls beyond the display cap must not surface its head — the cut edge is treated like an ``@``.""" - from skillsaw.rules.builtin.codex._helpers import safe_display + from skillsaw.diagnostics import safe_display value = "x" * 490 + "user:" + "S" * 600 + "@host.example/path" out = safe_display(value) @@ -358,7 +358,7 @@ def test_truncation_does_not_leak_a_severed_credential(self): def test_a_long_delimiterless_credential_is_fully_redacted(self): """A >512-char userinfo must not have its head emitted when the backward search window is clipped.""" - from skillsaw.rules.builtin.codex._helpers import _redact_userinfo + from skillsaw.diagnostics import _redact_userinfo out = _redact_userinfo("u:" + "T" * 600 + "@h.example") assert "TTTT" not in out diff --git a/tests/codex/test_plugin_json.py b/tests/codex/test_plugin_json.py index 393813f9..2ab6b128 100644 --- a/tests/codex/test_plugin_json.py +++ b/tests/codex/test_plugin_json.py @@ -7,8 +7,8 @@ from skillsaw.context import RepositoryContext, RepositoryType from skillsaw.rule import Severity -from skillsaw.formats.codex import ( - codex_declared_hook_files, +from skillsaw.formats.codex import codex_declared_hook_files +from skillsaw.paths import ( safe_exists, safe_is_dir, safe_is_file, diff --git a/tests/test_banned_apis.py b/tests/test_banned_apis.py new file mode 100644 index 00000000..128783e2 --- /dev/null +++ b/tests/test_banned_apis.py @@ -0,0 +1,101 @@ +"""Ratchet test banning new bare ``Path.resolve()`` calls in skillsaw source. + +``Path.resolve()`` raises ``OSError``, ``ValueError``, or ``RuntimeError`` +on hostile input (symlink loops, embedded NULs, unreadable parents), which +is why ``skillsaw.paths`` provides ``safe_resolve()`` and +``contained_resolve()``. This test freezes the set of files that already +contain bare ``.resolve()`` calls: existing debt is grandfathered per +issue #463, but any *new* file reaching for bare ``.resolve()`` fails here +and must use the ``skillsaw.paths`` wrappers instead. Paying down a +grandfathered file is welcome — remove it from the allowlist when you do. +""" + +import ast +from pathlib import Path + +SRC_ROOT = Path(__file__).resolve().parent.parent / "src" / "skillsaw" + +# Files (relative to src/skillsaw) that contained bare ``.resolve()`` calls +# when the ratchet was introduced. Do not add to this list — new code must +# use ``safe_resolve()`` / ``contained_resolve()`` from ``skillsaw.paths``. +# ``paths.py`` itself is exempt: it is where the wrappers live. +GRANDFATHERED_BARE_RESOLVE = frozenset( + { + "baseline.py", + "blocks/coderabbit.py", + "blocks/frontmatter.py", + "blocks/promptfoo.py", + "cli/_badge.py", + "cli/_baseline.py", + "cli/_explain.py", + "cli/_helpers.py", + "cli/_lint.py", + "config.py", + "context.py", + "docs/extractor.py", + "formats/promptfoo.py", + "formatters/text.py", + "lint_target.py", + "lint_tree.py", + "linter.py", + "marketplace/add.py", + "marketplace/init.py", + "rules/builtin/agentskills/unreferenced_files.py", + "rules/builtin/codex/marketplace_registration.py", + "rules/builtin/commands/naming.py", + "rules/builtin/content/broken_internal_reference.py", + "rules/builtin/content/instruction_drift.py", + "rules/builtin/content/unlinked_internal_reference.py", + "rules/builtin/instructions/imports_valid.py", + "rules/builtin/marketplace/registration.py", + "rules/builtin/plugins/json_required.py", + "utils.py", + } +) + + +def _files_with_bare_resolve() -> set: + """Relative paths of source files containing a bare ``.resolve()`` call. + + The scan is AST-based so docstrings and comments mentioning + ``path.resolve()`` do not count — only an actual attribute call named + ``resolve`` does. ``safe_resolve()``/``contained_resolve()`` are plain + function calls, not attribute calls, so they never match. + """ + offenders = set() + for py in sorted(SRC_ROOT.rglob("*.py")): + rel = py.relative_to(SRC_ROOT).as_posix() + if rel == "paths.py": + continue + tree = ast.parse(py.read_text(encoding="utf-8"), filename=str(py)) + for node in ast.walk(tree): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "resolve" + ): + offenders.add(rel) + break + return offenders + + +class TestBannedApis: + def test_no_new_bare_resolve_calls(self): + offenders = _files_with_bare_resolve() + new_offenders = offenders - GRANDFATHERED_BARE_RESOLVE + assert not new_offenders, ( + "Bare Path.resolve() calls found in files outside the grandfathered " + f"allowlist: {sorted(new_offenders)}. Use safe_resolve() or " + "contained_resolve() from skillsaw.paths instead — Path.resolve() " + "raises on symlink loops, embedded NULs, and unreadable parents." + ) + + def test_allowlist_has_no_stale_entries(self): + """A paid-down file must also leave the allowlist, so the ratchet + only ever tightens.""" + offenders = _files_with_bare_resolve() + stale = GRANDFATHERED_BARE_RESOLVE - offenders + assert not stale, ( + "Allowlist entries no longer contain bare .resolve() calls — " + f"remove them from GRANDFATHERED_BARE_RESOLVE: {sorted(stale)}" + )