転職したらスマレジだった件

スマレジのエンジニアやまてのテックブログです。マジレス大歓迎です。

トップ > 学習記録 > プログラミング研修でのフルボッコなコードレビューの記録 - ③Webアプリケーション基礎

プログラミング研修でのフルボッコなコードレビューの記録 - ③Webアプリケーション基礎

学習記録 PHP 入社時研修

こんにちは！

スマレジ・テックファームのWebエンジニアやまてと申します。


今回は『プログラミング研修でのフルボッコなコードレビューの記録 - ③Webアプリケーション基礎』ということで、昨年12月1日に入社してからの2カ月で受けた社内の研修について、引き続き振り返っていきたいと思います。


スマレジ・テックファームの研修についてですが、実務未経験で採用された際には、SESでの出向先が決まるまで、社内の教育担当のWebエンジニアの方から研修をしてもらえます。

ryamate.hatenablog.com

corp.smaregi.jp


研修内容は、以下のカリキュラムで、今回の記事はこちらに沿って投稿しています。

①PHPの基礎

②SQLの基礎

③Webアプリケーション基礎


③ Webアプリケーション基礎

「Webアプリケーション基礎」の課題については、 セキュリティ と バリデーション に関するレビュー内容に絞って、整理していきたいと思います（一番レビューで色々と指摘があった、かつ、重要と感じたため）。

Webアプリケーション基礎の課題の目的は、以下を理解することです。

  • HTML, CSSの概要
  • HTTPリクエスト（GET、POST）
  • Cookie、セッション
  • DB接続したデータの取得・表示
  • セキュリティ
  • バリデーション

例えば下記のような課題でした。


「HTML, CSS, PHPを利用して、以下の画像と似たような画面を実装してください。

」


このような問題が数ステップあり、OKもらえるまで修正しました。


今回触れていない HTTPリクエスト（GET、POST）、Cookie、セッションなどについても多数のレビューをいただきましたが、 セキュリティ と バリデーション について抜粋して、コードレビューを振り返っていきます。

私がもらったリアルなレビューなので、網羅的ではないですが、初心者が見落としがちなポイントが多いかと思っています。

※ 記事にする都合上、レビューの文面は意味が変わらない程度に編集しています。

1. セキュリティ

まずは、Webアプリケーションを作成する上で必須の、セキュリティに関するレビューです。

1.1. 「ユーザーの入力値をそのままSQLに入れないでください」

◀︎レビュー前

<?php
// 略

/**
 * リクエストされた検索条件に従ってproductテーブルからデータを取得する
 *  - 商品名は部分一致での検索とする。
 *  - 価格は入力値以上の範囲検索とする。
 *  - 表示件数の選択肢は10件,20件,30件とする。
 *  - 作成日時の降順、IDの昇順で取得・表示とする。
 *
 * @param array $searchValues (バリデーションエラーがなかった)検索条件
 * @return array $products リクエストされた検索条件で、productsテーブルからデータ取得された商品
 */
function searchProducts(array $searchValues):array
{

    // データベースに接続する
    $link = dbConnect();

        // productsテーブルからリクエストされた検索条件に従って商品名、色、商品単価を取得するSQL文
        $sql = <<<SQL
        SELECT
            product_name,
            color,
            price
        FROM
            products
        WHERE
            product_name LIKE '%{$searchValues['productName']}%'

💬レビュー

入力値をそのままSQLに入れることは絶対にしてはいけません。

SQLインジェクション によって不正にデータベースを操作されてしまいます。(例：商品名の入力を'; {任意のSQL文}とすると、任意のSQL文が実行できてしまいます)

対策として プリペアドステートメント とバインド変数を用いる方法があり、PDOでも利用可能です。

▶︎レビュー後

<?php
// 略

function searchProducts(array $searchValues):array
{

    // データベースに接続する
    $link = dbConnect();

    // productsテーブルからリクエストされた検索条件に従って商品名、色、商品単価を取得するSQL文
    $sql = <<<SQL
    SELECT
        product_name,
        color,
        price
    FROM
        products
    WHERE
        product_name LIKE :productName
        AND
        price >= :price
    ORDER BY
        created_at DESC,
        product_id ASC
    LIMIT
        :limit
    ;
    SQL;

    $stmt = $link->prepare($sql);

    // SQLインジェクションの対策として、プリペアドステートメントとバインド変数を用いる
    // - LIKE検索のワイルドカードについて、エスケープする
    $stmt->bindValue(':productName', '%'.addcslashes($searchValues['productName'], '\_%').'%', PDO::PARAM_STR);
    $stmt->bindValue(':price', $searchValues['price'], PDO::PARAM_INT);
    $stmt->bindValue(':limit', $searchValues['limit'], PDO::PARAM_INT);

    // SQLを実行する
    $stmt->execute();

    // データを取得する
    $products = $stmt->fetchAll(PDO::FETCH_ASSOC);

    // データベースの接続を解除する
    dbDisconnect($link);

    return $products;
}

絶対にしてはいけない、と言われるレベルのことをしてしまいました。

独学の頃にも学んだことがある気もしますが、重要性を理解できていませんでした！…


1.2. 「ユーザーの入力値を表示する際のエスケープを必ず行ってください」

◀︎レビュー前

<main>
    <div class="mx-4 my-2">
        <h1>Cookie</h1>
        <p><?= $name ?>さん、ようこそ！</p>

💬レビュー

ユーザーの入力値を表示する際のエスケープは、必ず行ってください。

  • エスケープしないと、HTMやJavaScriptを注入・変形されるXSS（Cross-Site Scripting）脆弱性が生じる
  • エスケープとは、メタ文字の持つHTMLの文法上特別な意味を打ち消すこと

▶︎レビュー後

<main>
    <div class="mx-4 my-2">
        <h1>Cookie</h1>
        <p><?= escape($name) ?>さん、ようこそ！</p>

<?php

/**
 * XSS(クロスサイト・スクリプティング)対策として、特殊文字をエスケープする
 */
function escape($string)
{
    return htmlspecialchars($string, ENT_QUOTES, 'UTF-8');
}

こちらは課題の中で対策はしていたのですが、対策の抜け漏れがあり、自身のセキュリティへの認識の甘さを感じました。

研修でのレビューを通して、これらのセキュリティはWebアプリケーションを作成する上で当然わかっていないといけない、そしてそれを理解できていない、ということが浮き彫りになりました。

転職のためのポートフォリオとして作成したWebアプリケーションに、ザルすぎるセキュリティホールがなかったか今更ながら不安になりました…すでにWebでの公開はしていませんが、今度ソースコードを振り返っておきたいです。


1.3. 「セキュリティについて体系的に勉強しておいてください」

コードレビューじゃないですが、「セキュリティは重要なので、現状のような指摘されてから対処するという場あたり的な状態から抜け出すために体系的に勉強しておいてほしい。まずはこれを読んでおいてほしい」と言われたものがこちらです。

www.ipa.go.jp

通読したのですが、代表的な脆弱性と、その脅威、対応策がまとめられていました。

代表的な攻撃手段を一通り把握できていると、ここには対応が必要ではないかと思い当たると思う、というか巻末にチェックシートがついてるので、対策に漏れがないかのチェックにも使えそうです。


また、『安全なウェブサイトの作り方』の執筆者の一人である徳丸浩先生の著書『安全なWebアプリケーションの作り方』については、現在読み進めているところです（1日1項目読み進めているけど、本が分厚いのであと3ヶ月くらいかかりそう）。

www.amazon.co.jp

徳丸本と呼ばれている有名なやつですね。読了後にはブログでアウトプットしていきたいと考えています。


2. バリデーション（入力値の受け取り、流れ）

バリデーション（入力値検証）は、ユーザーが入力した値をチェックするということです。

2.1. 「バリデーションの要件を満たすように実装してください」

◀︎レビュー前

ぐちゃぐちゃすぎて、いろんな意味で載せられません。

💬レビュー

バリデーションの要件を満たすように実装してください。

修正していただきたい内容は以下になります。

  • 入力があるかチェックする。
    • ない場合、初期値を設定。
    • ある場合、その入力に対してバリデーションを行う。
      • バリデーションでエラーの場合、 エラーメッセージを表示し、入力欄は空にする

▶︎レビュー後

<?php

require_once __DIR__ . '/lib/db_connect.php';
require_once __DIR__ . '/lib/escape.php';
require_once __DIR__ . '/lib/validate.php';

// 初期値を変数に代入する
$products = [];
$errors = [];

// 検索条件（商品名、価格、表示件数）のGETパラメータがあるかチェックする
if (isset($_GET['productName']) || isset($_GET['price']) || isset($_GET['limit'])) {
    $searchValues = [
        'productName' => isset($_GET['productName']) ? $_GET['productName'] : '',
        'price' => (isset($_GET['price']) && $_GET['price'] !== '') ? $_GET['price'] : '0',
        'limit' => isset($_GET['limit']) ? $_GET['limit'] : '10',
    ];

    // 検索条件（商品名、価格、表示件数）をバリデーションし、エラー発生した項目についてのエラーメッセージを変数に代入する
    $errors = validateSearchValues($searchValues);

    if (count($errors) > 0) {
        // バリデーションエラーがある場合、バリデーションエラーが発生した検索条件（商品名、価格、表示件数）について、空などの値を変数に代入する
        $searchValues = emptySearchValues($searchValues, $errors);
    } else {
        // バリデーションエラーがない場合、リクエストされた検索条件に従ってproductテーブルからデータを取得する
        $products = searchProducts($searchValues);
    }

    if ($_GET['price'] === '' || !isset($_GET['price'])) {
        $searchValues['price'] = '';
    }
} else {
    // GETパラメータなし(最初のアクセス)の場合、初期値を変数に代入する
    $searchValues = [
        'productName' => '',
        'price' => '',
        'limit' => '10',
    ];
}
// 商品検索画面を表示する
$title = '商品検索';
$content = __DIR__ . '/views/index.php';
include __DIR__ . '/views/layout.php';

2.2. 「『パラメータが入力されているか』のみの検証はempty() ではなく、 isset() を使ってください」

◀︎レビュー前

<?= $_GET['productName'] === '0' || !empty($_GET['productName']) ? escape($_GET['productName']) : '';?>

💬レビュー

今回は「パラメータが入力されているか」を検証する必要があります。動作としては問題ないと思いますが、そもそもempty()を使って判定する必要はありません。

PHPマニュアルには以下の通り記載があります。

empty() は本質的に !isset($var) || $var == false と同じことを簡潔に記述しているだけです。

PHP: empty - Manual

「パラメータが入力されているか」（!isset($var)）の判定は必要 ですが、「入力されたパラメータの値が何か」（$var == false）の判定は不要 です。

なので、今回のケースではisset()を用いるのが適切です（空入力を判定したい場合は、$var === ''の判定を加える）。

▶︎レビュー後

<?= isset($_GET['productName']) ? escape($_GET['productName']) : '';?>

※ 参考（レビュー後にisset()が何を返すかを var_dump で確認した）

<?php

// アクセスした(GETパラメータがない)とき
var_dump($_GET['price']); // null
var_dump(isset($_GET['price'])); // boolean false

// '0'を入力して検索ボタンを押したとき
var_dump($_GET['price']); // string '0' (length=1)
var_dump(isset($_GET['price'])); // boolean true

// 未入力で検索ボタンを押したとき
var_dump($_GET['price']); // string '' (length=0)
var_dump(isset($_GET['price'])); // boolean true


2.3. 「GETパラメータがあるかのチェックは値全てに対して行ってください」

◀︎レビュー前

<?php
// 略

// 検索ボタンを押した(URLパラメータを受け取った)場合、GETパラメータの値を変数(配列)に代入する
if (isset($_GET['productName'])) {

💬レビュー

productName、price、limit の３つの GET パラメータがあるので、この判定では不十分です。

検索ボタンを押した場合は productName をパラメータとして持っていますが、クエリストリングで productName を指定せずにGETリクエストを送信した場合、パラメータに productName がないためこの条件式は false となり、if文内の一連の処理が一切行われません。

以下の指定している初期値が表示されるだけになります。

<?php
// 略

$searchValues = [
    'productName' => '',
    'price' => '',
    'limit' => '10',
];

▶︎レビュー後

<?php
// 略

// 検索条件（商品名、価格、表示件数）のGETパラメータがあるかチェックする
if (isset($_GET['productName']) || isset($_GET['price']) || isset($_GET['limit'])) {

クエリストリングをいじられたときも想定する必要があるんですね。


2.4. 「三項演算子内では三項演算子を使わないでください」

（バリデーション、というか三項演算子についてですが…）

◀︎レビュー前

isset($_GET['price']) ? ($_GET['price'] === '' ? '0' : $_GET['price']) : '0'

💬レビュー

三項演算子内では三項演算子を使わない方が良いです。

三項演算子を "積み重ねて" 使用することは避けましょう。 ひとつの文の中で括弧で囲わずに複数の三項演算子を使用した際の PHP の振る舞いは、 他のプログラミング言語のそれと比べて、少々わかりにくいものです。

PHP: 比較演算子 - Manual

▶︎レビュー後

(isset($_GET['price']) && $_GET['price'] !== '') ? $_GET['price'] : '0'

確かに、修正前のコードはどこで区切るのか一見してわからなくて読みにくいですね。


3. バリデーション（入力値検証の中身）

3.1. 「empty() を使うのを、やめてください」

◀︎レビュー前

<?php
// 略

/**
 * 作成する商品の入力内容についてバリデーションする。create.phpにて使用。
 *
 * @param array $inputs 入力内容(作成する商品)
 * @return array $errors エラー発生した項目についてのエラーメッセージ
 */
function validateInputs(array $inputs):array
{
    $errors = [];

    // 商品コード(「必須」、「整数 かつ 0 <= n < 10000000000」)
    if (empty($inputs['productCode'])) {

💬レビュー

empty は 0 の場合、 true になるため、 productCode が 0 以上 OK なのであれば、この判定はNGです。empty() を使うのをやめておいた方がよいです。

▶︎レビュー後

<?php
// 略

    // 商品コード(「必須」、「整数 かつ 0 <= n < 10000000000」)
    if (!isset($inputs['productCode']) || strlen($inputs['productCode']) === 0) {

empty() を使いこなせず（というか使う場面ではないのに）、不適切な使い方をするので、ついに empty() 禁止令が出ました。


3.2. 「バリデーションでの 整数 の判定方法は、 正規表現 を使ってください」

◀︎レビュー前

<?php
// 略

// 価格が10000000000以上、もしくは整数ではない場合、エラーメッセージ「価格は9999999999以下の整数で入力してください。」を表示する
if ((!ctype_digit($searchValues['price']) && $searchValues['price'] !== '')) {
    $errors['price'] = '価格は正の整数で入力してください。';
} elseif ((int)$searchValues['price'] >= 10000000000) {
        $errors['price'] = '価格は9999999999以下で入力してください。';
}


www.kabanoki.net

このページをもとに考えた結果、なぞなぞ不正解でした…

💬レビュー

バリデーションでの整数の判定方法は、 正規表現 を使ってください。

preg_match('/^-?[0-9]+$/', $a)

▶︎レビュー後

<?php
// 略

// 価格が10000000000以上、もしくは整数ではない場合、エラーメッセージ「価格は9999999999以下の整数で入力してください。」を代入する
if (((int)$searchValues['price'] >= 10000000000) || !preg_match('/^-?[0-9]+$/', $searchValues['price'])) {
  $errors['price'] = '価格は9999999999以下の整数で入力してください。';
}


今回の記事は以上です！


入社するまで独学でやってきたため、コードレビューをいただく初めての機会だったのですが、ソースコードの内容にしろ、レビュー依頼をする際の文面にしろ、「読み手が読みやすいようにまとめる」ということが、何より大事だと思いました。（多く指摘をいただいたこともあり）

「読み手が読みやすいように」という点は、前職の頃からとても気をつけていたことではありますが、今後もますます気をつけていきます。


ありがとうございました。



ということで、 ぜんぶがんばるます。