コードが酷いと dis るだけじゃ何も変わらないと思うので直してみるよ

PHPer のみなさん。こんにちは。元気ですか? 僕は元気じゃないです。体調は最悪だし、マイケル･ジャクソンは亡くなってしまったし。僕の中のスーパースターがまた1人逝ってしまいました。ほんとに悲しい。そう僕とマイケルとの出会いはもうかれこれ何年になるだろうか…
…じゃなかった。マイケルの話はまたいずれするとして、今日は PHP の話をします。

　　　 〃　　 　 　 　 　 　 　 　 i,　 　 　 　 ,.　-‐
　　　r' 　 ｨ=ゝー-､-、､r=‐ヮｫ.〈　　　　／
　 　 !　　:l　　　　　　,ﾘ|}　　　 |. }　　　/　　 .こいつをみてくれ…。どう思う?
.　　 {. 　 |　 　 　 　 　 ′　 　 | }　 　 l
　 　 ﾚ-､{∠ﾆ'＝=ｧ 　 ､=＝ﾆゞ<　 　 |　　　 
　 　 !∩|.}. '"旬ﾞ`　　 ./''旬 ` f＾|　 　 |
　　　ｌ（( ﾞ′｀￣'"　　 f::`￣　 |l.|　　　| 　 　 http://codezine.jp/article/detail/4044
.　 　 ヽ.ヽ　　　　　 　 {:.　　　 ｌﾘ 　 　 |
.　　　 }.iｰi　　 　 　 ^ r'　　　 ,'　　　 ノ　　　 
　　 　 !|　ヽ.　　 ｰ=＝=- 　 /　 　 ⌒ヽ
.　　 ／}　　 ＼　 　 ｰ‐ 　 ,ｲ　　　　 　 l　 　 
　__/　‖　 .　　ヽ､＿!__／:::|＼ 　 　 　 ヽ

ECサイトの会員登録画面・ログイン機能を作成しよう！プログラミング未経験から始めるPHP入門～応用編(3) （1/3）：CodeZine

　､　　　　　　　　ヽ
　|ヽ ﾄ､ 　ﾄ､　ﾄ､ ､.`、
/|l. l. | |l l | | l |l.| |l. l
/'　j/ ﾉ|ﾙ'/ﾚ〃j/l　|
-‐7"　ヾー---┐|_.j
￣ 　　./ﾞﾆ,ﾆF､'' ｌ _ヽ
:: 　 ,.,. |ヽ 」９L.` Ｋ }.| 　　　　　　　すごく…ひどいです…
　　　　ｌ'　 """　 ｌ ) /
　　ｈ､,.ﾍ. 　 　 　ﾚ'/
　　　　　 　　　　ﾚ′
　r.二二.) 　 　 /　　
　　≡≡ 　 　,ｲ
.　　　　 　 ／　!
＼　　　／　　├､
::::::｀￣´ 　 /　 !ハ.

さっきたまたまはてブで見つけたのですが。ひどいっす。この記事を書かれたのは大家さんという方でデジハリで PHP 講座の講師をされているらしいのですが、まさか生徒さんにもこんな感じで教えられているわけじゃないですよね? ね? これだと生徒さんが可哀想ですよ…。*1
案の定はてブのブックマークコメントでも酷い言われようです。でも、ちょっと待って欲しい。みんなブックマークコメントで dis っただけで満足しちゃいないか? 自己完結して溜飲を下げてそれで終わり? もし、PHP 初心者かつプログラム初心者があのエントリを見て「こう書いたらいいんだ!」とか思ったらどうする!? そのプログラマはもちろん、もしかしたら僕ら PHPer まで不利益をこうむるんじゃなかろうか*2。なので dis って終わりではなくてちゃんと直してあげようじゃないか。こういう時こそ「コミュニティのやさしさ」みたいなものが発揮できるのではないだろうか。
…と僕は思ったので直してみることにしたよ。

$_POST じゃなくて $_REQUEST?

なんで $_POST じゃダメなん? ざっくりコードを覗いてみましたが $_POST でダメな箇所は見当たりませんでした。$_REQUEST は $_GET, $_POST, $_COOKIEの内容を含みます。なのでこのサンプルの場合、form に入力しなくてもブラウザのアドレスバーから

http://hostname/appname/member.php?login_name=hoge&login_pass=fuga

と入力すれば、form に入力する場合と同じ効果を得ることができます。このサンプルコードを見る限りユーザには form に id や password を入力させて認証をさせることを意識した機能になっているので、想定外の操作は出来なくするべきじゃないかなーと思いますよ。

スーパーグローバル変数を直に触るのはどうかとも思う

この記事はフレームワークを使用していないので、まぁ仕方ないかとも思いますが。でもせめて magic_quotes_gpc が On の場合は stripslashes するくらいは気を使ってもいいのではないかと思います。こんな感じの function を作って $_POST を引数に渡すといい感じ。

<?php
if (get_magic_quotes_gpc() === 1) {
   $_POST = stripslashes_deep($_POST);
}
// この function はCakePHPで実際使用されています
function stripslashes_deep($values) {
    if (is_array($values)) {
        foreach ($values as $key => $value) {
            $values[$key] = stripslashes_deep($value);
        }
    } else {
        $values = stripslashes($values);
    }
    return $values;
}

DB アクセスは PDO を使うといいと思う

この記事でもそうですが、なぜか PHP の入門記事は PDO を避ける傾向があるように思います。なんでだろう? RDB 毎に違う関数を覚える必要はないし、バインドパラメータさえ気をつけていればある程度はセキュアになるのに。*3以下コード

<?php
// ローカルホストでDBがec,ユーザがrootでパスワードなしの例
$pdo = new PDO('mysql:dbname=ec;host=localhost;', 'root', '');
$stmt = $pdo->prepare('SELECT * FROM m_customers WHERE customer_code=:code and pass=:pass');
$stmt->bindValue(':code', $_POST['login_id']);
$stmt->bindValue(':pass', $_POST['login_pass']);
$stmt->execute();

パスワードはハッシュ化しちゃおう

この記事ではパスワードが平文で保存されています。もし、何かの脆弱性を突かれてユーザID とパスワードが漏れてしまった場合とても困ったことになります。なのでパスワードはハッシュ化しちゃいましょう。
insert する際に sha1 とかでハッシュ化して保存、select 時に $_POST されたパスワードを sha1 して DB に問い合わせるといいのではないでしょうか。

View に出力する際は htmlspecialchars を使おう

いろんな人が口をすっっっっっっぱくして言っていますが、悲しいことにこの記事では使用されていません。

<?php
ようこそ<span class="person"><?php print($_SESSION["name"])?></span>さん！

はこうしましょう

<?php
ようこそ<span class="person"><?php print(htmlspecialchars($_SESSION["name"], ENT_QUOTES, 'UTF-8'))?></span>さん！

比較には === を使おう

皆さんこのエントリを覚えていますか?

PHPの比較の素晴らしさ加減は正常

== 演算子は PHPer からするともう使っちゃいけないものな感じなので比較する場合は === を使いましょう。ここらへんの扱いでキモさや理不尽さを感じるのであれば PHP を使わないほうがいいと思います。

まとめ

ざっくり見た感じでは指摘事項はこんなところかな、と思います。個人的には print より echo だろ、とかダブルクォートよりシングルクォートのほうがよくね? という部分はありますが。
足りない部分や間違っている部分などありましたら、トラックバックやコメントでお知らせ下さい。こういった添削・指摘の連鎖が起こることによって、相対的にレベルの高いコードが Web 上にもっと増えたら嬉しいな、と思っていますので。

*1:セキュリティ周りのことを教えるのは確かに難しい。内容もタイミングも。だからせめてちゃんと注記を入れて欲しい。「この記事で使用されているコードはセキュリティを考慮してません」とか「ログイン処理は概ねこんな仕組みで出来ていますがコピペはダメ、ゼッタイ」とか。

*2:これだから PHPer は…みたいな

*3:PDO は実際はプリペアードクエリではないって大垣さんの記事に書いてた。でも詳しくは知らないので誰か教えて下さい。

localdisk

PHP とか Java とか Web とか好きなことを書きます。