🛡️ Sentinel: [CRITICAL] Fix arbitrary code execution in type string evaluation#391
🛡️ Sentinel: [CRITICAL] Fix arbitrary code execution in type string evaluation#391bashandbone wants to merge 1 commit into
Conversation
…valuation Fixed a vulnerability in `_safe_eval_type` where `ast.Call` nodes were not restricted, potentially allowing arbitrary code execution during `eval()` since any callable could be executed if present in a type string annotation. Restricted `ast.Call` nodes to a strict whitelist. 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 the security of _safe_eval_type by restricting AST Call nodes to a small whitelist of allowed type-hint related functions, and documents the vulnerability and lessons learned in the Sentinel 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 1 issue, and left some high level feedback:
- Consider extracting the
allowed_funcswhitelist into a module-level constant (with a brief comment) so it’s easier to discover and update when new safe type-hint helpers are introduced. - When raising for a forbidden function call, you may want to include a more descriptive representation of
node.func(e.g.,ast.unparse(node.func)where available) to aid debugging whenfunc_nameisNoneor an attribute chain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the `allowed_funcs` whitelist into a module-level constant (with a brief comment) so it’s easier to discover and update when new safe type-hint helpers are introduced.
- When raising for a forbidden function call, you may want to include a more descriptive representation of `node.func` (e.g., `ast.unparse(node.func)` where available) to aid debugging when `func_name` is `None` or an attribute chain.
## Individual Comments
### Comment 1
<location path="src/codeweaver/core/di/container.py" line_range="141-149" />
<code_context>
+ # Security concern: Restrict ast.Call to strictly whitelisted functions to prevent
+ # arbitrary code execution during eval().
+ if isinstance(node, ast.Call):
+ allowed_funcs = {"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"}
+ func_name = None
+ if isinstance(node.func, ast.Name):
+ func_name = node.func.id
+ elif isinstance(node.func, ast.Attribute):
+ func_name = node.func.attr
+
+ if func_name not in allowed_funcs:
+ raise TypeError(f"Forbidden function call in type string: {func_name}")
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Attribute calls are validated only by attribute name, not by the owning object/module, which may weaken the intended security guarantees.
Because `ast.Attribute` calls are allowed solely by attribute name, any object exposing a `Depends`/`Field`/etc. attribute can be invoked. If this is meant to harden `eval` against arbitrary execution, consider also validating the attribute base (e.g., only allow attributes from specific known modules/symbols). Otherwise, an attacker could route a dangerous callable through a whitelisted attribute name and still bypass the restriction.
</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): | ||
| allowed_funcs = {"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"} | ||
| func_name = None | ||
| if isinstance(node.func, ast.Name): | ||
| func_name = node.func.id | ||
| elif isinstance(node.func, ast.Attribute): | ||
| func_name = node.func.attr | ||
|
|
||
| if func_name not in allowed_funcs: |
There was a problem hiding this comment.
🚨 issue (security): Attribute calls are validated only by attribute name, not by the owning object/module, which may weaken the intended security guarantees.
Because ast.Attribute calls are allowed solely by attribute name, any object exposing a Depends/Field/etc. attribute can be invoked. If this is meant to harden eval against arbitrary execution, consider also validating the attribute base (e.g., only allow attributes from specific known modules/symbols). Otherwise, an attacker could route a dangerous callable through a whitelisted attribute name and still bypass the restriction.
Security Review: Arbitrary Code Execution Fix in Type String EvaluationSummaryThis PR addresses a CRITICAL security vulnerability in the Security AnalysisVulnerability DetailsThe original implementation validated AST nodes for safe
Fix Assessment ✅The patch correctly addresses the vulnerability by:
Code Quality ReviewStrengths✅ Well-documented: Inline comment clearly explains the security concern Potential Concerns
Security Best Practices Verification✅ Principle of Least Privilege: Whitelist approach is correct (deny by default) Recommendations1. Add Security Tests (High Priority)# Suggested test cases in tests/unit/core/di/test_container_security.py
async def test_safe_eval_type_allows_whitelisted_functions():
"""Verify whitelisted functions are allowed in type strings."""
container = Container()
globalns = {"Depends": Depends, "Field": Field}
# Should not raise
result = container._safe_eval_type("Annotated[str, Depends()]", globalns)
assert result is not None
async def test_safe_eval_type_blocks_dangerous_functions():
"""Verify dangerous functions are blocked."""
container = Container()
globalns = {"exec": exec, "open": open, "__import__": __import__}
# Should return None (blocked)
assert container._safe_eval_type("exec('print(1)')", globalns) is None
assert container._safe_eval_type("open('/etc/passwd')", globalns) is None
assert container._safe_eval_type("__import__('os')", globalns) is None
async def test_safe_eval_type_blocks_dunder_access():
"""Verify dunder attribute access is blocked."""
container = Container()
globalns = {"str": str}
# Should return None (blocked)
assert container._safe_eval_type("str.__class__", globalns) is None
assert container._safe_eval_type("__builtins__", globalns) is None2. Consider Function Aliasing AttackVerify that the whitelist check can't be bypassed by aliasing: # Potential bypass attempt
globalns = {"malicious": exec}
type_str = "malicious('bad code')"Current code should block this (function name is 3. Review Whitelist Completeness
VerdictAPPROVE with recommendation for additional tests The security fix is sound and correctly implemented. The approach is the right one for mitigating arbitrary code execution. However, I strongly recommend:
The vulnerability documentation in CI Status
Recommendation: Wait for pending tests to complete, but CI failures appear unrelated to the security fix itself. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of Security Fix for Type String Evaluation
I've completed a comprehensive review of this critical security fix. Please see my detailed analysis in the PR comments.
Key Findings
✅ Security Fix is Sound: The whitelist approach correctly mitigates the arbitrary code execution vulnerability
module.Depends()) would provide stronger guarantees.
_safe_eval_type. I strongly recommend adding tests to verify:
- Whitelisted functions work correctly
- Dangerous functions are blocked (exec, open, import)
- Dunder access is prevented
- Edge cases are handled
Recommendations
- Merge this PR immediately - Critical security fix
- Add security tests in follow-up PR
- Consider hardening attribute validation per the security comment
- Investigate CI failures (appear unrelated to code changes)
Verdict: APPROVE ✅
This fix should be merged to address the critical vulnerability. The suggested enhancements can be addressed in follow-up work.
Response to Security Concern (Comment 3405203672)Re: Attribute calls validated only by attribute name This is an excellent security observation. You're absolutely correct that the current implementation validates only the attribute name ( Vulnerability AnalysisTheoretical Bypass: class Evil:
@staticmethod
def Depends():
exec('malicious code')
# Type string: "Evil.Depends()"
# Current validation: PASSES (attr == 'Depends')Actual Exploitability:
However, defense in depth is always warranted for security-critical code. Recommended Hardening OptionsOption 1: Validate Base Object (Strongest)if isinstance(node, ast.Call):
allowed_funcs = {"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"}
allowed_bases = {"pydantic", "codeweaver"} # Modules we trust
func_name = None
if isinstance(node.func, ast.Name):
func_name = node.func.id
elif isinstance(node.func, ast.Attribute):
# Validate BOTH the attribute name AND the base object
func_name = node.func.attr
if isinstance(node.func.value, ast.Name):
if node.func.value.id not in allowed_bases:
raise TypeError(f"Forbidden module: {node.func.value.id}")\n \n if func_name not in allowed_funcs:
raise TypeError(f"Forbidden function call: {func_name}")Option 2: Disallow Attribute Access Entirely (Simplest)if isinstance(node, ast.Call):
# Only allow direct calls like Depends(), not module.Depends()
if isinstance(node.func, ast.Name):
if node.func.id not in allowed_funcs:
raise TypeError(f"Forbidden function: {node.func.id}")
else:
raise TypeError("Only direct function calls allowed (no attribute access)")RecommendationI suggest implementing Option 1 as a follow-up hardening PR. This would:
@bashandbone Would you like me to prepare a patch implementing Option 1, or would you prefer to handle this as a follow-up task? Credit: Thank you for the thorough security analysis. This type of defense-in-depth thinking is exactly what's needed for security-critical code paths. |
There was a problem hiding this comment.
✅ APPROVED - Critical Security Fix
I've completed a thorough review of this security patch addressing arbitrary code execution in type string evaluation.
Summary
Vulnerability: The _safe_eval_type method allowed arbitrary ast.Call nodes during eval(), enabling potential RCE if attackers could inject malicious type annotations.
Fix: Implements a whitelist restricting ast.Call nodes to safe type-hinting functions: Depends, depends, Field, PrivateAttr, Tag, Parameter.
Security Assessment ✅
Strengths:
- Correct whitelist approach (deny by default)
- Handles both
ast.Nameandast.Attributefunction calls - Clear error messages for forbidden calls
- Works with existing AST validation (dunder blocking, node type restrictions)
- Well-documented in
.jules/sentinel.md
Defense in Depth:
The fix significantly hardens the eval environment. Combined with existing protections (minimal builtins, dunder restrictions), this creates multiple security layers.
Follow-up Recommendations
While approving this critical fix, I recommend addressing these in follow-up PRs:
-
Add Security Tests (High Priority)
- Test whitelisted functions work
- Test dangerous functions are blocked (
exec,open,__import__) - Test edge cases (nested calls, complex expressions)
-
Consider Attribute Base Validation (Medium Priority)
- Per comment 3405203672: validate the base object for attribute calls
- Current code allows
anymodule.Depends()if attr name matches - Suggest restricting to known safe modules (
pydantic,codeweaver)
-
Investigate CI Failures
- Build Docker Image: FAILED
- review / review: FAILED
- Appears unrelated to code changes but should be resolved
Verdict
APPROVE AND MERGE IMMEDIATELY
This is a well-implemented fix for a critical security vulnerability. The suggested enhancements are valuable but should not block merging this patch.
Great work documenting this in the Sentinel security log. This will help prevent similar vulnerabilities in the future.
Test Status Note: Python test jobs (3.12, 3.13, 3.14) are still pending. Based on code review, the changes should not break existing tests, but please verify before final merge.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability:
_safe_eval_typeinsidesrc/codeweaver/core/di/container.pyvalidated AST nodes for safeeval()but allowed allast.Callnodes. Since type strings are passed througheval()with some built-ins and user-provided namespaces, this could lead to Arbitrary Code Execution (ACE) if an attacker can inject a function call into the evaluated type string annotation.🎯 Impact: Attackers could execute arbitrary functions available in the global namespace during the dynamic resolution of type annotations.
🔧 Fix: Added strict validation for
ast.Callnodes, checking against a whitelist of known safe type-hinting related functions (Depends,depends,Field,PrivateAttr,Tag,Parameter).✅ Verification:
mise //:testpassing, specifically the dependency injection tests (uv run pytest tests/unit/core/di/).PR created automatically by Jules for task 18192920016130478227 started by @bashandbone
Summary by Sourcery
Restrict type string evaluation to a safe subset of function calls to close an arbitrary code execution vulnerability.
Bug Fixes:
Documentation: