Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve examples for MySQLi to show use of prepared queries #60

Open
wants to merge 3 commits into
base: master
from

Conversation

@AllenJB
Copy link

AllenJB commented Mar 14, 2020

The current documentation shows queries being made using variable interpolation. While in this particular case there is no SQL injection vulnerability, new developers should not be expected to be able to spot this or know what prepared queries are / how to use them.

I swapped up the order of the examples so it progresses from "simply query" to "prepared query" and added an explicit comment as to why a prepared query should be used.

@cmb69

This comment has been minimized.

Copy link

cmb69 commented Mar 15, 2020

Thanks for the PR! I'm not sure, though, what to do with this example; it certainly isn't the kind of code that should be used in production, but several of these issues are adressed in the comments (among others, that the (int) cast at the beginning prevents SQL injection in this case). Rewriting this example wrt. contemporary best practices may make it much more complex. Maybe we should just point that out more prominently (e.g. by adding a <note>)?

@AllenJB

This comment has been minimized.

Copy link
Author

AllenJB commented Mar 16, 2020

If these examples are considered so bad they shouldn't be used in "real code", then I would argue they should be deleted altogether. I don't think this set of examples is that bad. While I don't regularly use mysqli myself (I prefer PDO but spotted this issue while helping others), the only other obvious changes I can see are escaping values for HTML properly and adding <p> tags so the output formats better.

(In this case I was focusing on the SQL injection issue and also grepped the other mysqli and PDO documentation for similar issues, but found nothing obvious where it wasn't explicitly demonstrating SQL injection. Proper HTML escaping probably warrants a pass over the entire manual in its own right and thus a separate PR - it also doesn't lead to (potential) server side code execution (SQL is code) or data leaks)

Examples in the official documentation should be examples that new developers can copy and paste and adapt to their needs without falling into any big traps. They are going to do this without reading the entire page in many cases. Yes, experienced developers are almost certainly going to think the result is still terrible, but it should work and it should not lead new developers into obvious / major bad practices - in this case concatenating strings to create SQL queries. My view on this is, as a general rule, and especially for less experienced developers who aren't going to know when they can not use them, always use prepared queries (and I think the documentation should lead developers to doing this).

Regardless of how more experienced developers view this code, developers new to PHP, many new to programming, are going to copy and paste it - including just looking at and taking the part of it that looks like what they want to do this minute.

They aren't going to (and I don't think they should be expected to):

  • know when they should use prepared statements and when they can skip them (and safely use concatenation);
  • pay attention to the cast (and associated comment) at the top of the script or why it's significant;
  • read the user notes.
@kamil-tekiela

This comment has been minimized.

Copy link

kamil-tekiela commented Mar 21, 2020

FYI: I am not a PHP maintainer, I am just another frustrated PHP developer.

"Examples in the official documentation should be examples that new developers can copy and paste and adapt to their needs without falling into any big traps." The whole mysqli documentation is one big mess. No one should ever look at it and think this is something, which I can use in real code.

I would love to provide simpler more contemporary examples if only I could. I am afraid the general public's opinion is you should not use this extension and nobody wants the manual updated.

While I admire your willingness to fix one problem, you actually introduced more. Don't take this the wrong way, but I want to list out the problems:

  • You should never check for the return value of query(). This is a bad practice, which leads to messy and buggy code. Use mysqli error reporting instead.
  • ORDER BY rand() is not recommended. It is not actually adding any value to the example.
  • Never print the value of $mysqli->error to the user. This is a huge security issue!
  • while ($actor = $result->fetch_assoc()) is a bad practice. It's clunky and confusing. Replace with simple foreach ($result as $actor)
  • It looks like you never fetch any result so $stmt->num_rows === 0 will always be true. Besides, the use of num_rows in this context is not needed. If you really want to show a message in case no result is found just add else to the fetch_assoc()
  • There is no such method $stmt->fetch_assoc(). You can only call this method on mysqli_result
  • You never fetched any result, but you are trying to free it with $result->free(). Why do you need to free a result anyway?
  • When writing PHP code, please use good IDE with syntax checking. You have missed ; after prepare() call.

If this example were to be fixed it should look something like this:

<?php

// Let's pass in a $_GET variable to our example, in this case
// it's aid for actor_id in our Sakila database. Let's make it
// default to 1. Example:
//   http://example.org/script.php?aid=42
$aid = $_GET['aid'] ?? 1;

// Always enable mysqli error reporting. The following line enables error reporting (MYSQLI_REPORT_ERROR) and turns them into exceptions(MYSQLI_REPORT_STRICT)
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
// Connecting to and selecting a MySQL database named your_db_name
$mysqli = new mysqli('127.0.0.1', 'your_user', 'your_pass', 'your_db_name');
// You should always set the connection charset. The recommended MySQL charset is utf8mb4
$mysqli->set_charset('utf8mb4');

// Perform a simple SQL query without parameters
$sql = "SELECT actor_id, first_name, last_name FROM actor LIMIT 5";
$result = $mysqli->query($sql);

// Print our 5 actors in a list, and link to each actor
echo "<ul>";
foreach ($result as $actor) {
	echo "<li><a href='?" . htmlspecialchars(http_build_query(['aid' => $actor['actor_id']]), ENT_QUOTES) . "'>";
	echo htmlspecialchars($actor['first_name'] . ' ' . $actor['last_name'], ENT_QUOTES);
	echo "</a></li>";
}
echo "</ul>";

// Perform an SQL query with parameters
// We use a prepared query here to avoid errors caused by unexpected or improperly escaped values.
// These issues can lead to an SQL injection attack that may allow visitors to craft queries that
// perform unexpected changes or access data they would not otherwise be able to access.
$stmt = $mysqli->prepare("SELECT actor_id, first_name, last_name FROM actor WHERE actor_id=?");
// Bind parameters for markers
$stmt->bind_param("s", $aid);
// Execute query
$stmt->execute();
// Get the result
$result = $stmt->get_result();

// Now, we know only one result will exist in this example, so let's
// fetch it into an associated array where the array's keys are the table's column names
if ($actor = $result->fetch_assoc()) {
	echo "Sometimes I see " . htmlspecialchars($actor['first_name'] . " " . $actor['last_name'], ENT_QUOTES) . " on TV.";
}

What I have fixed:

  • Replaced clunky if statement with null-coalescing operator. Remove the suggestion that casting to integers is a good SQL injection prevention.
  • Enabled mysqli error reporting and explained why.
  • Added line set_charset(). It should always be present.
  • Removed all the "error reporting" stuff. It was contradicting itself. PHP has a built-in logger.
  • Changed while loop to foreach loop.
  • Removed $_SERVER['SCRIPT_FILENAME'] from the link.
  • Fixed XSS vulnerability in the HTML link
  • Fixed XSS vulnerability elsewhere
  • Removed superfluous \n
  • Added get_result() to fetch the result from the statement.
  • Removed unnecessary num_rows
  • Added if statement around fetch_assoc()

After testing this locally I confirm this is a working piece of code. It's a code I would use for teaching purposes, but not in production code. After all, the purpose of the manual is to teach good practices, not provide production ready code. In production code I would go for DB abstraction library with PDO underneath.

P.S. I just saw you supplied a correction in which you added $stmt->get_result(). My apologies.

@AllenJB

This comment has been minimized.

Copy link
Author

AllenJB commented Mar 21, 2020

I was specifically looking to fix one issue - and an important one (in my opinion). I don't have the time right now to do major rewrites. If you look carefully most of the content is from the previous version - as I mentioned, I moved some of the examples around to make more sense.

FYI: I am not a PHP maintainer, I am just another frustrated PHP developer.

"Examples in the official documentation should be examples that new developers can copy and paste and adapt to their needs without falling into any big traps." The whole mysqli documentation is one big mess. No one should ever look at it and think this is something, which I can use in real code.

I would love to provide simpler more contemporary examples if only I could. I am afraid the general public's opinion is you should not use this extension and nobody wants the manual updated.

While I admire your willingness to fix one problem, you actually introduced more. Don't take this the wrong way, but I want to list out the problems:

  • You should never check for the return value of query(). This is a bad practice, which leads to messy and buggy code. Use mysqli error reporting instead.

This is open to debate. Yes, I personally prefer extensions to throw exceptions and emit warnings (this is why I'm proposing to change the default error mode on PDO on the internals mailing list). All the documentation is written assuming that users don't change the defaults.

My argument for keeping the code as-is in this regard would be that this code works "either way". Part of the example can be copy and pasted into an existing script and it will work.

  • ORDER BY rand() is not recommended. It is not actually adding any value to the example.

Not my code. And again, not a hard and fast rule - while not recommended, in many cases it has no meaningful performance impact and is perfectly fine to use.

  • Never print the value of $mysqli->error to the user. This is a huge security issue!

Again, I just repeated what the examples here are already doing.

  • while ($actor = $result->fetch_assoc()) is a bad practice. It's clunky and confusing. Replace with simple foreach ($result as $actor)

Again, not my code. While foreach can be considered "cleaner", there's no real impact.

  • It looks like you never fetch any result so $stmt->num_rows === 0 will always be true. Besides, the use of num_rows in this context is not needed. If you really want to show a message in case no result is found just add else to the fetch_assoc()

Again, not my code - I only updated the existing example so it still works. This is an example to show what can be done with the mysqli result object.

  • There is no such method $stmt->fetch_assoc(). You can only call this method on mysqli_result

The example never calls fetch_assoc() on $stmt (my original commit did but I fixed it in a follow up)

  • You never fetched any result, but you are trying to free it with $result->free(). Why do you need to free a result anyway?

Not my code.

  • When writing PHP code, please use good IDE with syntax checking. You have missed ; after prepare() call.

An actual issue with my commit! Fixed.

If this example were to be fixed it should look something like this:

... perhaps you should consider submitting your own PR rather than trying to rewrite basically unrelated PRs. As I mentioned several times, including in my initial opening comment on this PR, this PR is to fix one specific issue with the examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.