Cron absolute skill path security check correction

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #60109 fix(cron): normalize absolute skill paths before skill_view (#59824)

  merged to main

  files: agent/skill_commands.py, agent/skill_utils.py, cron/scheduler.py, tests/agent/test_skill_utils.py, tests/cron/test_scheduler.py

  Show the diff

  • agent/skill_commands.py

    @@ -143,37 +143,9 @@ def _load_skill_payload(skill_identifier: str, task_id: str | None = None) -> tu
     
         try:
             from tools.skills_tool import SKILLS_DIR, skill_view
    -        from agent.skill_utils import get_external_skills_dirs
    -
    -        identifier_path = Path(raw_identifier).expanduser()
    -        if identifier_path.is_absolute():
    -            normalized = None
    -            trusted_roots = [SKILLS_DIR]
    -            try:
    -                trusted_roots.extend(get_external_skills_dirs())
    -            except Exception:
    -                pass
    -
    -            # Prefer the lexical path under a trusted skill root before
    -            # resolving symlinks.  Slash-command discovery can legitimately
    -            # find a skill via ~/.hermes/skills/<name> where <name> is a
    -            # symlink to a checked-out skill elsewhere.  Resolving first turns
    -            # that trusted visible path into an arbitrary absolute path that
    -            # skill_view() refuses to load.
    -            for root in trusted_roots:
    -                try:
    -                    normalized = str(identifier_path.relative_to(root))
    -                    break
    -                except ValueError:
    -                    continue
    +        from agent.skill_utils import normalize_skill_lookup_name
     
    -            if normalized is None:
    -                try:
    -                    normalized = str(identifier_path.resolve().relative_to(SKILLS_DIR.resolve()))
    -                except Exception:
    -                    normalized = raw_identifier
    -        else:
    -            normalized = raw_identifier.lstrip("/")
    +        normalized = normalize_skill_lookup_name(raw_identifier)
     
             loaded_skill = json.loads(
                 skill_view(normalized, task_id=task_id, preprocess=False)

  • agent/skill_utils.py

    @@ -507,6 +507,63 @@ def get_all_skills_dirs() -> List[Path]:
         return dirs
     
     
    +def normalize_skill_lookup_name(identifier: str) -> str:
    +    """Normalize a skill identifier to a ``skill_view()``-safe relative path.
    +
    +    Slash commands and cron jobs may store absolute paths to skills that live
    +    under ``~/.hermes/skills/`` (including via symlinks) or configured
    +    ``skills.external_dirs``. ``skill_view()`` rejects absolute names for
    +    security, so callers must translate trusted absolute paths to their
    +    relative form first.
    +    """
    +    raw_identifier = (identifier or "").strip()
    +    if not raw_identifier:
    +        return raw_identifier
    +
    +    identifier_path = Path(raw_identifier).expanduser()
    +    if not identifier_path.is_absolute():
    +        return raw_identifier.lstrip("/")
    +
    +    # Look the primary skills root up on tools.skills_tool at CALL time
    +    # (not via get_skills_dir()): callers and tests patch
    +    # ``tools.skills_tool.SKILLS_DIR`` and skill_view() itself resolves
    +    # against that module attribute, so normalization must agree with the
    +    # exact root skill_view() will enforce.  Import deferred to avoid a
    +    # module cycle (tools.skills_tool imports agent.skill_utils).
    +    try:
    +        from tools import skills_tool as _skills_tool
    +        primary_root = Path(_skills_tool.SKILLS_DIR)
    +    except Exception:
    +        primary_root = get_skills_dir()
    +
    +    trusted_roots = [primary_root]
    +    try:
    +        trusted_roots.extend(get_external_skills_dirs())
    +    except Exception:
    +        pass
    +
    +    # Prefer the lexical path under a trusted skill root before resolving
    +    # symlinks. Slash-command discovery can legitimately find a skill via
    +    # ~/.hermes/skills/<name> where <name> is a symlink to a checked-out
    +    # skill elsewhere. Resolving first turns that trusted visible path into
    +    # an arbitrary absolute path that skill_view() refuses to load.
    +    for root in trusted_roots:
    +        try:
    +            return str(identifier_path.relative_to(root))
    +        except ValueError:
    +            continue
    +
    +    try:
    +        return str(identifier_path.resolve().relative_to(primary_root.resolve()))
    +    except Exception:
    +        logger.debug(
    +            "Skill identifier %r is an absolute path outside trusted skills "
    +            "roots — passing through unchanged (skill_view will reject it)",
    +            raw_identifier,
    +        )
    +        return raw_identifier
    +
    +
     def _resolve_for_skill_ownership(path) -> Path:
         path_obj = path if isinstance(path, Path) else Path(str(path))
         try:

  • cron/scheduler.py

    @@ -2195,6 +2195,7 @@ def _build_job_prompt(job: dict, prerun_script: Optional[tuple] = None) -> str:
         from tools.skills_tool import skill_view
         from tools.skill_usage import bump_use
         from agent.skill_bundles import build_bundle_invocation_message, resolve_bundle_command_key
    +    from agent.skill_utils import normalize_skill_lookup_name
     
         parts = []
         skipped: list[str] = []
    @@ -2225,7 +2226,7 @@ def _build_job_prompt(job: dict, prerun_script: Optional[tuple] = None) -> str:
                 continue
     
             try:
    -            loaded = json.loads(skill_view(skill_name))
    +            loaded = json.loads(skill_view(normalize_skill_lookup_name(skill_name)))
             except (json.JSONDecodeError, TypeError):
                 logger.warning("Cron job '%s': skill '%s' returned invalid JSON, skipping", job.get("name", job.get("id")), skill_name)
                 skipped.append(skill_name)

  • tests/agent/test_skill_utils.py

    @@ -333,3 +333,45 @@ def test_macos_skill_on_real_macos_unaffected(self):
                 "agent.skill_utils.is_termux", return_value=False
             ):
                 assert skill_matches_platform(fm) is True
    +
    +
    +class TestNormalizeSkillLookupName:
    +    def test_relative_path_unchanged(self, tmp_path, monkeypatch):
    +        from agent.skill_utils import normalize_skill_lookup_name
    +
    +        # Relative identifiers early-return before any root lookup.
    +        assert normalize_skill_lookup_name("foo/bar") == "foo/bar"
    +
    +    def test_absolute_under_skills_dir_becomes_relative(self, tmp_path, monkeypatch):
    +        from agent.skill_utils import normalize_skill_lookup_name
    +
    +        skills_dir = tmp_path / "skills"
    +        skill_dir = skills_dir / "category" / "my-skill"
    +        skill_dir.mkdir(parents=True)
    +        # Patch the root skill_view() itself enforces — normalization reads
    +        # tools.skills_tool.SKILLS_DIR at call time so the two stay in sync.
    +        monkeypatch.setattr("tools.skills_tool.SKILLS_DIR", skills_dir)
    +        assert normalize_skill_lookup_name(str(skill_dir)) == "category/my-skill"
    +
    +    def test_absolute_via_symlink_uses_lexical_relative_path(self, tmp_path, monkeypatch):
    +        from agent.skill_utils import normalize_skill_lookup_name
    +
    +        skills_dir = tmp_path / "skills"
    +        skills_dir.mkdir()
    +        external = tmp_path / "external" / "my-skill"
    +        external.mkdir(parents=True)
    +        link = skills_dir / "my-skill"
    +        try:
    +            link.symlink_to(external)
    +        except OSError:
    +            pytest.skip("Symlinks not supported")
    +        monkeypatch.setattr("tools.skills_tool.SKILLS_DIR", skills_dir)
    +        assert normalize_skill_lookup_name(str(link)) == "my-skill"
    +
    +    def test_untrusted_absolute_returned_unchanged(self, tmp_path, monkeypatch):
    +        from agent.skill_utils import normalize_skill_lookup_name
    +
    +        monkeypatch.setattr("tools.skills_tool.SKILLS_DIR", tmp_path / "skills")
    +        monkeypatch.setattr("agent.skill_utils.get_skills_dir", lambda: tmp_path / "skills")
    +        outside = str(tmp_path / "outside" / "skill")
    +        assert normalize_skill_lookup_name(outside) == outside

  • tests/cron/test_scheduler.py

    @@ -2885,6 +2885,31 @@ def _mixed_skill_view(name: str) -> str:
             assert "go" in result
     
     
    +class TestBuildJobPromptAbsoluteSkillPath:
    +    """Cron jobs may store absolute skill paths; normalize before skill_view."""
    +
    +    def test_absolute_skill_path_normalized_before_skill_view(self, tmp_path):
    +        skills_dir = tmp_path / "skills"
    +        skill_dir = skills_dir / "alpha-skill"
    +        skill_dir.mkdir(parents=True)
    +        (skill_dir / "SKILL.md").write_text("# Alpha\nDo alpha.")
    +        absolute_path = str(skill_dir)
    +        seen_names: list[str] = []
    +
    +        def _skill_view(name: str) -> str:
    +            seen_names.append(name)
    +            if name == "alpha-skill":
    +                return json.dumps({"success": True, "content": "# Alpha\nDo alpha."})
    +            return json.dumps({"success": False, "error": f"Skill '{name}' not found."})
    +
    +        with patch("tools.skills_tool.SKILLS_DIR", skills_dir), \
    +             patch("tools.skills_tool.skill_view", side_effect=_skill_view):
    +            result = _build_job_prompt({"skills": [absolute_path], "prompt": "go"})
    +
    +        assert seen_names == ["alpha-skill"]
    +        assert "Do alpha." in result
    +
    +
     class TestBuildJobPromptBumpUse:
         """Verify that cron jobs bump skill usage counters so the curator sees them as active."""