Thanks. At this time, PHP and MySQL is about all I have thought about. Once I get a couple of projects completed, I will take a closer look at PDO.
Aside from sanitizing input, etc., this is a beginner’s mistake. I suggest this as an alternative:
if( ( $result = mysql_query(…) ) && mysql_num_rows( $result ) )
do this with the result set;
else
use mysql_error() to figure out what went wrong;
Now you’re only invoking mysql_num_rows() on a valid result set. Doing it your way, you’re assuming that the select statement actually returned a valid result set which obviously isn’t the case.
Mmmh not quite.
If the query returns a valid result of 0 rows, you will trigger mysql_error() and get… nothing.
Just because a query returns nothing, doesnt mean it threw an error.
I think I will have other uses for the sanitizeFromPost function. I moved this code:
function sanitizeFromPost($postName){
if (isset($_POST[$postName])) {
$str=(
get_magic_quotes_gpc() ?
stripslashes($_POST[$postName]) :
$_POST[$postName]
);
return (
function_exists('mysql_real_escape_string') ?
mysql_real_escape_string($str) :
addslashes($str)
);
} else return '';
}
I now have it in a file named sanitize_from_post.inc.php.
I tried using an include
include $_SERVER[‘DOCUMENT_ROOT’] . ‘/include/sanitize_from_post.inc.php’;
That did not work, and gave me an error on a line that reads :
$userid=sanitizeFromPost(‘userid’);
The error was that sanitizeFromPost is not a valid function. I also tried using include_once, but that did not do any better.
Did I fail to copy something to the include file?
Try echoing $_SERVER[‘DOCUMENT_ROOT’] to make sure your server isnt blocking this value. if it is, try just “include(‘include/sanitize_from_post.inc.php’)” as this will cause it to use the relative pathing, rather than absolute.
I have been using includes to handle my repeating parts of pages, and use the document root.
I did run the echo and it displayed the document root properly.
Do I need to have <?php at the start of the sanitize_from_post.inc.php file? It is included inside the php of the page I am working on.
Yes. It must have PHP tags to be interpreted as PHP.
The day is just beginning, and I have already learned something new.
I do not understand what you and Dorsey are talking about on my userid check. Is what I have good, or do I need to change something. The current code is:
if (mysql_num_rows(mysql_query("SELECT user_id FROM member_tbl WHERE user_id = '$userid'"))) {
$msg .= 'Userid already assigned. Please select another userid.<br>';
}
Thank you.
You’re not sanitizing $userid;
Consider this. What if i put in for my userid the string
“'; DROP TABLE member_tbl; SELECT ‘1’='1”
Now your query reads:
SELECT user_id FROM member_tbl WHERE user_id = ‘’; DROP TABLE member_tbl; SELECT ‘1’=‘1’
Yay, i deleted your members.
[FPHP]mysql_real_escape_string[/FPHP] is highly recommended for preventing this primitive form of attack.
The mysql_error bit is something that theoretically should never come up once you’ve established the sanity of your query. It’s essentially the try-catch theory for querying.
if(!$result = mysql_query($sql)) {
//There was an error in our query and it returned FALSE.
echo mysql_error();
} elseif(mysql_num_rows($result) == 0) {
//The query executed successfully (it returned a Result Resource), but had no results to return.
} else {
//The query both executed successfully, and returned at least 1 row of data.
}
I think I understand what you are saying, but doesn’t the previous code sanitize $userid?
$userid=sanitizeFromPost('userid');
if (strlen($userid)<5) {
$msg .= 'User ID should be 5 or more than 5 char length.<br>';
}
if (mysql_num_rows(mysql_query("SELECT user_id FROM member_tbl WHERE user_id = '$userid'"))) {
$msg .= 'Userid already assigned. Please select another userid.<br>';
}
I will make modifications later this evening and post the new code.
mysql_real_escape_string() should not be used to sanitize numerical values, it will do nothing to prevent SQL injection when special hex values are used.
Numerical data should be casted as the expected type, so for example a user id:
$userId = (int)$_POST[‘userid’];
// $userId is now an integer and safe to put in SQL.
I may be missing something, but I don’t think I want to cast this as int.
Maybe I should change and use something other than userid. This is, in this case, really a user name. In this case, casting your user id of the128guy would not let me store the entire ID.
StarLion,
The first line of code in your example reads:
if(!$result = mysql_query($sql)) {
Should I replace the ($sql) with something like:
(“SELECT user_id FROM member_tbl WHERE user_id = ‘$userid’”)
Personally i keep my query string seperate as a variable. It makes it easier to check it (echo $sql), and it’s sort of a half-step towards prepared statements.
as the182guy said, sanitize numerics by casting them, but your query structure indicates that you arnt using a numeric value, you’re using a string. (Numerical values dont get quoted in SQL.), which is why i suggested escape_string. And no, it’s not a solves-all, but it’s at least sanitization against the most basic attacks.
This is the code I currently have and it give me an error:
$userid=sanitizeFromPost('userid');
if (strlen($userid)<5) {
$msg .= 'User ID should be 5 or more than 5 char length.<br>';
}
if (mysql_num_rows(mysql_query("SELECT user_id FROM member_tbl WHERE user_id = '$userid'"))) {
$msg .= 'Userid already assigned. Please select another userid.<br>';
}
if (!$result = mysql_query($sql)) {
//There was an error in our query and it returned FALSE.
echo mysql_error();
} elseif(mysql_num_rows($result) == 0) {
//The query executed successfully (it returned a Result Resource), but had no results to return.
} else {
//The query both executed successfully, and returned at least 1 row of data.
}
The error is:
Notice: Undefined variable: sql inmember_info_ck.php on line 55
Line 55 is: if (!$result = mysql_query($sql)) {
So do I need to put in a line that reads:
$sql = (mysql_num_rows(mysql_query(“SELECT user_id FROM member_tbl WHERE user_id = ‘$userid’”)
And then change my query to be:
if ($sql) {
$sql = “SELECT user_id FROM member_tbl WHERE user_id = ‘$userid’”;
StarLion,
Maybe I am starting to understand. Here is what I now have:
$sql = "SELECT user_id FROM member_tbl WHERE user_id = '$userid'";
if (!$result = mysql_query($sql)) {
//There was an error in our query and it returned FALSE.
echo mysql_error();
} elseif(mysql_num_rows($result) == 0) {
//The query executed successfully (it returned a Result Resource), but had no results to return.
} else { $msg .= 'Userid already assigned. Please select another userid.<br>';
//The query both executed successfully, and returned at least 1 row of data.
}
Do I have this close to being correct?
Yes; though if you’re not going to do anything in the num_rows = 0 case, change if around so the elseif reads not-equal 0, and put your message in there, and then leave off the else clause.
Thanks. I think I get it now and will test it when I get back home in about 8 hours.