🛡️ Sentinel: [HIGH] Fix newline injection in skill name validation#100
🛡️ Sentinel: [HIGH] Fix newline injection in skill name validation#100calionauta wants to merge 1 commit into
Conversation
- 🚨 Severity: HIGH - 💡 Vulnerability: Regex patterns using `$` instead of `\Z` allowed skill names with trailing newlines to pass validation. - 🎯 Impact: If these strings are used in shell commands or file operations, it could lead to argument injection or path traversal. - 🔧 Fix: Replaced `$` with `\Z` in `src/agent_sync/publish/local_source.py` and `src/agent_sync/publish/external_source.py`. - ✅ Verification: Added `tests/test_publish_vulnerability.py` to confirm the vulnerability and verify the fix. Ran existing security and publish tests. 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 addresses a newline injection vulnerability in skill name validation by updating the regular expression in _is_valid_skill_name to use \Z instead of $ in both external_source.py and local_source.py. It also adds reproduction tests to verify this fix. The reviewer feedback highlights two key points: first, a security gap where the validation is not applied to skill names derived from repository IDs in _find_skills_in_repo; second, an opportunity to improve maintainability by consolidating the duplicated _is_valid_skill_name function into a shared module.
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.
| 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 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.
While fixing the newline injection in _is_valid_skill_name is a great security improvement, there is a critical gap in how this validation is applied.\n\nIn _find_skills_in_repo (around line 151), when a SKILL.md file is found at the root of the repository, the skill name is derived directly from the repository ID:\npython\nskill_name = repo_id.split('/')[-1]\n\nHowever, this derived skill_name is never validated using _is_valid_skill_name before being added to the skills list. If a repository name contains invalid characters, trailing newlines, or other malicious payloads, it will bypass validation entirely.\n\nPlease ensure that _is_valid_skill_name(skill_name) is also called and checked when processing root SKILL.md files.
| 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 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 helper function _is_valid_skill_name is duplicated exactly between src/agent_sync/publish/local_source.py and src/agent_sync/publish/external_source.py.\n\nTo improve maintainability and prevent future divergence, consider consolidating this validation logic into a shared module (such as src/agent_sync/publish/base.py) and importing it in both files.
1 participant
This PR fixes a security vulnerability where skill name validation in the publish modules was susceptible to newline injection.
In Python's
remodule, the$anchor matches either the end of the string or the position just before a newline at the end of the string. Using\Zensures an absolute end-of-string match, preventing trailing newlines from bypassing validation.Fixed in:
src/agent_sync/publish/local_source.pysrc/agent_sync/publish/external_source.pyAdded regression tests in
tests/test_publish_vulnerability.py.PR created automatically by Jules for task 10610656083544598381 started by @renatocaliari