Strip null bytes from strings and filter conditions.

[colinmollenhour]

Description (*)

Prevent null bytes injected into user-provided data from being included in saved data and query filters.

Fixed Issues (if relevant)

Related to #1087590

Questions or comments

Blob types are not affected, only varchar and text types. I've never seen any reason why Magento should store null bytes in varchar or text columns but please advise if you can think of any.

Contribution checklist (*)

• [ ] Pull request has a meaningful description of its purpose
• [ ] All commits are accompanied by meaningful commit messages
• [ ] All automated tests passed successfully (all builds are green)

[luigifab]

To wake up this PR, I like my old idea #832.

[colinmollenhour]

@luigifab null bytes and leading/trailing whitespace are quite different situations and fixing one doesn't affect the other..

I don't know of any cases where someone needs leading or trailing whitespace in the context of Magento, I've never seen one, but it is somewhat plausible so I actually don't disagree with #832. However, I don't think it is plausible that a user wants to insert a null byte somewhere unless said user is a hacker. 😆 But again, these are two different solutions to two different problems in my opinion.

[kiatng]

Fixed Issues (if relevant)

Related to #1087590

I cannot find the ref issue.

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit 9a49aa12. ± Comparison against base commit d433ae99.

[colinmollenhour]

I cannot find the ref issue.

Sorry, @kiatng, I honestly cannot recall what the ref'd issue was or why I didn't provide a full link to it.. 😞

[rjocoleman]

It appears as though this change broke all my grids. When filtering none have any results (except ironically when the new NULL condition is added).

This appears to be due to:

# lib/Varien/Db/Adapter/Pdo/Mysql.php
/**
 * Prepare Sql condition
 *
 * @param  string $text Condition value
 * @param  mixed $value
 * @param  string $fieldName
 * @return string
 */
protected function _prepareQuotedSqlCondition($text, $value, $fieldName)
{
        $sql = $this->quoteInto($text, str_replace("\0", '', $value));
        return str_replace('{{fieldName}}', $fieldName, $sql);
}

In the context for grid filters such as Product, previously, $this->quoteInto was passed $value as a Zend_Db_Expr, now due to the str_replace it's a string. The type is prematurely changed.

The quote function will return Zend_Db_Expr as a string $value->__toString(), or send strings to the database to get quoted.

This leads to something like this when filtering product name for Dog:

$text = "at_name.value LIKE ?";
$value = new Zend_Db_Expr("'%Dog%'");
$fieldName = "at_name.value";

$text_rpl = str_replace('{{fieldName}}', $fieldName, $text);
$old = $this->quoteInto($text_rpl, $value);
# $old: "at_name.value LIKE '%Dog%'"

$sql = $this->quoteInto($text, str_replace("\0", '', $value));
$new = str_replace('{{fieldName}}', $fieldName, $sql);
# $new: "at_name.value LIKE '\'%Dog%\''"

[addison74]

If it is a confirmed bug, it must be reverted quickly. Later a new PR can be created to solve the issue.

[fballiano]

I've created a PR to revert this one, just so that we don't forget about it. In the meanwhile let's wait for @colinmollenhour to comment :-)

[sreichel]

Change

        $sql = $this->quoteInto($text, str_replace("\0", '', $value));

to

        $value = is_string($value) ? str_replace("\0", '', $value) : $value;
        $sql = $this->quoteInto($text, $value);

should work.

[colinmollenhour]

Ahh, sorry everyone.. I forgot about Zend_Db_Expr in this context.. I agree with @sreichel suggested fix.

[sreichel]

If it is a confirmed bug, it must be reverted quickly. Later a new PR can be created to solve the issue.

Imho ...

If it is a confirmed bug - in an already released version - we should either revert OR fix it quickly in a hotfix-release.

If it is a confirmed bug - in 1.9.4.x/dev-branch - we should fix it quickly before next release.