🛡️ Sentinel: [CRITICAL] Fix arbitrary code execution in AST type evaluation#364
🛡️ Sentinel: [CRITICAL] Fix arbitrary code execution in AST type evaluation#364bashandbone wants to merge 1 commit into
Conversation
…uation 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 type annotation validator used before eval() by whitelisting only known-safe DI-related callables in ast.Call nodes and documents the security fix in the Sentinel 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 found 1 issue, and left some high level feedback:
- The whitelist of allowed call names is currently hard-coded inline; consider centralizing it into a constant or configuration at module level so it’s easier to audit and extend consistently across future security-related checks.
- Right now only bare ast.Name calls are allowed (e.g., Depends) and attribute-style calls (e.g., di.Depends) are always rejected and reported with the raw type name; if attribute-style usage is expected, you may want to explicitly support/forbid it with a clearer error message to avoid confusing developers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The whitelist of allowed call names is currently hard-coded inline; consider centralizing it into a constant or configuration at module level so it’s easier to audit and extend consistently across future security-related checks.
- Right now only bare ast.Name calls are allowed (e.g., Depends) and attribute-style calls (e.g., di.Depends) are always rejected and reported with the raw type name; if attribute-style usage is expected, you may want to explicitly support/forbid it with a clearer error message to avoid confusing developers.
## Individual Comments
### Comment 1
<location path="src/codeweaver/core/di/container.py" line_range="143-152" />
<code_context>
+ # Allowing generic ast.Call nodes could lead to Arbitrary Code Execution (ACE)
+ # during eval() as it permits running any callable in the global namespace.
+ # We strictly limit ast.Call usage to known safe functions used in annotations.
+ if isinstance(node, ast.Call) and (
+ not isinstance(node.func, ast.Name)
+ or node.func.id
+ not in {
+ "Depends",
+ "depends",
+ "Field",
+ "PrivateAttr",
+ "Tag",
+ }
+ ):
+ func_name = (
+ node.func.id if isinstance(node.func, ast.Name) else str(type(node.func))
</code_context>
<issue_to_address>
**question:** Consider whether rejecting all non-whitelisted calls (including attribute calls) is too strict for valid annotation use cases
This will reject any `ast.Call` whose `func` is not a bare `ast.Name` in the allowlist, so attribute-based helpers like `fastapi.Depends(...)` or `pydantic.Field(...)` will now error even when equivalent to their name-imported forms. If attribute access is a common pattern, this becomes a subtle breaking change. If acceptable for your threat model, consider also allowing `ast.Attribute` where the attribute name is in the safe set (e.g. `Depends`, `Field`), while still rejecting other callables.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if isinstance(node, ast.Call) and ( | ||
| not isinstance(node.func, ast.Name) | ||
| or node.func.id | ||
| not in { | ||
| "Depends", | ||
| "depends", | ||
| "Field", | ||
| "PrivateAttr", | ||
| "Tag", | ||
| } |
There was a problem hiding this comment.
question: Consider whether rejecting all non-whitelisted calls (including attribute calls) is too strict for valid annotation use cases
This will reject any ast.Call whose func is not a bare ast.Name in the allowlist, so attribute-based helpers like fastapi.Depends(...) or pydantic.Field(...) will now error even when equivalent to their name-imported forms. If attribute access is a common pattern, this becomes a subtle breaking change. If acceptable for your threat model, consider also allowing ast.Attribute where the attribute name is in the safe set (e.g. Depends, Field), while still rejecting other callables.
There was a problem hiding this comment.
Pull request overview
Hardens CodeWeaver’s DI type-string evaluation path (Container._safe_eval_type) to prevent arbitrary code execution by restricting which ast.Call nodes are allowed through AST validation before eval() runs.
Changes:
- Added an
ast.Callwhitelist in the AST validator to only allow known annotation helper calls (e.g.,Depends,depends,Field,PrivateAttr,Tag). - Updated the Sentinel security log with a new entry documenting the AST-call evaluation vulnerability and the whitelist-based 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 | Adds a strict ast.Call allowlist during AST traversal to block unsafe call expressions prior to eval(). |
| .jules/sentinel.md | Documents the vulnerability, its cause, and the mitigation approach in the Sentinel security log. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func_name = ( | ||
| node.func.id if isinstance(node.func, ast.Name) else str(type(node.func)) | ||
| ) | ||
| raise TypeError(f"Forbidden function call in type string: {func_name}") |
| ## 2025-02-14 - Arbitrary Code Execution (ACE) via AST Call Evaluation | ||
| **Vulnerability:** Allowed arbitrary `ast.Call` nodes in type annotation evaluation strings, potentially leading to arbitrary code execution if user input or malicious type strings are evaluated. |
Security Review - PR #364SummaryThis PR implements a critical security fix to prevent arbitrary code execution (ACE) in the dependency injection container's type annotation evaluation system. The fix adds a whitelist for Security AnalysisVulnerability Assessment:
Fix Implementation (container.py:139-157):
Code QualityStrengths:
Potential Concerns:
def test_safe_eval_type_rejects_malicious_calls():
container = Container()
# Should reject arbitrary function calls
assert container._safe_eval_type('system("malicious")', {}) is None
assert container._safe_eval_type('__import__("os")', {}) is None
assert container._safe_eval_type('eval("code")', {}) is None
CI StatusThe PR has 2 failing checks:
These failures appear unrelated to the security fix itself. The core tests (Lint, CodeQL, Socket Security) are passing. Recommendations
VerdictApprove with recommendations ✅ The security fix is sound and addresses a critical vulnerability effectively. The implementation follows security best practices (whitelist approach, fail-safe defaults). However, I strongly recommend adding explicit security tests before merging to ensure this protection is maintained in future refactorings. The failing CI checks appear to be infrastructure issues unrelated to this code change and should not block the merge of this critical security fix. Review conducted by Claude Code |
🚨 Severity: CRITICAL
💡 Vulnerability: The
TypeValidatorused before resolving typing annotations witheval()allowed anyast.Callto pass validation. This meant an attacker who could control type annotation strings (e.g., dynamically generated or user-provided) could execute arbitrary code functions present in the target module's namespace.🎯 Impact: A malicious user or external plugin could gain arbitrary code execution (ACE) privileges on the server or developer machine.
🔧 Fix: Added a whitelist strictly limiting allowable functions inside
ast.Callnodes to those natively used for DI annotations (i.e.Depends,depends,Field,PrivateAttr,Tag). Any other call function correctly throws aTypeError.✅ Verification: Ran formatting and comprehensive unit tests (
tests/unit/core/) ensuring existing DI resolutions properly continue functioning while unsafe eval behaviors are mitigated.PR created automatically by Jules for task 13648616864975803255 started by @bashandbone
Summary by Sourcery
Harden type annotation evaluation to prevent arbitrary code execution during dependency injection AST processing.
Bug Fixes:
Documentation: