1

I'm currently programming a script to login the user if the user types the right information in the Password- and Login-field on a login page. The script is working just fine, but I don't actually know what these two lines of code means and does for the overall user experience.

I'm soon going to an exam where I have to explain the meaning of the code, and it would be absolutely amazing if you guys helped me out by explaining what the two lines of code does below. This is the full script:

<?php  

require('db_connect.php');

if (isset($_POST['user_id']) and isset($_POST['user_pass'])) {

    $username = $_POST['user_id'];
    $password = $_POST['user_pass'];

    $query = "SELECT * FROM dataforlogin WHERE username='$username' and password='$password'";

    $result = mysqli_query($connection, $query) or die(mysqli_error($connection));
    $count = mysqli_num_rows($result);

    if ($count == 1) {

        header("location: ../staudal/dashboard/index.php");

    } else {

        echo "Fail";

  }
}

?>

The two lines of code that I'm having trouble understanding is:

$result = mysqli_query($connection, $query) or die(mysqli_error($connection));
$count = mysqli_num_rows($result);

What do they do and why?

aynber
  • 22,380
  • 8
  • 50
  • 63
  • 7
    This is somewhat out-of-date looking code (complete with [SQL injection vulnerabilities](https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php)) - you might want to find a more modern tutorial... – CD001 May 28 '19 at 15:46
  • 4
    The first one executes a MySQL query, the second one counts the number of rows that the query returned. This is all in the documentation, and also explained in any number of tutorials. – Barmar May 28 '19 at 15:47
  • 1
    @CD001 While it may not be good code, it's not really out of date. It uses `mysqli` rather than `mysql`. Unfortunately, most programmers (including tutorial writers) don't protect against SQL injection. – Barmar May 28 '19 at 15:48
  • 3
    @Barmar I'd argue a more modern tutorial would use bound parameters and `password_hash()` and wouldn't just echo out "Fail" when it goes wrong. – CD001 May 28 '19 at 15:50
  • 7
    @CD001 I don't think that's a matter of "old" or "modern", it's just "bad" versus "good". Bad code doesn't go out of style. – Barmar May 28 '19 at 15:52
  • @Barmar *"Bad code doesn't go out of style."* :D - oki, I chuckled! Fair point - just do a little string substitution in my original comment then ;) – CD001 May 28 '19 at 15:55

4 Answers4

21

That's a good question because these lines are mostly wrong or useless according to the modern standards of security and application design. It never occurred to me before there could be so much wrong in just two lines of code.

here is how it should be

$stmt = $mysqli->prepare("SELECT * FROM dataforlogin WHERE username = ?");
$stmt->bind_param("s", $username);
$stmt->execute();
$result= $stmt->get_result()
$user = $result->fetch_assoc();

if ($user && password_verify($password, $user['password']))
{
    // write some info into the session
    header("location: ../staudal/dashboard/index.php");
    exit;
} else {
    echo "invalid";
}
  • in the first line we are preparing the sql query with question marks placed where variables should go (so it is called a placeholder).
  • in the second line we are binding the actual variable to the placeholder, so it will be sent to mysql server separated from the query and there will be no way for them to interfere.
  • then the query gets actually executed.
  • then we are getting the mysqli_result variable, familiar to all users of either old mysql or new mysqli query - the actual source of data returned by the query.
  • then we are trying to fetch the selected row.
  • then we are checking two things at once,
    • whether our query returned any row
    • and if so, whether the password sent from the form is the same as one stored in the database using password_verify() function
  • the rest is the same as in your code save for two things
    • before redirecting a user you are supposed to write some information about them into the session, in order to recognize them on other pages
    • it is a good practice to add exit after sending the Location header.

Hope these explanations will be enough for your teachers

Seriously, this question should raise awareness about the state of PHP education. Most sources, online and offline, are teaching as though it is still PHP3 around, with only minor face-lifting in regard of deprecated functions. But the approach, which is wrong in so many ways, remains the same.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • indeed you should not use password column filter in the `WHERE` clause as it can be used for timing attacks as databases are designed to return data as quick as possible especially when password column is part of a (multicolumn) index, `password_hash()` forces you to use `password_verify()` what way your are fully protected against [time attacks](https://en.wikipedia.org/wiki/Timing_attack) on your password. – Raymond Nijland May 28 '19 at 16:49
  • 1
    or instead of use `or die(mysqli_error($connection))` you might want to implement using `mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);` to throw exceptions and use `try { ... } catch(Exception $e) {...}` blocks around the MySQL related code.. – Raymond Nijland May 28 '19 at 16:56
  • 1
    @raymond you **should not** catch exceptions by default. – Your Common Sense May 28 '19 at 17:25
  • matter of tasts and goals i geuss and i think you meant throw exceptions on default, but most likely this is better handling indeed by a custom class where you manually throw a MySQLConnectException when a connections fails as i also believe there where also some wierd `mysqli_report()` bugs on some PHP versions now i remember. – Raymond Nijland May 28 '19 at 19:10
  • Also your code **should** include a `exit()` or `__halt_compiler()` after `header()` as you can't trust the client to follow the http redirect header and the prevent futher (possible) unwanted execution.. – Raymond Nijland May 28 '19 at 19:16
  • 1
    Please kindly read the [PHP error reporting basics](https://phpdelusions.net/articles/error_reporting) – Your Common Sense May 28 '19 at 20:00
  • Makes Total sense more or less what iam doing also.. Well i have writen a custom exception handler that shows the exception with a debug stacktrace including code parts by reflection (including param values) when iam developing, when live the server logs it into a SQLite database as i dont really like using tekst files to log to, offcource the SQLite database is not downloadable by a url that would be real bad as i then leak sourcecode to the outside world – Raymond Nijland May 28 '19 at 22:10
0

  • $result = mysqli_query($connection, $query) or die(mysqli_error($connection));
  • This executes the MySQL query, such as SELECT * FROM table WHERE id = ? and then saves the results. Or it returns an error message if the query fails.

  • $count = mysqli_num_rows($result);
  • This just returns the number of rows your query returned.
    0

    First i like to say, Please use prepared statement.

    $result = mysqli_query($connection, $query) or die(mysqli_error($connection));
    

    The mysqli_query() function performs a query against the database and $connection variable opens a new connection to the MySQL server which you already created in db_connect.php and you checking whether the connection is established or not. If not then, die() function will run. It is used to print message and exit from the current php script.

    $count = mysqli_num_rows($result);
    

    mysqli_num_rows() function will returns the number of rows in a result set and you are storing that number into $count variable.

    Hope this helps you

    0

    I'm a junior developer but I hope this answer will help according to your code:

    1. $result = mysqli_query($connection, $query) or die(mysqli_error($connection));

      • $result is a php variable

      • mysqli_query($connection, $query) means that mysqli_query will use the database connection declared database in your db_connect.php to run the query [$query = "SELECT * FROM dataforlogin WHERE username='$username' and password='$password'";]

      • or die(mysqli_error($connection)); means that if mysqli_query does not find the specified database or the table, in this case (dataforlogin), it will display the connection error specified in your db_connect.php

    2. $count = mysqli_num_rows($result);

      • $count is a php variable

      • mysqli_num_rows($result); means that the $count is equall to the number of the rows found in the table (dataforlogin) for $result when mysqli_query($connection, $query) was run.

    Note: Please avoid using the line --- or die(mysqli_error($connection) because it may prevent the page from loading, it's not necessary.

    • *"Please avoid using the line --- or die(mysqli_error($connection) because it may prevent the page from loading, it's not necessary."* what if the code below is depend on a working MySQL connection.. – Raymond Nijland May 28 '19 at 16:44
    • Maybe it's necessary but I prefer to include it in my db_connect code and omit it in the main page, it still works for me. #JuniorDeveloper – Mikeotizels May 28 '19 at 18:06
    • You can check my answer for the in depth explanations. – Your Common Sense May 29 '19 at 09:20