SQL syntax error while using MySQL and PHP

Question

I am trying to make a thing, but I have hit a problem. I have tried all I know, but I am new to MySQL, so I have hit a dead end.

This code:

<?php
    require('cfg.php');
    mysql_connect($server, $user, $pass) or die(mysql_error());
    mysql_select_db($database) or die(mysql_error());

    if (isset($_GET['name'])){
        $name = $_GET['name'];
    }
    else
        if (isset($_POST['submit'])){
            $name = $_POST['name'];
            $name1 = $_POST['name1'];
            $name2 = $_POST['name2'];
            $name3 = $_POST['name3'];
            mysql_query("INSERT INTO data (name, name1, name2, name3) VALUES($name, $name1, $name2, $name3 ) ") or die(mysql_error());
            echo ("Data entered successfully!");
        }
?>

<html>
    <head>
        <title>Random giffgaff simmer</title>
    </head>
    <body>
        <form action="" method="post">
            <p>Your Username: <input type="text" name="name"></p>
            <p>Username 1: <input type="text" name="name1"></p>
            <p>Username 2: <input type="text" name="name2"></p>
            <p>Username 3: <input type="text" name="name3"></p>
            <p>Username 4: <input type="text" name="name4"></p>
            <p>Username 5: <input type="text" name="name5"></p>
            <p>Username 6: <input type="text" name="name6"></p>
            <p><input type="submit" name="submit" value="Submit"></p>
        </form>
    </body>
</html>

Brings this error:

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ' )' at line 1

Now, that would tell me that this SQL code has a syntax error:

INSERT INTO data (name, name1, name2, name3) VALUES($name, $name1, $name2, $name3 )

But I don't think I can see one?

This is hacker friendly. In other words, it is extremely susceptible to SQL injection. — Jeremy Harris, Apr 01 '12 at 09:38
Well, I have managed to get rid of that error, by filling in ALL the fields (which is annoying), but now i get this error: `Unknown column pizza in fieldlist` with pizza being the value of $name? — Liam W, Apr 01 '12 at 09:38
Your best bet is to learn how to connect to a MySQL database using [PDO](http://www.php.net/manual/en/book.pdo.php) and then use prepared statements with the `prepare()` and `bind_value()` or `bind_param()` functions. — Jeremy Harris, Apr 01 '12 at 09:41
possible duplicate of [How to include a PHP variable inside a mysql insert statement](http://stackoverflow.com/questions/7537377/how-to-include-a-php-variable-inside-a-mysql-insert-statement) — Your Common Sense, Apr 01 '12 at 10:22

Starx · Accepted Answer · 2012-04-01T10:18:19.000

4

You haven't quoted your query. You should quote every field like this

INSERT INTO data (name, name1, name2, name3) VALUES('$name', '$name1', '$name2', '$name3' )

As an tribute to TheCommonSense, I am providing a mysqli version using correct prepared statement for data safety

$db = new mysqli(...);
$stmt = $db -> prepare("INSERT INTO data (name, name1, name2, name3) VALUES(?, ?, ?, ?)");
$stmt -> bind_param("ssss", $name, $name1, $name2, $name3);
$stmt -> execute();
$db -> close()

edited Apr 01 '12 at 10:18

answered Apr 01 '12 at 09:42

Starx

77,474
47
185
261

(Warning: about to have a moment) Yeah, it's also the same thing 3 people posted 5 minutes before. – Corbin Apr 01 '12 at 09:46
@Corbin, Chill man, I am not the one that downvote any of your guys, I voted for your comment too. – Starx Apr 01 '12 at 09:47
@Corbin, Escaping is necessary, but to assume that every field needs to be escaped every time is indeed a superstition and TheCommonSense is right about this fact. – Starx Apr 01 '12 at 09:49
I didn't think you did. And really I should not have posted that comment. I've just noticed lately that people post duplicate answers all the time. I'm probably just bitter because I had someone copy/paste one of my answer earlier. (In short: sorry -- the random down votes coupled with the 5 same answers got to me lol) – Corbin Apr 01 '12 at 09:49
Of course not every field have to be escaped, but **quoted string** is the **only** field that have to (and should) be quoted. -1 then for omitting escaping. – Your Common Sense Apr 01 '12 at 09:53
@YourCommonSense, Love you man, I am learning so much from you today. – Starx Apr 01 '12 at 09:55
@YourCommonSense, Added a prepared statement for the escaping need. Please Review :) – Starx Apr 01 '12 at 10:02
`*sigh*`. Escaping is not a synonym for the "making data safe". Escaping stands for "making SQL string syntactically correct". So, if you have no strings in your query, you can't say you are using any escaping. – Your Common Sense Apr 01 '12 at 10:12
@YourCommonSense, What should I do then? – Starx Apr 01 '12 at 10:13
At least don't call it "escaping". "Added a prepared statement for the safety." it should be. As for the escaping, it shouldn't be mentioned alone but only with quoting. The same goes for the quotes - if you are using htem in the query - you have to always escape the content of the quoted string. And you should escape nothing else. – Your Common Sense Apr 01 '12 at 10:16
@YourCommonSense, Ok updated. So, the names on this case, should be escaped right? – Starx Apr 01 '12 at 10:19
which "this" case? there are two cases in your answer. – Your Common Sense Apr 01 '12 at 10:22
@YourCommonSense, On the OP's case, where he is only inserting names. – Starx Apr 01 '12 at 10:24
Every quoted string's content have to be escaped. The only escaping's purpose is to escape delimiting quotes. So - yes, he have to escape his names. I made a typo in my first comment here, just noticed that. It should read "quoted string is the only field that have to (and should) be escaped" – Your Common Sense Apr 01 '12 at 10:27

Corbin · Answer 2 · 2012-04-01T09:41:35.467

2

Strings must be quoted and escaped.

$name = (isset($_POST['name'])) ? $_POST['name'] : '';
$name = mysql_real_escape_string($name);
$query = "INSERT INTO blah (name, ...) VALUES ('{$name}', ...)";

You're going to want to look into SQL injection by the way. Also, before you get too far down the road, you should really go ahead and abandon mysql_* in favour of PDO. PDO offers support for multiple drivers* (MySQL/SQLite/MSSQL/etc), and can do prepared statements (cleaner/sort of safer than mysql_real_escape_string).

* this does not make SQL magically portable, but it does help.

edited Apr 01 '12 at 09:41

answered Apr 01 '12 at 09:38

Corbin

33,060
6
68
78

@YourCommonSense don't know if it was a typo, but the way you've worded it is definitely clearer :). – Corbin Apr 01 '12 at 09:41
That is your problem. It IS a mistake but you have no idea it is. You are saying "cleaner/sort of safer than mysql_real_escape_string" as though mysql_real_escape_string has anything to do with security while it doesn't. – Your Common Sense Apr 01 '12 at 09:49
Do you mean that I said "escaping data" instead of strings? I actually meant strings, but technically you're correct. It would be wrong to always escape data. In fact, escaping a number doesn't even make sense in a strict way. – Corbin Apr 01 '12 at 09:53
I mean you treat prepared statements and escaping as equal actions, as it seems from your wording. While it's different things. Prepared statements is not cleaner/sort of safer way of escaping. That's the point. And it is not hairsplitting. This topic is a field of massive delusion of PHP folks, so, one have to be extremely precise, allowing no ambiguousness. – Your Common Sense Apr 01 '12 at 10:06
@YourCommonSense, But, isn't the `$name` field necessary to be escaped. – Starx Apr 01 '12 at 10:15
@YourCommonSense That is true. Though they do have an overlap, their method of operation and their capabilities are very different. I've always seen these questions as a simple "show the OP how to prevent SQL injection and move on", but when criticized, it becomes apparent that it takes quite a bit of careful thought to avoid ambiguity and be precise and correct. By the way, have you consider posting an answer? It would likely be much more coherent to the OP than the feedback across various answers. – Corbin Apr 01 '12 at 10:15
@YourCommonSense, Yeah post an answer. Let us know the difference. – Starx Apr 01 '12 at 10:16
I just choose to correct your answer instead of writing my own. anyway, I had to link the other answer, not writing a duplicate. – Your Common Sense Apr 01 '12 at 10:20

Michael · Answer 3 · 2012-04-01T10:39:02.547

1

I'm assuming that $name, $name1, etc. are strings? You should be enclosing them in single-quotes. Try:

"INSERT INTO `data` (`name`, `name1`, `name2`, `name3`) VALUES ('$name', '$name1', '$name2', '$name3')"

Remember also to escape ~~all input~~ ~~user-provided strings~~ values that can potentially act as SQL injections (see here: http://php.net/manual/en/security.database.sql-injection.php) with mysql_real_escape_string() before passing them into the query, or switch to the mysqli extension and use prepared statements (best option).

edited Apr 01 '12 at 10:39

answered Apr 01 '12 at 09:38

Michael

11,912
6
49
64

By telling the OP "to escape all input", you are asking them to implement a sort of the magic quotes feature which is defamed and deprecated. – Your Common Sense Apr 01 '12 at 09:43
So you'd prefer they entered the data as it is given by the user? – Michael Apr 01 '12 at 09:44
1

I prefer *sane* data processing, based on knowledge and security, not some gospel and superstition. – Your Common Sense Apr 01 '12 at 09:46
1

@YourCommonSense You're asking them to take what you say as "gospel" by telling them they're wrong without giving any decent feedback as to why. – Jonnix Apr 01 '12 at 09:52
My experience is telling me that most of them don't care of any feedback. So, it's just a waste of time. (And there is a lot info already written, for ones who cares). I am open to repeat the same explanation again for the 100's time though, if I were asked though. However, I don't take sarcastic comment as such a question. – Your Common Sense Apr 01 '12 at 09:55
2

Whether they read it or not isn't really the point, if you don't provide feedback to back up your claims, nobody will ever learn, so you're just adding the the cycle. – Jonnix Apr 01 '12 at 10:00
@YourCommonSense, I've edited my answer to hopefully be more accurate (and hence useful). – Michael Apr 01 '12 at 10:05
You made it worse. Are you *really* think that only "user-provided strings" have to be escaped? So, if you are getting from the other table or a text file - no need to escape quotes in it? – Your Common Sense Apr 01 '12 at 10:08

sujal · Answer 4 · 2012-04-01T09:52:18.670

1

mysql_query("INSERT INTO data (name, name1, name2, name3) VALUES('$name', '$name1', '$name2', '$name3') ") or die(mysql_error());

or

   mysql_query("INSERT INTO data (name, name1, name2, name3) VALUES('".$name."', '".$name1."', '".$name2."', '".$name3."') ") or die(mysql_error());

Try this

edited Apr 01 '12 at 09:52

answered Apr 01 '12 at 09:46

sujal

1,058
3
18
29

No need to concatenate the vars – Starx Apr 01 '12 at 09:50
Actually, the answer is better without the or part – Starx Apr 01 '12 at 09:53

SQL syntax error while using MySQL and PHP

4 Answers4