vimeo / psalm

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

Conditional sink support #4669

Open LukasReschke opened 4 years ago

LukasReschke commented 4 years ago

e.g. print_r is only a sink depending on the second parameter.

Probably possible to take the logic from https://github.com/vimeo/psalm/commit/af008953a8022566477b0555a10e17dac58e6a2f and apply it somewhat adjusted to psalm-taint-sink.

Ref https://github.com/vimeo/psalm/issues/3665

muglug commented 4 years ago

It'll be

/**
 * @param mixed $var
 * @param bool $return
 * @return ($return is true ? string : true)
 *
 * @psalm-taint-specialize
 * @psalm-flow ($var) -> return
 * @psalm-taint-sink ($return is false ? 'html' : null) $var
 */
function print_r($var, bool $return = false) {}

Same for var_dump

ohader commented 3 years ago

I just realized that my previous approach in #5095 does not make much sense - instead I should have been used conditional tainted sinks like described in this issue.

The adjusted use-case of #5095 then probably would look like this: https://psalm.dev/r/274b78511c

I tried to kick-start this feature, but struggled when actually inferencing and assigning values in FunctionLikeDocblockScanner and FunctionLikeParameter - I could need some help here.

The current (unfinished) change is available at: https://github.com/vimeo/psalm/compare/master...ohader:4669_conditional_taint_sinks?expand=1

I'm currently working on a Psalm integration the TYPO3 project - in case somebody would like to pick of this task and sponsoring (payment) helps to get it done and merged to vimeo/psalm, please get in contact with me directly (oliver.hader@typo3.org or https://keybase.io/oliverhader). Thx

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

I found these snippets:

https://psalm.dev/r/274b78511c ```php
emmanuelGuiton commented 3 years ago

I think we should not consider that using print_r function with $result=true is always safe. It depends on what we do with the result. echo print_r($_GET, true); could be as harmful as print_r($_GET);

ohader commented 3 years ago

@emmanuelGuiton correct, it's not always safe... the current stub is strict here, the 1st parameter is always considered harmful - which leads to false-positives for $result = print_r($_GET, true) and $result never being used at all later on...

I've got a custom implementation which looks like

    /**
     * @param mixed $var
     * @param bool $return
     * @return ($return is true ? string : true)
     *
     * @psalm-taint-specialize
     * @typo3-taint-sink ($return is false ? 'html' : null) $var
     * @typo3-taint-source ($return is true ? 'input' : null)
     */
    function print_r($var, bool $return = false) {}

(I tried to integrate that handling into Psalms core sources, but failed - thus, it's part of a custom plugin currently)