🛡️ Sentinel: [CRITICAL] Fix Arbitrary Code Execution in AST validation#367
🛡️ Sentinel: [CRITICAL] Fix Arbitrary Code Execution in AST validation#367bashandbone wants to merge 1 commit into
Conversation
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 guide (collapsed on small PRs)Reviewer's GuideRestricts which function calls are allowed in type-annotation AST validation to close an eval-based arbitrary code execution vector, and documents the vulnerability and fix in the Sentinel security log. Flow diagram for AST validation whitelist before evalflowchart TD
A["Type annotation string"] --> B["ast.parse to AST"]
B --> C["TypeValidator.generic_visit"]
C --> D{"node is ast.Call"}
D -- no --> G["super.generic_visit"]
D -- yes --> E{"node.func is ast.Name"}
E -- no --> X["raise TypeError: Only direct function calls are allowed"]
E -- yes --> F{"node.func.id in {Depends, depends, Field, PrivateAttr, Tag}"}
F -- no --> Y["raise TypeError: Forbidden function call in type annotation"]
F -- yes --> G
G --> H["eval on validated AST"]
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:
- Consider centralizing the allowed function names for type-annotation calls into a shared constant (e.g.,
ALLOWED_TYPE_CALLS) so it’s easier to audit and extend in one place if additional safe helpers are introduced later. - The new
TypeErrormessages for forbidden calls could include the full node representation (or at least more context) to aid debugging, as just the function name may be ambiguous in larger codebases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the allowed function names for type-annotation calls into a shared constant (e.g., `ALLOWED_TYPE_CALLS`) so it’s easier to audit and extend in one place if additional safe helpers are introduced later.
- The new `TypeError` messages for forbidden calls could include the full node representation (or at least more context) to aid debugging, as just the function name may be ambiguous in larger codebases.
## Individual Comments
### Comment 1
<location path="src/codeweaver/core/di/container.py" line_range="143" />
<code_context>
+ if isinstance(node, ast.Call):
+ if not isinstance(node.func, ast.Name):
+ raise TypeError("Only direct function calls are allowed in type annotations")
+ if node.func.id not in {"Depends", "depends", "Field", "PrivateAttr", "Tag"}:
+ raise TypeError(f"Forbidden function call in type annotation: {node.func.id}")
+
</code_context>
<issue_to_address>
**suggestion:** Consider moving the allowlist of function names to a shared constant to avoid repeated set allocations and improve maintainability.
Right now the set literal is recreated for every `ast.Call` visit, which can add up on large ASTs. Hoisting it to a module- or class-level constant (e.g. `_ALLOWED_ANNOTATION_CALLS`) both avoids the repeated allocations and centralizes the allowlist so it can be updated or tested without touching the visitor logic.
Suggested implementation:
```python
if not isinstance(node.func, ast.Name):
raise TypeError("Only direct function calls are allowed in type annotations")
if node.func.id not in _ALLOWED_ANNOTATION_CALLS:
raise TypeError(f"Forbidden function call in type annotation: {node.func.id}")
```
To fully implement the change, add a module-level constant near the top of `src/codeweaver/core/di/container.py` (e.g. after the imports):
```python
_ALLOWED_ANNOTATION_CALLS = {"Depends", "depends", "Field", "PrivateAttr", "Tag"}
```
If this visitor is defined inside a class and you prefer class-level configuration, you can alternatively define it as a class attribute and reference it as `self._ALLOWED_ANNOTATION_CALLS` (or `cls._ALLOWED_ANNOTATION_CALLS` if using `@classmethod`), updating the check accordingly.
</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): | ||
| if not isinstance(node.func, ast.Name): | ||
| raise TypeError("Only direct function calls are allowed in type annotations") | ||
| if node.func.id not in {"Depends", "depends", "Field", "PrivateAttr", "Tag"}: |
There was a problem hiding this comment.
suggestion: Consider moving the allowlist of function names to a shared constant to avoid repeated set allocations and improve maintainability.
Right now the set literal is recreated for every ast.Call visit, which can add up on large ASTs. Hoisting it to a module- or class-level constant (e.g. _ALLOWED_ANNOTATION_CALLS) both avoids the repeated allocations and centralizes the allowlist so it can be updated or tested without touching the visitor logic.
Suggested implementation:
if not isinstance(node.func, ast.Name):
raise TypeError("Only direct function calls are allowed in type annotations")
if node.func.id not in _ALLOWED_ANNOTATION_CALLS:
raise TypeError(f"Forbidden function call in type annotation: {node.func.id}")To fully implement the change, add a module-level constant near the top of src/codeweaver/core/di/container.py (e.g. after the imports):
_ALLOWED_ANNOTATION_CALLS = {"Depends", "depends", "Field", "PrivateAttr", "Tag"}If this visitor is defined inside a class and you prefer class-level configuration, you can alternatively define it as a class attribute and reference it as self._ALLOWED_ANNOTATION_CALLS (or cls._ALLOWED_ANNOTATION_CALLS if using @classmethod), updating the check accordingly.
There was a problem hiding this comment.
Pull request overview
Hardens the DI container’s string-annotation type resolution by tightening AST validation before eval(), closing an Arbitrary Code Execution vector where previously any ast.Call could be evaluated if a callable was reachable via the eval namespace. Also documents the incident and mitigation in the Sentinel security log.
Changes:
- Add a strict whitelist for
ast.Callnodes during AST validation so only known-safe functions can be invoked in type-annotation strings. - Reject non-direct function calls (e.g., attribute calls like
obj.method()), further narrowing the eval surface. - Record the vulnerability and prevention guidance in
.jules/sentinel.md.
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 |
Restricts callable AST nodes to a safe whitelist prior to eval() during string type resolution. |
.jules/sentinel.md |
Documents the AST-validation ACE finding and mitigation in the security log. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Security: Restrict callable nodes in `eval` to prevent Arbitrary Code Execution (ACE) | ||
| if isinstance(node, ast.Call): | ||
| if not isinstance(node.func, ast.Name): | ||
| raise TypeError("Only direct function calls are allowed in type annotations") | ||
| if node.func.id not in {"Depends", "depends", "Field", "PrivateAttr", "Tag"}: | ||
| raise TypeError(f"Forbidden function call in type annotation: {node.func.id}") | ||
|
|
Code Review - PR #367: Fix Arbitrary Code Execution in AST validationSummaryThis PR addresses a CRITICAL security vulnerability in the dependency injection container where generic Security Analysis ✅The security fix is well-designed and effective:
Code Quality ✅The implementation is clean and follows best practices:
Recommendations1. Extract whitelist to a constant (Priority: Medium)As Sourcery AI noted, consider moving the whitelist to a module-level constant to improve maintainability and avoid repeated set allocations: # Near the top of the file, after imports
_ALLOWED_ANNOTATION_CALLS = frozenset({"Depends", "depends", "Field", "PrivateAttr", "Tag"})
# Then in the validation code:
if node.func.id not in _ALLOWED_ANNOTATION_CALLS:
raise TypeError(f"Forbidden function call in type annotation: {node.func.id}")Using 2. Add specific test for the vulnerability (Priority: Low)Consider adding a test case specifically for this CVE that tries to exploit the exact vulnerability pattern: def test_ast_call_whitelist_enforcement() -> None:
"""Verify that non-whitelisted function calls in annotations are blocked."""
container = Container()
globalns = {"callable_func": lambda: None, "Annotated": Annotated}
# This should be blocked - arbitrary callable in eval namespace
result = container._resolve_string_type("Annotated[int, callable_func()]", globalns)
assert result is None, "Non-whitelisted function calls should be blocked"3. CI failures are unrelatedThe failing CI checks appear to be:
The important checks are passing:
Verdict: ✅ APPROVE with minor suggestionsThis is a critical security fix that effectively closes an arbitrary code execution vulnerability. The implementation is solid, well-documented, and follows security best practices. The minor suggestions above are for code quality improvements but don't block approval. The code change itself is ready to merge once the Python unit tests complete successfully. The Docker build failure appears unrelated to this security fix. Great work on identifying and fixing this vulnerability! 🛡️ Review performed by Claude Code |
🚨 Severity: CRITICAL
💡 Vulnerability: Generic
ast.Callnodes were allowed during AST type parsing inTypeValidator.generic_visitwithinsrc/codeweaver/core/di/container.py. This input was subsequently fed toeval(). Without whitelisting, this allowed the execution of any callable in the restricted eval's global namespace, posing a potential Arbitrary Code Execution (ACE) risk.🎯 Impact: An attacker who can control the string representation of a type annotation (e.g., via malicious configuration or dependency injection inputs) could execute arbitrary functions available in the module's global scope, compromising the security of the application.
🔧 Fix: Added a strict whitelist for
ast.Callnodes inTypeValidator.generic_visit, limiting allowable functions strictly to safe known ones:Depends,depends,Field,PrivateAttr, andTag.✅ Verification: Ran unit tests using
pytestforcore,engine, andserverpackages to verify that no dependency injection logic or container behavior was broken, and that the ACE vector is closed.PR created automatically by Jules for task 4563127322899614225 started by @bashandbone
Summary by Sourcery
Harden AST-based type annotation validation to prevent arbitrary code execution during eval of string annotations.
Bug Fixes:
Documentation: