Displaying a greeting to one user selected from a database

Question

I execute the following SQL statement to get back an data object into a variable.

  try {
    $getUser = $db->query('select * from user where user.user_id = '.$user_id);
  } catch (Exception $e) {
    echo $e->getMessage();
    die();
  }

from this I print out the information on the page where necessary e.g. the following:

  <?php
  foreach ($getUser as $user) {
    echo '
    <div class="col-wd-12">
      <div class="col bodyHead">Welcome, '.$user['email'].'</div>
    </div>
    ';
  }
  ?>

The question is I know the size of the $getUser array to always be one. Am I doing this the correct way when returning the information? Is there a better way where I do not need to use a foreach loop everytime.

Answer 1

The question is I know the size of the $getUser array to always be one.

No you don't. You assume it is - and it's probably a safe assumption to make. But actually knowing that user_id is a primary key is the job of the underlying database, not of your application.

You would know that 1 record is returned, if the query you send to the database says "give me one record where user_Id is equal to..." - if your backend is mysql that's done with a LIMIT clause.

Your query is missing a terminating semicolon... but the worst offense is the concatenation of the .$user_id into the WHERE clause. Where's that value coming from? Is there any slightest chance that it might not be containing the value you're expecting, and that concatenating it could result in unexpected SQL statements being injected into your query? If that's the case, then your query might actually be returning all rows, not just one.

See give me parameterized SQL, or give me death! (or many, many, many other online resources) for more details.

Answer 2

As @Mat's Mug already suggested, your code is open to SQL injection, which makes it possible to read out any database values (and also may make it possible to execute commands, write and read files, etc).

Casting the value to int somewhere may prevent SQL injection in this situation, but you will eventually make a mistake and be vulnerable. It is impossible to always be sure that each value is already cast/filtered in some way when this happens somewhere else. You really need to use prepared statements.

You are also vulnerable to XSS via email.

Also:

• getUser should be named users. It's not a query, it's the result of a query, ie users.
• SQL keywords should be all upper-case, making them easier to distinguish from variables and making your query easier to read.
• use at least 4 spaces of indentation to make your code more readable.