vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.54k stars 659 forks source link

Global interferes with taint analysis for non-global by same name #8588

Open mmcev106 opened 1 year ago

mmcev106 commented 1 year ago

A taint should be reported here, but is not:

https://psalm.dev/r/0b805cb3ee

The taint is reported if lines 4 or 11 are commented. This example was distilled down from actual case spanning hundreds of lines.

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/0b805cb3ee ```php query($_GET['whatever']); } } ``` ``` Psalm output (using commit e7f05c3): No issues! ```
orklah commented 1 year ago

This issue is not directly related to taint analysis: https://psalm.dev/r/b664b254df

Psalm basically dropped the type of $mysqli when it was called through global. This is possibly caused by https://github.com/vimeo/psalm/blob/31b4dceaf43136de0f5d753660e291dd0919a219/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php#L58 returning mixed for unknown globals, then Psalm reusing the mixed type to try to propagate the type of the variable (not sure why it would do that on separate scope though, unless it assumes that variables with the same name are probably global too?).

However, if you're using global variables like that, I strongly recommend setting up the globals config: https://psalm.dev/docs/running_psalm/configuration/#globals This will probably solve your issue in a more easy way.

Also remember this part of the doc To [ensure comprehensive results](https://github.com/vimeo/psalm/issues/6156), Psalm should be run normally prior to taint analysis, and any errors should be fixed. The taint analysis is not a silver bullet. If Psalm have difficulties understanding your code, taint analysis will be even worse.

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/b664b254df ```php query($_GET['whatever']); } } ``` ``` Psalm output (using commit 61ef140): INFO: UnusedVariable - 4:12 - $mysqli is never referenced or the value is not used INFO: Trace - 11:36 - $mysqli: mysqli INFO: Trace - 13:36 - $mysqli: mixed INFO: MixedMethodCall - 14:18 - Cannot determine the type of $mysqli when calling method query INFO: PossiblyUndefinedStringArrayOffset - 14:24 - Possibly undefined array offset ''whatever'' is risky given expected type 'array-key'. Consider using isset beforehand. INFO: UnusedVariable - 10:9 - $mysqli is never referenced or the value is not used INFO: MissingReturnType - 9:14 - Method TestModule::method1 does not have a return type, expecting void ```
mmcev106 commented 1 year ago

The scope confusion is concerning. Also, the issue is still present even when the type is specified in psalm.xml under globals.

orklah commented 1 year ago

The scope confusion is concerning

I agree :p not sure where it comes from.

the issue is still present

I did not expect that though. Can you check what my snippet is returning when analysing in standard mode? We'll see if it stays mixed or if it's because the chain of taints is broken due to the same issue that makes the variable mixed

mmcev106 commented 1 year ago

The example still returns Cannot determine the type of $mysqli when calling method query when running psalm 4.29.0 normally (standard mode) with the type specified in in psalm.xml under globals.

orklah commented 1 year ago

Ok so the global issue breaks something pretty deep. Basically, what needs to be fixed first is this: https://psalm.dev/r/6c5c51c014

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/6c5c51c014 ```php