As discussed in the parent bug, it would be cool if jenkins could run seccheck-fast-mwext in a non-voting manner for extensions. (Its expected there will be some false positives, but should be relatively small for extensions, and would allow authors to see what they are and fix them).
It would be also cool to run seccheck-mwext as a non-voting post-merge test (Since that check is really slow, taking over 3 minutes to run, but will detect things the other one wont, especially related to hooks).
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Bawolff | T181660 Experiment using phan for static analysis | |||
| Resolved | Bawolff | T182599 Make jenkins run phan-taint-check-plugin non-voting and then voting | |||
| Resolved | Legoktm | T187497 Set phan-taint-check plugin version in extensions themselves instead of hardcoding it in CI |
I think I've mostly got the dockerization ready for this, do you have an example extension that should pass this, and one that fails (or a patch I can apply to the passing one to make it fail)?
That's awesome. Thank you :D
Abusefilter master has about 17 failures currently when running seccheck-fast-mwext. The InputBox extension passes seccheck-fast-mwext
Eventually yes. Right now there are a couple places in core that are very noisy for false positives (Talking in the hundreds), so I'm not sure its really needed now until that's fixed up a bit. I think extensions are definitely the a better place to concentrate on first.
Change 408368 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] Add job to run seccheck plugin
Change 408368 merged by jenkins-bot:
[integration/config@master] Add job to run seccheck plugin
Change 408746 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] Add mwext-php70-phan-seccheck-docker as experimental
Change 408746 merged by jenkins-bot:
[integration/config@master] Add mwext-php70-phan-seccheck-docker as experimental
After a bit of fiddling... https://integration.wikimedia.org/ci/job/mwext-php70-phan-seccheck-docker/5/console is the result against AbuseFilter. The checkstyle XML is also in a more human readable form at https://integration.wikimedia.org/ci/job/mwext-php70-phan-seccheck-docker/5/checkstyleResult/
For nearly all extensions, commenting "check experimental" will run the security check plugin. If that looks good, we can make it non-voting on most/all extensions? How do we know when to make it voting?
Also tbh a 3 minute job (seccheck-mwext) isn't that slow if we wanted to run the full mode against extensions pre-merge.
Awesome :D
For nearly all extensions, commenting "check experimental" will run the security check plugin. If that looks good, we can make it non-voting on most/all extensions? How do we know when to make it voting?
I would say for the next couple of weeks keep it non voting just in case of bugs in the plugin or something unexpected happens. But after an initial test period i would say we could make it voting as soon as it passes for an extension.
Also tbh a 3 minute job (seccheck-mwext) isn't that slow if we wanted to run the full mode against extensions pre-merge.
Change 410397 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] Run seccheck plugin as non-voting for all bundled extensions
Results from running locally using the CI docker container:
Change 410397 merged by jenkins-bot:
[integration/config@master] Run seccheck plugin as non-voting for all bundled extensions
Also you can just submit a dummy patch against the extensions and jenkins will run the job in the clean CI environment (e.g. https://gerrit.wikimedia.org/r/#/c/410195/ ) - I think my checkout is a few days behind.
Ok, fixes for the failures at: https://gerrit.wikimedia.org/r/#/c/410876/ https://gerrit.wikimedia.org/r/#/c/410869/ and https://gerrit.wikimedia.org/r/#/c/410894/ (In particular, the ImageMap one was an actual issue, in that i18n error messages were being used as raw html)
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.
@Bawolff let me know whenever you're ready to make this voting. According to https://tools.wmflabs.org/ci/last_run.html Renameuser is the only extension failing right now.
I'd like to aim to get this implemented by 1.31 branching to show off these extensions as examples.
Change 431003 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[integration/config@master] Lets make phan-taint-check voting
Change 431003 merged by jenkins-bot:
[integration/config@master] Lets make phan-taint-check voting
We're close. I think Cite, CategoryTree, and Renameuser are the only ones that don't pass under 1.2.0.