Semgrep Nan Injection codemod #758
Conversation
clavedeluna
force-pushed
the
semgrep-nan-inj
branch
from
795e78b
to
5f981a9
Compare
| from core_codemods.semgrep.semgrep_nan_injection import SemgrepNanInjection | ||
|
|
||
|
|
||
| class TestSemgrepNanInjection(BaseSASTCodemodTest): |
There was a problem hiding this comment.
I think we need some additional cases where the float(potential_nan) is used in a context other than simple assignment.
I also think we should add test cases for the other type casts that are identified by this Semgrep rule.
There was a problem hiding this comment.
I've added more testing for cases that semgrep triggers the rule.
clavedeluna
force-pushed
the
semgrep-nan-inj
branch
from
30e20fe
to
d3582f2
Compare
| def _get_var_in_call(self, node: cst): | ||
| match node.args[0].value: | ||
| case cst.Name(): | ||
| # float(var) | ||
| return node.args[0].value.value | ||
| case cst.BinaryOperation(): | ||
| # bool(float(var)), complex(float(var)), etc | ||
| return node.args[0].value.left.args[0].value.value | ||
|
|
There was a problem hiding this comment.
This looks weird.
The type hint for node should be cst.Call.
The case for cst.BinaryOperation() does not match the the intention, at least judging from the comment above. Do you mean cst.Call?
If that's the case, you should account for deeper nestings (e.g. float(complex(int(...))) ). Does the semgrep rule always guarantee a variable name there? It could be an expression.
There was a problem hiding this comment.
whoops, updated the type hint.
BinaryOperation because when nested cst really does interpret it as a binaryoperation, for all calls of bool, int, float, and complex. I checked them all. this is incorrect
I'll check the last part you mention.
There was a problem hiding this comment.
so checking in the playground
- looks like infinite nesting is a possibility ...
complex(bool(float(tid))will match. But this only matches as long as all funcs are one of complex, bool, float. Note thepattern-sanitizers, that means any other function will sanitize this away. - I do need to handle the non-var case for
float(request.POST.get("tid")), which is annoying it wasn't a semgrep example.
I think the reasonable thing to do here is to not implement #1, just test and not to it, but to do #2. Any other pattern won't be fixed for now. I'll try to do both :)
clavedeluna
force-pushed
the
semgrep-nan-inj
branch
from
04f0b49
to
5bdd002
Compare
|
alright, so I've updated and improved this codemod to handle as many cases as I could get to fire in semgrep. There are probably more, but in those cases we won't do anything. |
Co-authored-by: Dan D'Avella <dan.davella@pixee.ai>
Use Assignment Expression (Walrus) In Conditional Co-authored-by: pixeebot[bot] <104101892+pixeebot[bot]@users.noreply.github.com>
clavedeluna
force-pushed
the
semgrep-nan-inj
branch
from
5bdd002
to
463ca4d
Compare
clavedeluna
dismissed
drdavella’s stale review
addressed review and improved codemod
|
This codemod replaces typecast call at a line reported by semgrep (which is a sink of a request source)
x = float(uuid)and wraps it in an if/else statement to check if the value could be nan.
Notice that this new transformer is specific to semgrep results. It wouldn't work stand-alone. I also decided to not handle other edge cases since this is likely a codemod that won't be used much. If you think of something absolutely necessary we do need, I can add it.
Closes #691