🛡️ Sentinel: [CRITICAL] Prevent Arbitrary Code Execution in DI Container Type Resolution#356
🛡️ Sentinel: [CRITICAL] Prevent Arbitrary Code Execution in DI Container Type Resolution#356bashandbone wants to merge 1 commit into
Conversation
…ner Type Resolution This commit fixes a vulnerability in `_safe_eval_type` in `src/codeweaver/core/di/container.py` where allowing generic `ast.Call` nodes during AST validation before an `eval()` call could lead to Arbitrary Code Execution (ACE). The `ast.Call` nodes are now strictly whitelisted to specific, required functions (`Depends`, `Field`, `depends`, `PrivateAttr`). A journal entry has also been added to `.jules/sentinel.md` documenting this finding. 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 GuideTightens AST-based validation in the DI container’s safe type evaluation to prevent arbitrary code execution by whitelisting allowed function calls, and documents the new Sentinel finding in the security 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 2 issues, and left some high level feedback:
- Consider extracting the allowed function names for
visit_Callinto a shared constant or class attribute so the whitelist is easier to discover, reuse, and update consistently across any future validators. - Right now
visit_Callonly permitsast.Nametargets; if any legitimate usage calls these functions via attributes (e.g.di.Depends(...)), you may want to explicitly decide whether to supportast.Attributetargets and either handle or block them with a clear rationale. - To aid debugging and log analysis when a forbidden call is hit, consider including
lineno/col_offsetfrom the AST node in theTypeErrormessage so callers can more easily locate the problematic type string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the allowed function names for `visit_Call` into a shared constant or class attribute so the whitelist is easier to discover, reuse, and update consistently across any future validators.
- Right now `visit_Call` only permits `ast.Name` targets; if any legitimate usage calls these functions via attributes (e.g. `di.Depends(...)`), you may want to explicitly decide whether to support `ast.Attribute` targets and either handle or block them with a clear rationale.
- To aid debugging and log analysis when a forbidden call is hit, consider including `lineno`/`col_offset` from the AST node in the `TypeError` message so callers can more easily locate the problematic type string.
## Individual Comments
### Comment 1
<location path="src/codeweaver/core/di/container.py" line_range="141-150" />
<code_context>
super().generic_visit(node)
+ def visit_Call(self, node: ast.Call) -> None:
+ # Restrict ast.Call nodes to a specific whitelist to prevent Arbitrary Code Execution (ACE)
+ # vulnerabilities during dynamic type string evaluation using restricted eval.
+ if not (
+ isinstance(node.func, ast.Name)
+ and node.func.id in {"Depends", "Field", "depends", "PrivateAttr"}
+ ):
+ func_name = getattr(node.func, "id", type(node.func).__name__)
+ raise TypeError(f"Forbidden function call in type string: {func_name}")
+ self.generic_visit(node)
+
try:
</code_context>
<issue_to_address>
**suggestion:** Consider whether this strict ast.Call whitelist is too narrow for legitimate use cases.
This only permits calls where `node.func` is an `ast.Name` in `{Depends, Field, depends, PrivateAttr}`. Attribute calls (e.g. `some_module.Depends`, `pydantic.Field`) and aliases will be rejected, as will other safe helpers you may add later. If your eval context allows qualified calls or aliases, consider either:
- Supporting `ast.Attribute` where both the base and attribute are whitelisted, or
- Centralizing the whitelist (e.g., from the evaluation environment) to avoid divergence.
Otherwise, valid type strings using qualified names or aliases may fail unexpectedly.
Suggested implementation:
```python
super().generic_visit(node)
def visit_Call(self, node: ast.Call) -> None:
# Restrict ast.Call nodes to a specific whitelist to prevent Arbitrary Code Execution (ACE)
# vulnerabilities during dynamic type string evaluation using restricted eval.
#
# We support both bare names (e.g. Depends, Field) and qualified attributes
# (e.g. some_module.Depends, pydantic.Field) as long as the fully-qualified
# call target is in the explicit whitelist below.
allowed_call_targets = {
"Depends",
"Field",
"depends",
"PrivateAttr",
# qualified / attribute-call variants that are considered safe in the eval env
"fastapi.Depends",
"fastapi.params.Depends",
"pydantic.Field",
"pydantic.fields.Field",
"pydantic.PrivateAttr",
}
def _call_target_name(func: ast.expr) -> str:
"""Resolve a function expression to a dotted name for whitelisting."""
# Bare name: Depends
if isinstance(func, ast.Name):
return func.id
# Attribute chain: pydantic.Field, fastapi.params.Depends, etc.
parts = []
current = func
while isinstance(current, ast.Attribute):
parts.append(current.attr)
current = current.value
if isinstance(current, ast.Name):
parts.append(current.id)
return ".".join(reversed(parts))
# Fallback: return the node type name for error reporting
return type(func).__name__
target_name = _call_target_name(node.func)
if target_name not in allowed_call_targets:
raise TypeError(f"Forbidden function call in type string: {target_name}")
self.generic_visit(node)
```
To fully align this with your evaluation environment and avoid whitelist divergence:
1. Consider extracting `allowed_call_targets` into a shared constant (e.g., `SAFE_TYPE_EVAL_CALL_TARGETS`) that is also used when constructing the eval context/globals, and import/use that constant here instead of duplicating the set.
2. Adjust the contents of `allowed_call_targets` to exactly mirror the symbols and qualified names you expose in the restricted eval environment (for example, if you expose `Depends` under a different alias or only as `fastapi.Depends`, update the set accordingly).
3. If you introduce new safe helper functions for type strings later, add their fully-qualified names to this shared whitelist rather than modifying this visitor in multiple places.
</issue_to_address>
### Comment 2
<location path="src/codeweaver/core/di/container.py" line_range="148-149" />
<code_context>
+ isinstance(node.func, ast.Name)
+ and node.func.id in {"Depends", "Field", "depends", "PrivateAttr"}
+ ):
+ func_name = getattr(node.func, "id", type(node.func).__name__)
+ raise TypeError(f"Forbidden function call in type string: {func_name}")
+ self.generic_visit(node)
+
</code_context>
<issue_to_address>
**suggestion:** Improve the reported function name for non-ast.Name calls to aid debugging.
When `node.func` is not an `ast.Name`, `func_name` will end up as the node class name (e.g. `'Attribute'`), which isn’t very helpful. Consider:
- Special-casing `ast.Attribute` to use `ast.unparse(node.func)` (Py3.9+) or manually build `value.attr`, and
- Only falling back to the node type name when no better identifier is available.
That will make the `TypeError` for forbidden calls more informative.
Suggested implementation:
```python
if not (
isinstance(node.func, ast.Name)
and node.func.id in {"Depends", "Field", "depends", "PrivateAttr"}
):
if isinstance(node.func, ast.Name):
func_name = node.func.id
elif isinstance(node.func, ast.Attribute):
# Prefer ast.unparse when available (Python 3.9+)
if hasattr(ast, "unparse"):
func_name = ast.unparse(node.func)
else:
# Manually reconstruct the attribute chain (e.g. module.submodule.func)
attrs: list[str] = []
current = node.func
while isinstance(current, ast.Attribute):
attrs.append(current.attr)
current = current.value
if isinstance(current, ast.Name):
attrs.append(current.id)
attrs.reverse()
func_name = ".".join(attrs) if attrs else type(node.func).__name__
else:
func_name = type(node.func).__name__
```
From the context you provided, the `TypeError` raising line is not visible. Ensure that immediately after the new `func_name` block you still have:
```python
raise TypeError(f"Forbidden function call in type string: {func_name}")
```
and that `self.generic_visit(node)` is called after the `if not (...)` block so that allowed calls are still traversed:
```python
if not (...):
...
raise TypeError(...)
self.generic_visit(node)
```
No additional imports should be necessary as `ast` is already used in this file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def visit_Call(self, node: ast.Call) -> None: | ||
| # Restrict ast.Call nodes to a specific whitelist to prevent Arbitrary Code Execution (ACE) | ||
| # vulnerabilities during dynamic type string evaluation using restricted eval. | ||
| if not ( | ||
| isinstance(node.func, ast.Name) | ||
| and node.func.id in {"Depends", "Field", "depends", "PrivateAttr"} | ||
| ): | ||
| func_name = getattr(node.func, "id", type(node.func).__name__) | ||
| raise TypeError(f"Forbidden function call in type string: {func_name}") | ||
| self.generic_visit(node) |
There was a problem hiding this comment.
suggestion: Consider whether this strict ast.Call whitelist is too narrow for legitimate use cases.
This only permits calls where node.func is an ast.Name in {Depends, Field, depends, PrivateAttr}. Attribute calls (e.g. some_module.Depends, pydantic.Field) and aliases will be rejected, as will other safe helpers you may add later. If your eval context allows qualified calls or aliases, consider either:
- Supporting
ast.Attributewhere both the base and attribute are whitelisted, or - Centralizing the whitelist (e.g., from the evaluation environment) to avoid divergence.
Otherwise, valid type strings using qualified names or aliases may fail unexpectedly.
Suggested implementation:
super().generic_visit(node)
def visit_Call(self, node: ast.Call) -> None:
# Restrict ast.Call nodes to a specific whitelist to prevent Arbitrary Code Execution (ACE)
# vulnerabilities during dynamic type string evaluation using restricted eval.
#
# We support both bare names (e.g. Depends, Field) and qualified attributes
# (e.g. some_module.Depends, pydantic.Field) as long as the fully-qualified
# call target is in the explicit whitelist below.
allowed_call_targets = {
"Depends",
"Field",
"depends",
"PrivateAttr",
# qualified / attribute-call variants that are considered safe in the eval env
"fastapi.Depends",
"fastapi.params.Depends",
"pydantic.Field",
"pydantic.fields.Field",
"pydantic.PrivateAttr",
}
def _call_target_name(func: ast.expr) -> str:
"""Resolve a function expression to a dotted name for whitelisting."""
# Bare name: Depends
if isinstance(func, ast.Name):
return func.id
# Attribute chain: pydantic.Field, fastapi.params.Depends, etc.
parts = []
current = func
while isinstance(current, ast.Attribute):
parts.append(current.attr)
current = current.value
if isinstance(current, ast.Name):
parts.append(current.id)
return ".".join(reversed(parts))
# Fallback: return the node type name for error reporting
return type(func).__name__
target_name = _call_target_name(node.func)
if target_name not in allowed_call_targets:
raise TypeError(f"Forbidden function call in type string: {target_name}")
self.generic_visit(node)To fully align this with your evaluation environment and avoid whitelist divergence:
- Consider extracting
allowed_call_targetsinto a shared constant (e.g.,SAFE_TYPE_EVAL_CALL_TARGETS) that is also used when constructing the eval context/globals, and import/use that constant here instead of duplicating the set. - Adjust the contents of
allowed_call_targetsto exactly mirror the symbols and qualified names you expose in the restricted eval environment (for example, if you exposeDependsunder a different alias or only asfastapi.Depends, update the set accordingly). - If you introduce new safe helper functions for type strings later, add their fully-qualified names to this shared whitelist rather than modifying this visitor in multiple places.
| func_name = getattr(node.func, "id", type(node.func).__name__) | ||
| raise TypeError(f"Forbidden function call in type string: {func_name}") |
There was a problem hiding this comment.
suggestion: Improve the reported function name for non-ast.Name calls to aid debugging.
When node.func is not an ast.Name, func_name will end up as the node class name (e.g. 'Attribute'), which isn’t very helpful. Consider:
- Special-casing
ast.Attributeto useast.unparse(node.func)(Py3.9+) or manually buildvalue.attr, and - Only falling back to the node type name when no better identifier is available.
That will make the TypeError for forbidden calls more informative.
Suggested implementation:
if not (
isinstance(node.func, ast.Name)
and node.func.id in {"Depends", "Field", "depends", "PrivateAttr"}
):
if isinstance(node.func, ast.Name):
func_name = node.func.id
elif isinstance(node.func, ast.Attribute):
# Prefer ast.unparse when available (Python 3.9+)
if hasattr(ast, "unparse"):
func_name = ast.unparse(node.func)
else:
# Manually reconstruct the attribute chain (e.g. module.submodule.func)
attrs: list[str] = []
current = node.func
while isinstance(current, ast.Attribute):
attrs.append(current.attr)
current = current.value
if isinstance(current, ast.Name):
attrs.append(current.id)
attrs.reverse()
func_name = ".".join(attrs) if attrs else type(node.func).__name__
else:
func_name = type(node.func).__name__From the context you provided, the TypeError raising line is not visible. Ensure that immediately after the new func_name block you still have:
raise TypeError(f"Forbidden function call in type string: {func_name}")and that self.generic_visit(node) is called after the if not (...) block so that allowed calls are still traversed:
if not (...):
...
raise TypeError(...)
self.generic_visit(node)No additional imports should be necessary as ast is already used in this file.
There was a problem hiding this comment.
Pull request overview
This PR hardens the DI container’s string-based type resolution by tightening AST validation before a restricted eval(), preventing arbitrary function invocation during type parsing.
Changes:
- Added a dedicated
TypeValidator.visit_Callto whitelist only specific callables (Depends,Field,depends,PrivateAttr) during_safe_eval_typeAST validation. - Documented the security learning/prevention in
.jules/sentinel.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/codeweaver/core/di/container.py | Restricts ast.Call nodes to an explicit whitelist during safe type-string evaluation to reduce ACE risk. |
| .jules/sentinel.md | Records the vulnerability, lesson learned, and prevention guidance for the AST/eval hardening. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Security Review - PR #356SummaryThis PR addresses a CRITICAL security vulnerability in the dependency injection container's type resolution system. The fix properly restricts nodes during AST validation to prevent arbitrary code execution (ACE) vulnerabilities. Code Quality Assessment ✅What Was FixedThe vulnerability was in The FixAdded a
Any other function calls are rejected with a descriptive Security Analysis 🛡️Strengths:
Implementation Notes:
Testing ✅All security tests pass:
Potential Considerations
CI Status NoteTwo checks failed, but both are infrastructure-related, not code-related:
Passing checks:
Python tests (3.12, 3.13, 3.14) are pending but security tests pass locally. RecommendationAPPROVE ✅ This is a well-implemented security fix that:
The failing CI checks are infrastructure issues unrelated to the code changes. Once Python tests complete successfully, this PR should be merged promptly given the critical severity. Additional ContextThe vulnerability was correctly identified and the fix follows security hardening principles. The documentation in Risk Assessment: High severity vulnerability, low-risk fix with no breaking changes to legitimate functionality. |
Security Review - PR #356SummaryThis PR addresses a CRITICAL security vulnerability in the dependency injection container's type resolution system. The fix properly restricts ast.Call nodes during AST validation to prevent arbitrary code execution (ACE) vulnerabilities. Code Quality Assessment ✅What Was FixedThe vulnerability was in _safe_eval_type method in src/codeweaver/core/di/container.py:87-176. The existing AST validation allowed generic ast.Call nodes, which could permit execution of arbitrary callables if an attacker could control the type string being evaluated. The FixAdded a visit_Call method (lines 141-150) that whitelists only specific, required functions:
Any other function calls are rejected with a descriptive TypeError. Security Analysis 🛡️Strengths:
Implementation Notes:
Testing ✅All security tests pass:
Potential Considerations
CI Status NoteTwo checks failed, but both are infrastructure-related, not code-related:
Passing checks:
RecommendationAPPROVE ✅ This is a well-implemented security fix that:
The failing CI checks are infrastructure issues unrelated to the code changes. Once Python tests complete successfully, this PR should be merged promptly given the critical severity. Risk Assessment: High severity vulnerability, low-risk fix with no breaking changes to legitimate functionality. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Review - CRITICAL Vulnerability Fix
Summary
This PR addresses a critical security vulnerability in the DI container's type resolution system. The fix properly restricts nodes during AST validation to prevent Arbitrary Code Execution (ACE).
Vulnerability Analysis ✅
The Problem:
The _safe_eval_type method in src/codeweaver/core/di/container.py:141-150 previously allowed generic ast.Call nodes during AST validation before eval(). This created an ACE vulnerability where an attacker could inject malicious function calls if they controlled the type string being evaluated.
The Fix:
Added a visit_Call method that whitelists only specific, required functions:
Depends,Field,depends,PrivateAttr
Any other function call now raises a TypeError and returns None, preventing execution.
Code Quality Review ✅
Strengths:
- Defense in Depth: This fix adds another layer of protection beyond the existing dunder blocking
- Minimal Whitelist: Only allows the exact functions needed for legitimate type annotations
- Clear Error Messages: Provides helpful error messages when blocked (line 149)
- Proper Documentation: The inline comment clearly explains the security rationale
- Well-Tested: Security tests verify both allowed and blocked scenarios
Implementation Quality:
- Clean, focused change (11 lines added, 0 deleted)
- Follows existing code style and patterns
- Properly integrated into the existing
TypeValidatorvisitor class - Uses
self.generic_visit(node)to continue traversing child nodes
Test Coverage ✅
I verified the security tests in tests/di/test_container_security.py:
Passing Tests:
- ✅
test_safe_type_resolution- Valid types still resolve correctly - ✅
test_malicious_type_resolution- Malicious strings are blocked - ✅
test_dunder_blocking- Dunder access is prevented - ✅
test_safe_builtins_resolution- Builtins work without globals - ✅
test_type_builtin_not_exploitable-type()cannot be exploited
Additional Manual Testing:
I verified the whitelist works correctly:
- ✅ All 4 whitelisted functions (
Depends,Field,depends,PrivateAttr) are allowed - ✅ All dangerous functions (
open,eval,exec,compile,__import__,getattr,type) are blocked
Documentation ✅
The .jules/sentinel.md documentation is excellent:
- Clear description of the vulnerability
- Explains the learning and prevention strategy
- Will serve as valuable reference for future security reviews
Recommendations
Minor Enhancement (Optional):
Consider adding a dedicated test case to test_container_security.py that explicitly tests the new visit_Call whitelist:
def test_call_whitelist() -> None:
"""Verify that only whitelisted function calls are allowed."""
container = Container()
globalns = {
'Annotated': Annotated,
'Depends': Depends,
'Field': lambda: None,
'int': int,
'open': open,
'eval': eval,
}
# Whitelisted calls should work
assert container._resolve_string_type('Annotated[int, Depends()]', globalns) is not None
assert container._resolve_string_type('Annotated[int, Field()]', globalns) is not None
# Non-whitelisted calls should be blocked
assert container._resolve_string_type('open("file")', globalns) is None
assert container._resolve_string_type('eval("1+1")', globalns) is NoneHowever, the existing test coverage is already quite comprehensive.
CI Status ⚠️
I noticed two failing CI checks:
- ❌ Build Docker Image
- ❌ review / review
These appear to be unrelated to the security fix itself (the tests are passing). The failures might be due to:
- Transient CI infrastructure issues
- Unrelated build configuration problems
I recommend:
- Checking the CI logs for the specific error messages
- Re-running the failed jobs to see if they're transient failures
- If the failures persist, they should be addressed separately as they don't impact the security fix
Verdict
APPROVED ✅
This is a critical security fix that:
- ✅ Properly addresses the ACE vulnerability
- ✅ Implements a minimal, focused solution
- ✅ Includes comprehensive test coverage
- ✅ Follows security best practices (defense in depth, least privilege)
- ✅ Is well-documented for future reference
The code quality is excellent, the fix is sound, and the security impact is properly mitigated. I strongly recommend merging this PR once the CI issues are resolved.
Security Impact: This fix prevents potential arbitrary code execution if an attacker can control type strings being evaluated. While the attack vector requires specific conditions (attacker controlling type annotations), the fix properly closes this vulnerability.
Great work on identifying and fixing this security issue! 🛡️
There was a problem hiding this comment.
Security Review - CRITICAL Vulnerability Fix
Summary
This PR addresses a critical security vulnerability in the DI container's type resolution system. The fix properly restricts ast.Call nodes during AST validation to prevent Arbitrary Code Execution (ACE).
Vulnerability Analysis ✅
The Problem:
The _safe_eval_type method in src/codeweaver/core/di/container.py:141-150 previously allowed generic ast.Call nodes during AST validation before eval(). This created an ACE vulnerability where an attacker could inject malicious function calls if they controlled the type string being evaluated.
The Fix:
Added a visit_Call method that whitelists only specific, required functions:
- Depends, Field, depends, PrivateAttr
Any other function call now raises a TypeError and returns None, preventing execution.
Code Quality Review ✅
Strengths:
- Defense in Depth: This fix adds another layer of protection beyond the existing dunder blocking
- Minimal Whitelist: Only allows the exact functions needed for legitimate type annotations
- Clear Error Messages: Provides helpful error messages when blocked (line 149)
- Proper Documentation: The inline comment clearly explains the security rationale
- Well-Tested: Security tests verify both allowed and blocked scenarios
Implementation Quality:
- Clean, focused change (11 lines added, 0 deleted)
- Follows existing code style and patterns
- Properly integrated into the existing TypeValidator visitor class
- Uses self.generic_visit(node) to continue traversing child nodes
Test Coverage ✅
I verified the security tests in tests/di/test_container_security.py:
Passing Tests:
- ✅ test_safe_type_resolution - Valid types still resolve correctly
- ✅ test_malicious_type_resolution - Malicious strings are blocked
- ✅ test_dunder_blocking - Dunder access is prevented
- ✅ test_safe_builtins_resolution - Builtins work without globals
- ✅ test_type_builtin_not_exploitable - type() cannot be exploited
Additional Manual Testing:
I verified the whitelist works correctly:
- ✅ All 4 whitelisted functions (Depends, Field, depends, PrivateAttr) are allowed
- ✅ All dangerous functions (open, eval, exec, compile, import, getattr, type) are blocked
Documentation ✅
The .jules/sentinel.md documentation is excellent:
- Clear description of the vulnerability
- Explains the learning and prevention strategy
- Will serve as valuable reference for future security reviews
CI Status ⚠️
I noticed two failing CI checks:
- ❌ Build Docker Image
- ❌ review / review
These appear to be unrelated to the security fix itself (the tests are passing). The failures might be due to:
- Transient CI infrastructure issues
- Unrelated build configuration problems
I recommend:
- Checking the CI logs for the specific error messages
- Re-running the failed jobs to see if they're transient failures
- If the failures persist, they should be addressed separately as they don't impact the security fix
Verdict
APPROVED ✅
This is a critical security fix that:
- ✅ Properly addresses the ACE vulnerability
- ✅ Implements a minimal, focused solution
- ✅ Includes comprehensive test coverage
- ✅ Follows security best practices (defense in depth, least privilege)
- ✅ Is well-documented for future reference
The code quality is excellent, the fix is sound, and the security impact is properly mitigated. I strongly recommend merging this PR once the CI issues are resolved.
Security Impact: This fix prevents potential arbitrary code execution if an attacker can control type strings being evaluated. While the attack vector requires specific conditions (attacker controlling type annotations), the fix properly closes this vulnerability.
Great work on identifying and fixing this security issue! 🛡️
|
This is an excellent observation about potential limitations. Let me analyze the current implementation: Current Behavior Analysis: Looking at the codebase, the Why the Narrow Whitelist Works Here:
However, you raise valid points about maintainability: The suggestion to support Recommendation: For this critical security fix, I recommend:
This approach balances urgency (fixing a critical vulnerability) with long-term code quality (your architectural improvements). Would you like me to create a follow-up issue for the enhancement? |
🚨 Severity: CRITICAL
💡 Vulnerability: Found a vulnerability in
_safe_eval_typeinsrc/codeweaver/core/di/container.pywhere allowing genericast.Callnodes during AST validation before aneval()call could lead to Arbitrary Code Execution (ACE) if an attacker injects a malicious function call.🎯 Impact: An attacker could execute arbitrary code within the host machine's environment if they are able to control the string evaluated.
🔧 Fix: Added a
visit_Callmethod insideTypeValidatorto strictly whitelistast.Callnodes to specific, required functions (Depends,Field,depends,PrivateAttr).✅ Verification: Verified by targeted testing of security tests
uv run pytest tests/di/test_container_security.pyas well as full unit tests.PR created automatically by Jules for task 9261490303314997612 started by @bashandbone
Summary by Sourcery
Harden dynamic type evaluation by restricting allowed function calls in AST validation to prevent arbitrary code execution.
Bug Fixes:
_safe_eval_typeby whitelisting allowedast.Calltargets in the DI container type resolution.Documentation:
.jules/sentinel.mdwith details of the AST call restriction vulnerability, lessons learned, and preventive guidance.