🛡️ Sentinel: [CRITICAL] Restrict ast.Call in type string evaluation#355
🛡️ Sentinel: [CRITICAL] Restrict ast.Call in type string evaluation#355bashandbone wants to merge 1 commit into
Conversation
Fixed a vulnerability in `src/codeweaver/core/di/container.py` where the AST validator for type string evaluation allowed generic `ast.Call` nodes. This permitted arbitrary code execution during the subsequent `eval()` call if an attacker supplied a malicious string executing available functions in the environment (e.g., `os.system`). The fix restricts `ast.Call` nodes exclusively to the `Depends` function. Also updated `.jules/sentinel.md` with learnings on this vulnerability. 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 AST-based type string validator to forbid all function calls except the DI helper Depends(), preventing arbitrary code execution during _safe_eval_type, and documents the security issue and fix in the Sentinel security log. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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:
- In the
ast.Callrestriction, consider including the offending function name in the error message (e.g.,Forbidden call to {node.func.id}: only Depends() is allowed) to make debugging and log analysis easier when a disallowed call is encountered. - If you anticipate allowing more safe helpers in future, it may be clearer to define an explicit
ALLOWED_CALLS = {"Depends"}whitelist and check membership rather than inlining a single-name check, so the policy is easy to audit and extend.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `ast.Call` restriction, consider including the offending function name in the error message (e.g., `Forbidden call to {node.func.id}: only Depends() is allowed`) to make debugging and log analysis easier when a disallowed call is encountered.
- If you anticipate allowing more safe helpers in future, it may be clearer to define an explicit `ALLOWED_CALLS = {"Depends"}` whitelist and check membership rather than inlining a single-name check, so the policy is easy to audit and extend.
## Individual Comments
### Comment 1
<location path="src/codeweaver/core/di/container.py" line_range="140-141" />
<code_context>
raise TypeError(f"Forbidden dunder attribute: {node.attr}")
+ # Security: Restrict calls to prevent arbitrary code execution during type evaluation
+ if isinstance(node, ast.Call) and (
+ not isinstance(node.func, ast.Name) or node.func.id != "Depends"
+ ):
+ raise TypeError("Forbidden call type: only Depends() is allowed")
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider allowing attribute-based Depends calls (e.g. fastapi.Depends) if those are expected in annotations.
Right now this only permits calls where `node.func` is a bare `Name` with id `"Depends"`. Any use of `fastapi.Depends(...)` or `some_module.Depends(...)` will now raise a `TypeError`, which is a breaking change despite being semantically equivalent. If you want to keep those working, consider also allowing `ast.Attribute` nodes with `attr == "Depends"` (optionally constraining `value` to approved modules) while still rejecting arbitrary 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 != "Depends" |
There was a problem hiding this comment.
issue (bug_risk): Consider allowing attribute-based Depends calls (e.g. fastapi.Depends) if those are expected in annotations.
Right now this only permits calls where node.func is a bare Name with id "Depends". Any use of fastapi.Depends(...) or some_module.Depends(...) will now raise a TypeError, which is a breaking change despite being semantically equivalent. If you want to keep those working, consider also allowing ast.Attribute nodes with attr == "Depends" (optionally constraining value to approved modules) while still rejecting arbitrary callables.
|
🤖 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. |
![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 Arbitrary Code Execution Fix
I've reviewed PR #355 which addresses a critical arbitrary code execution vulnerability in the dependency injection container's type string evaluation logic.
Summary
This PR fixes a severe security vulnerability where the AST validator in _safe_eval_type (src/codeweaver/core/di/container.py:87-171) allowed arbitrary function calls through ast.Call nodes, potentially enabling remote code execution.
Changes Reviewed
File: src/codeweaver/core/di/container.py
- Lines 139-143: Added critical security check that restricts
ast.Callnodes to only theDependsfunction - This prevents arbitrary code execution by blocking calls like
os.system(...),eval(...), etc.
File: .jules/sentinel.md
- Lines 8-11: Documented the vulnerability, impact, and prevention strategies
- Provides valuable learning for future security reviews
Security Analysis
Vulnerability Assessment:
✅ Correctly Identified: The vulnerability was real - generic ast.Call nodes in eval contexts are extremely dangerous
✅ Appropriate Severity: CRITICAL rating is justified given the arbitrary code execution potential
✅ Targeted Fix: The solution is precise and surgical
Fix Quality:
✅ Defense in Depth: The fix adds explicit whitelisting on top of existing AST node type restrictions
✅ Minimal Attack Surface: Only allows the safe Depends function, which is necessary for the DI framework
✅ Proper Validation: Checks both that the node is an ast.Name and that the id is exactly "Depends"
Potential Attack Vector (now blocked):
# Before the fix, a malicious type string like:
"os.system('rm -rf /')"
# Could have been evaluated if 'os' was in the global namespaceCode Quality
Strengths:
- Clear, descriptive security comment explaining the rationale
- Follows existing code style and patterns
- Minimal change footprint (6 lines added)
- Well-documented in sentinel.md for future reference
Observations:
- The fix integrates seamlessly with existing AST validation logic (lines 114-137)
- Maintains compatibility with legitimate
Depends()usage in type annotations - The error message "Forbidden call type: only Depends() is allowed" is clear and actionable
Testing Considerations
def test_safe_eval_type_blocks_arbitrary_calls():
container = Container()
# Should reject os.system
assert container._safe_eval_type("os.system('echo test')", globals()) is None
# Should reject eval
assert container._safe_eval_type("eval('1+1')", globals()) is None
# Should allow Depends
assert container._safe_eval_type("Depends()", globals()) is not NoneCI Status
- ✅ Lint/Format: Passed
- ✅ CodeQL: Passed
- ✅ Socket Security: Passed
- ❌ Docker Build: Failed (appears to be an infrastructure issue, not related to this PR)
- ❌ Review workflow: Failed (Gemini CLI configuration issue, not code-related)
- ⏳ Python Tests (3.12, 3.13, 3.14): Pending
The code changes themselves look correct, but the Docker and review failures should be investigated separately.
Recommendations
- Add Security Tests: Create explicit unit tests that verify malicious call attempts are blocked
- Consider Logging: Consider adding security logging when forbidden patterns are detected (for monitoring/alerting)
- Documentation: Update any developer documentation about the DI system to mention these security constraints
Verdict
The security fix is sound and should be merged once:
- The pending Python unit tests pass
- Security-specific test coverage is added (recommended but not blocking)
The core vulnerability has been properly addressed with a minimal, targeted fix that follows security best practices. The documentation in sentinel.md is excellent for institutional knowledge.
Great work identifying and fixing this critical vulnerability! 🛡️
Reviewed by Claude Code - Security-focused code analysis
There was a problem hiding this comment.
Pull request overview
This PR hardens CodeWeaver’s DI container against arbitrary code execution when resolving string-based type annotations by tightening the AST validation performed before eval().
Changes:
- Restricts
ast.Callusage during_safe_eval_type()evaluation so onlyDepends(...)calls are permitted. - Documents 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 2 comments.
| File | Description |
|---|---|
src/codeweaver/core/di/container.py |
Tightens AST validation to block arbitrary function calls during type-string evaluation. |
.jules/sentinel.md |
Adds a security incident entry describing the vulnerability, lessons learned, and prevention steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Security: Restrict calls to prevent arbitrary code execution during type evaluation | ||
| if isinstance(node, ast.Call) and ( | ||
| not isinstance(node.func, ast.Name) or node.func.id != "Depends" | ||
| ): | ||
| raise TypeError("Forbidden call type: only Depends() is allowed") |
| # Security: Restrict calls to prevent arbitrary code execution during type evaluation | ||
| if isinstance(node, ast.Call) and ( | ||
| not isinstance(node.func, ast.Name) or node.func.id != "Depends" | ||
| ): | ||
| raise TypeError("Forbidden call type: only Depends() is allowed") |
Fixed an Arbitrary Code Execution vulnerability in
_safe_eval_typewhere genericast.Callnodes were allowed to execute during dynamic type evaluation.🚨 Severity: CRITICAL
💡 Vulnerability: The AST validation logic meant to ensure safety before running
eval()on type strings allowed unrestrictedast.Callnodes. This could allow an attacker to execute arbitrary code (e.g.,os.system(...)) via functions available in the evaluation context.🎯 Impact: Arbitrary code execution when parsing malicious user-provided type strings or configuration.
🔧 Fix: Modified
TypeValidatorto strictly limitast.Callinstances to the safeDependsfunction.✅ Verification: Ran unit tests and the AST validation is now strict.
PR created automatically by Jules for task 9014954985282109260 started by @bashandbone
Summary by Sourcery
Harden AST-based validation used in type string evaluation to prevent arbitrary code execution and document the newly discovered vulnerability and its mitigation.
Bug Fixes:
Documentation: