QuickSurveys throws exception on non-HTTPS
Closed, ResolvedPublic
Actions

Description

Spotted in beta:

Exception encountered, of type "InvalidArgumentException"
[23f2ff5a] /wiki/Special:RecentChanges?uselang=qqx InvalidArgumentException from line 71 of /srv/mediawiki/php-master/extensions/QuickSurveys/includes/SurveyFactory.php: The "external example survey" external survey link requires https.
Backtrace:
#0 /srv/mediawiki/php-master/extensions/QuickSurveys/includes/SurveyFactory.php(60): QuickSurveys\SurveyFactory::factoryExternal(array)
#1 [internal function]: QuickSurveys\SurveyFactory::factory(array)
#2 /srv/mediawiki/php-master/extensions/QuickSurveys/includes/QuickSurveys.hooks.php(96): array_map(string, array)
#3 /srv/mediawiki/php-master/extensions/QuickSurveys/includes/QuickSurveys.hooks.php(69): QuickSurveys\Hooks::getEnabledSurveys()
#4 /srv/mediawiki/php-master/includes/Hooks.php(204): QuickSurveys\Hooks::onResourceLoaderRegisterModules(ResourceLoader)
#5 /srv/mediawiki/php-master/includes/resourceloader/ResourceLoader.php(290): Hooks::run(string, array)
#6 /srv/mediawiki/php-master/includes/OutputPage.php(2759): ResourceLoader->__construct(GlobalVarConfig, Monolog\Logger)
#7 /srv/mediawiki/php-master/includes/OutputPage.php(538): OutputPage->getResourceLoader()
#8 /srv/mediawiki/php-master/includes/OutputPage.php(564): OutputPage->filterModules(array, string)
#9 /srv/mediawiki/php-master/includes/OutputPage.php(588): OutputPage->getModules(boolean, string, string)
#10 /srv/mediawiki/php-master/includes/OutputPage.php(3070): OutputPage->getModuleScripts(boolean, string)
#11 /srv/mediawiki/php-master/includes/OutputPage.php(3144): OutputPage->getScriptsForBottomQueue()
#12 /srv/mediawiki/php-master/includes/skins/Skin.php(619): OutputPage->getBottomScripts()
#13 /srv/mediawiki/php-master/includes/skins/SkinTemplate.php(427): Skin->bottomScripts()
#14 /srv/mediawiki/php-master/includes/skins/SkinTemplate.php(240): SkinTemplate->prepareQuickTemplate()
#15 /srv/mediawiki/php-master/includes/OutputPage.php(2314): SkinTemplate->outputPage()
#16 /srv/mediawiki/php-master/includes/MediaWiki.php(698): OutputPage->output()
#17 /srv/mediawiki/php-master/includes/MediaWiki.php(476): MediaWiki->main()
#18 /srv/mediawiki/php-master/index.php(41): MediaWiki->run()
#19 /srv/mediawiki/w/index.php(3): include(string)
#20 {main}

Reproduce with http://en.m.wikipedia.beta.wmflabs.org/w/index.php?title=Special:UserLogin&uselang=qqx

Details

Subject	Repo	Branch	Lines +/-
Send insecure surveys to client but do not allow rendering of them.	mediawiki/extensions/QuickSurveys	dev	+28 -12
Die gracefully when survey throws exception	mediawiki/extensions/QuickSurveys	dev	+8 -1
Make HTTPS requirement configurable.	mediawiki/extensions/QuickSurveys	master	+0 -0
Release 0.4.0 of QuickSurveys	mediawiki/extensions/QuickSurveys	master	+5 -1
Don't require QuickSurveys to use HTTPS links in labs	operations/mediawiki-config	master	+2 -0
Make HTTPS requirement configurable.	mediawiki/extensions/QuickSurveys	dev	+7 -1

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	leila	T112326 [Epic] Reader segmentation research - Q2
Resolved	• jhobs	T113443 First QuickSurvey for reader segmentation research - external survey
Resolved	Jdlrobson	T111445 GOAL: Run our first survey campaign
Resolved	• jhobs	T110661 Deploy QuickSurveys extension to the English Wikipedia
Resolved	Jdlrobson	T114485 QuickSurveys throws exception on non-HTTPS

Event Timeline

• demon created this task.Oct 2 2015, 3:11 PM

• demon raised the priority of this task from to Medium.

• demon updated the task description. (Show Details)

• demon added projects: Wikimedia-production-error, QuickSurveys.

• demon subscribed.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 2 2015, 3:11 PM

• demon moved this task from Untriaged to Dec2019/1.35.wmf.10+ on the Wikimedia-production-error board.Oct 2 2015, 3:11 PM

• demon added a project: Beta-Cluster-Infrastructure.Oct 2 2015, 3:14 PM

Restricted Application added a subscriber: Luke081515. · View Herald TranscriptOct 2 2015, 3:14 PM

hashar updated the task description. (Show Details)Oct 3 2015, 7:51 AM

hashar added a project: I18n.

hashar set Security to None.

Caused by QuickSurveys patch https://gerrit.wikimedia.org/r/#/c/239993/

Rob/Jon, there is no HTTPS on beta cluster. Maybe the $wgQuickSurveys values can instead be validated to be https as a test in operations/mediawiki-config.git ?

I am also wondering why it triggers with uselang=qqx

hashar moved this task from To Triage to Backlog on the Beta-Cluster-Infrastructure board.Oct 3 2015, 7:56 AM

Requiring HTTPS or not should be something you configure as an extension global.

greg mentioned this in T114486: Create #beta-cluster-reproducible tag and rename #beta-cluster to #beta-cluster-infrastructure.Oct 4 2015, 2:59 AM

Hopefully this is what is causing T113534 ... and we can finally get that resolved too.

greg edited projects, added Beta-Cluster-reproducible; removed Beta-Cluster-Infrastructure.Oct 5 2015, 4:37 PM

Jdlrobson added a project: Reading-Web-Sprint-57-The Fifth Element.Oct 8 2015, 4:25 PM

Jdlrobson moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-57-The Fifth Element board.Oct 8 2015, 5:10 PM

• jhobs claimed this task.Oct 8 2015, 11:04 PM

I can replicate on http://en.m.wikipedia.beta.wmflabs.org/wiki/Headings?uselang=qqx&quicksurvey=external-survey-external%20example%20survey

Change 244623 had a related patch set uploaded (by Jhobs):
Die gracefully when survey throws exception

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

gerritbot added a project: Patch-For-Review.Oct 8 2015, 11:33 PM

• jhobs moved this task from To Do to Doing on the Reading-Web-Sprint-57-The Fifth Element board.Oct 8 2015, 11:44 PM

• jhobs moved this task from Doing to Code Review on the Reading-Web-Sprint-57-The Fifth Element board.

Requiring HTTPS or not should be something you configure as an extension global.

I agree with @Krenair.

Trapping an exception, discarding it, and then discarding all of the configured surveys doesn't seem graceful. Rather, we could simply drop the HTTPS requirement on the Beta Cluster.

phuedx moved this task from Code Review to -1 (Needs More Work) on the Reading-Web-Sprint-57-The Fifth Element board.Oct 9 2015, 9:16 AM

So this was actually requested by @csteipp during security review - eternal surveys should he https.

The problem is that these URLs come from messages or config variables so anyone editing an interface could bring the whole site down for everyone in production. That's not good at all. The qqx code also renders the message key which is not even a url.

I'm keen to solve issue before this hits production. If we don't die silently what other options do we have?

It seems like we'd want to throw it away and throw a warning so some try catching is necessary...

I'd like to prevent non https links in production. If it's a config flag
that is false in beta, that would be fine.

@csteipp but how should we treat surveys which have been setup with 'http' would you expect this to fatal, to throw a warning or die silently or other? (in all cases I assume we would not allow the survey)

@Jdlrobson, I don't have a strong preference except that, like you say, we
don't allow the survey if it's not over https.

In general, I think gracefully showing a prominent warning would be the
best UX. Not sure if that's worth the dev cost over just throwing an
exception and responding whenever releng bugs you.

What about not configuring surveys for beta? Blank the message?

We need surveys configured for browser tests... Right now the only issue we are hitting is qqx code doesn't translate to a URL. We can work around that edge case, but I'm curious what we expect to happen with a badly configured survey whilst we do not have an interface for setting up surveys...

My feeling is we should do this on the frontend and log a warning in the JavaScript console when choosing a survey to show. This seems the best thing here. No need to fatal nor do this in PHP.

Change 245105 had a related patch set uploaded (by Alex Monk):
Make HTTPS requirement configurable.

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

Change 245140 had a related patch set uploaded (by Alex Monk):
Make HTTPS requirement configurable.

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

Change 245140 abandoned by Alex Monk:
Make HTTPS requirement configurable.

Reason:
Repo setup here is a bit weird... seems this has to go through the dev branch first

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

Change 245105 merged by jenkins-bot:
Make HTTPS requirement configurable.

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

Change 245140 restored by Alex Monk:
Make HTTPS requirement configurable.

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

Change 245140 had a related patch set uploaded (by Alex Monk):
Make HTTPS requirement configurable.

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

Change 245188 had a related patch set uploaded (by Alex Monk):
Don't require QuickSurveys to use HTTPS links in labs

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

Change 245188 merged by jenkins-bot:
Don't require QuickSurveys to use HTTPS links in labs

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

Thanks @Krenair!

phuedx moved this task from Ready for Signoff to Doing on the Reading-Web-Sprint-57-The Fifth Element board.Oct 12 2015, 12:24 PM

Change 245473 had a related patch set uploaded (by Phuedx):
Release 0.4.0 of QuickSurveys

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

phuedx moved this task from Doing to Code Review on the Reading-Web-Sprint-57-The Fifth Element board.Oct 12 2015, 12:42 PM

This is only a temporary fix.

This validation should still not be done in PHP and in current form this could blow up production where https surveys are enforced so more work must be done here.

Personally I dislike the config option as beta labs should mirror production as much as possible and in current form it doesn't. When fixing this we should also remove the config option.

Jdlrobson removed • jhobs as the assignee of this task.Oct 12 2015, 4:00 PM

Jdlrobson added a parent task: T110661: Deploy QuickSurveys extension to the English Wikipedia.

Jdlrobson moved this task from Code Review to To Do on the Reading-Web-Sprint-57-The Fifth Element board.

Jdlrobson claimed this task.Oct 12 2015, 6:01 PM

Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-57-The Fifth Element board.

Change 245473 merged by jenkins-bot:
Release 0.4.0 of QuickSurveys

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

Change 245510 had a related patch set uploaded (by Jdlrobson):
Assert that external surveys are in https in JavaScript

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

gerritbot added a project: Patch-For-Review.Oct 12 2015, 6:16 PM

Jdlrobson moved this task from Doing to Code Review on the Reading-Web-Sprint-57-The Fifth Element board.Oct 12 2015, 6:16 PM

phuedx mentioned this in T113534: Investigate QuickSurveys browser tests failures.Oct 13 2015, 2:02 PM