Page MenuHomePhabricator

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

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 : masterlayout: Seccheck back to voting for SecurePoll
mediawiki/extensions/SecurePoll : masterMake phan-taint-check plugin pass
mediawiki/extensions/SecurePoll : masterSuppress remaining taint-check issues
mediawiki/extensions/SecurePoll : masterFixup phan stuff

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

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

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

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 :)

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

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

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.

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

Ladsgroup triaged this task as High priority.Nov 15 2018, 11:25 AM

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

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.

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

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.

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.