🛡️ Sentinel: [CRITICAL] Prevent arbitrary code execution in AST type validation#378
🛡️ Sentinel: [CRITICAL] Prevent arbitrary code execution in AST type validation#378bashandbone wants to merge 1 commit into
Conversation
…validation Co-authored-by: bashandbone <89049923+bashandbone@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. |
Reviewer's GuideThis PR hardens the safe type-string evaluation in the DI container by whitelisting allowable function calls in the AST (preventing arbitrary code execution), and documents the new security finding in the Sentinel security log. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🤖 Hi @bashandbone, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @bashandbone, but I was unable to process your request. Please see the logs for more details. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider extracting the allowed function names for
ast.Callinto a shared constant (e.g.,_ALLOWED_TYPE_CALLS) so it’s easier to reuse, audit, and extend in one place. - When
func_namecannot be resolved (e.g., more complex call expressions), the error message will showForbidden function call in type string: None; you might want to special‑case this to give a clearer message about unsupported call shapes rather than printingNone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the allowed function names for `ast.Call` into a shared constant (e.g., `_ALLOWED_TYPE_CALLS`) so it’s easier to reuse, audit, and extend in one place.
- When `func_name` cannot be resolved (e.g., more complex call expressions), the error message will show `Forbidden function call in type string: None`; you might want to special‑case this to give a clearer message about unsupported call shapes rather than printing `None`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR hardens CodeWeaver’s DI container’s AST-based “safe eval” path for resolving string type annotations, addressing an arbitrary code execution risk caused by allowing unrestricted ast.Call evaluation during type string resolution.
Changes:
- Adds an
ast.Callwhitelist to the_safe_eval_type()AST validator to block arbitrary function calls during type evaluation. - Updates the Sentinel security log with details of the vulnerability, impact, and mitigation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/codeweaver/core/di/container.py |
Restricts which call expressions are allowed in type-annotation AST validation to reduce ACE risk during string type resolution. |
.jules/sentinel.md |
Documents the newly identified AST-call-based ACE vector and the mitigation approach. |
đź’ˇ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinstance(node, ast.Call): | ||
| func_name = None | ||
| if isinstance(node.func, ast.Name): | ||
| func_name = node.func.id | ||
| elif isinstance(node.func, ast.Attribute): | ||
| func_name = node.func.attr | ||
|
|
||
| if func_name not in { | ||
| "Depends", | ||
| "depends", | ||
| "Field", | ||
| "PrivateAttr", | ||
| "Tag", | ||
| "Parameter", | ||
| }: | ||
| raise TypeError(f"Forbidden function call in type string: {func_name}") | ||
|
|
| if func_name not in { | ||
| "Depends", | ||
| "depends", | ||
| "Field", | ||
| "PrivateAttr", |
🚨 Severity: CRITICAL
đź’ˇ Vulnerability: Found an Arbitrary Code Execution (ACE) vulnerability during dynamic type evaluation via safe
eval(). The AST validation didn't restrictast.Callnodes, allowing any arbitrary callable in the module's global namespace to be executed during type string resolution.🎯 Impact: A malicious type annotation string could invoke arbitrary functions that are available in the module's
globalns, resulting in arbitrary code execution.🔧 Fix: Implemented a strict whitelist for
ast.Callnodes. Now, only specific required functions likeDepends,depends,Field,PrivateAttr,Tag, andParametercan be called within type annotations during validation.âś… Verification: Ensure tests pass locally using
uv run pytest tests/unit/core/di.PR created automatically by Jules for task 9016044890702513400 started by @bashandbone
Summary by Sourcery
Harden AST-based type string evaluation to prevent arbitrary code execution from malicious type annotations.
Bug Fixes:
Documentation: