🛡️ Sentinel: [HIGH] Harden publish flow against leakage and injection

.jules/sentinel.md

@@ -2,3 +2,13 @@
 **Vulnerability:** Regex patterns using `$` instead of `\Z` allowed trailing newlines to bypass validation.
 **Learning:** In Python's `re` module, `$` matches at the end of the string OR just before a newline at the end of the string. This can be exploited to inject arguments if the validated string is passed to a shell command.
 **Prevention:** Always use `\Z` for absolute end-of-string matching in security-critical regex validators.
+
+## 2026-07-01 - Publish Flow Content Leakage
+**Vulnerability:** The public skill publishing flow was vulnerable to information disclosure via symbolic link traversal and brittle file exclusion logic.  followed symlinks into sensitive areas, and the hand-rolled ignore function only handled a subset of intended patterns.
+**Learning:**  (before 3.12 or if not specified) and  follow symlinks by default. Hand-rolling glob matching is error-prone;  is the robust standard.
+**Prevention:** Always use `symlinks=True` in `shutil.copytree` and `follow_symlinks=False` in `shutil.copy2` for security-sensitive copies. Use `fnmatch` for pattern matching.
+
+## 2025-05-15 - Publish Flow Content Leakage & Regex Injection
+**Vulnerability:** The public skill publishing flow was vulnerable to information disclosure via symbolic link traversal and brittle file exclusion logic. Additionally, `_is_valid_skill_name` regex allowed newline injection.
+**Learning:** `shutil.copytree` and `shutil.copy2` follow symlinks by default, potentially leaking content from outside the source directory. Hand-rolling glob matching is error-prone; `fnmatch.filter` is the robust standard. The `$` regex anchor in Python matches before a trailing newline.
+**Prevention:** Always use `symlinks=True` in `shutil.copytree` and `follow_symlinks=False` in `shutil.copy2` for security-sensitive copies. Use `fnmatch.filter` for pattern matching. Always use `\Z` for absolute end-of-string regex matching.

src/agent_sync/publish/external_source.py

@@ -204,7 +204,7 @@ def _is_valid_skill_name(name: str) -> bool:
     """
     import re
     # Match: starts with letter, then letters/numbers/hyphens, ends with letter/number
-    pattern = r'^[a-z][a-z0-9]*(-[a-z0-9]+)*$'
+    pattern = r'^[a-z][a-z0-9]*(-[a-z0-9]+)*\Z'
     return bool(re.match(pattern, name))
 
 

src/agent_sync/publish/git_publish.py

@@ -30,19 +30,16 @@
 
 def _ignore_func(*patterns):
     """Create a callable that returns a list of filenames to ignore."""
+    import fnmatch
+
     def _ignore(path, names):
         ignored = []
-        for name in names:
-            for pattern in patterns:
-                if pattern.startswith('*.'):
-                    if name.endswith(pattern[1:]):
-                        ignored.append(name)
-                        break
-                elif pattern.startswith('.'):
-                    if name == pattern or name.startswith(pattern.rstrip('/') + '/'):
-                        ignored.append(name)
-                        break
-        return ignored
+        for pattern in patterns:
+            # Use fnmatch.filter to robustly match patterns against names
+            matches = fnmatch.filter(names, pattern)
+            ignored.extend(matches)
+        return list(set(ignored))  # Deduplicate
+
     return _ignore
 
 from rich.console import Console
@@ -91,11 +88,13 @@ def do_git_publish(
         for src_path, dest_name in items:
             dest = items_dir / dest_name
             if src_path.is_dir():
-                shutil.copytree(src_path, dest, dirs_exist_ok=True,
+                # symlinks=True prevents content leakage by preserving links as links
+                shutil.copytree(src_path, dest, dirs_exist_ok=True, symlinks=True,
                                ignore=_ignore_func(*DEFAULT_IGNORE_PATTERNS))
             else:
                 dest.parent.mkdir(parents=True, exist_ok=True)
-                shutil.copy2(src_path, dest)
+                # follow_symlinks=False ensures we don't accidentally copy target content
+                shutil.copy2(src_path, dest, follow_symlinks=False)
 
         # Generate README
         readme_generator(items_dir, items, repo)
@@ -162,11 +161,11 @@ def publish_all(
                             dest_name = f"{source_id}/{skill_name}"
                             dest = skills_dir / dest_name
                             if skill.path.is_dir():
-                                shutil.copytree(skill.path, dest, dirs_exist_ok=True,
+                                shutil.copytree(skill.path, dest, dirs_exist_ok=True, symlinks=True,
                                                ignore=_ignore_func(*DEFAULT_IGNORE_PATTERNS))
                             else:
                                 dest.parent.mkdir(parents=True, exist_ok=True)
-                                shutil.copy2(skill.path, dest)
+                                shutil.copy2(skill.path, dest, follow_symlinks=False)
                             skills_to_publish.append((skill.path, dest_name))
                             break
 
@@ -188,7 +187,7 @@ def publish_all(
                 agent = all_agents.get(agent_name)
                 if agent:
                     dest = agents_dir / f"{agent_name}.md"
-                    shutil.copy2(Path(agent.path), dest)
+                    shutil.copy2(Path(agent.path), dest, follow_symlinks=False)
                     agents_to_publish.append((Path(agent.path), f"agents/{agent_name}.md"))
 
             if agents_to_publish:

src/agent_sync/publish/local_source.py

@@ -87,7 +87,7 @@ def _is_valid_skill_name(name: str) -> bool:
     """
     import re
     # Match: starts with letter, then letters/numbers/hyphens, ends with letter/number
-    pattern = r'^[a-z][a-z0-9]*(-[a-z0-9]+)*$'
+    pattern = r'^[a-z][a-z0-9]*(-[a-z0-9]+)*\Z'
     return bool(re.match(pattern, name))
 
 

tests/test_publish_security.py

@@ -0,0 +1,76 @@
+import os
+import shutil
+import tempfile
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import pytest
+from agent_sync.publish.local_source import _is_valid_skill_name as local_valid
+from agent_sync.publish.external_source import _is_valid_skill_name as external_valid
+from agent_sync.publish.git_publish import _ignore_func, DEFAULT_IGNORE_PATTERNS, do_git_publish
+
+def test_skill_name_newline_injection_fixed():
+    """SECURE: Skill names with trailing newlines MUST be rejected."""
+    # These should now be False
+    assert local_valid("valid-skill\n") is False
+    assert external_valid("valid-skill\n") is False
+    assert local_valid("valid-skill") is True
+    assert external_valid("valid-skill") is True
+
+def test_ignore_func_robustness_fixed():
+    """SECURE: _ignore_func MUST correctly handle DEFAULT_IGNORE_PATTERNS."""
+    ignore = _ignore_func(*DEFAULT_IGNORE_PATTERNS)
+
+    names = ["sessions", "valid_file.txt", "models.json", ".git", "cache", "my.log"]
+    ignored = ignore(Path("/tmp"), names)
+
+    assert "sessions" in ignored
+    assert "models.json" in ignored
+    assert ".git" in ignored
+    assert "cache" in ignored
+    assert "my.log" in ignored
+    assert "valid_file.txt" not in ignored
+
+@patch("agent_sync.publish.git_publish.shutil.copytree")
+@patch("agent_sync.publish.git_publish.shutil.copy2")
+@patch("agent_sync.publish.git_publish.git_commit_and_push")
+def test_git_publish_hardening(mock_push, mock_copy2, mock_copytree):
+    """SECURE: do_git_publish must call shutil with security-hardened parameters."""
+    mock_readme = MagicMock()
+
+    with tempfile.TemporaryDirectory() as tmp_dir:
+        src_dir = Path(tmp_dir) / "src_dir"
+        src_dir.mkdir()
+        src_file = Path(tmp_dir) / "src_file.txt"
+        src_file.write_text("content")
+
+        items = [(src_dir, "dest_dir"), (src_file, "dest_file.txt")]
+
+        # Trigger publish
+        do_git_publish(items, "skills", mock_readme, 2, "skills", "https://github.com/repo")
+
+        # Verify copytree was called with symlinks=True
+        assert mock_copytree.called
+        # It's called once for the directory
+        kwargs = mock_copytree.call_args.kwargs
+        assert kwargs.get("symlinks") is True, "Security: copytree MUST preserve symlinks"
+
+        # Verify copy2 was called with follow_symlinks=False
+        assert mock_copy2.called
+        kwargs = mock_copy2.call_args.kwargs
+        assert kwargs.get("follow_symlinks") is False, "Security: copy2 MUST NOT follow symlinks"
+
+def test_symlink_preservation_behavior():
+    """Verify that shutil.copytree with symlinks=True actually preserves links."""
+    with tempfile.TemporaryDirectory() as tmp_dir:
+        src = Path(tmp_dir) / "src"
+        src.mkdir()
+        secret = Path(tmp_dir) / "secret"
+        secret.write_text("shh")
+        link = src / "link"
+        link.symlink_to(secret)
+
+        dest = Path(tmp_dir) / "dest"
+        shutil.copytree(src, dest, symlinks=True)
+
+        assert (dest / "link").is_symlink(), "Expected symlink to be preserved"