vimeo / psalm

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

Taint analysis: support conditional escaping for class methods #5053

Open philstutz opened 3 years ago

philstutz commented 3 years ago

Follow up for #4661. Works for plain functions and static methods (really great :+1:, thanks for that). Could that be implemented for non-static methods too? May be a little tricky without taint-specialize/pure?

class DB {
    /**
     * @psalm-taint-specialize
     * @psalm-taint-escape ($type is "expression" ? null : "sql") $val
     */
    public function quote(string $val, string $type): string {
        return $val;
    }
}

function x(PDO $p, DB $db): void {
    $a = (string)$_REQUEST['a'];

    $p->exec($db->quote($a, '')); // false positive
    $p->exec($db->quote($a, 'expression'));
}

https://psalm.dev/r/06814dc3d3

psalm-github-bot[bot] commented 3 years ago

I found these snippets:

https://psalm.dev/r/06814dc3d3 ```php exec(DB::quoteStatic($a, '')); $p->exec(DB::quoteStatic($a, 'expression')); // error is correct $p->exec($db->quote($a, '')); // false positive $p->exec($db->quote($a, 'expression')); } ``` ``` Psalm output (using commit 2f58c6a): ERROR: TaintedSql - 25:11 - Detected tainted SQL ERROR: TaintedSql - 27:11 - Detected tainted SQL ERROR: TaintedSql - 28:11 - Detected tainted SQL ```
orklah commented 2 years ago

If someone want to work on that, it happens in MethodCallReturnTypeFetcher. More precisely on this method call: https://github.com/vimeo/psalm/blob/0cdb0dc04ad27379120ee5446b619ad1170cec4a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallReturnTypeFetcher.php#L505

When you check for other calls to this method, you'll find two: function calls and static method calls. Both function and static perform some kind of merge between FunctionLikeStorage::$removed_taints and FunctionLikeStorage::$conditionally_removed_taints but it's not done for methods.

In theory, all there is to do is just that. Take what's done in function and/or static and do the same. It's just a little bit tricky because when looking in details, function and static use variables that the method doesn't have in scope, so there's a little bit of refactoring and retrieving some more vars involved.

Feel free to ping me for help!

orklah commented 2 years ago

@AndrolGenhald Taint analysis seems broken on PDO, can you check if it could be related to the change with PDO stubs? Maybe we should activate extensions on psalm.dev?

AndrolGenhald commented 2 years ago

@orklah If it's broken on psalm.dev that's probably because it's using reflection to analyze PDO. It'll break entirely once #7515 is done, which might actually be preferable to some things working and some not. We should definitely look into enabling some or all extensions on psalm.dev.