Page MenuHomePhabricator

Renameuser extension does not pass phan-taint-check 1.2.0
Closed, ResolvedPublic

Description

<checkstyle version="6.5">
  <file name="./maintenance/renameUserCleanup.php">
    <error line="320" severity="error" message="Calling method \Wikimedia\Rdbms\Database::update() in \RenameUserCleanup::updateTable that outputs using tainted argument $updateCondsWithTime. (Caused by: ./maintenance/renameUserCleanup.php +319)" source="SecurityCheck-SQLInjection"/>
  </file>
</checkstyle>

However, Renameuser has a bunch of additional failures when run against the master branch of phan-taint-check. Basically RenameUserJob takes the table name and various column names for the sql queries from the job parameters, and since phan has no way of knowing if the job parameters are sane, this triggers sql injection warnings. Could probably be fixed by adding much $dbw->addIdentifierQuotes, or maybe we should just suppress errors on that method.

Event Timeline

Vvjjkkii renamed this task from Renameuser extension does not pass phan-taint-check 1.2.0 to 6pcaaaaaaa.Jul 1 2018, 1:09 AM
Vvjjkkii raised the priority of this task from Medium to High.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from 6pcaaaaaaa to Renameuser extension does not pass phan-taint-check 1.2.0.Jul 2 2018, 4:52 AM
CommunityTechBot lowered the priority of this task from High to Medium.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
Bawolff claimed this task.

Not sure what this is about, but after https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Renameuser/+/410876/ and https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Renameuser/+/424495/ seems like it works fine.

Locally, it also seems like it works fine with current master of phan-taint-check