The 2.x branch is now ready for release. It supports PHP7.0+ and php-ast 1.0.0+ (1.0.1 is strongly recommended on PHP7.1+).
Please release 2.0.0 from the content of the 2.x branch.
Description
Description
Details
Details
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Merge remote-tracking branch 'origin/2.x' | mediawiki/tools/phan/SecurityCheckPlugin | master | +0 -6 |
Event Timeline
Comment Actions
Change 520381 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Merge remote-tracking branch 'origin/2.x'
Comment Actions
So here's what I'd propose
- Create the new release:
git clone "https://gerrit.wikimedia.org/r/mediawiki/tools/phan/SecurityCheckPlugin" git checkout 2.x git tag -a v2.0.0 -m "Releasing version 2.0.0 - see 2.x development branch"
- [any further testing]
- merge r520381
I think this is all we can do until the composer-docker ast issues are resolved.
Comment Actions
Fine by me. Will you do the actual release, or should I? Then I'll let you know the results of the final testing on a bunch of extensions. And then yeah, we'll have to wait for ast in containers.
Of note, there are a few +2 patches coming next in 2.x. If it's fine by you, I'd like to merge them in 2.x after the release (letting jenkins test them possibly), and then move everything unmerged/unreviewed to master.
Comment Actions
Done: https://gerrit.wikimedia.org/r/admin/projects/mediawiki/tools/phan/SecurityCheckPlugin,tags
Feel free to merge the +2 patch sets to 2.x now. And we might want to have a discussion on Phab as to which ones make the most sense for a 2.1.0 or whatever release. And when that might happen.
Comment Actions
Great, thanks! I'm going to do the final testing tomorrow (UTC+2).
As for commits still in 2.x, most of them should be just cleanups or minor changes. I'll look again tomorrow and will let you know. It could also make sense to release a 2.0.1 version with +2ed commits, if they're all minor changes and ast isn't ready in the next couple of days.
Comment Actions
As for commits still in 2.x, most of them should be just cleanups or minor changes. I'll look again tomorrow and will let you know. It could also make sense to release a 2.0.1 version with +2ed commits, if they're all minor changes and ast isn't ready in the next couple of days.
Works for me, thanks a ton (again) for all of your work on this. There's absolutely no way this would've gotten done this summer without your efforts.
Comment Actions
Extension testing results
Using mwext-fast-config, on master. Using PHP 7.3.4 with php-ast 1.0.1.
AbuseFilter
includes/AbuseFilterModifyLogFormatter.php:34 SecurityCheck-DoubleEscaped Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped) includes/AbuseFilterModifyLogFormatter.php:35 SecurityCheck-DoubleEscaped Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped) includes/AbuseFilterModifyLogFormatter.php:42 SecurityCheck-DoubleEscaped Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped) includes/AbuseFilterModifyLogFormatter.php:43 SecurityCheck-DoubleEscaped Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped)
Notes: These seem to be false positives, and need some investigation.
Summary: 4 issues, 4 false positives
CheckUser
includes/specials/SpecialCheckUser.php:1600 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped) includes/specials/SpecialCheckUser.php:1602 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped) includes/specials/SpecialCheckUser.php:1607 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped) includes/specials/SpecialCheckUser.php:1611 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped) includes/specials/SpecialCheckUser.php:1613 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped) includes/specials/SpecialCheckUser.php:1617 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped) includes/specials/SpecialCheckUser.php:1619 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped) includes/specials/SpecialCheckUser.php:1625 SecurityCheck-DoubleEscaped Calling method \Xml::submitButton() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +473) (Caused by: Builtin-\Message::escaped)
Notes: I checked the first one for Xml::label, and in fact it's double escaped (so the other should be DE too). The submitButton is double escaped, too.
Summary: 8 issues, all correct
CentralAuth
includes/specials/SpecialCentralAuth.php:343 SecurityCheck-DoubleEscaped Calling method \Xml::fieldset() in \SpecialCentralAuth::showInfo that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +613) (Caused by: Builtin-\Message::escaped) includes/specials/SpecialCentralAuth.php:404 SecurityCheck-DoubleEscaped Calling method \Xml::fieldset() in \SpecialCentralAuth::showWikiLists that outputs using tainted argument $legend. (Caused by: ../../includes/Xml.php +613) (Caused by: includes/specials/SpecialCentralAuth.php +400) includes/specials/SpecialCentralAuth.php:633 SecurityCheck-DoubleEscaped Calling method \SpecialCentralAuth::foreignLink() in \SpecialCentralAuth::formatBlockStatus that outputs using tainted argument $text. (Caused by: includes/specials/SpecialCentralAuth.php +731) (Caused by: includes/specials/SpecialCentralAuth.php +630) includes/specials/SpecialCentralAuth.php:801 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse) includes/specials/SpecialCentralAuth.php:802 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse) includes/specials/SpecialCentralAuth.php:809 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse) includes/specials/SpecialCentralAuth.php:816 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse) includes/specials/SpecialCentralAuth.php:817 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse) includes/specials/SpecialCentralAuth.php:824 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse) includes/specials/SpecialCentralAuth.php:825 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse) includes/specials/SpecialCentralAuth.php:833 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse) includes/specials/SpecialCentralAutoLogin.php:594 SecurityCheck-DoubleEscaped Calling method \Xml::encodeJsCall() in \SpecialCentralAutoLogin::execute that outputs using tainted argument $[arg #2]. (Caused by: Builtin-\Xml::encodeJsCall) (Caused by: includes/CentralAuthHooks.php +1194)
Notes: The ones for Xml::fieldset are genuine; ditto for radioLabel. Couldn't test the others due to troubles with CentralAuth on my wiki.
Summary: 12 issues, 10 genuine, the other 2 likely genuine
Echo
includes/formatters/EchoModelFormatter.php:17 SecurityCheck-DoubleEscaped Calling method \wfExpandUrl() in \EchoModelFormatter::formatModel that outputs using tainted argument $[arg #1]. (Caused by: includes/formatters/EchoHtmlEmailFormatter.php +156) (Caused by: includes/formatters/EchoModelFormatter.php +13) includes/formatters/EchoModelFormatter.php:22 SecurityCheck-DoubleEscaped Calling method \wfExpandUrl() in \EchoModelFormatter::formatModel that outputs using tainted argument $[arg #1]. (Caused by: includes/formatters/EchoHtmlEmailFormatter.php +156) (Caused by: includes/formatters/EchoModelFormatter.php +20) scripts/generatecss.php:26 SecurityCheck-XSS Echoing expression that was not html escaped (Caused by: scripts/generatecss.php +25; scripts/generatecss.php +6)
Notes: The first two are a bit cryptic, they seem false positives, but I'm unsure. The third one is a false positive for CLI script, something already known.
Summary: 3 issues, 1 FP, 2 likely FPs
Flow
includes/Formatter/FeedItemFormatter.php:49 SecurityCheck-DoubleEscaped Calling method \FeedItem::__construct() in \Flow\Formatter\FeedItemFormatter::format that outputs using tainted argument $[arg #2]. (Caused by: ../../includes/changes/FeedItem.php +145) (Caused by: includes/Formatter/FeedItemFormatter.php +22) includes/Import/OptInController.php:393 SecurityCheck-DoubleEscaped Calling method \Flow\Import\OptInController::createRevision() in \Flow\Import\OptInController::archiveExistingTalkpage that outputs using tainted argument $content. (Caused by: includes/Import/OptInController.php +313) (Caused by: includes/Import/OptInController.php +388; includes/Import/OptInController.php +389; includes/Import/OptInController.php +390) includes/Import/OptInController.php:519 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Import\OptInController::editBoardDescription that outputs using tainted argument $content. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Import/OptInController.php +518; includes/Import/OptInController.php +519) includes/Import/OptInController.php:583 SecurityCheck-DoubleEscaped Assigning a tainted value to a variable that later does something unsafe with it (Caused by: includes/Import/OptInController.php +578; includes/Import/OptInController.php +583) includes/Import/OptInController.php:588 SecurityCheck-DoubleEscaped Calling method \Flow\Import\OptInController::createRevision() in \Flow\Import\OptInController::removeArchiveTemplateFromWikitextTalkpage that outputs using tainted argument $[arg #2]. (Caused by: includes/Import/OptInController.php +313) (Caused by: includes/Conversion/Utils.php +67; includes/Conversion/Utils.php +74; includes/Import/OptInController.php +588) includes/Import/OptInController.php:636 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Import\OptInController::editWikitextContent that outputs using tainted argument $newContent. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Import/OptInController.php +635) includes/Import/OptInController.php:636 SecurityCheck-DoubleEscaped Calling method \Flow\Import\OptInController::createRevision() in \Flow\Import\OptInController::editWikitextContent that outputs using tainted argument $[arg #2]. (Caused by: includes/Import/OptInController.php +313) (Caused by: includes/Conversion/Utils.php +67; includes/Conversion/Utils.php +74; includes/Import/OptInController.php +588; includes/Import/OptInController.php +635) includes/Import/OptInController.php:638 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Import\OptInController::editWikitextContent that outputs using tainted argument $newContent. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Import/OptInController.php +635) includes/Import/TemplateHelper.php:76 SecurityCheck-DoubleEscaped Assigning a tainted value to a variable that later does something unsafe with it (Caused by: includes/Import/TemplateHelper.php +76) includes/Import/Wikitext/ImportSource.php:73 SecurityCheck-DoubleEscaped Assigning a tainted value to a variable that later does something unsafe with it (Caused by: includes/Import/Wikitext/ImportSource.php +73) includes/Model/AbstractRevision.php:423 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Model\AbstractRevision::getContent that outputs using tainted argument $raw. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Model/AbstractRevision.php +401) includes/Model/AbstractRevision.php:546 SecurityCheck-DoubleEscaped Assigning a tainted value to a variable that later does something unsafe with it (Caused by: includes/Model/AbstractRevision.php +546) includes/Model/AbstractRevision.php:568 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Model\AbstractRevision::setContent that outputs using tainted argument $content. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Model/AbstractRevision.php +546; includes/Model/AbstractRevision.php +562; includes/Model/AbstractRevision.php +568) includes/View.php:87 SecurityCheck-DoubleEscaped Calling method \Flow\View::renderApiResponse() in \Flow\View::show that outputs using tainted argument $apiResponse. (Caused by: includes/View.php +367) (Caused by: includes/View.php +71) maintenance/FlowReserializeRevisionContent.php:103 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \FlowReserializeRevisionContent::execute that outputs using tainted argument $wikitext. (Caused by: includes/Import/OptInController.php +588) (Caused by: maintenance/FlowReserializeRevisionContent.php +102)
Notes: All of them are a bit cryptic and I didn't really investigate. All I can say is that they seem to be sharing the same root cause, which has to do with Utils::convert.
Summary: 15 issues, unknown validity
SpamBlacklist
includes/SpamBlacklistLogFormatter.php:7 SecurityCheck-DoubleEscaped Calling method \htmlspecialchars() in \SpamBlacklistLogFormatter::getMessageParameters that outputs using tainted argument $[arg #1]. (Caused by: includes/SpamBlacklistLogFormatter.php +6)
Notes: Should be the same as AbuseFilter
Summary: 1 issue, 1 FP
Thanks
No issues
ContentTranslation
No issues
UserMerge
No issues
Conclusions
IMHO, this is acceptably good. Echo and Flow would need some more investigation (especially Flow), but that's something that can be delayed I guess. Now I'm going to investigate the issue in LogFormatters, to see if it can be resolved easily. Aside from that, I guess we can move on, merging r520381 and starting to update extensions as soon as php-ast is ready.
Comment Actions
Found it. Once again, it has to do with the param taints specified in the plugin config. To put it simply, the AbuseFilter code is calling LinkRenderer::makeKnownLink passing in an escaped second argument. makeKnownLink's second argument has ESCAPES_HTML in the plugin config, so we have escaped+escaped => DoubleEscaped warning.
Now, the annotation for makeKnownLink is partly correct. More specifically, it really escapes HTML (via HtmlArmor as the comment says), *but* just as long as you pass it the HTML as string. Instead, if you pass it an HtmlArmor, it will be output as-is. Which is to say, you have to escape the content.
The code above is calling an intermediary function (makePageLink) with an escaped string, and that function puts it in an HtmlArmor and calls makeKnownLink, and we get the warning.
So, the solution.
We'd need a way to tell the plugin that makeKnownLink [1] only escapes HTML if it's passed a string, and not an HtmlArmor. Or maybe tweak param taint for MW functions. Or fix it somewhere else in the code. But in any case, I don't see anything too easy.
[1] - Note that everything I'm saying also stands for related functions: makeLink, makePreloadedLink and buildAElement
Comment Actions
I can't see any easy solution that we can implement now. Moreover, it should come together with better handling of HtmlArmor (as a sort of follow-up of https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/508312/), so I'd like to wait.
I sent https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/520789/ on top of the 2.x tip and will continue the work there in the next days.
@sbassett If you're fine with the current state, I'll leave you the honor to merge https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/520381/-1..2.
Comment Actions
Change 520381 merged by SBassett:
[mediawiki/tools/phan/SecurityCheckPlugin@master] Merge remote-tracking branch 'origin/2.x'
Comment Actions
IMHO, this is acceptably good.
Agreed - tests above look good, thanks. I think we're probably good with keeping the 2.x branch around for a while, possibly indefinitely? Even after we merge to master. Let me know if you feel otherwise for any reason.
And... just merged https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/520381/-1..2
Comment Actions
Sure, I think keeping the branch alive won't hurt; actually, I see it as a quite common practice to leave development branches in place. And now we're done with this task.
In the next days, I'm going to merge +2ed patches and cherry pick 'em to master. I still have to check what could make sense to add in a minor release, and will provide updates in a separate task. I'm also going to cherry pick and abandon the remaining patches, to empty the 2.x branch. And of course, I'll have to look at fixing a couple of known issues introduced with the upgrade, although for convenience I may delay that until we upgrade to phan 2.2, which shouldn't happen as part of the next minor release due to PHP70 incompatibility.