Page MenuHomePhabricator

Upgrade taint-check to 2.0.1 in all repos
Closed, ResolvedPublic

Description

Given the large numbers of false and true positives, this has to be done semi-manually. Once all repos are upgraded, we'll be able to switch to PHP72 jobs.

Event Timeline

Daimona created this task.Jul 3 2019, 8:01 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 3 2019, 8:01 AM
Daimona changed the task status from Open to Stalled.Jul 3 2019, 9:08 AM
Daimona renamed this task from Upgrade taint-check to 2.0 in all repos to Upgrade taint-check to 2.0.1 in all repos.Jul 9 2019, 6:38 PM
Daimona updated the task description. (Show Details)

OK, I did a manual batch run for all Wikimedia production repos and pushed them into gerrit. Unfortunately, a number of these fail to upgrade, presumably because of new heuristics spotting issues that were already there but weren't previously exposed.

For example:

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/521589

13:54:50   <file name="includes/AbuseFilterModifyLogFormatter.php">
13:54:50     <error line="34" severity="warning" message="Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped)" source="SecurityCheck-DoubleEscaped"/>
13:54:50     <error line="35" severity="warning" message="Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped)" source="SecurityCheck-DoubleEscaped"/>
13:54:50     <error line="42" severity="warning" message="Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped)" source="SecurityCheck-DoubleEscaped"/>
13:54:50     <error line="43" severity="warning" message="Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped)" source="SecurityCheck-DoubleEscaped"/>
13:54:50   </file>

Presumably each of these 40+ repos will need manual fixing? What joy. :-)

Yes, that was expected, see e.g. T227171#5306248. Note that some warnings (like all the ones for AF) are false positives, so don't trust the plugin too much. I'm going to verify the others, and amend your patches.

So I did a first batch of review (note that I cannot amend others' patchsets though). A few errors are unrelated (e.g. for the phan job), some were easy fixes, some of them are too complicated or too long and I still have to check them, a couple of them caused seccheck to crash (it doesnt like the string 'class'), and many others seem false positives, though they'd have to be checked more accurately.

Daimona is doing all the work here. :-)

Heh :D It's a long way to the top... I hope to remain with <20 repos failing for today, then I'll check the non-obvious ones tomorrow.

(Just did a final pass and found another 21 repos that needed updating; sadly a bunch of these also fail. Boo.)

No problem, I'm gonna see what's wrong with them. I hoped to get all the remaining repos done today, but I'd be happy with half of them, too.

Aye, done everything. Now we have 6 repos failing for unrelated reasons. I guess we'll have to fix the failing tests etc., or just give manual V+2. And then, we can switch the seccheck job back to PHP7.2!

FTR, the following extensions are missing:

  1. WebAuthn - requires ext-bcmath, T227043. The plan is to leave it last and switch to PHP72 all the same
  2. WikibaseLexemeCirrusSearch - Blocked on some unrelated test failure
  3. MobileFrontend - Jenkins on vacation apparently
  4. WikibaseQualityConstraints - Ready
Jdforrester-WMF closed this task as Resolved.Jul 12 2019, 12:45 AM

Effectively done.

sbassett triaged this task as Normal priority.Tue, Oct 15, 7:23 PM