SQL Injection in Magento

The collection API quotes the values you pass it and nothing else, so every raw select and every expression needs a bound parameter.

Syntax$select->where('entity_id = ?', $id);

Magento reaches MySQL through Magento\Framework\DB\Adapter\Pdo\Mysql, obtained from Magento\Framework\App\ResourceConnection::getConnection(). Values must arrive as bound parameters. Text pasted into a statement is executed as SQL.

What the collection API does, and does not, do

A collection is Magento's lazy, filterable list of entities. addFieldToFilter('sku', $sku) reaches prepareSqlCondition(), which builds sku = ? and quotes the value through quoteInto(). For the documented operators - eq, neq, like, in, gteq, from and to, and the rest - the value argument is quoted.

Everything else is yours to get right. $collection->getSelect()->where("sku = '$sku'") inserts your string verbatim. So does a Zend_Db_Expr, a concatenated joinLeft() condition, an ORDER BY assembled from a request parameter, and $connection->fetchAll("SELECT ... WHERE sku = '$sku'"). "The collection escaped it for me" holds only for the value argument of a recognised operator.

Bind instead

Magento\Framework\DB\Select::where($cond, $value = null, $type = null) takes the value separately: where('entity_id = ?', $id). fetchAll($sql, $bind) and fetchOne($sql, $bind) take a bind array. quoteInto($text, $value) builds a quoted fragment when you need the string itself.

In developer mode the edited PHP applies on the next request; flush with bin/magento cache:flush if the output sits inside a cached block. In production mode run bin/magento setup:di:compile first.

Example

Example · php
<?php
// app/code/Vendor/Loyalty/Model/ResourceModel/PointLedger.php
declare(strict_types=1);

namespace Vendor\Loyalty\Model\ResourceModel;

use Magento\Framework\App\ResourceConnection;

class PointLedger
{
    public function __construct(
        private readonly ResourceConnection $resource
    ) {
    }

    public function getBalance(int $customerId, string $currencyCode): int
    {
        $connection = $this->resource->getConnection();

        $select = $connection->select()
            ->from(
                $this->resource->getTableName('vendor_loyalty_ledger'),
                ['balance' => 'SUM(points)']
            )
            ->where('customer_id = ?', $customerId)
            ->where('currency_code = ?', $currencyCode);

        return (int) $connection->fetchOne($select);
    }
}

// The adapter quotes each bound value as it renders the statement:
//   SELECT SUM(points) AS `balance` FROM `vendor_loyalty_ledger`
//   WHERE (customer_id = 42) AND (currency_code = 'EUR')
// A currency code of "EUR' OR '1'='1" becomes a single quoted string
// and matches no rows, instead of becoming part of the condition.

When to use it

  • Add a report query without opening a hole where the date range is pasted into the SQL.
  • Review a third-party module that builds getSelect()->where() out of request parameters.
  • Make an admin search box safe when the term reaches a LIKE condition.
  • Replace a concatenated joinLeft() condition in inherited code with bound parameters.

More examples

Collection filters: the two forms, and the one to avoid

addFieldToFilter('status', 1) and addFieldToFilter('status', ['eq' => 1]) desugar to the same condition; the short form is what you will see in most code. Reach for getSelect() only for conditions the filter API cannot express, and pass the value as the second argument so the adapter quotes it.

Example · php
<?php
// app/code/Vendor/Loyalty/Model/LedgerSearch.php
declare(strict_types=1);

namespace Vendor\Loyalty\Model;

use Vendor\Loyalty\Model\ResourceModel\Ledger\CollectionFactory;

class LedgerSearch
{
    public function __construct(
        private readonly CollectionFactory $collectionFactory
    ) {
    }

    public function findActive(string $currencyCode): array
    {
        $collection = $this->collectionFactory->create();

        // Shorthand: a bare scalar means the 'eq' operator.
        $collection->addFieldToFilter('status', 1);

        // Longhand: exactly the same filter, written out.
        // $collection->addFieldToFilter('status', ['eq' => 1]);

        // Both forms quote the value through quoteInto().
        $collection->addFieldToFilter('currency_code', ['eq' => $currencyCode]);

        // DO NOT do this - the string is inserted verbatim:
        // $collection->getSelect()->where("currency_code = '$currencyCode'");

        // Bind it when you do need getSelect():
        $collection->getSelect()->where('points <> ?', 0);

        return $collection->getItems();
    }
}

A raw statement with named binds

fetchAll($sql, $bind) and fetchOne($sql, $bind) accept named placeholders. The table name is interpolated because it came from getTableName(), not from a request; values never are. Reach for a raw statement when a GROUP BY with HAVING is clearer than the select builder, and keep every user-supplied value in the bind array.

Example · php
<?php
// app/code/Vendor/Loyalty/Model/ResourceModel/ExpiringPoints.php
declare(strict_types=1);

namespace Vendor\Loyalty\Model\ResourceModel;

use Magento\Framework\App\ResourceConnection;

class ExpiringPoints
{
    public function __construct(
        private readonly ResourceConnection $resource
    ) {
    }

    /**
     * @return array<int, array<string, string>>
     */
    public function getExpiringBefore(string $date, int $minimumPoints): array
    {
        $connection = $this->resource->getConnection();
        $table = $this->resource->getTableName('vendor_loyalty_ledger');

        $sql = "SELECT customer_id, SUM(points) AS balance"
            . " FROM {$table}"
            . " WHERE expires_at < :expires_at"
            . " GROUP BY customer_id"
            . " HAVING balance >= :minimum_points";

        return $connection->fetchAll($sql, [
            'expires_at' => $date,
            'minimum_points' => $minimumPoints,
        ]);
    }
}

A LIKE search with the wildcards escaped

Magento\Framework\DB\Helper::addLikeEscape($value, $options = []) exists because quoting alone does not stop a search term of "%" from matching every row. Reach for it on any search box that reaches a LIKE. Options are allow_symbol_mask, allow_string_mask and position (any, start, end).

Example · php
<?php
// app/code/Vendor/Loyalty/Model/ResourceModel/CodeLookup.php
declare(strict_types=1);

namespace Vendor\Loyalty\Model\ResourceModel;

use Magento\Framework\App\ResourceConnection;
use Magento\Framework\DB\Helper as DbHelper;

class CodeLookup
{
    public function __construct(
        private readonly ResourceConnection $resource,
        private readonly DbHelper $dbHelper
    ) {
    }

    /**
     * @return string[]
     */
    public function searchCodes(string $term): array
    {
        $connection = $this->resource->getConnection();

        // addLikeEscape() escapes %, _ and \ in the term and returns a
        // quoted expression; 'position' => 'any' wraps it in % ... %.
        $quoted = $this->dbHelper->addLikeEscape($term, ['position' => 'any']);

        $select = $connection->select()
            ->from($this->resource->getTableName('vendor_loyalty_code'), ['code'])
            ->where('code LIKE ?', $quoted)
            ->limit(50);

        return $connection->fetchCol($select);
    }
}

Related

m2 collectionsm2 repositoriesm2 search criteriam2 security inputm2 perf mysql

Discussion

  • Be the first to comment on this lesson.