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

Related Gerrit Patches:

integration/config : master	layout: Seccheck back to voting for SecurePoll
mediawiki/extensions/SecurePoll : master	Make phan-taint-check plugin pass
mediawiki/extensions/SecurePoll : master	Suppress remaining taint-check issues
mediawiki/extensions/SecurePoll : master	Fixup phan stuff

Related Objects

Mentioned Here: T207344: Phan-taint-check-plugin not available for PHP > 7.0
T202365: Add phan-taint-check-plugin to SecurePoll extension
T207297: Phan SecurityCheck-XSS and SecurityCheck-SQLInjection errors in SecurePoll extension

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

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

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...

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

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/

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.

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

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.

I made {T208727}

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

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

Still errors and I can't merge anything.

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

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

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)

Updated with the output of seccheck 2.0.1.

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

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

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.

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

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

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

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

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