don't register taints for numeric variables #6813
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
orklah
commented
Nov 3, 2021
| @@ -38,7 +38,7 @@ public static function analyze( | |||
| $expr_type = $statements_analyzer->node_data->getType($expr); | |||
|
|
|||
| if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { | |||
| if ($expr_type) { | |||
| if ($expr_type && $expr_type->hasObjectType()) { | |||
There was a problem hiding this comment.
Note: this was done to fix #3709 originally but this had the side-effect of turning any int into a numeric-string. I was not comfortable skipping taint analysis for a possible string, so I reduced the scope of this fix in order to retrieve the real int type later.
orklah
added
the
release:fix
muglug
approved these changes
Nov 4, 2021
staabm
reviewed
Nov 4, 2021
| // numeric types can't be tainted | ||
| if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph | ||
| && $input_type->isSingle() | ||
| && ($input_type->isInt() || $input_type->isFloat()) |
There was a problem hiding this comment.
Probably, and there's surely more! We'll gladly accepts PRs :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this fixes #4872
I was pretty confused for a moment, I expected two fixes: get rid of the taint when passing through params, and when passing through a return type
However, as soon as I fixed the params one, both disappeared. I then realised that echo was treated as a function and it stopped registering taint given they were ints. I searched for sinks that are not functions (or treated as such by psalm) and couldn't find some. Maybe I'm mistaken on this.
Note also that this PR represent a subtle change in behaviour: declaring that a function takes an int as a param or gives int as a return type in phpdoc will now remove possible taints. It may be possible to do so only if the type does not come from docblock but it seemed more aligned with Psalm's policy to trust types even in docblock