Partial fix Buggy string comparisons

PaulB

PaulB

Well-known member
Affected version
2.2.5
In PHP, some comparisons with == or in_array may return true when the programmer expects them to return false. For example, "00" == "0000" is true in PHP, as is in_array("00", ["0000"]). XenForo 2 performs loose comparisons in some places that can potentially result in bugs:
  • XF\Criteria\User#_matchUsername: lax in_array usage will result in usernames like 010 matching usernames like 10 and vice versa
  • XF\Db\Schema\Column#addValues: lax in_array usage will treat non-duplicate values as duplicates
  • XF\Db\Schema\Column#removeValues: lax array_search usage can remove the wrong value
  • XF\Db\Schema\Column#getDefinition: $existing->values != $values will return false for ['00'] != ['000']
  • XF\Mvc\Entity\ValueFormatter#applyValueConstraints, line 377: in_array check on allowedValues could result in invalid values passing the check
  • XF\Legacy\DataWriter#_applyFieldValueLimits: same as previous
  • XFMG\Import\Importer#getMediaTypeAndFilePathFromExtension: in_array check on file extensions could result in a match for the wrong file extension (e.g., sample.10 would match for file extension 010)
  • XFMG\Repository\Media#getMediaTypeFromExtension: same as previous
  • XF\Http\Upload#isValid: same as previous
  • XF\Service\AbstractService\MirrorCreator#isMirrorable: in_array check on media type could result in a disallowed media type being allowed
  • XFMG\XF\Entity\Attachment#validateXfmgMediaMirrorCategory: same as previous
  • XFRM\Service\ResourceItem\Icon#validateImageAsIcon:same as previous
  • (vendor-patch) Laminas\Mail\Headers#has: loose in_array check can result in has() returning true when it should return false (reported upstream: https://github.com/laminas/laminas-mail/issues/152)
  • (vendor) Laminas\Validator\File\*: combination of == and in_array result in files treated as having the wrong extensions (reported upstream: https://github.com/laminas/laminas-validator/issues/98)
  • (vendor) Symfony\Component\Console#find, line 666: correct use of ===, but missing strict check on in_array, so the wrong command could be treated as an exact match
  • (vendor) Symfony\Component\Console\Input#hasParameterOption, #getParameterOption: I'm not too sure what the implications are here without digging deeper, but it looks like these need to be a strict in_array checks.
  • XF\Entity\CaptchaQuestion#isCorrect: invalid answers can be treated as correct
  • XF\Pub\Controller#canBypassPolicyAcceptance: non-whitelisted paths could be treated as whitelisted
  • XF\Search\Source\AbstractSource#parseKeywords, line 128: non-stop words could be treated as stop words
  • XF\Searcher\AbstractSearcher#validateCriteriaValue, lines 245, 247, and 267: unique values could be filtered out due to use of SORT_REGULAR (e.g., ['10', '010'] becomes ['10']); invalid values could be treated as valid; there may be additional implications
  • XF\Validator\Url#isValid, line 63: forbidden schemas could be treated as allowed
There are probably other places with faulty string comparisons. Here are some bugs that are likely a result of such comparisons:

Content tagging:
  1. Tag a thread with tag 000.
  2. Save the tags.
  3. Try to tag the same thread with tag 00.
  4. The tag addition will silently fail.
Tag URLs:
  1. Tag a thread with tag 0.
  2. Save the tags.
  3. Click the 0 tag to be brought to its canonical page (tags/0/).
  4. Rather than seeing the tag page for tag 0, you'll see the response for tags/.
 
  • XF\Admin\Controller\DataPortability#actionExport: Export will fail for username 0
  • XF\Job\UserDeleteCleanUp#run: Will erroneously report completion without running for username 0
  • XF\Job\UserRenameCleanUp#run: Will erroneously report completion without running for username 0
  • XF\Pub\Controller\Member#actionIndex: Won't work when querying for username or member_name 0
  • XF\Service\Thread\Creator#_validate: Thread and post username will be overwritten with placeholders if username is 0
  • XF\Service\Thread\Replier#_validate: Post username will be overwritten with placeholder if username is 0
  • XF\Service\User\ContentChange#__construct: Will always throw a LogicException if username is 0
 
Thank you for reporting this issue, it has now been resolved. We are aiming to include any changes that have been made in a future XF release (2.2.7).

Change log:
Patch loose string comparisons such as `!$username` and `in_array($username, $usernames)` that can result in unexpected behaviors when wierd strings are used.
Click to expand...
There may be a delay before changes are rolled out to the XenForo Community.
 
We have fixed the majority of the reported issues but the following issues remain for now and require a bit more work:
  • XF\Searcher\AbstractSearcher#validateCriteriaValue, lines 245, 247, and 267: unique values could be filtered out due to use of SORT_REGULAR (e.g., ['10', '010'] becomes ['10']); invalid values could be treated as valid; there may be additional implications
  • XF\Service\Thread\Creator#_validate: Thread and post username will be overwritten with placeholders if username is 0
  • XF\Service\Thread\Replier#_validate: Post username will be overwritten with placeholder if username is 0
We will likely need to do manual patching of the following.
Bumping to minimum PHP 7.2 in XF 2.3 doesn't help us with Laminas Mail 2.14.1 as it requires PHP 7.3. Issue #98 for laminas-validator is still open but it looks like the issue may be fixed? I assume that is in a PHP 7.3 version too though.

Symfony ones will need to be reported upstream too. Has this been done?
  • Symfony\Component\Console#find, line 666: correct use of ===, but missing strict check on in_array, so the wrong command could be treated as an exact match
  • Symfony\Component\Console\Input#hasParameterOption, #getParameterOption: I'm not too sure what the implications are here without digging deeper, but it looks like these need to be a strict in_array checks.
 
Top Bottom