vimeo / psalm

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

Assertions in Taint Flow Graph #7870

Open ohader opened 2 years ago

ohader commented 2 years ago

https://psalm.dev/r/97bae91b9c

<?php // --taint-analysis
/**
 * @psalm-taint-guard html $value
 * @todo `psalm-taint-guard` does not exist yet
 */
function assertProperEncoding(string $value): void
{
    if (htmlspecialchars($value, ENT_QUOTES) !== $value) {
        throw new \RuntimeException('Tags are not allowed');
    }
}

$value = (string)($_GET['value'] ?? '');
assertProperEncoding($value);
echo $value;

The example above would throw an exception if <script>// some code</script> would be assigned to $value, but continue as expected in case just some-plaint-text would be given.

The taint flow graph does not recognize this assertion and still reports the mentioned source code example as tainted. In order to avoid false-positives, the graph needs to be updated and the tainted flag cleared for $value once assertProperEncoding($value) has been called.

Stubs might use a new @psalm-taint-guard html $value (or ..-reset or ..-assert - not sure about the naming yet).

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

I found these snippets:

https://psalm.dev/r/97bae91b9c ```php
AndrolGenhald commented 2 years ago

Stubs might use a new @psalm-taint-guard html $value (or ..-reset or ..-assert - not sure about the naming yet).

Or @psalm-taint-remove, -validate, -throw. I like -assert since it's analogous to other assertions, but in that case it's a bit confusing because @psalm-taint-assert html would assert that the variable isn't tainted with html. -remove is also a bit confusing because from the name it seems like it removes the taint, when really it's just verifying that there was no taint. I suppose -guard seems the best unless someone can come up with something better. We definitely need to make sure users are aware that the function is supposed to throw instead of returning a bool (and preferable find a name that still works with -if-true tacked on the way @psalm-assert-if-true works). Maybe something like @psalm-assert-untainted, although it doesn't have the @psalm-taint prefix like the rest of the tags?

This works for now, but I'm guessing you already know that.

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

I found these snippets:

https://psalm.dev/r/4a6e7f89d0 ```php