Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -1 down vote favorite

Could you have a quick look over my code to see if its safe from SQL injection etc.. and suggest any amendments?

<html>
<head><title>Retrieve Your Login Code</title>
</head>
<body bgcolor=#ffffff>

<?php

echo "<h2>Search Results:</h2><p>";

//If they did not enter a search term we give them an error
if ($find == "")
{
echo "<p>You forgot to enter a search term!!!";
exit;
}

// Otherwise we connect to our Database
mysql_connect("server.com", "ipn", "pw!") or     die(mysql_error());
mysql_select_db("ipn") or die(mysql_error());

// We perform a bit of filtering
$find = strtoupper($find);
$find = strip_tags($find);
$find = trim ($find);

//Now we search for our search term, in the field the user specified
$iname = mysql_query("SELECT * FROM ibn_table WHERE itransaction_id = '$find'");

//And we display the results
while($result = mysql_fetch_array( $iname ))
{
echo "<b>Name: </b>";
echo $result['iname'];
echo " "; 
echo "<br>";
echo "<b>E-mail: </b>";
echo $result['iemail'];
echo "<br>";
echo "<b>Transaction Date: </b>";
echo $result['itransaction_date'];
vecho "<br>";
echo "<b>Payment Amount: &pound</b>";
echo $result['ipayerid'];
echo "<br>";
//And we remind them what they searched for
echo "<b>Search Term </b>(Transaction ID): </b> " .$find;
//}
echo "<br>";
echo "<br>";
echo "<b>Login Code: </b>";
echo $result['ipaymentstatus'];
echo "<br>";
}

//This counts the number or results - and if there wasn't any it gives them a little     message explaining that
$anymatches=mysql_num_rows($iname);
if ($anymatches == 0)
{
echo "Sorry, but we can not find an entry to match your search, please make sure the     correct details have been entered...<br><br>";
}


?> 


</body>
</html>
share|improve this question
1  
mysql_connect (and related) are depreciated in PHP v5.5 php.net/manual/en/function.mysql-connect.php You should switch to use mysqli or more preferably a PDO system. This alone will greatly help against SQL injection. – MECU Mar 28 at 1:17
1  
Could you not take the time to at least properly indent your code before asking others to proofread it? – meagar Apr 25 at 13:59

2 Answers

up vote 4 down vote

Really this won't be safe unless you use parameterized queries, which allow the database to combine the query in the form that is safest.

See the PHP docs for mysqli.prepare and this helpful article

Also, just for your own sanity, I'd try separating your PHP from your HTML. You might want to look into a templating system as well, like Smarty, just to make your code easier to manage in the long run.

share|improve this answer
up vote -1 down vote 
// We perform a bit of filtering
$find = strtoupper($find);
$find = strip_tags($find);
$find = trim ($find);
$find = mysql_real_escape_string($find);

one more step and you are safer. I recommend using PDO or mysqli to prevent sql injection completely.

If you are not learning PHP and mySQL, but creating an application, I recommend using framework like symfony or lavarel instead :)

share|improve this answer
Thanks for that, ived added in the last string. How do i use mysqli insted? im very very new to all this. – Marc Jones Mar 26 at 13:14
So you are learning about PHP and mySQL. I suggest you searching in Stackoverflow to find a good way to starting learning PHP and MYSQL :) You can start from here stackoverflow.com/questions/1610543/… – Vdt Mar 26 at 13:19
ok, so with that string added, it should be safe enough? – Marc Jones Mar 26 at 14:01
my code assumes that register_globals is turned on, how do i change it to make it work with register_globals off?? – Marc Jones Mar 26 at 14:34
yes it is ;) @MarcJones put another question on stackoverflow :) – Vdt Mar 26 at 15:50
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.