はじめに
この記事は前回の続きです。
まさかコードレビューをしてくださるとは思ってもいなくて、改めてこの場でお礼を申します。
rana_kualuさん、貴重な時間を割いていただき誠にありがとうございました。また、具体的な指摘や参考記事の紹介など非常に勉強になりました。
ただ悲しいのは私の実力不足で、記事を読んだり調べても理解できなかったこともあり、手始めにどうすればいいかわかったハッシュ化から手をつけました。
$sql="INSERT INTO member(id,m_name,pass,email)
VALUES('',:m_name,:pass,:email)";
$stmt=$s->prepare($sql);
$stmt->bindValue(':m_name',$_REQUEST['m_name'],PDO::PARAM_STR);
$stmt->bindValue(':pass',$hash,PDO::PARAM_STR);
$stmt->bindValue(':email',$_REQUEST['email'],PDO::PARAM_STR);
$stmt->execute();
紹介して頂いた記事のコードをコピペしてもできなかったので、調べて上記の記述でできました。
が、使っていたPHPのバージョンが古すぎてハッシュ化に対応していません。
なのでXAMPPをアップデータしたところ、今度はXAMPPが使えなく状況になり、３日間格闘しても直らず、結局前に導入していたMAMPに切り替えることでようやくハッシュ化に対応しました。
１つの問題解決しましたが他にも問題はあって、どう対処していこうかと悩み、いっそもう記事にも書いてあった通りフレームワークを導入した方が他の問題も解決するんじゃないか？と考えます。
その時、丁度フレームワーク（具体的にはLaravel）で再構築してみてはどうかとお声をかけてもらったのでLaravelの導入が決まりました。
Laravelで簡易掲示板を再構築
早速掌田 津耶乃『PHPフレームワーク Laravel入門』を購入して、ひとまず山田祥寛『独習PHP第３版』を読み進めることに。
流石に簡易掲示板を作ったのか、書いてある内容が理解できてわかる喜びをかみしめていました。
個人的に理解力＝知識×経験×興味・関心・知的好奇心だと思っているので、少しはレベルアップしているなと実感があります。
また、プログラミングスクールでJavaをやっていたので、オブジェクト指向についても理解があったことが助かりました。
こうしてLaravelの勉強を始めたのですが、案の定データベース接続が上手くできませんでしたね。
といいますか、Laravelが今までやってきたこととは異次元世界だったので、本に書いてあることをやってもただ単に何か知らないけどできた、という感じでした。
そんな私の救世主がこちらのplumsa『絶対に挫折させないアプリ開発 はじめてのLaravel』です。この本がなければ間違いなくLaravelで再構築できませんでした。本当にありがとうございました。
併せてえむにわブックマート『スタートダッシュ！Laravel』も購入したところ、Dockerについて書いてあったので更に
湊川あいの、わかば家。『#マンガでわかるDocker ① 〜概念・基本コマンド編〜 【ダウンロード版】 #技術書典 4』
[湊川あいの、わかば家。『#マンガでわかるDocker ② 〜開発環境を作ろう編〜 #技術書典 5 【ダウンロード版】』
も、いずれ必要になるからと購入。
とにかくこの頃は早く就職したいと焦ったので、絶対に挫折させない～でも最初からやらずに最後のコントローラーを使っての作成しかやっていませんでした（それでもLaravelについて初めてこうすればいいんだという感触は得たのでやはり素晴らしい本です）
データベースの接続も何とかできて、いざ再構築と思っていたのですが、MAMPをアップデートしてしまいMySQLが使えなくなりました。
XAMPPも使えないままですから、どうすればいいかと思いましたが、手始めにHomesteadを導入して見事に失敗。
折角Dockerについての本も買ったんだからとDockerToolboxを利用して（OSはWindows7です）Docker導入だ、と試してみてもクジラが出てこず、最終的に父のパソコンを借りて（Windows10Home）何とか再構築できました（父のパソコンではXAMPPを使用。Homesteadはやっぱりできず、DockerToolboxは試していません）
GitHubもプッシュできなくなる等（一番最後に解決しました。別のファイルからプッシュしたからです）、システムに苦戦することが多く、Laravelでコーディングができるということだけで嬉しかった記憶があります。
長くなりましたがこちらがその成果です。
sonsyu0103/keiziban_laravel
前回の反省からセキュリティに不安があり、公開はしていません。
再構築にあたって、
plumsa『絶対に挫折させないアプリ開発 はじめてのLaravel』
掌田 津耶乃『PHPフレームワーク Laravel入門』
の２冊とReadDoubleという公式リファレンスサイトを日本語訳しているサイトを活用すれば掲示板の部分は出来上がりました。
私は 竹澤 有貴 栗生 和明 新原 雅司 大村 創太郎『PHPフレームワーク Laravel Webアプリケーション開発 バージョン5.5 LTS対応』も買いましたが、難しすぎて必須ではないという次第です。参考になるところもデータベース関連はじめあります。
基本はsonsyu0103/keizibanを踏襲しつつ、
画像投稿機能はSTEP2-7. 画像をアップロードしてみよう
ダウンロード機能はLaravelでCSVダウンロード機能を実装する
お問い合わせ機能はLaravel 5.7 で問い合わせフォームを作るとLaravelでお問い合わせフォームを作ってみる
と、参考にさせていただきました。
またLaravelインストール後の初期設定と入門/簡単にMVCでHelloWorld＆データ受け渡しを行うでエラーメッセージの日本語化をしています。
ということで反省点について書いてみたいと思います。
設計について
掲示板を作るにあって、「会員制にするし、利用するのは多くて数十人ぐらいだろう」と想定して作りました。
なので削除も要望があったらadminで対応すればいいと思っていました。
再構築をするにあたって、何万人もの人が利用したらいちいち手動で全部対応するのか、と設計の甘さを痛感しました。
また、レスの仕様にも悩まされました。
$resus=Resu::where('sure_id',$request->code)->orderBy('Created_at')->get();
スレッドのIDが一致するもののみ指定したものですが、これによってデータベースにそのスレッドにおけるIDを表示することができていません。
「このレスはこのスレッドの何番目ですよ」とデータベースに登録できていません。
@foreach($resus as $resu)
{{ $loop->iteration }}:{{ $resu->name }}:{{ $resu->created_at }}<br>
{!! nl2br(e($resu->mess)) !!}<br><br>
@if($resu->image)
<img src="{{ asset('storage/images/'.$resu->image) }}"><br><br>
@endif
@endforeach
view上では$loopを使うことで数字表示できていますが、ページネーション機能を使うと、２ページの最初のレスでも１から始まります。
検索機能があっても、スレッドのどこにあるのかはわかりません。
resusテーブルにresu_idのカラムを作っても、どうすればそのスレッドにおけるIDを保存できるのか考えてみても
if($resus==null){
$resuId=1
}else{
$count=Resu::where('sure_id',$request->code)->count();
$resuId=1+$count;
}
違うとは思いますが、考え方の方向性としてはこうなるのでしょうか（今考えました）
他にも設計の甘さがあるとは思いますが、個人的に大きく痛感したのは利用者の人数とレスの仕様の２つでした。
rana_kualuさんのコメントの取り組み
Laravelを導入したことにより、rana_kualuさんのご指摘がどれほど改善されたのかを述べます。
残念ながら私には採点する力がないので、、いきなり全て完全にクリアできたというわけではなく、まだできていないことがあるだろうなとみています。
・SET NAMESは禁止。DSNでcharsetを使う。
SET NAMESは今回使っておりません。強いて言うなればコマンドプロントでデータベース作成時にDEFAULT CHARACTER SET utf8と使ったことでしょうか。
・SQLの文字列組み立ては禁止。プレースホルダを使う。login/member_add_done他数カ所にSQLインジェクション脆弱性がある。
$sure=new Sure;
$sure->sure_name=$request->sure_name;
$sure->user_id=\Auth::user()->id;
$sure->save();
調べたところLaravelはバインディング行っているそうなのでこのような記述でよいと思います。SQLインジェクション対策もwhereRaw()を使っていないので問題ないかと。
・login/member_edit_done、UPDATEのキーに$_SESSION['id']ではなく$_POST['id']を使っているので他人のデータを書き換えられる。
$user=User::find(\Auth::User()->id);
全て\Auth::User()->idを使いました。$_SESSION['id']のLaravelバージョンということで。
・downform/form_check にXSS脆弱性がある。$postを使わず$_POSTを使っている。たぶん他にもある。
@foreach($sures as $sure)
<a href="resus/{{ $sure->id }}">{{ $sure->id }}:{{ $sure->sure_name }}:{{ $sure->created_at }}</a><br><br>
@endforeach
{{ $sures->links() }}
{{}}を使えばHTMLエスケープされるそうです。おそらくLaravelで再構築したため問題は解決できたかと。
・downform/form_doneにおそらくメールヘッダインジェクション脆弱性がある(確認してない)
メールアドレスの登録はphp artisan make:authを使っています。しかし、
$this->validate($request,[
'u_name'=>'required|max:100',
'email'=>'required|email',
'pass'=>'confirmed'
私が作成したユーザー情報の更新において、メールヘッダインクジェクション脆弱性があるかもしれません。
・main/keizi_resuでmove_uploaded_fileのアップ先ファイル名にユーザ入力値を使っている。サーバ側で命名すべき。
if($request->file('image')->isValid())
{
$filename=$request->image->store('public/images');
$resu->image=basename($filename);
}
store()を使っているのでサーバー側が一意の名前をつけてくれます。素晴らしいです。
・アップロードされたファイルを検証せずに公開ディレクトリに上げているので死ぬ。
$this->validate($request,[
'name'=>'required|max:100',
'mess'=>'required|max:1000',
'image'=>'image|max:1024'
少なくともファイルの種類は検証できました。mimesとimageの違いがよくわかっていません。
・パスワードはハッシュ化する。
$hash=password_hash($request->pass,PASSWORD_BCRYPT);
$user->password=$hash;
この通りしました。
・sanitize関数はNG。
時々エラーで見かけたのでおそらくLaravelがやってくれているので使っていません。
・login/member_add_checkおよびdownform/form_checkには入力値検証があるがlogin/member_add_doneとdownform/form_doneにないので、いきなり完了画面にデータを送りつけて出鱈目なデータを登録できる
$this->validate($request,[
'keyword'=>'required|integer',
]);
今回はバリデーションしかしていませんが、どうなんでしょう。今回もまたいきなり完了画面にデータを送りつけることができるのでしょうか。
・login/member_dispほか、isset($_SESSION['login'])はtrue/falseなのでわざわざfalseと比較する必要はない。
//スレ一覧
Route::get('/sures','SureController@indexSure')
->middleware('auth');
そもそも今回は->middleware('auth')で無理やりログイン状態にしています。
・downform/download_done、日付の範囲指定を文字列でやっているのでインデックスが効かない。BETWEENとかを使うといい。
$this->validate($request,[
'keyword'=>'required|integer',
]);
$keyword=$request->keyword;
$results=Resu::where('sure_id',$request->keyword)
->orderBy('Created_at')
->get();
ダウンロードの条件を日付の範囲指定ではなく、スレッドのIDに変更しました。
・全体的にCSRFを受ける。
<h2>スレッド作成</h2>
<p>新しいスレッドを作るときはこちらでどうぞ</p>
@include('errors')
<form method="POST" action="{{ url('sure') }}">
{{ csrf_field() }}
<p>新しく作るスレッドのタイトル</p>
<input type="text" name="sure_name">
<input type="submit" value="作成">
</form>
{{ csrf_field() }}を使うことで回避できたと思います。
・投稿内容や名前に"があったら出力したCSVが崩れる。
Laravelに任せてしまっています。
・CSV名が固定なので、A氏がCSV出力→B氏がCSV出力→A氏がCSVダウンロードした場合にBのCSVが落ちてくる。
$response->headers->set('Content-Type','application/octet-stream');
$response->headers->set('Content-Disposition','attachment; filename="download.csv"');
この記述で名前が変更できるそうです。
・SQL結果セットの列は番号ではなくカラム名を使った方がいい。
@foreach($sures as $sure)
<a href="resus/{{ $sure->id }}">{{ $sure->id }}:{{ $sure->sure_name }}:{{ $sure->created_at }}</a><br><br>
@endforeach
この通り、カラム名を使いました。
ご指摘いただいたことは以上で終わりです。
改めてrana_kualuさん、ありがとうございました。
私も初心者の方にコードレビューできるようになりたいと心より思いました。
まとめ
Laravelに手を出しましたが、まだまだOAuthをはじめとする様々な機能があることでしょう。
少しずつできるようになりたいですし、今回は焦っていたのでフレームワークについて今一度
小川雄大, 柄沢 聡太郎, 橋口 誠『パーフェクトPHP』のフレームーワークを作るなどで学びたいとも思います。
そして、Webエンジニアに転職して新しいパソコンを買って、Dockerとか試したいですね。
ここまでお読みいただきありがとうございました。
余談
前回と併せた活用した本を積み上げてみました。総重力６，９キロです。
隣のガタカは励まされた素晴らしい映画です。サイズ比較に用いていますが。
「この人の下で働きたい」と思ってJavaとPHPを学び始めました。
知らなければSES企業に入社してCCNAとか資格を取っていたんだろうと思います。