Page 1 of 1

Query error using variables (I think)

Posted: Thu May 03, 2007 6:51 am
by mcccy005
Basically I have an input form, and the form validates correctly when the submit button is pressed and succesfully redirects the user to the next page. The problem is the data isn't actually going into the database but there's no error being displayed anywhere.
The code I am using works fine on another page except the only difference is the query so I'm thinking that must be the problem so can anyone see any problems with this query??
(I'm THINKING the error might be with the way I implement the $region_id and $secure_parking variables which are defined higher up in the php file that this query is in, but I've tried different variations and can't get it to work at all.)
Either help with the query would be good, or otherwise some way to test why data isn't going into the database but no error is being displayed would be great. Thanks in advance.

Code: Select all

query = "INSERT into marinas (marina_name, abn, region_id, contact_first_name, contact_last_name, street_address,
		     suburb, state, postcode, postal_address, postal_state, postal_postcode, telephone_work, telephone_fax, 
                                     telephone_mobile, telephone_emergency, email, secure_parking) 
		     VALUES ('{$_POST["marina_name"]}',
			'{$_POST["abn"]}',
			'$region_id',
			'{$_POST["contact_first_name"]}',
			'{$_POST["contact_last_name"]}',
			'{$_POST["street_address"]}',
			'{$_POST["suburb"]}',
			'{$_POST["state"]}',
			'{$_POST["postcode"]}',
			'{$_POST["postal_address"]}',
			'{$_POST["postal_state"]}',
			'{$_POST["postal_postcode"]}',
			'{$_POST["telephone_work"]}',
			'{$_POST["telephone_fax"]}',
			'{$_POST["telephone_mobile"]}',
			'{$_POST["telephone_emergency"]}',
			'{$_POST["email"]}',
			'$secure_parking')";

Posted: Thu May 03, 2007 7:25 am
by jayshields
- No $ preceeding query variable
- No execution of query
- Use or die("Cannot execute query ".mysql_error()."<br />".$query); after query execution
- Don't put $_POST variables straight into queries.
- Don't use the quotes inside quotes without escaping them

Posted: Thu May 03, 2007 7:38 am
by mcccy005
With regard to the $ before query, that was my error copying the code over.
There is an execution of the query in another script (as it's sent to another file as I'm using a bunch of objects I created to create a series of related input forms and these all work fine for another page using the same objects);
WIll have a look into the die thingy you're talking about;
What's wrong with putting $_POSt straight into queries?? (Like I said it works fine with another page which is what I also use; and I see it as a waste of memory storing each $_POST variable into a separate variable just to throw it straight into a query. Seems like duplication to me, but maybe theres something I don't know about???;
and I have no idea what you mean by escaping a quote inside a quote sorry???

Many thanks again,

Posted: Thu May 03, 2007 7:46 am
by Chris Corbyn
mcccy005 wrote:With regard to the $ before query, that was my error copying the code over.
There is an execution of the query in another script (as it's sent to another file as I'm using a bunch of objects I created to create a series of related input forms and these all work fine for another page using the same objects);
WIll have a look into the die thingy you're talking about;
What's wrong with putting $_POSt straight into queries?? (Like I said it works fine with another page which is what I also use; and I see it as a waste of memory storing each $_POST variable into a separate variable just to throw it straight into a query. Seems like duplication to me, but maybe theres something I don't know about???;
and I have no idea what you mean by escaping a quote inside a quote sorry???

Many thanks again,
Various issues here. You're trying to literally place $_POST["foo"] into a double quoted string so the "foo" part is causing a syntax error in PHP itself. You need to concatenate, use single quotes for the 'foo' bit, or just lose the quotes entirely since you're in string context.

> What's wrong with putting $_POSt straight into queries??

Bad, very bad. Look up "SQL Injection attacks". By allowing user-submitted data into queries you're just going to run against MySQL you give an attacker the opportunity to run any query they like in your database! You also run he risk of getting syntax errors in the query itself if you have quotes, or invalid characters in there.

mysql_real_escape_string() helps keep you safe ;)

Posted: Thu May 03, 2007 7:48 am
by d3ad1ysp0rk
Just by looking at the syntax highlighted code, it's quite easy to see what's wrong.

change

Code: Select all

VALUES ('{$_POST["marina_name"]}',
to

Code: Select all

VALUES ('" . $_POST["marina_name"] . "',
and so forth.

But you really should change those post variables out, for security reasons.

Posted: Thu May 03, 2007 9:29 am
by staar2
i could write in one field 'drop table marinas;. And you should check data before sending it to database. Use at least mysql_escape_string(). :!:

Posted: Thu May 03, 2007 9:30 am
by staar2
//edit oops almost same post like others. Sorry didn't read them.

Posted: Thu May 03, 2007 9:33 am
by jayshields
I think dropping the table would be a minor worry, aslong as back ups are being made - what benefit would the attacker get from dropping a table anyway?

Posted: Fri May 04, 2007 7:14 am
by mcccy005
Ok; so here's the code from the file that DOES work:

Code: Select all

$db_connection=houseboat_db_connect( );
$region_info_query = "INSERT into regions (region_name, state)
		VALUES ('{$_POST["region_name"]}',
		'{$_POST["state"]}')";
However, when I now change it to the following:

Code: Select all

$db_connection=houseboat_db_connect( );

	$region_name=mysql_real_escape_string($_POST["region_name"], $db_connection); //Line 9
	$state=mysql_real_escape_string($_POST["state"]); //Line 10

	$region_info_query = "INSERT into regions (region_name, state)
			VALUES ('$region_name',
			'$state')";
I get the following errors:
mysql_real_escape_string() expects parameter 2 to be resource, object given in X:\XXX.php on line 9;
and
mysql_real_escape_string() [function.mysql-real-escape-string]: Access denied for user 'ODBC'@'localhost' (using password: NO)
as well as mysql_real_escape_string() [function.mysql-real-escape-string]: A link to the server could not be established in X:\XXX.php, both on line 10. (I am aware one line is using the above set $db_connection, and the other is not (ie. it's using the last set connection which should be $db_connection).

This is how I set $db_connection in a globally accessible folder:

Code: Select all

function houseboat_db_connect( )
{
	$host_name = 'localhost'; //host name to connect to mysql server
	$username = 'root'; //username to connect to mysql server
	$password = 'XXXX'; //password to connect to mysql server
	$database_name = 'houseboat_db'; //name of database to connect to in mysql
	
	/*Connects to houseboat database on mysql server using the following
	arguments: (host_name, mysqli_username, mysql_password, mysql_database_name)*/
	$db_connection = new mysqli($host_name, $username, $password, $database_name);
	

	/*Returns error if unable to connect to the above database*/
	if(mysqli_connect_errno( ))
	{
		new error("Unable to connect to database. Error number: ".mysqli_connect_error());
		exit;
	}
			
	/*Returns the object which stores the database connection*/
	return $db_connection;
}
(And yep I know I shouldn't be using the root user, but its purely for testing purposes at this stage).

This function works fine in every other file I access it from.

Posted: Fri May 04, 2007 11:15 am
by RobertGonzalez
You are connecting with mysqli, so you need to use the mysqli escape function mysqli_real_escape_string().

Posted: Fri May 04, 2007 8:15 pm
by mcccy005
Awesome; thanks. That seems to work.
I just wanted to verify that the following code sql injection safe as well as ok in regard to the quotes etc. mentioned earlier in the post?? (It works fine):

Code: Select all

$region_info_query = "INSERT into regions (region_name, state)
		     VALUES ('".$db_connection->real_escape_string($_POST['region_name'])."', '".
		   	   $db_connection->real_escape_string($_POST['state'])."')";
Many thanks again in advance.

Posted: Fri May 04, 2007 11:17 pm
by RobertGonzalez
It is safer. But do you really want to take something directly from a user input and put into your database without knowing what it is? Seriously... what if you are execting a string and the user supplies a number. What if you are expecting only alpha numeric values and the user supplies special characters? There should be something in place to catch that.

Posted: Sun May 06, 2007 8:45 pm
by mcccy005
Yeah I do actually do that.

My code is structured as follows:
(from the top to the bottom)

* input_set (contains an array of input_pages and other stuff)
* input_page (contains an array of input_fields), extends input_set
* input_field (contains a data-type to be stored in this field (eg. numeric, date, money, address, email, ABN, password etc.), extends input_page
* text_field, password_field, radio_field, etc., extends input_field

When each page first loads, I call a validate( ) function which checks whether the field (ie. $_POST['field_name'] has to have data in it; and if the field does have data in it (regardless of whether it has to or not), the field will still have to have a data_type associated with it (eg. telephone number, email etc.), I then validate that it is in fact the appropriate data_type. If it is not, then the invalid fields will be highlighted and no data will be put into the database.