fix(taint): resolve direct lambda calls at call time#22
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb0145e778
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(node.func, ast.Name) and lambda_bindings is not None and node.func.id in lambda_bindings: | ||
| _resolve_lambda_body_at_call( | ||
| lambda_bindings[node.func.id], |
There was a problem hiding this comment.
When a name can be bound to different lambdas in different branches, this single lambda_bindings[node.func.id] lookup only resolves whichever branch happened to be walked last. For example, if cond: cb = lambda: eval(src) else: cb = lambda: eval('safe'); cb(); src = 'clean' misses the raw eval(src) path: the direct-call pass records only the else lambda, and the if lambda is later resolved by the final-state pass after src has been cleaned. The binding state needs to be branch-local/merged conservatively rather than overwritten by traversal order.
Useful? React with 👍 / 👎.
| for param in positional_params[len(pos_taints) :]: | ||
| scope[param.arg] = function_taint |
There was a problem hiding this comment.
When a direct call omits a lambda parameter that has a default, this seeds the parameter with function_taint instead of the default expression's taint. For src = read_raw(p); cb = lambda x=src: eval(x); cb(); src = 'clean', runtime evaluates the raw default, but both the direct-call pass and the final lambda pass treat x as clean/trusted, so PY-WL-107 is missed.
Useful? React with 👍 / 👎.
| keyword_values = [taint for name, taint in kw_taints.items() if name is not None] | ||
| scope[args.kwarg.arg] = keyword_values[0] if keyword_values else function_taint |
There was a problem hiding this comment.
For direct calls that use **raw_kwargs, kw_taints stores the expansion under None, but this comprehension drops that entry when binding the lambda's **kw parameter. A case like raw_kwargs = {'x': read_raw(p)}; cb = lambda **kw: eval(kw['x']); cb(**raw_kwargs) therefore records kw as function_taint and misses the sink even though the raw dictionary is what the lambda receives.
Useful? React with 👍 / 👎.
| if isinstance(stmt.value, ast.Lambda): | ||
| lambda_bindings[target.id] = stmt.value | ||
| else: | ||
| lambda_bindings.pop(target.id, None) |
There was a problem hiding this comment.
When a lambda is first stored in one local and then assigned to another, this else path removes the destination binding instead of copying the source binding. For inner = lambda: eval(src); cb = inner; cb(); src = 'clean', the direct call through cb is no longer resolved at call time, and the later final-state lambda pass sees only the cleaned src, producing the same false negative this change is trying to prevent.
Useful? React with 👍 / 👎.
| for param, taint in zip(positional_params, pos_taints, strict=False): | ||
| scope[param.arg] = taint |
There was a problem hiding this comment.
These parameter bindings only update the local taint scope; _resolve_call still consults the enclosing function's _CURRENT_LAMBDA_BINDINGS by name. If a lambda parameter shadows an outer lambda binding, e.g. outer = lambda: eval(src); cb = lambda outer: outer(); cb(lambda: eval('1')); src = 'clean', the analyzer resolves outer() to the captured outer lambda instead of the argument that is actually called, recording a spurious raw sink.
Useful? React with 👍 / 👎.
The PR's original regression test exercised the free-variable case (`src = read_raw(p); cb = lambda: eval(src); cb(); src = "clean"`), which the worst-ever-taint pass (#20) already covers — it fires without this change, so it guards nothing. Replace it with tests for the defect this change actually closes: a raw value bound to a lambda PARAMETER at a direct or inline call site (`cb = lambda x: eval(x); cb(raw)` and `(lambda x: eval(x))(raw)`). The worst-ever pass resets lambda params to neutral and so cannot see the argument taint — these were silent false-negatives (no fallback warning) on the prior engine and now fire via call-time resolution. Add a clean-arg negative guard against the call-time path over-firing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Verified and merging — with a corrected regression test (pushed as 1e76458). The engine fix is real and valuable, but not for the reason stated. I reproduced all four cases against
The advertised free-variable case is already covered by #20's worst-ever-taint pass — it fires on So I kept the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e76458677
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(node.func, ast.Name) and lambda_bindings is not None and node.func.id in lambda_bindings: | ||
| _resolve_lambda_body_at_call( | ||
| lambda_bindings[node.func.id], |
There was a problem hiding this comment.
When a lambda-bound name is later rebound by a non-Assign target such as a for/with/match capture, _assign_target only updates var_taints and leaves _CURRENT_LAMBDA_BINDINGS pointing at the old lambda. For example, cb = lambda: eval(src); for cb in [lambda: eval('1')]: cb(); src = 'clean' calls the loop lambda at runtime, but this branch still resolves cb() against the earlier lambda with the raw src, producing a PY-WL-107 false positive that the final-state pass would not have emitted after the clean assignment. Rebindings outside Assign/AnnAssign need to clear or conservatively merge the lambda binding state before this lookup is trusted.
Useful? React with 👍 / 👎.
| for param, taint in zip(positional_params, pos_taints, strict=False): | ||
| scope[param.arg] = taint | ||
| for param in positional_params[len(pos_taints) :]: | ||
| scope[param.arg] = function_taint |
There was a problem hiding this comment.
When a direct lambda call uses *args, this zip treats the whole expanded iterable as only one positional argument, so any later formal parameters fall through to function_taint. A case like args = read_raw(p); cb = lambda x, y: eval(y); cb(*args) receives raw data for y at runtime, but the call-time pass marks only x raw and the sink is missed. The taint from a starred positional expansion needs to be applied conservatively to all parameters it may fill, not consumed as a single slot.
Useful? React with 👍 / 👎.
| if isinstance(stmt.value, ast.Lambda): | ||
| lambda_bindings[target.id] = stmt.value | ||
| else: |
There was a problem hiding this comment.
Restricting _CURRENT_LAMBDA_BINDINGS updates to simple Name = lambda assignments misses direct calls through lambdas bound by tuple/list unpacking. For example, src = read_raw(p); cb, _ = (lambda: eval(src), None); cb(); src = 'clean' executes the raw sink at the call site, but this branch never records cb as a lambda, and the later final-state pass sees only the cleaned src, so PY-WL-107 is silently missed. The binding update needs to cover destructured lambda RHS elements as well as simple assignments.
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
_CURRENT_LAMBDA_BINDINGSto track local names bound toast.Lambdanodes during the variable-level walk and seed/reset it incompute_variable_taints._resolve_lambda_body_at_callto resolve a lambda body against the current enclosingvar_taintsplus the call's resolved positional/keyword taints and call it at direct call sites and inline lambda calls from_resolve_call.call_site_arg_taintsconservatively instead of overwriting so earlier call-time raw taints cannot be laundered by a later final-state lambda pass.test_107_lambda_called_before_later_clean_assignment_still_firesto cover the reported PoC case.Testing
uv run ruff checkon the modified files and it completed with no issues (passed).uv run mypyon the modified files and it completed with no issues (passed).python -m py_compileon the modified module and it completed successfully (passed).tests/unit/scanner/rules/test_sink_rules.pywith a temporary minimalyaml.safe_loadshim (to avoid installing PyYAML in the environment) and all relevant tests passed (31 passed).PyYAML(dependency installation via the environment tunnel also failed), so the full test run could not be completed in this environment (failure due to missing optional dependency).Codex Task