Page MenuHomePhabricator

Flow: Update to stop using buildCssLinks and for related ConfirmEdit change
Closed, ResolvedPublic

Description

Per T87871#2464320, it's no longer practical to expose partial html snippets for <head>. Anything related to module loading should be loaded as a whole because we want to manage state across it. And this breaks if callers call them in the wrong order or if not all of them are called.

In order to ensure dependencies are not loaded twice, we also need to know which style modules are loaded before loading script modules, as such, there is basically no way to expose a partial like buildCssLinks. I want to deprecate and remove these methods. None of the four partials are called anywhere, except buildCssLinks and getScriptsForBottomQueue, these have one external caller: Flow.

https://github.com/wikimedia/mediawiki-extensions-Flow/blob/3df503188c/includes/SpamFilter/ConfirmEdit.php#L44-L45

			$html = $wgOut->buildCssLinks() .
				$wgOut->getScriptsForBottomQueue( true ) .
				$html;

Can someone figure out whether this is still used, whether it still works, and how it can be done instead? It's seems unlikely to me that this is working properly. Where does this HTML end up being sent to? How can the client render that partial in a meaningful way?

Event Timeline

Krinkle created this task.Jul 15 2016, 2:26 PM
Restricted Application added a subscriber: Zppix. · View Herald TranscriptJul 15 2016, 2:26 PM

This currently blocks https://gerrit.wikimedia.org/r/299148 and completion of T87871.

To reproduce the current state:

  1. Set up Flow and ConfirmEdit, with ConfirmEdit configured to use FancyCaptcha (or you can do the "current state" test in production at https://www.mediawiki.org/wiki/Talk:Sandbox). For MediaWiki-Vagrant, you can enable the 'flow' and 'confirmedit' roles (I don't think wikimediaflow is required).
  2. Go to a Flow page logged out.
  3. Post a new topic with any title, and a body containing a URL, e.g. http://example.com .
  4. It will show a CAPTCHA message, with associated JS and CSS. Both are added to the <body> and the CSS does apply. The CAPTCHA also works.

The JS line is:

<script>(window.RLQ=window.RLQ||[]).push(function(){mw.loader.state({"user":"ready"});mw.loader.load(["ext.confirmEdit.fancyCaptcha"]);});</script>

If I just remove the buildCssLinks and getScriptsForBottomQueue calls (leaving only the getForm HTML itself), the JS and CSS don't load and there is a visible difference:

As shown by @Mattflaschen-WMF, SpamFilter::validate() is used when saving content through the API. E.g. when creating a new topic. This is essentially the same as what EditPage and VisualEditor do when saving an edit.

The underlying ConfirmEdit extension expects access to an OutputPage. It outputs a Form (which takes OutputPage as argument). The form serialises to some amount of HTML, and it also uses OutputPage to load a JavaScript module (otherwise the Refresh button and potentially more wouldn't work).

Current code:

$html = $captcha->getForm( $context->getOutput() );
return $wgOut->buildCssLinks()
	. $wgOut->getScriptsForBottomQueue( true )
	 . $html;

This HTML is transferred as the message of a Status object, and inserted by JavaScript. Example:

"Please confirm you are a human by solving the below captcha:
 <script>(window.RLQ=window.RLQ||[]).push(function(){
  mw.loader.state({"ext.confirmEdit.fancyCaptcha.styles":"ready","mediawiki.legacy.commonPrint":"ready","mediawiki.legacy.shared":"ready","mediawiki.sectionAnchor":"ready","mediawiki.skinning.interface":"ready","skins.vector.styles":"ready","site.styles":"ready","noscript":"ready","user.cssprefs":"ready"});});</script>
<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=ext.confirmEdit.fancyCaptcha.styles%7Cmediawiki.legacy.commonPrint%2Cshared%7Cmediawiki.sectionAnchor%7Cmediawiki.skinning.interface%7Cskins.vector.styles&amp;only=styles&amp;skin=vector"/>
<meta name="ResourceLoaderDynamicStyles" content=""/>
<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=site.styles&amp;only=styles&amp;skin=vector"/>
<script>(window.RLQ=window.RLQ||[]).push(function(){
  mw.loader.state({"user":"ready"});mw.loader.load(["ext.confirmEdit.fancyCaptcha"]);});</script>
<div><label for="wpCaptchaWord">CAPTCHA Security check</label><div class="fancycaptcha-captcha-container"><div class="fancycaptcha-captcha-and-reload"><div class="fancycaptcha-image-container"><img class="fancycaptcha-image" src="/w/index.php?title=Special:Captcha/image&amp;wpCaptchaId=991107306" alt=""/><small class="confirmedit-captcha-reload fancycaptcha-reload">Refresh</small></div></div>
<input name="wpCaptchaWord" class="mw-ui-input" id="wpCaptchaWord" size="12" autocomplete="off" autocorrect="off" autocapitalize="off" tabindex="1" placeholder="Enter the text you see on the image"/><input type="hidden" name="wpCaptchaId" id="wpCaptchaId" value="991107306"/></div></div>
"

OutputPage::buildCssLinks() and getScriptsForBottomQueue() return both too much and too little for this to be reliable. These methods also contain a lot of default stuff for the current Skin and MediaWiki in general which are loaded a second time. Potentially damaging the cascading order or causing unrelated code to execute in an unexpected way. It loads the Vector stylesheet a second time, and produces meta ResourceLoaderDynamicStyles of which there is only supposed to be one on any page.

This presumably causes issues if you are on a page with a different skin (e.g. useskin=monobook) and get another Vector stylesheet.

For VisualEditor, handling was hardcoded in ve.init.mw.ArticleTarget.js. This would probably be a good fit for Flow as well.

Mattflaschen-WMF renamed this task from Figure out Flow's use of OutputPage::buildCssLinks() to Flow: Update to stop using buildCssLinks and for related ConfirmEdit change.Jul 27 2016, 2:28 AM
Mattflaschen-WMF claimed this task.

Change 301897 had a related patch set uploaded (by Mattflaschen):
ConfirmEdit: Stop using buildCssLinks, adapt to ConfirmEdit refactor

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

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptAug 2 2016, 2:05 AM

Change 301897 merged by jenkins-bot:
ConfirmEdit: Stop using buildCssLinks, adapt to ConfirmEdit refactor

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

Re-check after 1.28.0-wmf.14 deployment.

In 1.28.0-wmf.13 (https://www.mediawiki.org/wiki/Talk:Sandbox), the captcha is not dismissed after 'Cancel' - needs to be rechecked in wmf.14

Re-checked in 1.28.0-wmf.14 (ba1c692)- Flow spam filter warning is displayed. instead of captcha.

Re-checked in 1.28.0-wmf.14 (ba1c692)- Flow spam filter warning is displayed. instead of captcha.

For future reference: You have to use a longer message (try making it sound like a real sentence) to bypass that error (unrelated error, from an AbuseFilter anti-spam rule).

jmatazzoni closed this task as Resolved.Aug 19 2016, 12:10 AM