🛡️ Sentinel: [CRITICAL] Fix Arbitrary Code Execution vulnerability in type annotation evaluation#357
Conversation
Implemented strict whitelisting of allowed function names for `ast.Call` nodes in `container.py`'s safe evaluation of string type annotations. Unrestricted usage previously allowed execution of arbitrary builtins or module functions. Added `Depends`, `depends`, `Field`, `PrivateAttr`, and `Tag` to the whitelist to maintain necessary DI and Pydantic functionality while neutralizing the vulnerability. Recorded critical learning in `.jules/sentinel.md`. 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 dynamic type annotation evaluation to a safe subset of function calls and documents the security fix in the Sentinel log to prevent arbitrary code execution via AST-based evaluation. 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 extracting the allowed
ast.Callfunction names into a named constant (e.g.,SAFE_CALL_NAMES) near the visitor class so they can be reused, extended, or audited more easily. - The new
ast.Callhandling currently allows only bare names; if the framework ever uses qualified calls (e.g.,mod.Depends), document that this is intentionally forbidden or adjust the logic to safely support such patterns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the allowed `ast.Call` function names into a named constant (e.g., `SAFE_CALL_NAMES`) near the visitor class so they can be reused, extended, or audited more easily.
- The new `ast.Call` handling currently allows only bare names; if the framework ever uses qualified calls (e.g., `mod.Depends`), document that this is intentionally forbidden or adjust the logic to safely support such patterns.
## Individual Comments
### Comment 1
<location path="src/codeweaver/core/di/container.py" line_range="136-137" />
<code_context>
+ # Restrict Arbitrary Code Execution (ACE) via type hints by strictly whitelisting ast.Call
+ if isinstance(node, ast.Call):
+ if isinstance(node.func, ast.Name):
+ if node.func.id not in {"Depends", "depends", "Field", "PrivateAttr", "Tag"}:
+ raise TypeError(f"Forbidden call in type string: {node.func.id}")
+ else:
+ raise TypeError("Forbidden call in type string: only simple names are allowed")
</code_context>
<issue_to_address>
**🚨 suggestion (security):** The hard-coded whitelist set may be better centralized and possibly made configurable to avoid scattering security-critical policy.
Since the allowed function names are inlined here, consider extracting them into a shared constant (e.g. `ALLOWED_TYPE_HINT_CALLS`) or config if they’re reused or may change with supported frameworks. Centralizing this policy helps avoid divergence and simplifies auditing these security restrictions.
Suggested implementation:
```python
import ast
# Whitelisted callable names allowed inside type-hint strings when parsing AST
# Centralizing this list makes the security policy easier to audit and update.
ALLOWED_TYPE_HINT_CALLS = {"Depends", "depends", "Field", "PrivateAttr", "Tag"}
```
```python
# Restrict Arbitrary Code Execution (ACE) via type hints by strictly whitelisting ast.Call
if isinstance(node, ast.Call):
if isinstance(node.func, ast.Name):
if node.func.id not in ALLOWED_TYPE_HINT_CALLS:
raise TypeError(f"Forbidden call in type string: {node.func.id}")
else:
raise TypeError("Forbidden call in type string: only simple names are allowed")
```
If this whitelist needs to be framework-configurable in the future, you can:
1. Move `ALLOWED_TYPE_HINT_CALLS` into a dedicated settings/config module and import it here.
2. Or extend it from configuration, e.g. `ALLOWED_TYPE_HINT_CALLS = DEFAULT_ALLOWED_TYPE_HINT_CALLS | settings.EXTRA_ALLOWED_TYPE_HINT_CALLS`.
This depends on how configuration is handled elsewhere in your project.
</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 node.func.id not in {"Depends", "depends", "Field", "PrivateAttr", "Tag"}: | ||
| raise TypeError(f"Forbidden call in type string: {node.func.id}") |
There was a problem hiding this comment.
🚨 suggestion (security): The hard-coded whitelist set may be better centralized and possibly made configurable to avoid scattering security-critical policy.
Since the allowed function names are inlined here, consider extracting them into a shared constant (e.g. ALLOWED_TYPE_HINT_CALLS) or config if they’re reused or may change with supported frameworks. Centralizing this policy helps avoid divergence and simplifies auditing these security restrictions.
Suggested implementation:
import ast
# Whitelisted callable names allowed inside type-hint strings when parsing AST
# Centralizing this list makes the security policy easier to audit and update.
ALLOWED_TYPE_HINT_CALLS = {"Depends", "depends", "Field", "PrivateAttr", "Tag"} # Restrict Arbitrary Code Execution (ACE) via type hints by strictly whitelisting ast.Call
if isinstance(node, ast.Call):
if isinstance(node.func, ast.Name):
if node.func.id not in ALLOWED_TYPE_HINT_CALLS:
raise TypeError(f"Forbidden call in type string: {node.func.id}")
else:
raise TypeError("Forbidden call in type string: only simple names are allowed")If this whitelist needs to be framework-configurable in the future, you can:
- Move
ALLOWED_TYPE_HINT_CALLSinto a dedicated settings/config module and import it here. - Or extend it from configuration, e.g.
ALLOWED_TYPE_HINT_CALLS = DEFAULT_ALLOWED_TYPE_HINT_CALLS | settings.EXTRA_ALLOWED_TYPE_HINT_CALLS.
This depends on how configuration is handled elsewhere in your project.
There was a problem hiding this comment.
Pull request overview
Hardens the DI container’s “safe” evaluation of string type annotations to prevent arbitrary code execution during dependency resolution, and records the incident in the Sentinel security log.
Changes:
- Added an
ast.Callwhitelist inContainer._safe_eval_type()’s AST validator to only allow framework-required call targets (Depends,depends,Field,PrivateAttr,Tag). - Documented the vulnerability and mitigation in
.jules/sentinel.md.
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 callable targets in evaluated type strings to a small safe whitelist. |
.jules/sentinel.md |
Adds a Sentinel entry describing the ACE/RCE vector and the whitelist-based mitigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Restrict Arbitrary Code Execution (ACE) via type hints by strictly whitelisting ast.Call | ||
| if isinstance(node, ast.Call): | ||
| if isinstance(node.func, ast.Name): | ||
| if node.func.id not in {"Depends", "depends", "Field", "PrivateAttr", "Tag"}: | ||
| raise TypeError(f"Forbidden call in type string: {node.func.id}") | ||
| else: | ||
| raise TypeError("Forbidden call in type string: only simple names are allowed") |
| <!-- SPDX-FileCopyrightText: 2025 Knitli Inc. --> | ||
| <!-- SPDX-License-Identifier: MIT OR Apache-2.0 --> |
PR Review: Security Fix for Arbitrary Code Execution VulnerabilitySummaryThis PR addresses a CRITICAL security vulnerability in the dependency injection container's safe AST evaluation logic. The fix prevents arbitrary code execution during type annotation parsing by implementing strict whitelisting of allowed function calls. Security AnalysisThe VulnerabilityThe original code in Attack Vector: Any code that could influence type annotations (configuration files, user input, dynamic imports) could potentially execute arbitrary Python code. The FixThe PR adds lines 133-139 in # Restrict Arbitrary Code Execution (ACE) via type hints by strictly whitelisting ast.Call
if isinstance(node, ast.Call):
if isinstance(node.func, ast.Name):
if node.func.id not in {"Depends", "depends", "Field", "PrivateAttr", "Tag"}:
raise TypeError(f"Forbidden call in type string: {node.func.id}")
else:
raise TypeError("Forbidden call in type string: only simple names are allowed")Effectiveness: ✅ This fix is correct and effective. It:
Code QualityStrengths
ConcernsThe PR description explicitly states:
However, no tests were added to verify this critical security fix. While
Recommendation: Add these critical tests before merging: def test_forbidden_calls_in_annotated_types() -> None:
"""Verify that function calls other than whitelisted ones are blocked."""
container = Container()
globalns = {"Annotated": Annotated, "int": int, "print": print}
# Should block print()
result = container._resolve_string_type("Annotated[int, print()]", globalns)
assert result is None
# Should allow Depends()
globalns["Depends"] = Depends
result = container._resolve_string_type("Annotated[int, Depends()]", globalns)
assert result is not NoneWhitelist AnalysisThe allowed function names are:
Question: Should CI FailuresThe PR has failing CI checks:
These failures appear unrelated to the code changes (based on the limited log output), but should be investigated before merge. Recommendations
VerdictStatus: The security fix itself is excellent and critical, but the lack of test coverage for such a critical vulnerability is concerning. This is a textbook example where tests are not optional - they serve as regression prevention for a severe security issue. Once tests are added, this should be merged immediately as it addresses a CRITICAL vulnerability. Security Impact: 🔴 CRITICAL (ACE vulnerability) |
🚨 Severity: CRITICAL
💡 Vulnerability: Unrestricted
ast.Callnode parsing in dynamic type evaluation insrc/codeweaver/core/di/container.pypermitted any callable in the global scope (e.g.os.system) to be executed during resolution.🎯 Impact: Allowed Arbitrary Code Execution (ACE) under the guise of dependency type injection/resolution.
🔧 Fix: Strictly restricted allowed
ast.Callnames to well-known safe functions required for framework behavior:Depends,depends,Field,PrivateAttr, andTag.✅ Verification: Verify tests pass and that attempting to dynamically resolve unsafe string types like
Annotated[int, print()]explicitly raises aTypeError.PR created automatically by Jules for task 4029442110563029413 started by @bashandbone
Summary by Sourcery
Harden safe AST evaluation for dynamic type annotations to prevent arbitrary code execution and document the security fix in Sentinel notes.
Bug Fixes:
Documentation: