What's wrong with this query?

Question

Someone help me:

$query = "INSERT INTO tbl_users(user, password, password_def, userid, level
          , regdate, lastdate, email) VALUES('$username', sha1('$password')
          , sha1('$password'), '$userid', '0', NOW(), NOW(), '$email');";

$userid is a ramdon md5 id.

It gives me this error:

posttokenError: Account not created 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 '\'esck21\', sha1(\'password\'), sha1(\'password\'), 
    \'14bd25cbe111c2975232b33ee8c2' at line 1

I think I'm gonna have a heart attack. Thanks.

zombat · Accepted Answer · 2010-01-24T07:56:34.613

2

Judging by the error message, it looks like you might be calling some kind of escaping function on the entire query, such as addslashes($query) or mysql_real_escape_string($query). This will escape every quote in the query, when really what you want to do is only escape the quotes that are inside your variables.

If this is the case, then you want to be doing something like this instead:

$query = "INSERT INTO tbl_users(user, password, password_def, userid, level,
regdate,lastdate, email) VALUES('".mysql_real_escape_string($username)."',
sha1('".mysql_real_escape_string($password)."'), 
sha1('".mysql_real_escape_string($password)."'), 
'".mysql_real_escape_string($userid)."', '0', 
NOW(), NOW(), '".mysql_real_escape_string($email)."')";

That will properly escape your data without erroneously escaping the rest of the query. Once you've done this, do not run $query as a whole string through other forms of escaping.

edited Jan 24 '10 at 07:56

answered Jan 24 '10 at 07:50

zombat

92,731
24
156
164

Shouldn't the `password` field be backticked? – Alix Axel Jan 24 '10 at 07:57
I don't know why this answer is downvoted. The error message clearly shows backslashes where they shouldn't be, resulting in unquoted values being inserted where a string value is expected and the unquoted value is an interpreted as an invalid columnname (I believe) – Decent Dabbler Jan 24 '10 at 07:59
1

@Alix: `password` is not a MySQL reserved word (see http://dev.mysql.com/doc/refman/5.1/en/reserved-words.html), so it is a valid identifier. It is the name of a function, but unless it is used with function syntax (ie. `PASSWORD()` with brackets), MySQL parses it the same as any other identifier. – zombat Jan 24 '10 at 08:00
@zombat: That wasn't true for MySQL 4, not sure about MySQL 5 though. – Alix Axel Jan 24 '10 at 08:01
I didn't downvote, but if SHA1 is a PHP function - shouldn't it be inside the `mysql_real_escape_string()`? – OMG Ponies Jan 24 '10 at 08:02
@OMG Ponies - `SHA1()` is also a MySQL function. See http://dev.mysql.com/doc/refman/5.1/en/encryption-functions.html#function_sha1. If it was me, I'd probably do it in PHP to put the CPU load on the front end instead of in the database, but I didn't want to muck with the original query for the answer. – zombat Jan 24 '10 at 08:04
@zombat: I've tested the password field in MySQL 5, you're right. – Alix Axel Jan 24 '10 at 08:06
@Alix - yeah, I have faith in the MySQL reserved words list. Also, I know I personally used "password" as a field name long before I learned about backticks. :D Of course, using backticks is always a good idea! – zombat Jan 24 '10 at 08:10
I was using: $query = $this->_db->real_escape_string($query); now, there are a lot of warnings: Warning: mysql_real_escape_string() [function.mysql-real-escape-string]: Access denied for user 'SYSTEM'@'localhost' (using password: NO) and a 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 '\r\nregdate,lastdate, email) VALUES(\'\',\r\nsha1(\'\'), \r\nsha1(\'\'), \r\n\'\' at line 1 – Johann Jan 24 '10 at 08:12
It's working now, I forgot to remove $query = $this->_db->real_escape_string($query); – Johann Jan 24 '10 at 08:18
@zombat: I would rather put the payload on mysql, as mysql is probably faster. Furthermore, if you let MySQL do the hashing (on insert, as well as on comparing when selecting) you will always be sure your code is portable. If another middleware application (e.g. not PHP) wants to query the database you will be garantueed the hashing is always MySQL's implementation. Plus, you keep the responsibilities close to the actual data source. Much of this is also true for DATETIME calculations (which coincidently are far more superior than PHP date functions IMHO). – Decent Dabbler Jan 24 '10 at 08:32
1

Since I got curious about this mysql SHA1() vs PHP sha1(), I found this article that recommends a subquery when you are selecting records with mysql SHA1(). That makes sense, but it feels a bit hackish though. Here's the article: http://saviourc.wordpress.com/2009/09/26/sha1-speed-differences-php-vs-mysql-part-2/ – Decent Dabbler Jan 24 '10 at 08:42
1

@fireeyedboy - true, MySQL's implementation of the same functions would be faster than PHPs, but it's a lot easier to add front-end application servers to bear the load than it is to add database servers. :) – zombat Jan 24 '10 at 08:45
1

@fireeyedboy - wow, interesting article. I would consider that a bug in MySQL's optimizer if considers the `SHA1()` function non-deterministic for a hard value and runs it on every record! The sub-select makes for an interesting work-around. – zombat Jan 24 '10 at 08:50
1

@zombat: I guess you got a point there. And about Mysql being faster: well, that's what I thought initially, but turns out to be not so cristal clear as I would have thought, as per the article I linked to. – Decent Dabbler Jan 24 '10 at 08:58

score 1 · Answer 2 · edited May 23 '17 at 11:55

1

You need to backtick (`) the password field:

$query = "INSERT INTO `tbl_users` (`user`, `password`, `password_def`, `userid`
              , `level`, `regdate`, `lastdate`, `email`) 
          VALUES('$username', 'sha1($password)', 'sha1($password)', '$userid'
              , '0', NOW(), NOW(), '$email');";

You should always backtick your fields, tables and databases.

One more thing: pay attention to SQL Injections, use mysql_real_escape_string().

After some discussion I'm conviced your problem lies with your quoting usage, check zombat answer.

edited May 23 '17 at 11:55

Community

1
1

answered Jan 24 '10 at 07:34

Alix Axel

151,645
95
393
500

@CSSJediEsck21: It might have been the `sha1()` function, should be `'sha1($pass)'` instead of `sha1('$pass')`. I've updated it. Try again. – Alix Axel Jan 24 '10 at 07:44
@Alix, re: the semi-colon: Whoops, guess nothing. Never seen the semi-colon included before, unless it's directly in an sql file. Learned something new – munch Jan 24 '10 at 07:51
@Alix: SHA1() is a mysql function. It shouldn't be enclosed in quotes. – Decent Dabbler Jan 24 '10 at 07:52
Backticking is only necessary when the database, table or column name uses MySQL **keywords** – OMG Ponies Jan 24 '10 at 07:53
@fireeyedboy: AFAIK `sha1()` is a PHP function, `SHA1()` is a MySQL one. I've never seen functions in MySQL in lowercase, not sure if they are case-sensitive or not. – Alix Axel Jan 24 '10 at 07:55
1

PS.: mysql is pretty forgiving about not backticking keywords. As a precaution it doesn't hurt though. – Decent Dabbler Jan 24 '10 at 07:55
@fireeyedboy: He used the MySQL `NOW()` function in uppercase, so that makes it really hard to guess which function he's trying to use. – Alix Axel Jan 24 '10 at 07:59
@Alix: I see what you are saying, but enquoting it still wouldn't have made a difference then, because PHP wouldn't parse the function anyway. – Decent Dabbler Jan 24 '10 at 08:05

What's wrong with this query?

2 Answers2