Last modified: 2014-08-03 17:09:20 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T66822, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 64822 - VisualEditor: ve.ui.MWMediaSearchWidget fails on private wikis due to forced use of JSON-P (logged-out API)
VisualEditor: ve.ui.MWMediaSearchWidget fails on private wikis due to forced ...
Status: RESOLVED FIXED
Product: VisualEditor
Classification: Unclassified
Editing Tools (Other open bugs)
unspecified
All All
: High normal
: VE-deploy-2014-07-31
Assigned To: Moriel Schottlender
:
: 68141 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-04 12:46 UTC by Barry Coughlan
Modified: 2014-08-03 17:09 UTC (History)
9 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Barry Coughlan 2014-05-04 12:46:05 UTC
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.

2. Open the 'Media' button in the VE toolbar and search for something.
3. 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
Comment 1 Krinkle 2014-05-04 23:03:34 UTC
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.
Comment 2 Barry Coughlan 2014-05-06 11:07:01 UTC
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.
Comment 3 Barry Coughlan 2014-06-03 04:41:36 UTC
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.
Comment 4 ryan.glasnapp 2014-06-17 19:05:41 UTC
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
Comment 5 Krinkle 2014-07-28 21:13:27 UTC
(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
Comment 6 Krinkle 2014-07-28 21:14:45 UTC
(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.
Comment 7 Krinkle 2014-07-28 21:16:48 UTC
(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/
Comment 8 Krinkle 2014-07-28 21:30:39 UTC
(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 ) {
Comment 9 James Forrester 2014-07-28 21:52:48 UTC
*** Bug 68141 has been marked as a duplicate of this bug. ***
Comment 10 Gerrit Notification Bot 2014-07-28 22:03:54 UTC
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
Comment 11 Gerrit Notification Bot 2014-07-30 17:13:44 UTC
Change 150054 merged by jenkins-bot:
MWMediaSearchWidget: Use json/jsonp for local/foreign api respectively

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

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links