🛡️ Sentinel: [HIGH] Fix data leakage and newline injection in publish flow

.jules/sentinel.md

@@ -2,3 +2,19 @@
 **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.
+
+## 2025-05-15 - [HIGH] Data Leakage and Newline Injection in Publish Flow
+**Vulnerability:**
+1. Brittle file exclusion logic in `git_publish.py` failed to ignore sensitive directories like `sessions/` and `cache/`.
+2. Newline injection was possible in skill name validation regexes due to usage of `$` anchor.
+3. Content leakage via symbolic links: `shutil.copytree` followed symlinks by default, potentially copying sensitive files from outside the skill directory.
+
+**Learning:**
+1. Hand-rolling directory exclusion logic is error-prone; standard libraries like `fnmatch` should be used for pattern matching.
+2. The `$` anchor in Python regex matches either the end of the string or the position before a newline at the end of the string. `\Z` should always be used for strict end-of-string matching in security-sensitive validations.
+3. `shutil.copytree` and `shutil.copy2` follow symlinks by default. In multi-tenant or user-controlled environments, this can lead to data leakage if a user can create a symlink to a sensitive file.
+
+**Prevention:**
+1. Use `fnmatch.filter` for robust file/directory exclusion patterns.
+2. Use `\Z` instead of `$` in validation regexes.
+3. Explicitly set `symlinks=True` in `copytree` or `follow_symlinks=False` in `copy2` when handling user-provided directory structures.

src/agent_sync/publish/external_source.py

@@ -199,12 +199,14 @@ def _find_skills_in_repo(cache_path: Path, source: SourceConfig) -> list[SkillSo
 
 def _is_valid_skill_name(name: str) -> bool:
     """Check if skill name is valid.
-    
+
     Valid: lowercase, numbers, hyphens. No leading/trailing/consecutive hyphens.
     """
     import re
+
     # Match: starts with letter, then letters/numbers/hyphens, ends with letter/number
-    pattern = r'^[a-z][a-z0-9]*(-[a-z0-9]+)*$'
+    # Use \Z instead of $ to prevent newline injection
+    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))
+
     return _ignore
 
 from rich.console import Console
@@ -91,11 +88,19 @@ 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,
-                               ignore=_ignore_func(*DEFAULT_IGNORE_PATTERNS))
+                # Preserve symlinks as links instead of following them (security)
+                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)
+                # copy2 follows symlinks by default; we must use follow_symlinks=False
+                # to prevent leaking content from outside the skill directory.
+                shutil.copy2(src_path, dest, follow_symlinks=False)
 
         # Generate README
         readme_generator(items_dir, items, repo)
@@ -162,11 +167,17 @@ 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,
-                                               ignore=_ignore_func(*DEFAULT_IGNORE_PATTERNS))
+                                # Preserve symlinks (security)
+                                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
 

src/agent_sync/publish/local_source.py

@@ -82,12 +82,14 @@ def _create_skill_source(path: Path) -> SkillSource | None:
 
 def _is_valid_skill_name(name: str) -> bool:
     """Check if skill name is valid.
-    
+
     Valid: lowercase, numbers, hyphens. No leading/trailing/consecutive hyphens.
     """
     import re
+
     # Match: starts with letter, then letters/numbers/hyphens, ends with letter/number
-    pattern = r'^[a-z][a-z0-9]*(-[a-z0-9]+)*$'
+    # Use \Z instead of $ to prevent newline injection
+    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,52 @@
+import pytest
+import re
+from pathlib import Path
+import shutil
+import tempfile
+from agent_sync.publish.git_publish import _ignore_func, DEFAULT_IGNORE_PATTERNS
+from agent_sync.publish.local_source import _is_valid_skill_name as _is_valid_local
+from agent_sync.publish.external_source import _is_valid_skill_name as _is_valid_external
+
+def test_ignore_func_fix():
+    # Verifies that _ignore_func correctly ignores 'sessions', 'cache', 'models.json'
+    ignore = _ignore_func(*DEFAULT_IGNORE_PATTERNS)
+
+    names = ['sessions', 'cache', 'models.json', 'SKILL.md', '.git', 'test.log']
+    ignored = ignore(None, names)
+
+    # These SHOULD be ignored
+    should_be_ignored = {'sessions', 'cache', 'models.json', '.git', 'test.log'}
+
+    for name in should_be_ignored:
+        assert name in ignored, f"Fix Failed: {name} was NOT ignored but should be!"
+
+    assert 'SKILL.md' not in ignored
+
+def test_skill_name_newline_injection_fix():
+    # Verifies that _is_valid_skill_name rejects trailing newlines
+    name_with_newline = "valid-skill\n"
+
+    assert _is_valid_local(name_with_newline) is False, "Fix Failed: local_source still accepts trailing newline"
+    assert _is_valid_external(name_with_newline) is False, "Fix Failed: external_source still accepts trailing newline"
+
+def test_copytree_does_not_follow_symlinks_fix():
+    # Verifies that symlinks are preserved as links (security fix)
+    with tempfile.TemporaryDirectory() as tmp_dir:
+        src = Path(tmp_dir) / "src"
+        dest = Path(tmp_dir) / "dest"
+        src.mkdir()
+
+        secret_file = Path(tmp_dir) / "secret.txt"
+        secret_file.write_text("sensitive data")
+
+        # Create a symlink inside src pointing to secret_file outside src
+        link = src / "link_to_secret"
+        link.symlink_to(secret_file)
+
+        # In git_publish.py we now use symlinks=True
+        shutil.copytree(src, dest, symlinks=True)
+
+        assert dest / "link_to_secret" in dest.iterdir()
+        assert (dest / "link_to_secret").is_symlink(), "Fix Failed: symlink was followed!"
+        # It should still point to the original secret_file (or at least be a symlink)
+        assert (dest / "link_to_secret").readlink() == secret_file