BASEプロダクトチームブログ

ネットショップ作成サービス「BASE ( https://thebase.in )」、ショッピングアプリ「BASE ( https://thebase.in/sp )」のプロダクトチームによるブログです。

BASE で使っているPHPフレームワークにプルリクエストを送ったけど、先を越された話2

タイトル:BASE で使っているPHPフレームワークにプルリクエストを送ったけど、先を越された話2

この記事はBASE Advent Calendar 2020の12日目の記事です。 devblog.thebase.in

こんにちは!BASE株式会社 ServiceDevのShopグループ所属でエンジニアをしている炭田(@tanden)です。

「BASE」の裏側で動いているアプリケーションはCakePHP 2を使っています。そのCakePHP 2にプルリクエストを送ったけど先を越されてしまった話をします。

過去にも弊社の田中(@tenkoma)が同じような記事を書いていたので、そちらも合わせてご覧いただけると嬉しいです! devblog.thebase.in

プルリクエストの内容

今回自分がプルリクエストを送ったのは、Validation::time()の不具合の修正です。Validation::time()は渡された文字列が妥当な時刻の形式になっているかどうかをチェックします。

渡された文字列が妥当な時刻かどうかを検証します。時刻は 24時間形式 (HH:MM) または am/pm ([H]H:MM[a|p]m) です。秒までは検査できません。

データバリデーション - 2.xから引用

しかし、ある開発作業のコードレビューの中で、Validation::time()を使おうとなり、実際にValidation::time()を使ったコードをテストをしていたところ「12:00のようなHH:MMの形式以外は受け付けないはずなのに、12のような『:MM』がない文字列でもパスしてしまう」現象が起きてしまいました。なぜかと思い、CakePHP 2のValidation::time()のコードを確認してみると、以下のようになっていました。

/**
 * Time validation, determines if the string passed is a valid time.
 * Validates time as 24hr (HH:MM) or am/pm ([H]H:MM[a|p]m)
 * Does not allow/validate seconds.
 *
 * @param string $check a valid time string
 * @return bool Success
 */
    public static function time($check) {
        return static::_check($check, '%^((0?[1-9]|1[012])(:[0-5]\d){0,2} ?([AP]M|[ap]m))$|^([01]\d|2[0-3])(:[0-5]\d){0,2}$%');
    }

https://github.com/cakephp/cakephp/blob/2.x/lib/Cake/Utility/Validation.php#L386-L396

原因となる正規表現

せっかくなので原因となる正規表現を詳しく見ていきます。前半部分の((0?[1-9]|1[012])(:[0-5]\d){0,2} ?([AP]M|[ap]m))は「AM/PM形式の時刻のチェックをしている」部分になります。今回の問題点は24時間表記(HH:MM)の部分なので飛ばして|ORで区切られた後半部分を確認していきます。

後半部分は以下のようになっています。

^([01]\d|2[0-3])(:[0-5]\d){0,2}$

^^の次に指定された文字列で始まっていることを示すメタ文字で、$$の前に指定された文字列で終っていることを示すメタ文字です。なので、その間の([01]\d|2[0-3])(:[0-5]\d){0,2}を見ていけば、今回の24時間表記の時間のバリデーションの問題の原因がわかりそうです。前半の([01]\d|2[0-3])では「0もしくは1が先頭にきて、次に任意の数字(0から9)がくる OR(|) 20, 21, 22, 23のいずれかがくる」文字列にマッチさせていることがわかります。これは24時間表記のHHの「00 - 23」までを表し特に問題なさそうです。

次に時間の「分」の部分をマッチさせる後半の(:[0-5]\d){0,2}では「まず:がきて、0から5のいずれかの次に任意の数字がくる文字列((:[0-5]\d))の0回以上2回以下の繰り返し({0,2})」になっています。ここで問題になるのは「0以上2回以上の繰り返し」が指定されているので「:以下が無し、:00:00:00」の3パターンの文字列にマッチしてしまいます。ドキュメントだと、24時間表記の場合はHH:MMのみで秒はチェックしないとあるので、ドキュメントとの相違が2つ含まれていることがわかります。

  • HHのみの形式でもtrue
  • HH:MM:SSの形式でもtrue

なので、{0,2}の部分を外してしまって、以下にような正規表現にするとドキュメントとの相違点が無くなりそうです。

- '%^((0?[1-9]|1[012])(:[0-5]\d){0,2} ?([AP]M|[ap]m))$|^([01]\d|2[0-3])(:[0-5]\d){0,2}$%'
+ '%^((0?[1-9]|1[012])(:[0-5]\d){0,2} ?([AP]M|[ap]m))$|^([01]\d|2[0-3]):[0-5]\d$%'

この部分を修正して、テストも一緒にプルリクエストとして出せば取り込まれて直せそうだな、めでたしめでたし(※)と思い、CakePHP 2向けに以下のプルリクエストを作成して送った(最初に出したプルリクエスト)のですが、早速以下のコメントをいただきました。

2.x is at security maintenance mode only now.

バージョン2系はもうセキュリティ面以外のメンテナンスはされていないのですね...(知りませんでした)。Validationメソッドはできるだけカスタムメソッドを使って実装したくなかったので、できれば本体に取り込んだものを使いたかったのに...残念。

※ 秒数を受けないようにする対応はAM/PM表記用の正規表現においても対応する必要がありました。

CakePHP 4にもプルリクエストを出してみる

CakePHP2での変更はかないませんでしたが、せっかくなのでバージョン4系のValidation::time()ではこの問題が修正されているのか見てみることにしました。

以下は修正前の古いコードです。

/**
     * Time validation, determines if the string passed is a valid time.
     * Validates time as 24hr (HH:MM[:SS][.FFFFFF]) or am/pm ([H]H:MM[a|p]m)
     *
     * Seconds and fractional seconds (microseconds) are allowed but optional
     * in 24hr format.
     *
     * @param mixed $check a valid time string/object
     * @return bool Success
     */
    public static function time($check): bool
    {
        if ($check instanceof DateTimeInterface) {
            return true;
        }
        if (is_array($check)) {
            $check = static::_getDateString($check);
        }

        if (!is_scalar($check)) {
            return false;
        }

        $meridianClockRegex = '^((0?[1-9]|1[012])(:[0-5]\d){0,2} ?([AP]M|[ap]m))$';
        $standardClockRegex = '^([01]\d|2[0-3])((:[0-5]\d){0,2}|(:[0-5]\d){2}\.\d{0,6})$';

        return static::_check($check, '%' . $meridianClockRegex . '|' . $standardClockRegex . '%');
    }

https://github.com/cakephp/cakephp/blob/4.next/src/Validation/Validation.php#L628-L655

starndardClockの正規表現を見てみると...

([01]\d|2[0-3])((:[0-5]\d){0,2}

時間の「分」をマッチさせる箇所で「0回以上2回以下」の指定がそのまま残っていました。これでは、HHのみの文字列でもtrueが返ってしまいます。なので、以下のように修正するプルリクエストをテストと一緒に出してみました。

- '^([01]\d|2[0-3])((:[0-5]\d){0,2}|(:[0-5]\d){2}\.\d{0,6})$';
+ '^([01]\d|2[0-3])((:[0-5]\d){1,2}|(:[0-5]\d){2}\.\d{0,6})$';

時間の「分」をマッチさせる箇所で「0回以上2回以下」のところを「1回以上2回以下」に変更しています。しかし、コメントで"We should update the documentation instead. Treating 12 as 12:00 seems like a reasonable thing to me."(HHの形式でも違和感ないから、仕様が書いてあるコメントの方を変更しよう)となり、「あれ、仕様の方を変えるのか...」と一瞬思いましたが「たしかに、00から23をtimeとして扱うのはわからなくもない」と考え、修正用のプルリクエストはクローズしてメソッドのコメントだけ変えるプルリクエストを作成し、こちらは無事に取り込まれました。

さらにコメントを貰うが放置してしまう

メソッドのコメントを修正するプルリクエストを作っているときに別の方から"I dont think TimeType will marshal HH only time if that matters."とコメントをいただきました。自分は最初このコメントの意味がよくわからず(marshal?)、またちょうど業務で忙しくなってきたタイミングだったので、メソッドのコメントを修正したプルリクエストを出したままでそのまま放置していました。しかし、後日CakePHPのリポジトリを覗いてみると以下のプルリクエストがマージされていました。

Restrict Validation::time() so it requires minutes not just hours

「実装の方を修正してる!"marshal"(things in order)ってそういうことかー!」と悔しい思いをしました(せっかくなら実装に差分があるコミットを取り込まれたかった...笑)。ですが、「OSSでも日頃の業務と同じくプルリクエスト上で色々議論しながら作っていくんだな」と当たり前のことを体験することができました。こちらの修正は4.2.0でリリースされるようです(4.2.0のマイルストーン)。とりあえずプルリクエストを出しておくのは大事ですね!

ちなみに:CakePHP 2でValidation::time()の不具合に対応するには

CakePHP 2をお使いの場合、Validation::time()の不具合に対応するには以下の正規表現でカスタムのバリデーションメソッドを作成すれば、「HHのみの形式」や「秒数付きの文字列」は受けつけないようにすることが可能です。

'%^((0?[1-9]|1[012])(:[0-5]\d){0,1} ?([AP]M|[ap]m))$|^([01]\d|2[0-3]):[0-5]\d$%'

まとめ

日頃お世話になっているフレームワークに少しは貢献できたかなと思いつつ、今度は実装を修正したコミットが取り込まれるよう機会があれば再チャレンジしたいと思います!

明日は同じServiceDevのShopグループの元木さんです!

「BASE Advent Calendar」の記事一覧はこちら。

追記

この記事を書いていて、「そういえばCakePHP 3の方はまだ対応されていない気がするな」と思い、CakePHP 3のコードを見てみると、やはり同じような実装になっていたのでプルリクエストをとりあえず出してみました。どうなるかわかりませんが、今回は最後までやりきりたいと思います!

CakePHP 3に出したプルリクエスト