-1
\$\begingroup\$

Could you have a quick look over my code to see if it's 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>
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – MECU Mar 28 '13 at 1:17
  • 2
    \$\begingroup\$ Could you not take the time to at least properly indent your code before asking others to proofread it? \$\endgroup\$ – meagar Apr 25 '13 at 13:59
6
\$\begingroup\$

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.

\$\endgroup\$
-1
\$\begingroup\$
// 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 :)

\$\endgroup\$
  • \$\begingroup\$ Thanks for that, ived added in the last string. How do i use mysqli insted? im very very new to all this. \$\endgroup\$ – Marc Jones Mar 26 '13 at 13:14
  • \$\begingroup\$ 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/… \$\endgroup\$ – nXqd Mar 26 '13 at 13:19
  • \$\begingroup\$ ok, so with that string added, it should be safe enough? \$\endgroup\$ – Marc Jones Mar 26 '13 at 14:01
  • \$\begingroup\$ my code assumes that register_globals is turned on, how do i change it to make it work with register_globals off?? \$\endgroup\$ – Marc Jones Mar 26 '13 at 14:34
  • \$\begingroup\$ yes it is ;) @MarcJones put another question on stackoverflow :) \$\endgroup\$ – nXqd Mar 26 '13 at 15:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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