🛡️ Sentinel: [CRITICAL] Fix arbitrary code execution in type evaluation via restricted AST call whitelist#362
Conversation
…on via restricted AST call whitelist 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 GuideTightens the AST-based validation in _safe_eval_type to prevent arbitrary code execution by strictly whitelisting allowed function calls in type strings and records the incident and mitigation 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. |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider extracting the allowed function names (
Depends,depends,Field,PrivateAttr,Tag) into a clearly named module-level constant so the whitelist is easier to audit, reuse, and extend without touching the validator logic. - The new
ast.Callcheck only permits bareNamecalls and will reject patterns likemodule.Depends(...)or aliased imports; if that is intentional, it might be worth making this explicit in the inline comment or adjusting the logic to handle safeast.Attributeaccess patterns where appropriate.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the allowed function names (`Depends`, `depends`, `Field`, `PrivateAttr`, `Tag`) into a clearly named module-level constant so the whitelist is easier to audit, reuse, and extend without touching the validator logic.
- The new `ast.Call` check only permits bare `Name` calls and will reject patterns like `module.Depends(...)` or aliased imports; if that is intentional, it might be worth making this explicit in the inline comment or adjusting the logic to handle safe `ast.Attribute` access patterns where appropriate.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 dependency injection type-string resolution by tightening AST validation before eval() is used in Container._safe_eval_type, preventing arbitrary code execution through untrusted function calls embedded in type strings.
Changes:
- Restrict
ast.Callnodes during type-string AST validation to a small whitelist of permitted call names (Depends,depends,Field,PrivateAttr,Tag). - Add an inline security comment in the validator documenting the ACE risk and the mitigation.
- Record the vulnerability and mitigation in the Sentinel security log.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/codeweaver/core/di/container.py | Adds strict ast.Call whitelisting in TypeValidator to prevent eval-based ACE via callable invocation. |
| .jules/sentinel.md | Documents the ACE finding and the whitelisting mitigation in the Sentinel log. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🚨 Severity: CRITICAL
💡 Vulnerability: The
_safe_eval_typefunction insrc/codeweaver/core/di/container.pycontained an Arbitrary Code Execution (ACE) vulnerability. The customTypeValidatorloosely permitted anyast.Callnodes when checking type strings before evaluating them witheval(). This allowed the execution of any callable mapped in theglobalnsparameter.🎯 Impact: An attacker could potentially inject malicious functions or payloads into type string configurations, resulting in arbitrary code execution within the execution environment.
🔧 Fix: Implemented strict whitelisting for
ast.Callnodes insideTypeValidator.generic_visit(). It now only accepts function calls toDepends,depends,Field,PrivateAttr, andTag. An inline security comment was added addressing the ACE concern, as required.✅ Verification: Tested against a local script injecting a malicious function (
execute_evil) into a type string annotation. It now securely fails with aTypeErrorinstead of allowing the execution. Also ran the test suite locally ensuring no regression.PR created automatically by Jules for task 289020645167947115 started by @bashandbone
Summary by Sourcery
Harden type string evaluation by restricting allowed AST function calls and document the associated ACE vulnerability and mitigation in the Sentinel security log.
Bug Fixes:
Documentation: