Help us understand the problem. What is going on with this article?

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

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

この前htmlspecialcharsをチェックする拡張を作っていて、PDO->query()でも同じようなことがしたくなった。
なのでHTMLに限定せずに汎用的にチェックできるようにした。

例えば、

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検索用のデータが入っていたとする。

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>にする。

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

そうすると

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



 [OK] No errors

PHPStanはパスする。

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

PHPStanの内部的な動作は

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変数は安全であることも分かる。
標準ではimplodesprintfの引数にconstantでない変数がある場合は標準化されて通常のarray<int,string>などになってしまう(constantのみの関数呼び出しならconstantのまま計算してくれる。ソースを読んで初めて気付いたすごい細かい設計)。

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

            $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解析もそこまで細かくできないし。
自動で計算できるのならそっちの方がいい。

Why not register and get more from Qiita?
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
Comments
Sign up for free and join this conversation.
If you already have a Qiita account
Why do not you register as a user and use Qiita more conveniently?
You need to log in to use this function. Qiita can be used more conveniently after logging in.
You seem to be reading articles frequently this month. Qiita can be used more conveniently after logging in.
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away