fix: track assignment aliases in component code scanner#14041
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe AST security scanner now tracks multiple module aliases, shadowed bindings, branch-specific states, and nested scopes. Assignment-based access to restricted modules is resolved through direct, chained, destructured, annotated, and named-expression aliases, with regression coverage for safe rebinding and scope isolation. ChangesSecurity alias resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
bug
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/base/langflow/agentic/helpers/code_security.py`:
- Around line 335-357: Update the AST visitor handling loop and comprehension
targets so they call _bind_assignment_target with the iterated value, rather
than only traversing Store-name nodes. Cover both for/async-for targets and
comprehension.target, conservatively binding aliases so calls through names such
as m or n are scanned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc9c0159-d301-4827-a29e-3cc8da52667d
📒 Files selected for processing (2)
src/backend/base/langflow/agentic/helpers/code_security.pysrc/backend/tests/unit/agentic/helpers/test_code_security.py
| def visit_Assign(self, node: ast.Assign): | ||
| self.visit(node.value) | ||
| for target in node.targets: | ||
| self.visit(target) | ||
| self._bind_assignment_target(target, node.value) | ||
|
|
||
| def visit_AnnAssign(self, node: ast.AnnAssign): | ||
| self.visit(node.annotation) | ||
| if node.value is not None: | ||
| self.visit(node.value) | ||
| self.visit(node.target) | ||
| self._bind_assignment_target(node.target, node.value) | ||
|
|
||
| def visit_NamedExpr(self, node: ast.NamedExpr): | ||
| self.visit(node.value) | ||
| self.visit(node.target) | ||
| self._bind_assignment_target(node.target, node.value) | ||
|
|
||
| def visit_AugAssign(self, node: ast.AugAssign): | ||
| self.visit(node.target) | ||
| self.visit(node.value) | ||
| if isinstance(node.target, ast.Name): | ||
| self._bind_name(node.target.id, frozenset()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the scanner has no For/While/comprehension target handling.
fd -t f code_security.py --exec rg -nP '\bdef visit_(For|AsyncFor|While|comprehension|ListComp|SetComp|DictComp|GeneratorExp)\b' {}
echo "---- assignment-style handlers present: ----"
fd -t f code_security.py --exec rg -nP '\bdef visit_(Assign|AnnAssign|NamedExpr|AugAssign)\b' {}Repository: langflow-ai/langflow
Length of output: 421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/backend/base/langflow/agentic/helpers/code_security.py"
echo "== Outline =="
ast-grep outline "$FILE" --view expanded | sed -n '1,240p'
echo
echo "== Binding helpers and visitor methods =="
sed -n '1,260p' "$FILE" | nl -ba | sed -n '1,260p'
echo
echo "== Search for generic_visit / target binding / scan entrypoints =="
rg -n "generic_visit|_bind_assignment_target|_bind_name|_scan_flow_component_code|visit_For|visit_AsyncFor|visit_comprehension|visit_ListComp|visit_SetComp|visit_DictComp|visit_GeneratorExp" "$FILE"Repository: langflow-ai/langflow
Length of output: 2997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/backend/base/langflow/agentic/helpers/code_security.py"
echo "== Relevant visitor and binding code =="
sed -n '235,520p' "$FILE"
echo
echo "== Any explicit loop/comprehension visitors or target binders elsewhere =="
rg -n "def visit_(For|AsyncFor|While|comprehension|ListComp|SetComp|DictComp|GeneratorExp)|_bind_assignment_target|_resolved_assignment_value|generic_visit" "$FILE"
echo
echo "== AST behavior probe: NodeVisitor generic traversal over For/comprehension targets =="
uv run python3 - <<'PY'
import ast
class V(ast.NodeVisitor):
def visit_Name(self, node):
print("Name:", node.id, type(node.ctx).__name__)
self.generic_visit(node)
code = """
import os
for m in (os,):
pass
result = [n for n in (os,)]
"""
V().visit(ast.parse(code))
PYRepository: langflow-ai/langflow
Length of output: 14153
Loop and comprehension targets need alias binding. for/async for targets and comprehension.target are only traversed as ast.Name(..., Store) nodes here, so they never reach _bind_assignment_target; aliases like for m in (os,): m.system(...) or [n.system(...) for n in (os,)] slip past the scanner. Handle those targets conservatively in the visitor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/base/langflow/agentic/helpers/code_security.py` around lines 335
- 357, Update the AST visitor handling loop and comprehension targets so they
call _bind_assignment_target with the iterated value, rather than only
traversing Store-name nodes. Cover both for/async-for targets and
comprehension.target, conservatively binding aliases so calls through names such
as m or n are scanned.
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-1.10.3 #14041 +/- ##
=================================================
Coverage ? 58.63%
=================================================
Files ? 2306
Lines ? 221140
Branches ? 34222
=================================================
Hits ? 129676
Misses ? 89978
Partials ? 1486
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Summary
Testing
Merge order
Summary by CodeRabbit
Bug Fixes
Tests