psalm: Taint should be ignored on int cast

Well, I believe the example in the issue (psalm.dev/r/0d4f65473b) was on point.

I agree to @mmcev106 that it is inconsistent because escaped strings are problematic in the same way:

class A {
    public function deleteUser(PDO $pdo) : void {
        $userId = self::getUserId();
        $sth = $pdo->prepare("delete from users where email = ?");
        $sth->bindValue(1, $_GET["email"]);
        $sth->execute();
    }
}

I just stumbled over this after upgrading from version 5.12.0 and bisecting it down to commit 94a98ccddd68445665813c4989b5d75d555695ed, merge 7a7d6a25af430473d343bfe8005c32fb7851b140 and issue #10238.

The original issue is describing an attack vector known as IDOR, which can be seen as cross-cutting concern for multiple taint types in PsalmPHP.

The mentioned commit caused a bunch of false-positive failures on our side. We are testing 3rd party code (plugins) we cannot directly control.

In general I agree that any attacker-controlled parameter can lead to a vulnerability - however the mentioned change was too intrusive. It should be reverted and re-introduced as opt-in feature. Here’s is why:

• the change was introduced with PsalmPHP 5.16.0 as bugfix, which changes behavior a lot and should have been considered as a breaking change - actually it’s enforce tainted numerics instead of allow tainted numerics

  Fixes Allow tainted numerics except for ‘html’ and ‘has_quotes’ by @cgocast in https://github.com/vimeo/psalm/pull/10242

• the meaning of the documented taint types at https://psalm.dev/docs/security_analysis/#taint-types were changed

  sql - used for strings that could contain SQL shell - used for strings that could contain shell commands …

• it is not complete (as outlined in previous comments already), here is another example that came to my mind
  • shell_exec(escapeshellcmd((int)$_GET['inject'])); is not considered tainted
  • shell_exec((int)$_GET['inject']); is considered tainted
• fixing the those tainted numerics is time consuming and difficult to impossible when dealing with 3rd party code and libraries, examples
  • Symfony Access-Control (authentication) can be configured in multiple different ways, e.g. via routes or (custom), tags or attributes → simply adding a @psalm-taint-escape to all occurrences is not possible for those scenarios (except writing a complex custom plugin that is parsing route configuration and the roles model)
  • Laravel CSRF tokens and other similar techniques that are for instance signing server-side request params using HMACs are located in different framework layers (templating/view or routing/dispatcher) and cannot be handled by the taint-flow graph
• existing TaintTest case were adjusted to match the new code - those tests were there for good reasons

tl;dr: Instead of applying “follow-up work” on an incomplete concept, I’d like to see those changes being reverted and probably re-implemented as explicit opt-in feature.