0

In my webpage, I ask a user to fill out a form specifying their age, race, gender, and state. Upon submission, the data is submitted to the same page and the page will process it with the following code to submit it to a database (all database login info is faked in this example):

<?php
if($_COOKIE['infoGiven']==true){    
    $con=mysql_connect('localhost','asasdfasd','asdf','asdfasdf');
    if($_COOKIE['like']==true){
        $sql="INSERT INTO LIKE(state, age, gender, race)
        VALUES(\'".$_POST["state"]."\'".$_POST["age"]."\'".$_POST["gender"]."\'".$_POST["race"].")";
        $con->query($sql);
    }
    if($_COOKIE['like']!=true){
        $sql="INSERT INTO DISLIKE(state, age, gender, race)
        VALUES(\'".$_POST["state"]."\'".$_POST["age"]."\'".$_POST["gender"]."\'".$_POST["race"].")";
        $con->query($sql);
    }
}
?>

This should simply submit the user data to the database, but instead I receive a blank page with a "500" error. There is no code inside the . Keep in mind that, prior to form submission, the page renders properly.

user2182349
  • 9,569
  • 3
  • 29
  • 41
Damphair
  • 9
  • 6
  • You have an error. Add _ini_set('display_errors',1);_ to the top of the file. – user2182349 Dec 11 '17 at 03:43
  • Don't use `mysql_*` functions in new code, way too long deprecated. Also, this code is prone to SQL injections. – marekful Dec 11 '17 at 03:44
  • Along with what marekful said, mysqli is the better replacement. It'd also be good to look into parameterized statements and stored procedures. To avoid SQL injections, look into sanitizing input data. Going to add this into my answer, it's pretty important stuff! – HB- Dec 11 '17 at 03:47
  • 2
    **$con->query($sql);** isn't going to work. You would have to use **mysql_query** - which has been deprecated. – user2182349 Dec 11 '17 at 03:52
  • **WARNING**: If you're just learning PHP, please, do not use the [`mysql_query`](http://php.net/manual/en/function.mysql-query.php) interface. It’s so awful and dangerous that it was removed in PHP 7. A replacement like [PDO is not hard to learn](http://net.tutsplus.com/tutorials/php/why-you-should-be-using-phps-pdo-for-database-access/) and a guide like [PHP The Right Way](http://www.phptherightway.com/) explains best practices. Your user data is **not** [properly escaped](http://bobby-tables.com/php.html) and there are [SQL injection bugs](http://bobby-tables.com/) and can be exploited. – tadman Dec 11 '17 at 04:05
  • Your second `if` statement could be an `else`. From a relational database design perspective a single table that can express like or dislike is a lot better than two tables. Additionally, try not to call tables things that are [reserved keywords](https://dev.mysql.com/doc/refman/5.7/en/keywords.html). – tadman Dec 11 '17 at 04:07
  • this is worrying. Its 2017 and developers are still using `mysql_*` – Rotimi Dec 11 '17 at 04:13

1 Answers1

1

You're missing the commas in your insert statement; you also don't have to escape single quotes when you're utilizing double quotes.

As your code is right now, you're trying to insert one really concatenated string. 

$sql="INSERT INTO LIKE(state, age, gender, race)
        VALUES(\'".$_POST["state"]."\'".$_POST["age"]."\'".$_POST["gender"]."\'".$_POST["race"].")";

Should be:

$sql="INSERT INTO `LIKE`(state, age, gender, race)
        VALUES('".$_POST["state"]."', '".$_POST["age"]."', '".$_POST["gender"]."', '".$_POST["race"]."')";

But that's to illustrate the punctuation issues with the parameters: don't use that code itself, because the alternatives are more secure and less deprecated.

Better version using mysqli prepared statements:

$db = new mysqli("DBHOST","DBUSERNAME","DBPASS","DBNAME");
$stmt = $db->prepare("INSERT INTO `LIKE`(state, age, gender, race) VALUES (?,?,?,?)");

$stmt->bind_param('i', $VALUE)

^ bind parameters like this. The first parameter is the type of the second parameter (integer in this case, 's' for string, etcetera). $VALUE is the variable to bind. The number of parameters bound has to match the numbers of placeholders/question marks; this prepared statement expects four.

To bind multiple parameters in one line, you can use the format bind_param('sss', $string1, $string2, $string3), to pick an example. Then:

$stmt->execute();
$stmt->close();

I'd also check your connection object to make sure it's successfully connecting to the database with the actual login information. Along with that, don't forget to sanitize/validate your input before it goes into the database. This covers it well.

HB-
  • 635
  • 2
  • 8
  • 21
  • In this case, validation would take care of sanitization. Perhaps you could post an example. – user2182349 Dec 11 '17 at 03:50
  • 1
    This addresses some of the most superficial concerns in the question without dealing with the other extraordinarily serious problems. Although you do briefly mention things, demonstrating is far better than handing someone "working" code that is a huge liability. – tadman Dec 11 '17 at 04:05
  • Having 4 parameters for a statement that expects 4 parameters isn't superficial, but there's definitely a lot here that could cause serious problems. I'm updating my answer in a sec with mysqli, but I'll let OP research appropriate data sanitization/validation on their own. – HB- Dec 11 '17 at 04:09
  • Prepared statements is what you should recommend. – Jay Blanchard Dec 11 '17 at 04:15
  • Yep. Please let me know if I've left anything else uncovered. – HB- Dec 11 '17 at 04:25
  • I tried putting your code into my document, but it didn't work. Is there any extra information I could provide to help? – Damphair Dec 11 '17 at 07:12
  • Did you use the prepared statement part? Can I see a copy-and-paste of the actual code you put in, either in a fiddle or in an edited update? – HB- Dec 11 '17 at 09:13