🛡️ Sentinel: [CRITICAL] Fix Arbitrary Code Execution in dynamic type evaluation#383
🛡️ Sentinel: [CRITICAL] Fix Arbitrary Code Execution in dynamic type evaluation#383bashandbone wants to merge 1 commit into
Conversation
…evaluation 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 GuideAdds a strict allowlist for function calls in dynamic type annotation evaluation to fix an arbitrary code execution vulnerability, and documents the issue and mitigation in the Sentinel security history. 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 set of allowed function names (and the cyclopts.Parameter rule) into a shared constant or helper so it’s easier to reuse and extend without touching the AST walk logic.
- The error message
Forbidden function call in type string.could include the offending function name (node.func.id/attr) to make debugging misconfigurations or legitimate new use cases easier while still not leaking internal details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the set of allowed function names (and the cyclopts.Parameter rule) into a shared constant or helper so it’s easier to reuse and extend without touching the AST walk logic.
- The error message `Forbidden function call in type string.` could include the offending function name (`node.func.id`/`attr`) to make debugging misconfigurations or legitimate new use cases easier while still not leaking internal details.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
Hardens the DI container’s string-based type resolution to prevent arbitrary code execution by restricting which AST function-call nodes are permitted before evaluating type strings.
Changes:
- Added an explicit allowlist for
ast.Callnodes during AST validation of type strings to block unexpected function execution. - Documented the dynamic type-evaluation ACE vulnerability and the mitigation in the Sentinel incident log.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/codeweaver/core/di/container.py |
Adds an allowlist gate for ast.Call nodes during AST validation prior to eval() in _safe_eval_type(). |
.jules/sentinel.md |
Records the ACE vulnerability and the high-level prevention strategy for future reference. |
đź’ˇ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Prevent Arbitrary Code Execution (ACE) vulnerability by strictly whitelisting allowed function calls | ||
| if isinstance(node, ast.Call): | ||
| is_allowed = False | ||
| # Allow standard function names | ||
| if (isinstance(node.func, ast.Name) and node.func.id in { |
🚨 Severity: CRITICAL
đź’ˇ Vulnerability: An Arbitrary Code Execution (ACE) vulnerability was found in
src/codeweaver/core/di/container.pyduring dynamic type evaluation. Genericast.Callnodes were permitted when parsing string-based type annotations usingast.parseand subsequently evaluated viaeval().🎯 Impact: Malicious configuration inputs or strings injected during type resolution could execute arbitrary callable functions in the application's global namespace, severely compromising the system's security.
🔧 Fix: Implemented an explicit allowlist in the
TypeValidatorclass when traversingast.Callnodes. Now, only safe, explicitly allowed functions (Depends,Field,PrivateAttr,Tag,Parameter, andcyclopts.Parameter) are permitted to be evaluated. Any unrecognized or malicious function calls will securely raise aTypeError.âś… Verification: Unit tests covering the dependency injection container and resolution mechanics were executed and passed successfully. No regressions were introduced into the project. The changes were validated against static analysis (Ruff) and security linters.
PR created automatically by Jules for task 14565857484303462979 started by @bashandbone
Summary by Sourcery
Harden dynamic type evaluation in the DI container to prevent arbitrary code execution and document the security fix in Sentinel notes.
Bug Fixes:
Documentation: