Page MenuHomePhabricator

QuickSurveys throws exception on non-HTTPS
Closed, ResolvedPublic

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

Related Objects

Event Timeline

demon created this task.Oct 2 2015, 3:11 PM
demon raised the priority of this task from to Normal.
demon updated the task description. (Show Details)
demon added a subscriber: demon.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 2 2015, 3:11 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.
hashar added subscribers: hashar, rmoen, Jdlrobson.EditedOct 3 2015, 7:55 AM

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

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

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

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

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

phuedx added a subscriber: phuedx.EditedOct 9 2015, 9:16 AM

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.

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.

demon added a comment.Oct 9 2015, 5:15 PM

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

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

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

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

Change 245140 abandoned by Jdlrobson:
Make HTTPS requirement configurable.

Reason:
Let's continue conversation at https://gerrit.wikimedia.org/r/246251

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

Change 244623 abandoned by Phuedx:
Die gracefully when survey throws exception

Reason:
None from me.

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

Change 245510 merged by jenkins-bot:
Send insecure surveys to client but do not allow rendering of them.

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

Jdlrobson closed this task as Resolved.Oct 28 2015, 10:39 PM