Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions src/skillsaw/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
104 changes: 104 additions & 0 deletions src/skillsaw/diagnostics.py
Original file line number Diff line number Diff line change
@@ -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
6 changes: 3 additions & 3 deletions src/skillsaw/docs/extractor.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down
69 changes: 5 additions & 64 deletions src/skillsaw/formats/codex.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve legacy imports for the moved path helpers

When an existing custom rule imports either previously exposed non-underscore helper (from skillsaw.formats.codex import safe_exists or safe_is_symlink), this import list no longer binds it, so custom-rule loading raises an ImportError and aborts the lint. The repository explicitly preserves legacy import locations for custom rules in tests/test_module_layering.py:9-14; keep compatibility re-exports here while making paths.py canonical.

AGENTS.md reference: AGENTS.md:L88-L89

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining: skillsaw.formats.codex does not exist on main or in any release — it was introduced on this unreleased feature branch, so no external code can import from it. Moving the helpers to their intended home before first release is exactly why no deprecation shim is warranted.

from skillsaw.utils import read_json


Expand Down Expand Up @@ -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.<predicate>()``, 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")
Comment thread
qodo-code-review[bot] marked this conversation as resolved.


def inline_documents(declared: Any, key: str) -> List[Dict[str, Any]]:
"""One document per inline object in a Codex manifest field.

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
4 changes: 1 addition & 3 deletions src/skillsaw/lint_tree.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading