SQLやコマンド実行を書いたときのうっかりを警告するための文字列チェック PHPStan Extension を作った

作った。
https://github.com/nishphp/phpstan-safestring-rule


この前[htmlspecialcharsをチェックする拡張](https://qiita.com/nishimura/items/eb6f4c9040f1b9ddbfed)を作っていて、`PDO->query()`でも同じようなことがしたくなった。
なのでHTMLに限定せずに汎用的にチェックできるようにした。


例えば、

```php:ProductSearchForm.php
<?php

namespace App;

class ProductSearchForm
{
    /** @var ?string */
    public $name;

    /** @var ?string */
    public $code;

    /** @var array<mixed> */
    public $categoryIds;

    /** @var ?string */
    public $queryString;
}
```

こんなクラスにDB検索用のデータが入っていたとする。

```php:ProductDb.php
<?php

namespace App;

use PDO;

class ProductDb
{
    /** @var PDO */
    private $pdo;

    public function __construct(PDO $pdo)
    {
        $this->pdo = $pdo;
    }

    /**
     * @return array<int,ProductDto>
     */
    public function getProductList(ProductSearchForm $form): array
    {

        $conds = [];
        $params = [];
        if ($form->name !== null){
            $conds[] = 'name = ?';
            $params[] = $form->name;
        }

        if ($form->code !== null){
            $conds[] = 'code = ?';
            $parmas[] = $form->code;
        }

        if ($form->categoryIds){
            $conds[] = sprintf('category_id in (%s)', implode(',', $form->categoryIds));
        }

        if ($conds)
            $where =  'where ' . implode(' and ', $conds);
        else
            $where = '';
        $sql = sprintf('select * from products %s', $where);

        $stmt = $this->pdo->prepare($sql);
        $stmt->execute($params);
        $ret = $stmt->fetchAll(PDO::FETCH_CLASS, ProductDto::class);
        if (!$ret)
            return [];

        /** @var array<int,ProductDto> $ret */
        return $ret;
    }
}
```

こんなDB処理を書く。あと.neonに設定を追記する。
そうすると

```
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ---------------------------------------------------------- 
  Line   ProductDb.php                                             
 ------ ---------------------------------------------------------- 
  42     PDO::prepare() Parameter #1 (string) is not safe-string.  
 ------ ---------------------------------------------------------- 

                                                                                                                       
 [ERROR] Found 1 error
```

エラーを報告してくれる。

ちゃんとプリペアドステートメントを使ってるはずなのに…とか思ってたら、`in (%s)` は内容が不明な文字列を渡していたことに気付く。
それで、`ProductSearchForm->$categoryIds`がmixedになっているのが良くないので、`array<int>`にする。

```php:ProductSearchForm.php
    /** @var array<int> */
    public $categoryIds;
```

そうすると

```
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                                                       
 [OK] No errors
```

PHPStanはパスする。



PHPStanは文字列の中の情報を保持しているので、内部的には普通の`string`と`$foo = 'my string';`で作った文字列は別の型になっている。
それを利用して、プログラマーが自分で書いたであろう文字列だけ通過させるようなExtensionにした。

PHPStanの内部的な動作は

```php:ProductDb.php
        $conds = [];

        //
        // $conds: Constant Array ([])
        //
        if ($form->name !== null){
            $conds[] = 'name = ?';
            $params[] = $form->name;
        }
        //
        // $conds: Union (Constant Array ([])|Constant Array ([0 => 'name = ?']))
        //

        if ($form->code !== null){
            $conds[] = 'code = ?';
            $parmas[] = $form->code;
        }
        //
        // $conds: Union (Constant Array ([]) |
        //                Constant Array ([0 => 'name = ?']) |
        //                Constant Array ([0 => 'code = ?']) |
        //                Constant Array ([0 => 'name = ?', 1 => 'code = ?']))
        //
```

このようになっている。変数の中の文字列パターンが全て分かっているので、ここまでの`$conds`変数は安全であることも分かる。
標準では`implode`や`sprintf`の引数に`constant`でない変数がある場合は標準化されて通常の`array<int,string>`などになってしまう（`constant`のみの関数呼び出しなら`constant`のまま計算してくれる。ソースを読んで初めて気付いたすごい細かい設計）。

先程の`sprintf('where id > %d', (int)$id);` のような変換は、不明な文字列ではあるものの、HTMLテンプレートやSQLに使う文字列としては特に問題ないはずである。
`$form->categoryIds` は `int`の配列だから、それを`implode`して`sprintf`してもまあまあ安全だろう。
ということで、

```php
            $conds[] = sprintf(
                'category_id in (%s)',
                // constant-string

                implode(',', $form->categoryIds)
                // implode(constant-string, int|constant|union(constant|int|...)...) => safe-string
            );
            // sprintf(constant-string|safe-string, safe-string, ...) => safe-string
            // $conds => array<int,safe-string>
```

`string`よりは安全な`safe-string`という型に変換するようにした。

SQLを手で書く人はあまり居ないかもしれないけれど、他にもコマンドを実行したりファイルシステムのpathを指定したりするような箇所で使える。
ORMを使うとしても、念の為に`Illuminate\Support\Facades\DB::select`等に警告を設定しておけば知らないところで安全でないSQLが実行されることを防げる。


基本的な型の動作を変更するためにオーバーライドした関数は実験的だと言われたので、この先本体がバージョンアップしたらどうなるかは分からないところではあるけども。


あと余談。
型の自動解析をしていると、関数が長くなったからと言って細かく分けるのはやめた方が良いというような、コードの書き方にも影響が出てくる。

せっかく変数の中身が分かっているのに、関数を細かく分割すると変数の中身の情報が消えちゃう。
かと言って配列とUnion Typeの入り組んだ型をコメントで書くのはつらい。PHPStanのPHPDoc解析もそこまで細かくできないし。
自動で計算できるのならそっちの方がいい。