patternphpMinor
Prepared statements from security viewpoint
Viewed 0 times
statementspreparedsecurityfromviewpoint
Problem
I've decided to go with OOP style and prepared statements, and so far I like it a lot more than the procedural style. It's much more understandable in my opinion.
For this code review, I've just included my PHP code that interacts with my MySQL database. Everything is working how it should, but I'd like to know if you see anything that could be improved upon. Also, I'd like a security analysis/review of the code. Is my code safe from SQL injection since I'm using prepared statements? Is there anything else I should be doing?
I have this function in a folder/file above public_html, and just require it on the pages that need it.
This is the function I use to pull the question text, and answer integers from the database to display on the website.
This is the code that gets run after the user takes the math quiz, submits his answers, and I validate that the answers are all integers, etc.
``
For this code review, I've just included my PHP code that interacts with my MySQL database. Everything is working how it should, but I'd like to know if you see anything that could be improved upon. Also, I'd like a security analysis/review of the code. Is my code safe from SQL injection since I'm using prepared statements? Is there anything else I should be doing?
I have this function in a folder/file above public_html, and just require it on the pages that need it.
function connectToDatabase() {
$connect = new mysqli('localhost', 'myUserName123', 'myPassword123', 'myDatabaseName');
if ($connect->connect_error) {
exit;
}
return $connect;
}This is the function I use to pull the question text, and answer integers from the database to display on the website.
function getQuestionsAndAnswers($connect) {
$questionArray = array();
$answersArray = array();
$getStuff = $connect->prepare('SELECT `question`, `answer` FROM `Quizzes` WHERE `questionNumber`bind_param('i', $questnum);
$questnum = 21;
$getStuff->execute();
$getStuff->bind_result($singleQuestion, $singleAnswer);
while ($getStuff->fetch()) {
array_push($questionArray, $singleQuestion);
array_push($answersArray, $singleAnswer);
}
$getStuff->close();
$connect->close();
return array($questionArray, $answersArray);
}This is the code that gets run after the user takes the math quiz, submits his answers, and I validate that the answers are all integers, etc.
``
$connect = connectToDatabase(); // open database connection here
$stmt1 = $connect->prepare("SELECT MAX(id) FROM QuizAdministration");
$stmt1->execute();
$stmt1->bind_result($maxId);
$stmt1->fetch();
$stmt1->close();
$stmt2 = $connect->prepare("INSERT INTO PlayerAnswers (QuizAdministrationId`,Solution
This looks pretty good. I'm glad to see the improvement.
You inexplicably call
The main issue I see now is
The solution is to ask the database to generate the next ID. Every SQL database supports this in a slightly different way. For MySQL, you should declare the
To make this work for you, you'll need to insert a row into
You inexplicably call
$connect->bind_params() using variables that have not yet been set.The main issue I see now is
$QuizAdministrationid = $maxId + 1, which is prone to a race condition. If two users submit the form simultaneously, it's possible that both pages could see the same result from SELECT MAX(id) … and generate the same next $QuizAdministrationid.The solution is to ask the database to generate the next ID. Every SQL database supports this in a slightly different way. For MySQL, you should declare the
id column of QuizAdministration to be AUTO_INCREMENT. Then, leave id column unspecified when you do the INSERT. MySQL will automatically fill in the id column with the next available number. To find out what number MySQL picked for you, use $connect->insert_id.To make this work for you, you'll need to insert a row into
QuizAdministration first, then use the id of the new row to insert the PlayerAnswers.$stmt2 = $connect->prepare("INSERT INTO QuizAdministration (`quizNumber`, `cookie`, `ip`, `score`) VALUES (?, ?, ?, ?)");
$quizNumber = 1;
$playerCookie = session_id();
$playerIP = getUserIp();
$playersPercent = 90;
$stmt2->bind_param('issi', $quizNumber, $playerCookie, $playerIP, $playersPercent);
$stmt2->execute();
$stmt2->close();
$quizAdministrationid = $connect->insert_id;
$stmt3 = $connect->prepare("INSERT INTO PlayerAnswers (`QuizAdministrationId`, `questionNumber`, `playerAnswer`) VALUES (?, ?, ?)");
for ($yty = 1; $yty bind_param('iii', $QuizAdministrationid, $questnum, $questansw);
$stmt3->execute();
}
$stmt3->close();Code Snippets
$stmt2 = $connect->prepare("INSERT INTO QuizAdministration (`quizNumber`, `cookie`, `ip`, `score`) VALUES (?, ?, ?, ?)");
$quizNumber = 1;
$playerCookie = session_id();
$playerIP = getUserIp();
$playersPercent = 90;
$stmt2->bind_param('issi', $quizNumber, $playerCookie, $playerIP, $playersPercent);
$stmt2->execute();
$stmt2->close();
$quizAdministrationid = $connect->insert_id;
$stmt3 = $connect->prepare("INSERT INTO PlayerAnswers (`QuizAdministrationId`, `questionNumber`, `playerAnswer`) VALUES (?, ?, ?)");
for ($yty = 1; $yty <= 20; $yty += 1) {
$questnum = $yty;
$questansw = $_POST['qora'.$yty];
$stmt3->bind_param('iii', $QuizAdministrationid, $questnum, $questansw);
$stmt3->execute();
}
$stmt3->close();Context
StackExchange Code Review Q#43740, answer score: 8
Revisions (0)
No revisions yet.