🛡️ Sentinel: [HIGH] Harden publish flow against leakage and injection - #117
🛡️ Sentinel: [HIGH] Harden publish flow against leakage and injection#117calionauta wants to merge 550 commits into
Conversation
- Material for MkDocs theme (beautiful, modern) - Auto-generate CLI docs from Click decorators via mkdocs-click - GitHub Actions workflow for automatic deployment on push - Organized docs structure (agents/, protocols/, examples/) - Dark mode, instant navigation, search, code copy - By cali (Renato Caliari)
- Restored index.html (main landing page) - Added unified deploy workflow - Docs at /docs (MkDocs generated) - By cali (Renato Caliari)
- By cali (Renato Caliari)
- Created scripts/generate_cli_docs.py that extracts command info from Click - Updated GitHub Actions to run script before building docs - Generates complete CLI reference with options and subcommands - Works with nested command groups (ExtendedHelpGroup) - By cali (Renato Caliari)
- Added docs/protocols/gitagent.md with complete GitAgent info - Added GitAgent to navigation (between DotAgents and comparison) - Workflow guaranteed: index.html → root, docs → /docs - By cali (Renato Caliari)
- Workflow: index.html → root, site_docs/* → /docs/ (except index.html) - Docs index renamed to home.html to avoid conflict - Removed dotagents-comparison.md (not useful for users) - By cali (Renato Caliari)
- docs.yml was deploying MkDocs to root (wrong!) - deploy.yml correctly puts index.html → root, docs → /docs - By cali (Renato Caliari)
- mkdir -p _site comes before cp - Added verification steps for debugging - By cali (Renato Caliari)
- Copy entire site_docs folder to _site/docs - Remove docs/index.html to avoid conflict with root index - Simpler approach that should work - By cali (Renato Caliari)
- SkillsDiff imports Config internally, so patch target must be skills_diff - Also simplified deploy workflow for docs - By cali (Renato Caliari)
- SkillsDiff imports Config inside __init__, so can't patch at module level - Test now uses tmp_path and monkeypatch to test without repo configured - By cali (Renato Caliari)
- Test now checks for 'Skills Divergence Report' instead of 'No repository' - Command actually works and shows local skills even without repo - By cali (Renato Caliari)
- Some runners have repo configured, others don't - Test should accept either output - By cali (Renato Caliari)
- Root has index.html (main site) - /docs has everything (MkDocs site, including its own index.html) - Simple cp -r approach - By cali (Renato Caliari)
Three-stage pipeline: 1. test - Runs tests first (must pass) 2. build-docs - Builds and validates docs structure 3. deploy - Verifies structure before upload Key validations: - Both root index.html AND docs/index.html must exist - All required doc pages must be present - Tests must pass before anything deploys Also added scripts/validate-deploy.sh for local validation. - By cali (Renato Caliari)
- build-docs uploads as 'site-docs' - deploy downloads 'site-docs' and creates 'github-pages' - This avoids the 'multiple artifacts' error - By cali (Renato Caliari)
… command - Add skip_security_panel and skip_confirm params to publish_skills() - Unified publish command now shows security panel once before execution - Skills and agents publishing no longer re-show the same warning
- Skills now use the same security_scanner as agents - Skills are scanned for: API keys, tokens, private URLs, absolute paths - Files with dangerous names (auth, token, key, secret) are skipped - Flagged files are published with warning (same behavior as agents) - Unified security message in CLI publish command See: https://github.com/renatocaliari/agent-sync-public
…epos
- Add templates/README_public_repo.md and templates/README_skills.md
- Publish now creates proper README files automatically
- Templates use {username}, {repo_name}, {full_repo_name} placeholders
- Removes old generate_readme() (was basic, replaced by templates)
- Fixes duplicate imports in publish.py
- Skills now show detailed issues (like agents) in Phase 3b - Count flagged files before confirm prompt - Warn user about flagged files before publishing - Fix MarkupError with [bold] instead of [green] - Fix outdated reference to agent-sync-configs → agent-sync-private
- Add _interactive_flagged_selection() helper (DRY for skills and agents) - Add _render_flagged_table() helper for consistent display - Unified selection flow: user picks which flagged items to publish - Clean up duplicate _git_push function in publish.py - Update security warning message to reflect interactive selection - Remove redundant confirmation when flagged items already selected
Tests added: - TestRenderFlaggedTable (3 tests) - TestReadmeGeneration (4 tests) - TestInteractiveFlaggedSelection (1 passing, 2 need more mocking) - Integration tests for --skills, --agents, and --all flags Passing tests: 8/17 CLI integration tests show interactive selection working correctly
Result: {"status":"keep","metric":243,"agent_cards":0,"feature_cards":0,"loc":0,"tests_passed":44,"total_tests":44}
Fixed UnboundLocalError in publish command when do_agents=True but do_skills=False. The skills_flagged variable was only initialized inside the do_skills block.
- Fixed test assertions to match new unified security warning - Added _interactive_flagged_selection mock to all tests - 45 tests passing across publish, security, validators
- test_publish_cli.py: 9 tests for unified publish command - test_publish.py: 5 tests for publish functionality - test_publish_interactive.py: 17 tests for interactive selection - test_security_scanner.py: 24 tests for security patterns - test_validators.py: 7 tests for URL/repo validation - Fixed UnboundLocalError bug in cli.py - Total: 257+ tests passing
- Removed KEY_API, KEY_SECRET from expected rules (not in PATTERNS) - Added SSH_KEY to expected rules - Fixed test_all_critical_have_critical_severity to handle 4-element tuples - Fixed SyntaxWarning for C:\ escape sequence in docstring
Result: {"status":"keep","metric":243,"agent_cards":0,"feature_cards":0,"loc":0,"tests_passed":258,"total_tests":258}
- _stage_skills() now checks git ls-tree HEAD before deleting repo working tree skills not in hub. Skills tracked by HEAD are preserved. - _sync_from_repo() now respects dry_run parameter. Breaks the cycle: hub accidentally loses skills → push propagates loss → skills retired forever. Now push is additive-safe by default.
…orphans - Adds orphan warning in normal push flow when there are changes (previously only shown in 'no changes' and '--strict' paths) - Replaces duplicate _warn_about_orphans definitions with single fn - Updates message to reflect HEAD guard: orphans PRESERVED by default - Adds warning log when HEAD git ls-tree fails (was silent)
- Removed deprecated publish group entirely (no legacy code) - Updated test_publish_private_sync.py docstrings and comments - Updated test_repos_commands.py invocations from publish to share - Renamed help category from 'Share & Publish' to 'Share' - All 551 tests passing
- Extracted _internal_backup_flow() with full push logic - push delegates to _internal_backup_flow - backup also calls _internal_backup_flow - Fixes 'Context.__init__() got unexpected keyword argument dry_run' error
- Removed push CLI command (decorators + function) - Removed push from help categories - Removed all legacy alias mentions from README - Updated tests to use backup instead of push - Only backup and share commands remain
…dead code - Remove 'state.json' from EXCLUDE_PATTERNS: too generic, caused false positives (e.g. agentmemory-snapshots/state.json was excluded, breaking memory backups). User can still exclude specific state.json paths via sync.exclude in config. - Remove 21 lines of unreachable code in _should_exclude: leftover from commit 0a4d11e that replaced the function body but left the old version as dead code below the new return statement. All 551 existing tests pass.
Three related fixes for the backup stage's exclude logic:
1. **Custom user 'sync.exclude' now recurses into subdirs.** Previously the
user-supplied pattern list used a bare fnmatch() call, so
'exclude: ["node_modules/"]' only matched the literal 'node_modules'
filename — not 'node_modules/foo.js' or 'a/node_modules/b.js'. The
hardcoded EXCLUDE_PATTERNS had special-case recursion for
'pattern.endswith("/")' but the user list did not. Refactored to a
single _matches_pattern() helper used by both lists, supporting:
- 'node_modules/' (with slash) — recursive
- 'node_modules' (bare name) — recursive (convenience)
- '**/name' (any depth) — recursive
- '*.bak', '**/*.lock' — standard globs as before
- bare 'name' (without glob chars) — recursive in subdirs
2. **Add '.git/' to hardcoded EXCLUDE_PATTERNS.** Stops nested VCS
metadata from leaking into backups (e.g. agentmemory-snapshots/.git/
was being copied into the private repo, creating git-in-git).
3. **Update CHANGELOG.md with the v0.40.0-alpha and v0.41.0-alpha
entries** that were missing (last entry was v0.32.0-alpha from 12
days ago, with 7 intermediate alpha tags undocumented).
Tests: 8 new in TestExcludePatternMatching class covering recursion,
default .git/ exclusion, state.json preservation, glob behavior, and
defensive empty-pattern handling. 559/559 passing (was 551).
…tion agent_discovery.py: replace literal '/Users/cali/.pi/agent/AGENTS.md' in dataclass field example and get_available_agents() docstring with portable 'Path.home() / ".pi/agent/AGENTS.md"' form. tests/test_publish_agents.py: same fix for portability across CI runners (ubuntu-latest doesn't have /Users/cali). agent_registry.yaml: annotate extra_paths section explaining the pi.dev ~/.pi/ → ~/.pi/agent/ migration. All keys are wired to code via Agent._get_extra_paths() — do not remove. Inline comments mark which fallbacks are legacy-only vs. still-active.
…mlink
~/.pi/extensions and ~/.pi/themes are now symlinks to
~/.pi/agent/{extensions,themes} on this machine, so the dual-path
entries in extra_paths caused:
- Double-load risk in pi (same content at two paths)
- Double-backup in private repo (global_extensions/ + global_themes/ mirrored)
- Drift risk between canonical and legacy copies
Removed:
- global_extensions key (full)
- global_themes key (full)
- Second list entry from extensions and themes (no longer needed)
Kept (real content at root, no symlink):
- global_prompts (~/.pi/prompts — currently empty but root-level)
- global_skills_local (~/.pi/skills — legacy hub, may be populated)
- pyrightconfig (~/.pi/pyrightconfig.json — root file with content)
Other agents (claude-code, gemini-cli, qwen-code) untouched: only pi.dev
ever referenced ~/.pi/.
Context doc: replaced literal /Users/cali/.pi/agent/AGENTS.md with
portable Path.home() form.
The stage function _stage_pi_extra_paths had a category_map that omitted 'prompts' and 'themes', while the matching restore function _restore_pi_extra_paths listed both. Result: any content added to ~/.pi/agent/prompts/ or ~/.pi/agent/themes/ was never backed up to the private repo, but restore expected to find it there. Real-world impact: themes/opencode.json on disk (mtime Jun 21) was not being refreshed in the private repo, leaving a stale copy from May 21 that would have been restored on a rollback. Fix: add 'prompts' and 'themes' entries to the stage category_map. Added test_sync_stage.py with focused coverage: - prompts/ dir staged to configs/pi.dev/prompts/ - themes/ dir staged to configs/pi.dev/themes/ - extensions/ coverage unchanged (regression guard)
Symlinked ~/.pi/prompts -> ~/.pi/agent/prompts, then reduced registry to single entry (matching extensions/themes treatment). Removed the global_prompts key entirely. Without this, ~/.pi/prompts content would still be backed up to BOTH configs/pi.dev/prompts/ and configs/pi.dev/global_prompts/ — same dual-path duplication bug that was fixed for extensions/themes in commit 48f70a5.
Two changes:
1. Clarify 'No Symlinks' mandate scope. The rule was about agent-sync's
sync code (use Native/Config/Copy methods, not symlink fallbacks).
User-side FS symlinks to avoid dual-path duplication are out of
scope. Without this clarification, the recent symlinks at
~/.pi/{extensions,themes,prompts} could be misread as violations.
2. Add 'Dev Workflow' section after Architecture Mandates. Documents
how to verify the editable install is active (which agent-sync) and
the override recipe when the system install shadows local edits.
This is the workflow gap that allowed the recent 'tests pass
against OLD code' incident to go undetected for several commits.
Two pre-commit config fixes uncovered while validating the venv setup: 1. The run-tests hook was 'python -m pytest' which used whatever python was on PATH. If user was not in the venv, this would test the SYSTEM-installed agent_sync package (v0.42.0a0, old code) rather than local source edits. Same trap that the CLI symlink fix addressed. Changed entry to '.venv/bin/python -m pytest' for unambiguous behavior. 2. The pygrep-primary repo (https://github.com/pycqa/pygrep-primary) no longer exists upstream. Its presence in the config blocked pre-commit initialization for ALL hooks. Removed the dead entry; ruff-pre-commit (already present) covers lint needs. Also fixed: the local repo block had 'rev: v0.0.1' which is invalid for repo: local and was causing 'InvalidConfigError' on pre-commit run. Removed.
… orphans - prune_skills CLI was calling git commit without staging the deletion via _prune_orphan_skills, causing 'nothing to commit' error - SyncManager.push now warns about orphan skills when no changed files exist, so share run auto-sync doesn't silently ignore them - Added test_push_returns_empty_list_when_no_changes_but_orphans - Added TestSkillsPruneFlags smoke tests for --dry-run and --yes - Fixed stale install: added prompts/themes to category_map - Fixed pre-existing callable | None annotation crash on Python 3.14
Adds hooks/ to pi.dev extra_paths so ~/.pi/agent/hooks/ (hooks.yaml and auto-install configs) is backed up to the sync repo. - agent_registry.yaml: hooks extra_path entry - base.py: hooks_paths property - sync.py: hooks in stage + restore category maps - test_sync_stage.py: coverage test for hooks dir staging
- Fix hooks path: ~/.pi/agent/hooks/ (plural, wrong) → ~/.pi/agent/hook/ (singular, correct) - Remove bin, git, global_* entries from extra_paths (not config) - Remove dead code in sync.py (category_map, dir_categories, git skip blocks) - Remove dead properties from agents/base.py - Add pythonpath to pyproject.toml so tests use local source - Clean up test mock (remove dead attr assignments) - All comments in English
- After staging, remove any subdir in configs/pi.dev/ that's not in the current category_map values, single_file_map subdirs, or 'packages'. This handles renames (hooks/ → hook/) and removed entries (bin, git, global_*) — preventing stale orphan dirs in the private repo. - Print per-category file count during stage so users see what was backed up (hook/, prompts/, themes/, extensions/, lsp/, models/, pyrightconfig/). - Add test_orphan_dirs_pruned to lock in the cleanup behavior.
- Fix newline injection vulnerability in skill name validation by using \Z anchor. - Refactor _ignore_func in git_publish.py to use robust fnmatch.filter matching. - Prevent content leakage by disabling symlink following in shutil.copytree and shutil.copy2 during publish. - Add comprehensive security regression tests in tests/test_publish_security.py. Co-authored-by: renatocaliari <1719651+renatocaliari@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request hardens the publishing flow against symlink traversal and regex injection by using absolute end-of-string anchors in regex validation, adopting fnmatch for pattern matching, and configuring shutil copy functions to not follow symlinks. The reviewer identified a critical edge case where is_dir() returns True for directory symlinks, which would still trigger shutil.copytree and leak content; they suggested checking is_symlink() first. Additionally, the reviewer recommended using shutil.ignore_patterns for cleaner code and corrected incomplete sentences in the sentinel documentation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 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) |
There was a problem hiding this comment.
If src_path is a symbolic link pointing to a directory, src_path.is_dir() will return True. Calling shutil.copytree on it will follow the symlink and copy all of its target contents, which leads to the exact content leakage/information disclosure vulnerability this PR aims to prevent. To fix this, explicitly check if src_path is a symlink first and copy it as a symlink using shutil.copy2(..., follow_symlinks=False).
| 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) | |
| if src_path.is_symlink(): | |
| dest.parent.mkdir(parents=True, exist_ok=True) | |
| shutil.copy2(src_path, dest, follow_symlinks=False) | |
| elif src_path.is_dir(): | |
| # 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) | |
| # follow_symlinks=False ensures we don't accidentally copy target content | |
| shutil.copy2(src_path, dest, follow_symlinks=False) |
| 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) |
There was a problem hiding this comment.
If skill.path is a symbolic link pointing to a directory, skill.path.is_dir() will return True. Calling shutil.copytree on it will follow the symlink and copy all of its target contents, leading to content leakage. Explicitly check if skill.path is a symlink first and copy it as a symlink using shutil.copy2(..., follow_symlinks=False).
| 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) | |
| if skill.path.is_symlink(): | |
| dest.parent.mkdir(parents=True, exist_ok=True) | |
| shutil.copy2(skill.path, dest, follow_symlinks=False) | |
| elif skill.path.is_dir(): | |
| 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, follow_symlinks=False) |
| **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. |
There was a problem hiding this comment.
The sentences in this section are incomplete and contain missing words/subjects. Please complete them to ensure the documentation is clear and professional.
| **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. | |
| **Vulnerability:** The public skill publishing flow was vulnerable to information disclosure via symbolic link traversal and brittle file exclusion logic. Copy operations followed symlinks into sensitive areas, and the hand-rolled ignore function only handled a subset of intended patterns. | |
| **Learning:** `shutil.copytree` (before 3.12 or if not specified) and `shutil.copy2` follow symlinks by default. Hand-rolling glob matching is error-prone; `fnmatch.filter` is the robust standard. |
| 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 |
There was a problem hiding this comment.
Instead of manually re-implementing pattern filtering using fnmatch.filter, you can use Python's built-in shutil.ignore_patterns which is standard, highly optimized, and returns a set of ignored names directly.
| 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 | |
| return shutil.ignore_patterns(*patterns) |
This PR addresses several high-priority security issues in the
publishflow that could lead to information disclosure or command/argument injection._is_valid_skill_nameto use\Zinstead of$, preventing skill names with trailing newlines from bypassing validation.git_publish.pywith a robustfnmatch-based filter that correctly respects allDEFAULT_IGNORE_PATTERNS(e.g.,sessions/,models.json).shutiloperations to preserve symlinks as links rather than following them, preventing the accidental inclusion of sensitive system files in public skill repositories.Verification:
tests/test_publish_security.pyconfirms all fixes.PR created automatically by Jules for task 8613642642801538767 started by @renatocaliari