🛡️ Sentinel: [CRITICAL] Fix arbitrary code execution in AST eval#374
🛡️ Sentinel: [CRITICAL] Fix arbitrary code execution in AST eval#374bashandbone wants to merge 1 commit into
Conversation
This commit patches a critical vulnerability in `TypeValidator`'s AST evaluation inside `src/codeweaver/core/di/container.py` where `ast.Call` nodes were permitted without restriction. This lack of validation allowed Arbitrary Code Execution (ACE) via injection of malicious callables into string type annotations evaluated during dynamic dependency injection. The patch adds strict whitelist filtering for functions relevant only to DI/Pydantic configurations (`Depends`, `depends`, `Field`, `PrivateAttr`, `Tag`, and `Parameter`). It properly handles standard function resolution and updates the internal security journal. 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 GuideLocks down the AST-based type string evaluator used in the DI container by whitelisting allowed function calls inside annotations and updates the security Sentinel documentation with details of the vulnerability and its mitigation. 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 found 1 issue, and left some high level feedback:
- Instead of inlining
allowed_callsinside the visitor, consider hoisting it to a module-level constant or class attribute so it’s easier to share, audit, and update as the set of allowed callables evolves. - The blanket
# noqa: C901on_safe_eval_typesuggests the function is doing too much; consider extracting the AST validation logic (or at least the call-node checks) into a dedicated helper/visitor class to keep complexity localized and more maintainable. - The
ast.Callcheck currently whitelists by simple function/attribute name, which could match unintended symbols; consider tightening this by also validating the module/qualname or restricting the namespace from which these callables can be resolved.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of inlining `allowed_calls` inside the visitor, consider hoisting it to a module-level constant or class attribute so it’s easier to share, audit, and update as the set of allowed callables evolves.
- The blanket `# noqa: C901` on `_safe_eval_type` suggests the function is doing too much; consider extracting the AST validation logic (or at least the call-node checks) into a dedicated helper/visitor class to keep complexity localized and more maintainable.
- The `ast.Call` check currently whitelists by simple function/attribute name, which could match unintended symbols; consider tightening this by also validating the module/qualname or restricting the namespace from which these callables can be resolved.
## Individual Comments
### Comment 1
<location path="src/codeweaver/core/di/container.py" line_range="144-150" />
<code_context>
+ if isinstance(node, ast.Call):
+ if isinstance(node.func, ast.Name):
+ func_name = node.func.id
+ elif isinstance(node.func, ast.Attribute):
+ func_name = node.func.attr
+ else:
+ raise TypeError(f"Forbidden call function node type: {type(node.func).__name__}")
+
+ allowed_calls = {"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"}
+ if func_name not in allowed_calls:
+ raise TypeError(f"Forbidden function call in type string: {func_name}")
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Relying only on the function name for `ast.Attribute` calls may allow unexpected call targets.
Because only `node.func.attr` is checked for `ast.Attribute`, any `*.Depends` / `*.Field` / etc. call is accepted, regardless of what object actually provides the attribute. A user could supply an object in `globalns` with a malicious `.Depends` that still passes this check. To strengthen ACE protection, also validate the base (`node.func.value`) or resolve the callable from `globalns` and ensure it matches the expected functions instead of trusting the attribute name alone.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| elif isinstance(node.func, ast.Attribute): | ||
| func_name = node.func.attr | ||
| else: | ||
| raise TypeError(f"Forbidden call function node type: {type(node.func).__name__}") | ||
|
|
||
| allowed_calls = {"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"} | ||
| if func_name not in allowed_calls: |
There was a problem hiding this comment.
🚨 issue (security): Relying only on the function name for ast.Attribute calls may allow unexpected call targets.
Because only node.func.attr is checked for ast.Attribute, any *.Depends / *.Field / etc. call is accepted, regardless of what object actually provides the attribute. A user could supply an object in globalns with a malicious .Depends that still passes this check. To strengthen ACE protection, also validate the base (node.func.value) or resolve the callable from globalns and ensure it matches the expected functions instead of trusting the attribute name alone.
Security Fix ReviewI've reviewed this critical security fix for the arbitrary code execution vulnerability in AST evaluation. Here's my assessment: ✅ Security Fix AnalysisVulnerability Addressed: The fix correctly addresses a critical ACE vulnerability where arbitrary functions could be called during type annotation evaluation in the DI container's Fix Implementation: The patch adds a whitelist validation for
Test Coverage: Excellent! The codebase has comprehensive security tests in
🔍 Code QualityStrengths:
Minor Observations:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens CodeWeaver’s DI container string-annotation type resolution to prevent arbitrary code execution during AST-based evaluation of type strings.
Changes:
- Adds an allowlist for
ast.Callnodes in_safe_eval_typeto block unsafe function calls during type string evaluation. - Updates Sentinel security notes documenting the discovered AST-eval ACE/RCE vector 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 function calls are permitted in AST-validated type-string evaluation. |
.jules/sentinel.md |
Documents the vulnerability and the mitigation approach in Sentinel notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Security: Restrict ast.Call nodes to known safe functions to prevent arbitrary code execution (ACE). | ||
| # Allowing arbitrary functions (like 'eval' or malicious callables) inside type annotations is dangerous. | ||
| if isinstance(node, ast.Call): | ||
| if isinstance(node.func, ast.Name): | ||
| func_name = node.func.id | ||
| elif isinstance(node.func, ast.Attribute): | ||
| func_name = node.func.attr | ||
| else: | ||
| raise TypeError(f"Forbidden call function node type: {type(node.func).__name__}") | ||
|
|
||
| allowed_calls = {"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"} | ||
| if func_name not in allowed_calls: | ||
| raise TypeError(f"Forbidden function call in type string: {func_name}") |
| # Security: Restrict ast.Call nodes to known safe functions to prevent arbitrary code execution (ACE). | ||
| # Allowing arbitrary functions (like 'eval' or malicious callables) inside type annotations is dangerous. | ||
| if isinstance(node, ast.Call): |
🚨 Severity: CRITICAL
💡 Vulnerability:
_safe_eval_typerelied on unrestrictedast.Callevaluation inside string-based type annotations. Any malicious string passed containing functions mapping back to the global namespace could execute arbitrary code during resolution.🎯 Impact: Attackers providing malicious strings to evaluation mechanisms could hijack execution flow or gain RCE through
eval()bypass by utilizing python's internal mechanisms like__builtins__['eval']()or imported OS functions.🔧 Fix: Implemented explicit whitelist checks to limit allowed
ast.Callnodes uniquely to valid Dependency Injection / Pydantic field definitions (Depends,depends,Field,PrivateAttr,Tag,Parameter), explicitly raisingTypeErrorfor arbitrary ones.✅ Verification: Ensure the targeted unit test coverage
pytest tests/unit/core/diandmise //:checkpassed flawlessly. Tested locally mimicking an ACE exploit and successfully blocked the behavior.PR created automatically by Jules for task 1920070606635761678 started by @bashandbone
Summary by Sourcery
Harden AST-based type string evaluation to prevent arbitrary code execution via unsafe function calls in dependency injection type resolution.
Bug Fixes:
Documentation: