🛡️ Sentinel: [HIGH] Fix regex newline injection and data leakage in publish flow#114
🛡️ Sentinel: [HIGH] Fix regex newline injection and data leakage in publish flow#114calionauta wants to merge 1 commit into
Conversation
… publish flow This PR addresses two security vulnerabilities in the `publish` flow: 1. Regex Hardening: Replaced `$` with `\Z` in `_is_valid_skill_name` validators to prevent newline injection. 2. Robust File Exclusion: Refactored `_ignore_func` in `git_publish.py` to use `fnmatch.filter`. The previous implementation failed to exclude many default patterns like `sessions`, `cache`, and `models.json` because it only checked for patterns starting with `*.` or `.`. Verified with unit tests and reproduction scripts. 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 improves security and robustness by updating the regex validation in _is_valid_skill_name to use \Z instead of $ to prevent newline injection, refactoring the file exclusion logic in _ignore_func to use fnmatch.filter for robust pattern matching, and documenting these security learnings. The review feedback suggests optimizing the _ignore function by using a set to improve lookup performance for shutil.copytree from O(N) to O(1), and centralizing the duplicated _is_valid_skill_name function into a shared module to adhere to DRY principles.
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.
| 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 get all names matching the pattern | ||
| # This handles glob patterns like *.log and exact matches like sessions | ||
| matches = fnmatch.filter(names, pattern) | ||
| ignored.extend(matches) | ||
| return list(set(ignored)) # Deduplicate |
There was a problem hiding this comment.
We can optimize the _ignore function by using a set from the beginning. This avoids creating intermediate lists, eliminates the need to convert the list to a set and back to a list at the end, and returns a set directly. Since shutil.copytree performs membership checks (name in ignored_names) for each file/directory, returning a set improves the lookup complexity from
| 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 get all names matching the pattern | |
| # This handles glob patterns like *.log and exact matches like sessions | |
| matches = fnmatch.filter(names, pattern) | |
| ignored.extend(matches) | |
| return list(set(ignored)) # Deduplicate | |
| ignored = set() | |
| for pattern in patterns: | |
| # Use fnmatch.filter to get all names matching the pattern | |
| # This handles glob patterns like *.log and exact matches like sessions | |
| ignored.update(fnmatch.filter(names, pattern)) | |
| return ignored |
| 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)) |
There was a problem hiding this comment.
The _is_valid_skill_name function is duplicated identically between src/agent_sync/publish/external_source.py and src/agent_sync/publish/local_source.py. To adhere to the DRY (Don't Repeat Yourself) principle and ensure consistent validation rules across the codebase, consider centralizing this function in src/agent_sync/validators.py and importing it where needed.
1 participant
🛡️ Sentinel: Fix regex newline injection and flawed file exclusion in publish flow
This PR addresses two security vulnerabilities in the
publishflow:$with\Zin_is_valid_skill_namevalidators to prevent newline injection._ignore_funcingit_publish.pyto usefnmatch.filter. The previous implementation failed to exclude many default patterns likesessions,cache, andmodels.jsonbecause it only checked for patterns starting with*.or..Verified with unit tests and reproduction scripts.
PR created automatically by Jules for task 12878127919235723623 started by @renatocaliari