Page MenuHomePhabricator
Create Task
Maniphest T205972

Fixup taint-check-plugin errors in SecurePoll
Closed, ResolvedPublic
Actions

Description

<?xml version="1.0" encoding="ISO-8859-15"?>
<checkstyle version="6.5">
  <file name="includes/ballots/RadioRangeBallot.php">
    <error line="240" severity="warning" message="Calling method \Xml::radio() in \SecurePoll_RadioRangeBallot::getQuestionForm that outputs using tainted argument $[arg #4]. (Caused by: ../../includes/Xml.php +342) (Caused by: includes/ballots/RadioRangeBallot.php +222; includes/ballots/RadioRangeBallot.php +239)" source="SecurityCheck-DoubleEscaped"/>
    <error line="240" severity="warning" message="Calling method \Xml::radio() in \SecurePoll_RadioRangeBallot::getQuestionForm that outputs using tainted argument $score. (Caused by: ../../includes/Xml.php +342) (Caused by: includes/ballots/RadioRangeBallot.php +239)" source="SecurityCheck-DoubleEscaped"/>
    <error line="241" severity="warning" message="Calling method \Xml::radio() in \SecurePoll_RadioRangeBallot::getQuestionForm that outputs using tainted argument $[arg #4]. (Caused by: ../../includes/Xml.php +342) (Caused by: includes/ballots/RadioRangeBallot.php +222; includes/ballots/RadioRangeBallot.php +239)" source="SecurityCheck-DoubleEscaped"/>
    <error line="241" severity="warning" message="Calling method \Xml::radio() in \SecurePoll_RadioRangeBallot::getQuestionForm that outputs using tainted argument $score. (Caused by: ../../includes/Xml.php +342) (Caused by: includes/ballots/RadioRangeBallot.php +239)" source="SecurityCheck-DoubleEscaped"/>
    <error line="242" severity="warning" message="Calling method \Xml::radio() in \SecurePoll_RadioRangeBallot::getQuestionForm that outputs using tainted argument $[arg #4]. (Caused by: ../../includes/Xml.php +342) (Caused by: includes/ballots/RadioRangeBallot.php +222; includes/ballots/RadioRangeBallot.php +239)" source="SecurityCheck-DoubleEscaped"/>
    <error line="242" severity="warning" message="Calling method \Xml::radio() in \SecurePoll_RadioRangeBallot::getQuestionForm that outputs using tainted argument $score. (Caused by: ../../includes/Xml.php +342) (Caused by: includes/ballots/RadioRangeBallot.php +239)" source="SecurityCheck-DoubleEscaped"/>
  </file>
  <file name="includes/pages/VoterEligibilityPage.php">
    <error line="417" severity="warning" message="HTMLForm info field in raw mode needs to escape default key (Caused by: Builtin-\Html::rawElement; Builtin-\Html::rawElement; ../../includes/Html.php +210; Builtin-\Html::rawElement; Builtin-\Html::rawElement; includes/pages/VoterEligibilityPage.php +415; ../../includes/Html.php +304; ../../includes/Html.php +260; ../../includes/Html.php +304; ../../includes/Html.php +260)" source="SecurityCheck-XSS"/>
  </file>
</checkstyle>

After all is fixed the non-voting job extension-seccheck-non-voting should be changed to extension-seccheck

Details

ProjectBranchLines +/-Subject
integration/configmaster+1 -1layout: Seccheck back to voting for SecurePoll
mediawiki/extensions/SecurePollmaster+4 -3Make phan-taint-check plugin pass
mediawiki/extensions/SecurePollmaster+50 -6Suppress remaining taint-check issues
mediawiki/extensions/SecurePollmaster+4 -4Fixup phan stuff
Customize query in gerrit

Related Objects

Event Timeline

Reedy created this task.Oct 2 2018, 12:39 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 2 2018, 12:39 PM
Reedy added a comment.Oct 2 2018, 12:42 PM

So the first one is interesting...

It's complaining about https://github.com/wikimedia/mediawiki-extensions-SecurePoll/blob/master/cli/wm-scripts/bv2013/populateEditCount.php#L45-L55

Presumably specifically $beforeTime but it's definitely quoted above - https://github.com/wikimedia/mediawiki-extensions-SecurePoll/blob/master/cli/wm-scripts/bv2013/populateEditCount.php#L15

Reedy added a subscriber: Bawolff.Oct 2 2018, 12:53 PM
cscott added a subscriber: cscott.Oct 17 2018, 4:28 PM

I filed T207297: Phan SecurityCheck-XSS and SecurityCheck-SQLInjection errors in SecurePoll extension out of an abundance of caution, but it's probably a dup. Slightly different errors, though...

gerritbot added a subscriber: gerritbot.Oct 17 2018, 4:29 PM

Change 468015 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/SecurePoll@master] Fixup phan stuff

https://gerrit.wikimedia.org/r/468015

gerritbot added a project: Patch-For-Review.Oct 17 2018, 4:29 PM
sbassett added a subscriber: sbassett.Oct 17 2018, 7:30 PM

Seems like there's been a decent effort to get SecurePoll to play nicely w/ the seccheck plugin: T202365, https://gerrit.wikimedia.org/r/460211/

Reedy added a comment.Oct 17 2018, 8:00 PM
In T205972#4675386, @sbassett wrote:

Seems like there's been a decent effort to get SecurePoll to play nicely w/ the seccheck plugin: T202365, https://gerrit.wikimedia.org/r/460211/

That commit caused the problems in this ticket :)

Reedy merged a task: T207343: Phan failing on SecurePoll patches.Oct 17 2018, 10:47 PM
Reedy added a subscriber: Huji.
gerritbot added a comment.Oct 26 2018, 1:58 AM

Change 468015 merged by C. Scott Ananian:
[mediawiki/extensions/SecurePoll@master] Fixup phan stuff

https://gerrit.wikimedia.org/r/468015

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.2; 2018-10-30).Oct 26 2018, 2:00 AM
Huji added a subscriber: • Ladsgroup.Nov 6 2018, 7:11 AM

As evident from https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/SecurePoll/+/471556/ and https://integration.wikimedia.org/ci/job/mwext-php70-phan-seccheck-docker/24376/console this issue is persisting. Also adding @Ladsgroup

I really want to work on this myself, but T207344 prevents me. Any help here or there would be much appreciated.

• Ladsgroup added a comment.Nov 6 2018, 10:54 AM

I made {T208727}

gerritbot added a comment.Nov 10 2018, 1:10 PM

Change 472783 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/SecurePoll@master] Suppress remaining taint-check issues

https://gerrit.wikimedia.org/r/472783

gerritbot added a comment.Nov 10 2018, 6:14 PM

Change 472783 merged by jenkins-bot:
[mediawiki/extensions/SecurePoll@master] Suppress remaining taint-check issues

https://gerrit.wikimedia.org/r/472783

ReleaseTaggerBot edited projects, added MW-1.33-notes (1.33.0-wmf.4; 2018-11-13); removed MW-1.33-notes (1.33.0-wmf.2; 2018-10-30).Nov 10 2018, 7:00 PM
• Ladsgroup triaged this task as High priority.Nov 15 2018, 11:25 AM

Still errors and I can't merge anything.

Bawolff added a comment.Nov 15 2018, 12:17 PM

Did https://gerrit.wikimedia.org/r/#/c/integration/config/+/473724 to make it non-voting until kinks are worked out

Bawolff added a comment.Nov 15 2018, 12:45 PM

Well that's odd. My test change didn't trigger any errors: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/SecurePoll/+/473726/

Huji added a comment.Dec 3 2018, 7:33 PM

This continues to be an issue. See https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/476439/ as a recent example.

Umherirrender renamed this task from Fixup Phan errors in SecurePoll to Fixup taint-check-plugin errors in SecurePoll.Jun 1 2019, 7:28 AM
Umherirrender removed a project: Patch-For-Review.
Umherirrender updated the task description. (Show Details)
Daimona updated the task description. (Show Details)Jul 12 2019, 11:09 AM
Daimona added a subscriber: Daimona.

Updated with the output of seccheck 2.0.1.

gerritbot added a comment.Jul 14 2019, 1:51 PM

Change 522914 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/SecurePoll@master] Make phan-taint-check plugin pass

https://gerrit.wikimedia.org/r/522914

gerritbot added a project: Patch-For-Review.Jul 14 2019, 1:51 PM
gerritbot added a comment.Jul 14 2019, 2:24 PM

Change 522938 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[integration/config@master] layout: Seccheck back to voting for SecurePoll

https://gerrit.wikimedia.org/r/522938

Daimona claimed this task.Jul 14 2019, 2:28 PM

Note that I had to suppress some DoubleEscaped warnings: the label passed to Xml::radio is indeed double escaped. However, it comes from a long chain of calls to SecurePoll_Context/getMessage/parse etc., which seems to be copying what Message::parse does. And I'm not touching that chain.

gerritbot added a comment.Jul 15 2019, 3:34 PM

Change 522914 merged by jenkins-bot:
[mediawiki/extensions/SecurePoll@master] Make phan-taint-check plugin pass

https://gerrit.wikimedia.org/r/522914

gerritbot added a comment.Jul 15 2019, 3:37 PM

Change 522938 merged by jenkins-bot:
[integration/config@master] layout: Seccheck back to voting for SecurePoll

https://gerrit.wikimedia.org/r/522938

Stashbot added a comment.Jul 15 2019, 3:38 PM

Mentioned in SAL (#wikimedia-releng) [2019-07-15T15:38:39Z] <James_F> Zuul: [SecurePoll] Make seccheck voting T205972

Daimona closed this task as Resolved.Jul 15 2019, 3:39 PM
Daimona removed a project: Patch-For-Review.
ReleaseTaggerBot added a project: MW-1.34-notes (1.34.0-wmf.14; 2019-07-16).Jul 15 2019, 4:00 PM
sbassett moved this task from Backlog to Done on the phan-taint-check-plugin board.Oct 15 2019, 7:01 PM
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL