🛡️ Sentinel: [CRITICAL] Fix Arbitrary Code Execution in dynamic type evaluation#397
🛡️ Sentinel: [CRITICAL] Fix Arbitrary Code Execution in dynamic type evaluation#397bashandbone wants to merge 1 commit into
Conversation
- Add strict whitelist for `ast.Call` nodes during AST validation before `eval()` in `src/codeweaver/core/di/container.py` - Limits calls strictly to: `Depends`, `depends`, `Field`, `PrivateAttr`, `Tag`, `Parameter` - Resolves flake8 C901 warnings - Appends security journal learning 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 safety of dynamic type string evaluation in the DI container by restricting which function calls are allowed in the AST prior to eval, and documents the vulnerability and fix 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. |
![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 hoisting the
allowed_funcsfrozenset to a module- or class-level constant so it isn’t reallocated on every AST node visit and is easier to audit/update centrally. - For
ast.Attributecalls you only validate the attribute name (node.func.attr), which could still allow calls on unexpected receivers; if feasible, also restrict or validate the base object to avoidobj.Depends(...)-style bypasses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider hoisting the `allowed_funcs` frozenset to a module- or class-level constant so it isn’t reallocated on every AST node visit and is easier to audit/update centrally.
- For `ast.Attribute` calls you only validate the attribute name (`node.func.attr`), which could still allow calls on unexpected receivers; if feasible, also restrict or validate the base object to avoid `obj.Depends(...)`-style bypasses.
## Individual Comments
### Comment 1
<location path="src/codeweaver/core/di/container.py" line_range="141" />
<code_context>
+ elif isinstance(node.func, ast.Attribute):
+ func_name = node.func.attr
+
+ allowed_funcs = frozenset({"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"})
+ if func_name not in allowed_funcs:
+ raise TypeError(f"Forbidden function call in type string: {func_name}")
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid recreating the allowed_funcs frozenset on every Call node visit.
Because `allowed_funcs` is defined inside the visit logic, it’s reallocated for every `ast.Call`. Since the set is static, please move it to a module-level constant or class attribute (e.g., `ALLOWED_CALL_FUNCS`) so it’s constructed once and reused.
Suggested implementation:
```python
if func_name not in ALLOWED_CALL_FUNCS:
```
Add a module-level constant in `src/codeweaver/core/di/container.py`, near the top of the file alongside other constants (or just below the imports), for example:
```python
ALLOWED_CALL_FUNCS = frozenset({"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"})
```
If this code is inside a class and you prefer a class attribute, define instead:
```python
class SomeVisitorOrContainer(...):
ALLOWED_CALL_FUNCS = frozenset({"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"})
```
and reference it as `self.ALLOWED_CALL_FUNCS` (or `cls.ALLOWED_CALL_FUNCS` in a `@classmethod`) in place of `ALLOWED_CALL_FUNCS` in the snippet above.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| elif isinstance(node.func, ast.Attribute): | ||
| func_name = node.func.attr | ||
|
|
||
| allowed_funcs = frozenset({"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"}) |
There was a problem hiding this comment.
suggestion (performance): Avoid recreating the allowed_funcs frozenset on every Call node visit.
Because allowed_funcs is defined inside the visit logic, it’s reallocated for every ast.Call. Since the set is static, please move it to a module-level constant or class attribute (e.g., ALLOWED_CALL_FUNCS) so it’s constructed once and reused.
Suggested implementation:
if func_name not in ALLOWED_CALL_FUNCS:Add a module-level constant in src/codeweaver/core/di/container.py, near the top of the file alongside other constants (or just below the imports), for example:
ALLOWED_CALL_FUNCS = frozenset({"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"})If this code is inside a class and you prefer a class attribute, define instead:
class SomeVisitorOrContainer(...):
ALLOWED_CALL_FUNCS = frozenset({"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"})and reference it as self.ALLOWED_CALL_FUNCS (or cls.ALLOWED_CALL_FUNCS in a @classmethod) in place of ALLOWED_CALL_FUNCS in the snippet above.
|
🤖 I'm sorry @bashandbone, but I was unable to process your request. Please see the logs for more details. |
Security Review - PR #397SummaryThis PR addresses a CRITICAL security vulnerability (Arbitrary Code Execution) in the dependency injection container's type evaluation system. The fix implements strict whitelisting for callable functions in AST evaluation. Vulnerability AnalysisThe Issue: Example Attack Vector: # Before the fix, this could be exploited:
annotation = "Annotated[str, os.system('malicious_command')]"Fix Quality Assessment✅ Strengths:
Code ReviewLines 133-143 (New Security Check): if isinstance(node, ast.Call):
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
allowed_funcs = frozenset({"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"})
if func_name not in allowed_funcs:
raise TypeError(f"Forbidden function call in type string: {func_name}")Analysis:
Testing Concerns
Recommendation: Add security tests like: def test_safe_eval_type_rejects_os_system():
container = Container()
result = container._safe_eval_type("os.system('echo pwned')", {"os": __import__('os')})
assert result is None # Should reject, not execute
def test_safe_eval_type_allows_depends():
container = Container()
from codeweaver.core.di.dependency import Depends
result = container._safe_eval_type("Annotated[str, Depends()]", {"Annotated": Annotated, "Depends": Depends})
assert result is not None # Should allowCI StatusThe PR has some failing checks:
Blocker: Cannot approve until Docker build and all tests pass. Whitelist CompletenessThe current whitelist includes:
Question: Are there any other legitimate callables used in type hints in this codebase? Consider searching for patterns like Sentinel Log EntryThe entry in
This should be corrected to match the actual discovery/fix date. Final Recommendation⏸️ WAITING FOR CI Once CI passes (especially tests and Docker build):
The security fix itself is sound and addresses a serious vulnerability. The implementation follows security best practices with whitelisting and defense-in-depth. Security Impact: 🔴 CRITICAL - This fixes an RCE vulnerability |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The
TypeValidatorclass insidesrc/codeweaver/core/di/container.pyallowed genericast.Callnodes while evaluating type hint strings usingeval().🎯 Impact: This permitted Arbitrary Code Execution (ACE) of any callable available in the module's global namespace, severely compromising the environment.
🔧 Fix: Implemented strict whitelisting for
ast.Callnodes. The validator now explicitly checksnode.func.idandnode.func.attragainst{"Depends", "depends", "Field", "PrivateAttr", "Tag", "Parameter"}before allowing the AST to proceed toeval().✅ Verification: Run
uv run pytest tests/unit/ --no-covto ensure no DI or functionality regressions exist, which has been completed.PR created automatically by Jules for task 10843776226278475619 started by @bashandbone
Summary by Sourcery
Restrict dynamic type evaluation to a safe subset of callable names to close an arbitrary code execution hole and document the security incident in the Sentinel log.
Bug Fixes:
Documentation: