Page MenuHomePhabricator

VisualEditor: ve.ui.MWMediaSearchWidget fails on private wikis due to forced use of JSON-P (logged-out API)
Closed, ResolvedPublic1 Estimated Story Points

Description

Author: b.coughlan2

Description:
Tested on MW 1.22.6 with stable snapshot of VisualEditor, but looking at the master branches I'm quite sure that this still applies.

Steps to reproduce

  1. Set wiki to have read access only for users:

    $wgGroupPermissions['*']['read'] = false; $wgGroupPermissions['user']['read'] = true;

    Note that if testing on local environment, you will have to first open VE with the 'Edit' button, so that Parsoid is not locked out by this configuration.
  1. Open the 'Media' button in the VE toolbar and search for something.
  2. The AJAX request will return 'readapidenied', because it won't recognise the user credentials.

Cause

ve.ui.MWMediaSearchWidget.js makes a JSONP request to the localhost. However, MediaWiki's ApiMain.php:180 (https://git.wikimedia.org/blob/mediawiki%2Fcore.git/9db61c9ab58b11b639a1f95916b37b57530ec674/includes%2Fapi%2FApiMain.php#L180) will remove user credentials from JSONP requests for security reasons. Therefore, the user is treated as not being logged in and the 'readapidenied' message is returned.

Solution

There is already a TODO here from Trevor Parschal bf268e82:

// TODO: Only use JSON-P for cross-domain.
// jQuery has this logic built-in (if url is not same-origin ..)
// but isn't working for some reason.

However, I can't see anything in the jQuery $.ajax docs that says it will switch from JSONP to JSON for same-origin requests.

So if it's not automatic, the obvious fix is to add a function that checks for same-origin in the Javascript. But since this issue is hard to debug and may occur again for other extension developers, I think it would be better to patch the MW core so that it only strips user credentials for cross-origin JSONP requests ApiMain.php:180.

I didn't want to make a patch for this without checking about the security implications, and also to ask if there is an existing utility function in MediaWiki which checks if a request is same-origin?

Workaround

If you are only searching the private wiki and no external sources, a temporary workaround is to change "'datatype': 'jsonp'" to "'datatype': 'json'" in ve.ui.MWMediaSearchWidget.js


Version: unspecified
Severity: normal

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:12 AM
bzimport set Reference to bz64822.

There is most certainly logic in $.ajax to switch between xhr/json and script/jsonp. This has been in jQuery for many years.

The reason it isn't working might be related to the usage of protocol-relative urls, or the presence of CORS support.

To cite the relevant parts:

// A cross-domain request is in order when url parts mismatch
if ( s.crossDomain == null ) {

		parts = rurl.exec( s.url.toLowerCase() );
		s.crossDomain = !!( parts[ 1 ] !== ajaxLocParts[ 1 ] || .. );

}
..
jQuery.ajaxTransport( "script", function(s) {

		// This transport only deals with cross domain requests
		if ( s.crossDomain ) {
			..
		}

});
..
if ( xhrSupported ) { jQuery.ajaxTransport(function( s ) {

		if ( !s.crossDomain .. ) {
			return { send: function() {
				var xhr = s.xhr();
				..

..

Anyway, no work around is needed. It is a well-known feature of $.ajax, we just need to start using it. And if it isn't working, find out what we're doing wrong on our end.

b.coughlan2 wrote:

The crossDomain logic will switch between the 'script' and 'xhr' transports, but both requests will still have be JSONP and have the callback parameter.

See here: http://jsfiddle.net/yvzSL/228/

In your developer tools console you can see that the local request uses XHR, but it is still a JSONP request over XHR. I have read through the $.ajax docs a number of times now and I see no indication that it should behave otherwise.

b.coughlan2 wrote:

I did some more re-thinking about this.

I examined the jQuery code and the 'crossDomain' attribute is just for selecting the transport (XMLHttpRequest or <script> tag). It does not select data types (json/jsonp), i.e. if the datatype is 'jsonp' and the request is local, you will get a 'jsonp' request with a 'callback' parameter over XMLHttpRequest. The MW core will assume that any request with a 'callback' parameter is from another domain, and will treat the request as unauthenticated.

MW core needs a better way to detect if a request is really cross-origin. The solution is CORS, but this is not supported in a standard way on IE9 (http://caniuse.com/cors). The most "correct" fix would be to change the MW core to use CORS to detect cross-origin requests, but this might break plugins relying on the API in IE<=9, so it would be a risky fix until IE9 is no longer a concern for anyone.

I'm now convinced that the simplest fix for now is to add some logic in the VisualEditor JS to detect if the request will be local, based on the target URL of the request. Then use the 'json' datatype if it is local, and the 'jsonp' datatype if not. Here is jQuery's logic for checking that: https://github.com/jquery/jquery/blob/master/src/ajax.js#L518

Note that I tried this out by implementing custom prefilters/transports for jQuery (to leverage the crossDomain code already present), but this approach is hacky and a dead end.

I ran into the same problem myself my private wiki (1.23 and the REL1_23 branch of VE)

What are the security implications if I make the workaround change mentioned above? I have a foreign API configured, but it's one I control.

Thanks!
--Ryan

(In reply to Barry Coughlan from comment #2)

The crossDomain logic will switch between the 'script' and 'xhr' transports,
but both requests will still have be JSONP and have the callback parameter.

See here: http://jsfiddle.net/yvzSL/228/

In your developer tools console you can see that the local request uses XHR,
but it is still a JSONP request over XHR. I have read through the $.ajax
docs a number of times now and I see no indication that it should behave
otherwise.

You read wrong. &callback= (or rather s.jsonpCallback) is only applied when the "jsonp" transport is activated (builds on top of the "script" transport).

And the "json" dataType only activates "json" in case of crossDomain.

Making a plain api request to a non-crossdomain url does NOT result in a callback parameter. Instead it results in a plain JSON response that is parsed by JSON.parse, no callback or javascript execution comes into play.

MediaWiki core uses $.ajax in this in many places. For example mediawiki.user.js[1] uses it to fetch user rights and groups information from the API. This behaviour has been this way for quite a while.

Like I said, if VisualEditor and/or your wiki is doing it differently, there is most likely a bug in the code invoking $.ajax.

[1] https://github.com/wikimedia/mediawiki-core/blob/08c17086e1/resources/src/mediawiki/mediawiki.user.js#L27-L41

(In reply to Krinkle from comment #5)

[callback] is only applied when the "jsonp" transport is activated (builds
on top of the "script" transport).

And the "json" dataType only activates "json" in case of crossDomain.

I mean, the "json" dataType only activates "jsonp" in case of crossDomain.

(In reply to Barry Coughlan from comment #2)

The crossDomain logic will switch between the 'script' and 'xhr' transports,
but both requests will still have be JSONP and have the callback parameter.

See here: http://jsfiddle.net/yvzSL/228/

In your developer tools console you can see that the local request uses XHR,
but it is still a JSONP request over XHR. I have read through the $.ajax
docs a number of times now and I see no indication that it should behave
otherwise.

That example is broken. The first request is is malformed. "json jsonp" is not a valid dataType. This value is invalid and unsupported. It seems to default to a script tag instead, but as you can see the callback in that code never fires. Here's a forked version of that example where I added number "1" and "2". You'll find that "DONE 1" never appears.

http://jsfiddle.net/L5PtK/

(In reply to Krinkle from comment #7)

(In reply to Barry Coughlan from comment #2)

The crossDomain logic will switch between the 'script' and 'xhr' transports,
but both requests will still have be JSONP and have the callback parameter.

See here: http://jsfiddle.net/yvzSL/228/

In your developer tools console you can see that the local request uses XHR,
but it is still a JSONP request over XHR. I have read through the $.ajax
docs a number of times now and I see no indication that it should behave
otherwise.

That example is broken. The first request is is malformed. "json jsonp" is
not a valid dataType. This value is invalid and unsupported. It seems to
default to a script tag instead, but as you can see the callback in that
code never fires. Here's a forked version of that example where I added
number "1" and "2". You'll find that "DONE 1" never appears.

http://jsfiddle.net/L5PtK/

To clarify, I know dataType supports multiple values, but that's for converting the response (e.g. xml over jsonp). But you can't convert the actual data from json to jsonp or from json to jsonp (unless an API would embed javascript inside json or something like that).

As for VisualEditor, it should use json when contacting the local API. and jsonp when contacting any foreign API (whether on the same domain or not).

There is logic for this in jQuery that automatically switches from "json" to "jsonp" for cross domain urls.

However that doesn't trigger the way I expected. It makes the assumption that 1) you should only make 'json' requests to APIs on the same domain or with CORS authentication set up. Then if a browser doesn't support CORS it will use script with JSONP instead of XHR. So it functions like a fallback for browsers not supporting CORS, *not* as a convenience to switch to JSONP automatically for any cross domain url.

jquery.js

if ( !options.crossDomain || support.cors ) {

  • Bug 68141 has been marked as a duplicate of this bug. ***

Change 150054 had a related patch set uploaded by Mooeypoo:
Switch between json/jsonp for local/remote api search

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

Change 150054 merged by jenkins-bot:
MWMediaSearchWidget: Use json/jsonp for local/foreign api respectively

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

Change 217444 had a related patch set uploaded (by MarkAHershberger):
Fix check of JSON return value

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

MarkAHershberger subscribed.

This bug has popped up again because the check of config.local is wrong.

Krinkle set Security to None.

Change 217444 merged by jenkins-bot:
MWMediaResourceProvider: Use exist instead of bool check on API values

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

Change 223853 had a related patch set uploaded (by Jforrester):
MWMediaResourceProvider: Use exist instead of bool check on API values

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

Change 223853 merged by jenkins-bot:
MWMediaResourceProvider: Use exist instead of bool check on API values

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