vimeo / psalm

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

How to inherit taint annotations #5748

Open ohader opened 3 years ago

ohader commented 3 years ago

https://psalm.dev/r/b28df72592

I'm looking for a way to enforce inheriting @psalm-taint-* behavior from stubs to specific implementations. Please see the referenced example and play around by (un)commenting overridden method in more specific class.

Thanks in advance for any pointer/guidance!

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

I found these snippets:

https://psalm.dev/r/b28df72592 ```php dangerous()); ``` ``` Psalm output (using commit 40bc7cf): No issues! ```
ohader commented 3 years ago

Since this is marked as "enhancement", I guess a fix would require some explicit @psalm-taint-descend annotation, correct? I could try working on a corresponding pull-request.

weirdan commented 3 years ago

Or inheriting them by default. I'm not very familiar with taint checking part, so I don't know if that's desirable. @muglug what do you think?

ohader commented 3 years ago

A custom plugin-based work-around for sinks would look like this - I'm putting it here in hope it helps others, I had to search some hours identifying proper process flow and correct event-handler. Transforming that into a generic handling for vimeo/psalm would be the next step.

<?php
namespace TYPO3Security\PsalmPlugin\Analysis\Handler;

use Psalm\Codebase;
use Psalm\Plugin\EventHandler\AfterFunctionLikeAnalysisInterface;
use Psalm\Plugin\EventHandler\Event\AfterFunctionLikeAnalysisEvent;
use Psalm\Storage\FunctionLikeStorage;

/**
 * Custom implementation handling & descending taints (abstract -> specific)
 * @link https://github.com/vimeo/psalm/issues/5748
 *
 * @see FunctionLikeDocblockScanner::addDocblockInfo() // taints/sinks
 * @see FunctionLikeDocblockParser::parse() // taints/sinks
 */
class DescendTaintHandler implements AfterFunctionLikeAnalysisInterface
{
    /**
     * @var array<string, bool>
     */
    private static $processed = [];

    public static function afterStatementAnalysis(AfterFunctionLikeAnalysisEvent $event): ?bool
    {
        // @todo Consider, whether this shall be applied explicitly, e.g. using `psalm-taint-descend`

        $codebase = $event->getCodebase();
        $className = $event->getStatementsSource()->getFQCLN();
        if ($className === null) {
            return null;
        }

        $classStorage = $codebase->classlike_storage_provider->get($className);
        // @todo Change method name in Psalm to `getFunctionLikeStorage`
        $functionStorage = $event->getClasslikeStorage();

        $identifier = $className . '::' . $functionStorage->cased_name;
        if ($classStorage->stubbed
            || self::trackProcessed($identifier)
            || empty($classStorage->parent_classes) && empty($classStorage->parent_interfaces)
        ) {
            return null;
        }

        self::descendTaintSinks($codebase, $functionStorage, array_keys($classStorage->parent_classes));
        self::descendTaintSinks($codebase, $functionStorage, array_keys($classStorage->parent_interfaces));
        return null;
    }

    private static function descendTaintSinks(Codebase $codebase, FunctionLikeStorage $functionStorage, array $ancestorClassNames): void
    {
        $methodNameLowerCased = strtolower($functionStorage->cased_name);
        foreach ($functionStorage->params as $index => $parameter) {
            foreach ($ancestorClassNames as $ancestorClassName) {
                if (!$codebase->classlike_storage_provider->has($ancestorClassName)) {
                    continue;
                }
                $ancestorClassStorage = $codebase->classlike_storage_provider->get($ancestorClassName);
                $ancestorFunctionStorage = $ancestorClassStorage->methods[$methodNameLowerCased] ?? null;
                if ($ancestorFunctionStorage === null) {
                    continue;
                }
                $ancestorParameter = $ancestorFunctionStorage->params[$index] ?? null;
                if ($ancestorParameter === null) { // unlikely, since it would violate Liskov's substitution principle
                    continue;
                }
                if (!empty($ancestorParameter->sinks)) {
                    $parameter->sinks = $ancestorParameter->sinks;
                }
            }
        }
    }

    /**
     * @param string $identifier
     * @return bool whether it has been processed already
     */
    private static function trackProcessed(string $identifier): bool
    {
        $processed = !empty(self::$processed[$identifier]);
        self::$processed[$identifier] = true;
        return $processed;
    }
}