🛡️ Sentinel: [HIGH] Fix security regressions in publish flow#106
🛡️ Sentinel: [HIGH] Fix security regressions in publish flow#106calionauta wants to merge 1 commit into
Conversation
Vulnerability: 1. Data leakage in publish ignore logic due to incorrect pattern matching. 2. Symlink content leakage during publishing (following symlinks to sensitive files). 3. Regex newline injection bypass in skill name validation. Fix: 1. Refactored `_ignore_func` to use `fnmatch.fnmatchcase` for robust, case-sensitive filtering. 2. Hardened `shutil.copytree` and `shutil.copy2` calls in the publish flow by setting `symlinks=True` and `follow_symlinks=False` respectively. 3. Updated `_is_valid_skill_name` regex anchors from `$` to `\Z` in both `local_source.py` and `external_source.py`. Verification: Consolidated regression tests in `tests/test_publish_security_regression.py` covering all three issues. All 563 project tests passed. 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces security hardening measures to prevent symlink content leakage and regex newline injection during the agent and skill publishing flows. It also adds comprehensive security regression tests. The reviewer provided valuable feedback pointing out that Path.is_dir() follows symlinks by default, which could still trigger shutil.copytree on directory symlinks and leak content. Additionally, the reviewer noted that the new fnmatch.fnmatchcase filter is case-sensitive, which could allow sensitive files with mixed casing to bypass the ignore filter on certain filesystems.
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.
| @@ -91,11 +89,16 @@ def do_git_publish( | |||
| for src_path, dest_name in items: | |||
| dest = items_dir / dest_name | |||
| if src_path.is_dir(): | |||
There was a problem hiding this comment.
In Python, Path.is_dir() follows symbolic links by default. If src_path is a symlink pointing to a directory, src_path.is_dir() will return True, causing shutil.copytree to be called. shutil.copytree will then follow the symlink and copy the entire contents of the target directory, leading to the exact symlink content leakage vulnerability this PR aims to prevent. To fix this, ensure that src_path is not a symlink before treating it as a directory.
| if src_path.is_dir(): | |
| if src_path.is_dir() and not src_path.is_symlink(): |
| @@ -162,11 +165,16 @@ def publish_all( | |||
| dest_name = f"{source_id}/{skill_name}" | |||
| dest = skills_dir / dest_name | |||
| if skill.path.is_dir(): | |||
There was a problem hiding this comment.
Similar to the issue in do_git_publish, skill.path.is_dir() follows symbolic links by default. If skill.path is a symlink pointing to a directory, it will be followed and its entire contents will be copied by shutil.copytree. To prevent symlink content leakage, ensure that skill.path is not a symlink before calling shutil.copytree.
| if skill.path.is_dir(): | |
| if skill.path.is_dir() and not skill.path.is_symlink(): |
| if name == pattern or name.startswith(pattern.rstrip('/') + '/'): | ||
| ignored.append(name) | ||
| break | ||
| if fnmatch.fnmatchcase(name, pattern): |
There was a problem hiding this comment.
The use of fnmatch.fnmatchcase performs a case-sensitive comparison. On case-insensitive filesystems (such as macOS or Windows), or if a user accidentally uses different casing, sensitive files like .Env, Models.json, or SESSIONS would bypass this ignore filter and be published. To prevent potential data leakage, perform a case-insensitive match by converting both the filename and the pattern to lowercase.
| if fnmatch.fnmatchcase(name, pattern): | |
| if fnmatch.fnmatchcase(name.lower(), pattern.lower()): |
1 participant
This PR fixes three critical security regressions discovered in the
publishworkflow:_ignore_funcwas failing to correctly match and exclude sensitive patterns likesessionsormodels.json. It was refactored to usefnmatch.fnmatchcase, which is more robust and standard.publishflow was usingshutil.copytreeandshutil.copy2without explicitly disabling symlink following. This could allow an attacker to trick the tool into uploading the content of sensitive files (e.g.,/etc/passwd) if a symlink was present in a skill or agent directory. Hardened these calls to preserve symlinks rather than following them.$anchor, which matches trailing newlines. This could lead to validation bypasses. Updated to use the absolute end-of-string anchor\Z.Comprehensive regression tests were added in
tests/test_publish_security_regression.py.PR created automatically by Jules for task 5881913216238059413 started by @renatocaliari