wp_send_json to allow for JSONP

[sc0ttkclark]

// Current
function wp_send_json( $response ) {
function wp_send_json_success( $data = null ) {
function wp_send_json_error( $data = null ) {

// Proposed
function wp_send_json( $response, $callback = null ) {
function wp_send_json_success( $data = null, $callback = null ) {
function wp_send_json_error( $data = null, $callback = null ) {

The wp_send_jsonp and related functions could take another second parameter for $callback. It could default to null and if !== null then it could output the JSON in a JSONP compatible way:

if ( null !== $callback ) {
	@header( 'Content-Type: application/javascript; charset=' . get_option( 'blog_charset' ) );
	echo esc_js( $callback ) . '(' . json_encode( $response ) . ')';
}
else {
	@header( 'Content-Type: application/json; charset=' . get_option( 'blog_charset' ) );
	echo json_encode( $response );
}

The related functions would also get a second parameter for $callback in them, which would default to null and pass directly into wp_send_json:

$callback = 'my_js_callback'; // This can alternatively come from the $.ajax request 'callback' parameter

if ( is_wp_error( $response ) ) {
	wp_send_json_error( $response->get_error_message(), $callback );
}
else {
	wp_send_json_error( $response, $callback );
}

This is handy for when using $.ajax to make JSONP requests. Currently, wp_send_json won't work with JSONP requests, but the solution as outlined above would cause no bugs or impact beyond a parameter being added and the two alternate lines above.

[modemlooper]

+1

[sc0ttkclark]

JSON REST API supports JSONP, and requires a way to send JSON vs JSONP responses.

Currently they have to roll their own JSON vs JSONP response handling for this, you can see where it handles the responses here:

​https://github.com/WP-API/WP-API/blob/master/lib/class-wp-json-server.php#L182

Adding JSONP to wp_send_json would allow them to remove a few lines of code and the benefits would be that any plugin / theme developer could then roll their own JSONP responses too.

So it seems to me, that as the new WP API is on track for eventual merge into core, this would help simplify at least a small fraction of it's inclusion.

[georgestephanis]

I guess part of me wonders if this would inadvertently open up existing custom endpoints to cross-domain requests that it's not expecting?

[sc0ttkclark]

Replying to georgestephanis:

I guess part of me wonders if this would inadvertently open up existing custom endpoints to cross-domain requests that it's not expecting?

If callback is a new parameter not currently used by any existing endpoints using wp_send_json, would it?

[georgestephanis]

Replying to sc0ttkclark:

If callback is a new parameter not currently used by any existing endpoints using wp_send_json, would it?

I mean, custom endpoints that use wp_send_json would instantly start working cross-domain. They could already be abused via shell scripts and the like, but with the callback, any other website could cross-domain hijack your cookies / authentication to pull data that they shouldn't be able to.

[sc0ttkclark]

If the custom endpoint is coded for that, and origin policies have been overridden, that could happen -- see allowed_http_origins and allowed_http_origin filters.

I don't suspect that'll be much of a problem since lamen won't be using JSONP and callbacks, right?

Also my code in the initial description can be improved further:

$content_type = 'application/json';
$json_response = json_encode( $response );

// JSONP callback support
if ( null !== $callback ) {
	// JSONP requires content type of application/javascript
	$content_type = 'application/javascript';

	// JSONP uses callback for response
	$json_response = esc_js( $callback ) . '(' . json_response . ')';
}

@header( 'Content-Type: ' . $content_type . '; charset=' . get_option( 'blog_charset' ) );
echo $json_response;

[georgestephanis]

For reference, proof of concept that automatically allowing JSONP would turn into an exploitable data leakage -- ​https://gist.github.com/georgestephanis/c8bdd3079b0cfba85067 (mentioned it to Scott on Twitter)

[sc0ttkclark]

JSON REST API also has a way to enable/disable JSONP, so it's possible we could check for a constant here for that same purpose, so it could be disabled by default or enabled by default, however the core team sees fit.

[sc0ttkclark]

Replying to georgestephanis:

For reference, proof of concept that automatically allowing JSONP would turn into an exploitable data leakage -- ​https://gist.github.com/georgestephanis/c8bdd3079b0cfba85067 (mentioned it to Scott on Twitter)

My earlier reply applies to that, I also replied on the gist directly too.

[georgestephanis]

The comparison against allowed_http_origins is done via get_http_origin() which relies on the HTTP_ORIGIN header -- which by my understanding is unreliable at best, and not reliably supported cross-browser. (happy to be wrong, just based off my cursory glance)

My entire thrust on all of this is that turning on JSONP for all existing uses of wp_send_json() could (I think) potentially open up data leakage from existing endpoints. If it's something that folks have to opt in to when using the wp_send_json() function, whether by constant, filter, or argument, I'm totally fine with it.

[sc0ttkclark]

This ticket is to request an additional $callback parameter that must explicitly be passed into the wp_send_json / wp_send_json_success / wp_send_json_error functions. An additional constant check could be utilized, which could default JSONP support to on or off.

My proposal would not affect *any* existing endpoints and break anything. It would merely add support for JSONP when explicitly requested by *new* code.

[georgestephanis]

Awesome! Didn't see that in my first reading, hence my

I guess part of me wonders if this would inadvertently open up existing custom endpoints to cross-domain requests that it's not expecting?

Ignore me then!

[sc0ttkclark]

Replying to georgestephanis:

Ignore me then!

No way, that's great feedback you've provided and these are things people absolutely need to remember when using things like JSONP and cross-domain vulnerabilities.

[rmccue]

Replying to sc0ttkclark:

So it seems to me, that as the new WP API is on track for eventual merge into core, this would help simplify at least a small fraction of it's inclusion.

FWIW, since wp_send_json also exits (that is, calls exit/die), it's not suitable for use in the API. The API is explicitly designed to be re-entrant, and as such, there's only a single use of exit in the plugin. The use of exit is also separated from the JSON handling, so we couldn't do a simple refactor to change this.

That said, a common function to check callbacks would be nice. Here's the code we have at the moment:

$jsonp_enabled = apply_filters( 'json_jsonp_enabled', true );

if ( isset( $_GET['_jsonp'] ) ) {
	if ( ! $jsonp_enabled ) {
		echo $this->json_error( 'json_callback_disabled', __( 'JSONP support is disabled on this site.' ), 400 );
		return false;
	}

	// Check for invalid characters (only alphanumeric allowed)
	if ( preg_match( '/\W/', $_GET['_jsonp'] ) ) {
		echo $this->json_error( 'json_callback_invalid', __( 'The JSONP callback function is invalid.' ), 400 );
		return false;
	}
}

// ...

if ( isset( $_GET['_jsonp'] ) ) {
	echo $_GET['_jsonp'] . '(' . $result . ')';
} else {
	echo $result;
}

[rmccue]

Replying to georgestephanis:

The comparison against allowed_http_origins is done via get_http_origin() which relies on the HTTP_ORIGIN header -- which by my understanding is unreliable at best, and not reliably supported cross-browser. (happy to be wrong, just based off my cursory glance)

Supported in IE 8+ (10+ for full support), Chrome 4+, Firefox 3.5+, Opera 12+ and Safari 4+. (​caniuse, ​MDN).

That said, there are potential security issues of allowing users to do this; callbacks need to be properly sanitized and checked. Allowing wp_send_json to include this callback argument while also checking it (with the aforementioned function) would be the best scenario, IMO.

[sc0ttkclark]

Would still love to see JSONP support added to wp_send_json, and we could indeed abstract the callback verification ala Nonce checking via wp_verify_json_callback or similar function, which WP API could then utilize too. It would be great to stop wp_send_json from exiting, so WP API could use it for it's purposes and since it's on it's way into core anyways, but if we have to duplicate code then it's not really too bad once we at least split off the callback validation of input.

[nacin]

wp_send_json_success(), wp_send_json_error(), and wp_send_json() are designed to "do one thing only", that is admin-ajax.php + our JavaScript utility wp.ajax. Happy to reconsider, but I think this is a particular use case and not something that needs more things stuffed into it.

[adamsilverstein]

Replying to nacin:

wp_send_json_success(), wp_send_json_error(), and wp_send_json() are designed to "do one thing only", that is admin-ajax.php + our JavaScript utility wp.ajax. Happy to reconsider, but I think this is a particular use case and not something that needs more things stuffed into it.

I like the idea of supporting JSONP in core, perhaps better than stuffing the functionality into the existing json calls, we could add a new set of calls: wp_send_jsonp(), etc...

[sc0ttkclark]

I will support any solution that gets JSONP support into core, mirroring wp_send_json as wp_send_jsonp is fine by me! Just thought that the use case WP API used by combining the two was a better approach. But given it's need for callback handling and sanitization, perhaps it's own function is better merited.

[rmccue]

Replying to chriscct7:

I don't think this is actually part of the REST API component, as it's not used by the API, but rather by people using admin-ajax and the like. There's code we can share, but I think Administration would be a better component?

[rmccue]

Abstract JSONP validation to allow reuse

[rmccue]

Added patch to split the REST API JSONP callback validation code into a separate function, but nothing else. This allows plugins to use the built-in validation and is really just good practice anyway. I agree with @nacin that we don't really need a function to send JSONP in core (especially with the API infrastructure in).

Extremely Important Note: If you send JSONP in your custom response, make sure you prefix the response with /**/. This will mitigate the ​Rosetta Flash exploit. You should also send the X-Content-Type-Options: nosniff header, or even better, use the REST API infrastructure.

With it split out, we should be able to unit test this now too.

[rmccue]

Add unit tests

[rmccue]

Added patch including unit tests.

[rachelbaker]

Latest patch 28523.2.diff​ looks good. Next step is a commit.

[rachelbaker]

@rmccue Yo!

[rachelbaker]

In 37646:

REST API: Create the general wp_check_jsonp_callback() function for validating JSONP callback functions.

Move the REST API JSONP callback validation check into a separate function named wp_check_jsonp_callback(). This allows plugins to use the built-in validation when handling JSONP callbacks.
Extremely Important Note: If you send JSONP in your custom response, make sure you prefix the response with /**/. This will mitigate the Rosetta Flash exploit. You should also send the X-Content-Type-Options:nosniff header, or even better, use the REST API infrastructure.

Props rmccue.
Fixes #28523.