Link to issue:

This one is only fixable in Joomla 5 - as you are mid upgrade we can't take advantage of the new J4 functionality as it may be run by people mid-upgrade on J3. But indeed we should migrate this in the 5.x branch

But indeed we should migrate this in the 5.x branch

Is it already possible to make a PR for J5 ?

Yup make it to this branch https://github.com/joomla/joomla-cms/tree/5.0-dev

This one is only fixable in Joomla 5 - as you are mid upgrade we can't take advantage of the new J4 functionality as it may be run by people mid-upgrade on J3

That's not right. The mentioned code here is running on Joomla 4 application context already, so you are safe to use prepared statements if required. However, the suggested changes here are not really needed, I just copy the answer from @nikosdion for a similar request sometime ago here for reference:

Using a prepared statement for a variable we positively know it can only ever by an integer is a bad use of prepared statements. Prepared statements are used to isolate us of potential SQL injection vulnerabilities when the data comes from an untrusted source. Our own database table's autonumber field can't be considered untrusted

@joomdonation Interesting, then this issue can be closed.

The only question that remains is whether elsewhere in the Joomla code this is also strictly enforced. If not, confusion may arise.

@sandewt I think script.php is only file because here, we do not have to deal with user's input. Code in admin.script is run on update process, so we will have to be careful with making changes to make sure no bugs are introduced which prevent update from success. Personal I would not touch if not required.

@sandewt I think script.php is only file because here, we do not have to deal with user's input. Code in admin.script is run on update process, so we will have to be careful with making changes to make sure no bugs are introduced which prevent update from success. Personal I would not touch if not required.

I agree with that.

I agree with that.

Thanks all will close this issue.

Closed

Prepared statements are only necessary when you are handling data which is NOT guaranteed to come from a trusted source and its value is NOT known in advance. Remember that prepared statements trade execution speed for safety. Using them will make query execution slower, especially if your database is accessed over TCP/IP, even more so when it's on a different physical (bare metal) or logical (VM, container) server, most definitely if you are running lots of queries in a tight loop as is the case with upgrades or with CLI scripts which batch-process large amounts of data. To understand how prepared statements work at a lower level you can read, for example, how to handle them with the MySQL C API (which you'll see is very close to how mysqli does thing in the PHP userland): https://dev.mysql.com/doc/c-api/8.0/en/c-api-prepared-statement-interface-usage.html

Coming back to this issue, the code presented is perfectly safe without the use of prepared statements because all of the values are hardcoded i.e. their type and the entire set of their possible values is known when the code is written. Its execution context is irrelevant, only where the data comes from is relevant. We can easily deduce that passing these values to the quote method of the database driver is safe as it will deterministically result in a properly escaped string, e.g on MySQL it will either be "atum" or "cassiopeia".

The most common abuse of prepared statements I have seen in Joomla core code is something along the following lines:

$type = 'component';
$query = $db->getQuery(true)
  ->select('*')
  ->from('#__extensions')
  ->where($db->quoteName('element') . ' = :component')
  ->where($db->quoteName('type') . ' = :type')
  ->bind(':component', $component)
  ->bind(':type', $type);

This is an abuse of prepared statements because the value of :type is always known. It's only the value of :component which could come from an untrusted source. The better way to write this query is:

$query = $db->getQuery(true)
  ->select('*')
  ->from('#__extensions')
  ->where($db->quoteName('element') . ' = :component')
  ->where($db->quoteName('type') . ' = ' . $db->quote('component'))
  ->bind(':component', $component);

or, if you actually want to save a few microseconds of execution time, this:

$query = $db->getQuery(true)
  ->select('*')
  ->from('#__extensions')
  ->where([
    $db->quoteName('element') . ' = :component'),
    $db->quoteName('type') . ' = ' . $db->quote('component')
  ])
  ->bind(':component', $component);

Please note that hardcoded values are not the only use case where not using prepared statements is safe. For instance, the same idea would apply if $type is a type- and range-checked variable. For example:

if (!is_string($type) || !in_array($type, ['component', 'template'])) {
  throw new \DomainException('The type can only be component or template');
}

$query = $db->getQuery(true)
  ->select('*')
  ->from('#__extensions')
  ->where([
    $db->quoteName('element') . ' = :component'),
    $db->quoteName('type') . ' = ' . $db->quote($type)
  ])
  ->bind(':component', $component);

The reason this last code block is safe is that even though we are using quote directly on an input variable, the preceding if-block has validated that a. it is definitely a string and b. its value is EITHER component OR template which we can deterministically find to be safe to use with all Joomla implementations of the database driver's quote method across all database drivers shipped with Joomla.

Don't write one-size-fits-all code and definitely don't apply blanket axioms in your code. Understand what your code does, where its values come from, what they contain, and then you can make an informed decision on whether to use prepared statements or not. At worst, you've caught some fuzzy use cases of your code which eluded you when you first wrote it, when you were caught up in the tunnel vision of solving the one particular problem you set out to solve.

@nikosdion Thanks for deep and detailed explanation.

@joomdonation You're welcome :)

@nikosdion

Thanks for your explanation. Your design approach can be applied in a much broader context. ?