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

Event Timeline

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/

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

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.

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

Still errors and I can't merge anything.

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

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