プログラムコードの間違いがどこなのか分かりません

Question

素人でプログラムを楽しんでいる者です。プログラムコードの間違いがあると指摘されているのですが、私にはどこが間違っているのか分かりませんでした。お分かりになる方がおられましたらご指摘をお願いいたします。

if(filter_input(INPUT_POST,'●●●')){
$colom="●●●=:●●●,";
}
の文が沢山並んでいますが、SQL文に入るカラム名の数が決まっていないので、filter_inputの返り値がbool型なのでifを使い$colomに連結して最後にまとめてUPDATE SEI SET $colom の様に入れました。この部分が間違いなのでしょうか？それとも、自動コミットを使わずに$dbh->beginTransaction(); を2回使った部分が間違いなのでしょうか？UPDATEとDEIETEの二つのSQL文がありますが、これは1つのbeginTransaction();とcommit();の間に入れた方が良いのでしょうか？このプログラムは管理人しか触らないので、UPDATE中に誰かがDELETEするようなことは起きないので二つに分けても問題ないと思っているのですが。でもやはり一つにまとめるべきでしょうか？どこが間違っているのご指摘お願いいたします。

session_save_path('../sess');
session_start();
require '../function/main_function.php';


////////////////////////トークン/////////////////////////////
$token=token();
$_SESSION['token']=$token;

///////////////////個人データUPDATE////////////////////////////
if(filter_input(INPUT_POST,'no')){
$colom="NO=:NO,";
}
if(filter_input(INPUT_POST,'simei')){
$colom.="SIMEI=:SIMEI,";
}
if(filter_input(INPUT_POST,'id')){
$colom.="ID=:ID,";
}
if(filter_input(INPUT_POST,'password')){
$colom.="PASSWORD=:PASSWORD,";
}
if(filter_input(INPUT_POST,'address')){
$colom.="ADDRESS=:ADDRESS,";
}
if(filter_input(INPUT_POST,'mail')){
$colom.="MAIL=:MAIL,";
}
if(filter_input(INPUT_POST,'tel')){
$colom.="TEL=:TEL,";
}
if(filter_input(INPUT_POST,'fu')){
$colom.="FU=:FU,";
}
if(filter_input(INPUT_POST,'ky')){
$colom.="KY=:KY,";
}
$colom=substr($colom,0,-1);

try{
$dbh=DBconect();
$dbh->beginTransaction();    

$stmt=$dbh->prepare("UPDATE SEI SET $colom WHERE TIME=:TIME");

///////////////////////bindParam////////////////////////////////////
if(filter_input(INPUT_POST,'time')){
$stmt->bindParam(':TIME',$_POST['time'], PDO::PARAM_STR);
}
if(filter_input(INPUT_POST,'no')){
$stmt->bindParam(':NO',$_POST['no'], PDO::PARAM_STR);
}
if(filter_input(INPUT_POST,'simei')){
$stmt->bindParam(':SIMEI',$_POST['simei'], PDO::PARAM_STR);
}
if(filter_input(INPUT_POST,'id')){
$stmt->bindParam(':ID',$_POST['id'], PDO::PARAM_STR);
}
if(filter_input(INPUT_POST,'password')){
$stmt->bindParam(':PASSWORD',$_POST['password'], PDO::PARAM_STR);
}
if(filter_input(INPUT_POST,'address')){
$stmt->bindParam(':ADDRESS',$_POST['address'], PDO::PARAM_STR);
}
if(filter_input(INPUT_POST,'mail')){
$stmt->bindParam(':MAIL',$_POST['mail'], PDO::PARAM_STR);
}
if(filter_input(INPUT_POST,'tel')){
$stmt->bindParam(':TEL',$_POST['tel'], PDO::PARAM_STR);
}
if(filter_input(INPUT_POST,'fu')){
$stmt->bindParam(':FU',$_POST['fu'], PDO::PARAM_STR);
}
if(filter_input(INPUT_POST,'ky')){
$stmt->bindParam(':KY',$_POST['ky'], PDO::PARAM_STR);
}
/////////////////////////////////////////////////////////////////////////
$stmt->execute();

$dbh->commit();
$stmt = null;
$dbh = null;

}catch( Exception $e ){
$dbh->rollback();
}
/////////////////////////削除（kozindata.phpより）/////////////////////////////////////

if(filter_input(INPUT_GET,'sak')){

try{
$dbh=DBconect();
$dbh->beginTransaction();    

$stmt=$dbh->prepare("DELETE FROM SEITO WHERE TIME=:TIME");
$stmt->bindParam(':TIME',$_GET['sak'], PDO::PARAM_STR);       
$stmt->execute();

$dbh->commit();
$stmt = null;
$dbh = null;

}catch( Exception $e ){
$dbh->rollback();
}
}
header('location:./kozindata.php');

コード

Comments

[m.ts10806]

全体の流れを再現できるコードでないので全てを見ることはできませんし、コードレビューを他者に頼んでも、その指摘をした人の要件を満たせるわけではないですよ

[javahack]

指摘してくれる人がいるなら、ここで聞くよりその人にどこが間違っているか確認した方が早くないですか？

[amura]

気になったのは$colomに何もセットされない場合（GETの時も）が有ればでしょうか。

[m.ts10806]

no以外が通ったNotice出ますね。

Answer 1

どんな間違いが指摘されたか知らないので、これで解決するかは知りません。
その人と同じ観点で見れるわけでもないですからね。

ユーザー入力をそのまま突っ込んでるのが間違い($colom)
SQLインジェクションの脆弱性あり　←失礼きちんと読めていませんでした
$colomとあるが「カラム」のことであれば正しくはcolumnなので変数名は任意の文字列とは言えスペル間違い
※初期値の考慮や「すべてない場合」の考慮がなかったり、Whereでtimeにしているのがよくわからない。コードだけでは判断できない仕様部分も多い

filter_inputの返却値は正しくフィルタリングできれば入力値が取得できるので$_POSTからわざわざとる必要はなし。
filter_inputの返却値受け取ってからそれぞれ判断した方がいいし、オプションは駆使しましょう。

Exception捕捉してるのにロールバックしてるだけなのは学習や開発中はNG。どこかに出力すること。

あと、私がレビュワならインデント全くない時点で中身読まずに突っ返すかもしれません。

他にもありそうですけど、きりがないので、あとは自分でその指摘してくれた人にヒアリングしてください。

Comments

[ockeghem]

SQLインジェクションありますか? パッと見はないように思いますが。

[m.ts10806]

確かに言われてみれば…。
書き方変えます。

Answer 2

書いたコードが他人から「間違っている」と言われた場合に取るべき対処は

1. そう言った人に間違っている場所を指摘してもらう
2. テストして間違っている箇所を探す（あるいは全て正しい事を証明する）

の２通りありますが、１で間違っている箇所がわかったからといって、２が不要になることはありません。

結論「テストしようぜ」

---

追記
あなたが書いたコードが全てあなたの思う通りに動いているのであれば、「間違っている」と言った人が間違っています。

あなた自身が何がおかしいのかわかっていない（現状で自分の要求を満たしている）コードの潜在的なバグを他人に指摘させるのはただの**作業依頼（レビューの丸投げ）**になりますので、ここで聞くのはどうかと思います。