Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Psalm's taint analysis does not work for magic properties with no phpdoc declaration #3668

Closed
TysonAndre opened this issue Jun 24, 2020 · 4 comments
Labels

Comments

@TysonAndre
Copy link
Contributor

https://psalm.dev/r/22857fa662

Expected: If a property with name foo is set to an unsafe value, then psalm should warn about reads from foo being tainted, but not properties with other names such as other, whether or not there is an @property tag
Observed: No TaintedInput warning is emitted if the @property tag is absent. The example would also properly emit a warning if __get/__set are removed

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/22857fa662
<?php // --taint-analysis

// Adding an (at)property string $taint would make psalm properly warn about this.
class Magic {
    private $_params = [];
    public function __set(string $a, $value) {
        $this->_params[$a] = $value;
    }
    public function __get(string $a) {
        return $this->_params[$a];
    }
}
$m = new Magic();
$m->taint = $_GET['input'];
echo $m->taint;
// There is no edge from Magic::$taint to echo. There's an edge from magic::__get to echo, though
Psalm output (using commit de85e7c):

No issues!

@muglug muglug added the bug label Jun 24, 2020
@muglug muglug closed this as completed in b84cf74 Jun 25, 2020
@TysonAndre
Copy link
Contributor Author

Expected: If a property with name foo is set to an unsafe value, then psalm should warn about reads from foo being tainted, but not properties with other names such as other, whether or not there is an @property tag

ERROR: TaintedInput - src/dynamic.php:15:10 - Detected tainted html (see https://psalm.dev/205)
  $_GET
    <no known location>

  $_GET['unsafe'] - src/dynamic.php:14:13
    $a->x = $_GET['unsafe'];

  call to X::__set - src/dynamic.php:14:13
    $a->x = $_GET['unsafe'];

  X::__set#2 - src/dynamic.php:9:41
    public function __set(string $name, $value): void {

  $this->params[$name] - src/dynamic.php:10:9
        $this->params[$name] = $value;

  X::$params - src/dynamic.php:10:9
        $this->params[$name] = $value;

  X::$params
    <no known location>

  X::$params - src/dynamic.php:7:23
        return $this->params[$name];

  $this->params[$name] - src/dynamic.php:7:16
        return $this->params[$name];

  X::__get - src/dynamic.php:6:21
    public function __get(string $name) {

  call to echo - src/dynamic.php:15:10
    echo $b->y;

<?php
class X {
    private $params = [];

    /** @return mixed */
    public function __get(string $name) {
        return $this->params[$name];
    }
    public function __set(string $name, $value): void {
        $this->params[$name] = $value;
    }
}
function test(X $a, X $b) {
    $a->x = $_GET['unsafe'];
    echo $b->y;
}

I'd been hoping for a way to track taintedness of individual magic properties, even with an annotation or a stub class - here, it seems like $this->params is marked as tainted, so tainting a single property marks other properties as tainted.

Would automatically creating entries for dynamic properties be possible through a plugin/config setting/annotation at runtime before the taintedness is added? Would that conflict with psalm's threading(multiprocessing) support?

@TysonAndre
Copy link
Contributor Author

- `PropertyExistenceProviderInterface` - can be used to override Psalm's builtin property existence checks for one or more classes.

That might be what I want

@TysonAndre
Copy link
Contributor Author

It's possible to do this in a plugin, basing it on tests/Config/Plugin/Hook/FooPropertyProvider.php, if the __get and __set are deliberately no-op stubs in a stub class

    /**
     * @return ?bool
     * @override
     */
    public static function doesPropertyExist(
        string $fq_classlike_name,
        string $property_name,
        bool $read_mode,
        StatementsSource $source = null,
        Context $context = null,
        CodeLocation $code_location = null
    ) {
        // Pretending all magic properties exist and are declared so that individual property names taintedness will be checked separately.
        return true;
    }

    /**
     * @return ?bool
     * @override
     */
    public static function isPropertyVisible(
        StatementsSource $source,
        string $fq_classlike_name,
        string $property_name,
        bool $read_mode,
        Context $context = null,
        CodeLocation $code_location = null
    ) {
        return true;
    }

    /**
     * @return ?Type\Union
     * @override
     */
    public static function getPropertyType(
        string $fq_classlike_name,
        string $property_name,
        bool $read_mode,
        StatementsSource $source = null,
        Context $context = null
    ) {
        return Type::getMixed();
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants