Ajax: Drop the json to jsonp auto-promotion logic

mgol · web-flow · commit e7b3bc488d01 · 2020-07-27T19:15:57.000+02:00
Previously, `jQuery.ajax` with `dataType: 'json'` with a provided callback was automatically converted to a jsonp request unless one also specified `jsonp: false`. Today the preferred way of interacting with a cross-domain backend is CORS which works in all browsers jQuery 4 will support. Auto-promoting JSON requests to JSONP ones introduces a security issue as the developer may be unaware they're not just downloading data but executing code from a remote domain. This commit disables the auto-promoting logic. BREAKING CHANGE: to trigger a JSONP request, it's now required to specify `dataType: "jsonp"`; previously some requests with `dataType: "json"` were auto-promoted to JSONP. Fixes gh-1799 Fixes gh-3376 Closes gh-4754
diff --git a/src/ajax/jsonp.js b/src/ajax/jsonp.js
@@ -18,7 +18,7 @@ jQuery.ajaxSetup( {
 } );
 
 // Detect, normalize options and install callbacks for jsonp requests
-jQuery.ajaxPrefilter( "json jsonp", function( s, originalSettings, jqXHR ) {
+jQuery.ajaxPrefilter( "jsonp", function( s, originalSettings, jqXHR ) {
 
 	var callbackName, overwritten, responseContainer,
 		jsonProp = s.jsonp !== false && ( rjsonp.test( s.url ) ?
@@ -29,69 +29,65 @@ jQuery.ajaxPrefilter( "json jsonp", function( s, originalSettings, jqXHR ) {
 				rjsonp.test( s.data ) && "data"
 		);
 
-	// Handle iff the expected data type is "jsonp" or we have a parameter to set
-	if ( jsonProp || s.dataTypes[ 0 ] === "jsonp" ) {
+	// Get callback name, remembering preexisting value associated with it
+	callbackName = s.jsonpCallback = typeof s.jsonpCallback === "function" ?
+		s.jsonpCallback() :
+		s.jsonpCallback;
 
-		// Get callback name, remembering preexisting value associated with it
-		callbackName = s.jsonpCallback = typeof s.jsonpCallback === "function" ?
-			s.jsonpCallback() :
-			s.jsonpCallback;
+	// Insert callback into url or form data
+	if ( jsonProp ) {
+		s[ jsonProp ] = s[ jsonProp ].replace( rjsonp, "$1" + callbackName );
+	} else if ( s.jsonp !== false ) {
+		s.url += ( rquery.test( s.url ) ? "&" : "?" ) + s.jsonp + "=" + callbackName;
+	}
 
-		// Insert callback into url or form data
-		if ( jsonProp ) {
-			s[ jsonProp ] = s[ jsonProp ].replace( rjsonp, "$1" + callbackName );
-		} else if ( s.jsonp !== false ) {
-			s.url += ( rquery.test( s.url ) ? "&" : "?" ) + s.jsonp + "=" + callbackName;
+	// Use data converter to retrieve json after script execution
+	s.converters[ "script json" ] = function() {
+		if ( !responseContainer ) {
+			jQuery.error( callbackName + " was not called" );
 		}
+		return responseContainer[ 0 ];
+	};
 
-		// Use data converter to retrieve json after script execution
-		s.converters[ "script json" ] = function() {
-			if ( !responseContainer ) {
-				jQuery.error( callbackName + " was not called" );
-			}
-			return responseContainer[ 0 ];
-		};
-
-		// Force json dataType
-		s.dataTypes[ 0 ] = "json";
+	// Force json dataType
+	s.dataTypes[ 0 ] = "json";
 
-		// Install callback
-		overwritten = window[ callbackName ];
-		window[ callbackName ] = function() {
-			responseContainer = arguments;
-		};
+	// Install callback
+	overwritten = window[ callbackName ];
+	window[ callbackName ] = function() {
+		responseContainer = arguments;
+	};
 
-		// Clean-up function (fires after converters)
-		jqXHR.always( function() {
+	// Clean-up function (fires after converters)
+	jqXHR.always( function() {
 
-			// If previous value didn't exist - remove it
-			if ( overwritten === undefined ) {
-				jQuery( window ).removeProp( callbackName );
+		// If previous value didn't exist - remove it
+		if ( overwritten === undefined ) {
+			jQuery( window ).removeProp( callbackName );
 
-			// Otherwise restore preexisting value
-			} else {
-				window[ callbackName ] = overwritten;
-			}
+		// Otherwise restore preexisting value
+		} else {
+			window[ callbackName ] = overwritten;
+		}
 
-			// Save back as free
-			if ( s[ callbackName ] ) {
+		// Save back as free
+		if ( s[ callbackName ] ) {
 
-				// Make sure that re-using the options doesn't screw things around
-				s.jsonpCallback = originalSettings.jsonpCallback;
+			// Make sure that re-using the options doesn't screw things around
+			s.jsonpCallback = originalSettings.jsonpCallback;
 
-				// Save the callback name for future use
-				oldCallbacks.push( callbackName );
-			}
+			// Save the callback name for future use
+			oldCallbacks.push( callbackName );
+		}
 
-			// Call if it was a function and we have a response
-			if ( responseContainer && typeof overwritten === "function" ) {
-				overwritten( responseContainer[ 0 ] );
-			}
+		// Call if it was a function and we have a response
+		if ( responseContainer && typeof overwritten === "function" ) {
+			overwritten( responseContainer[ 0 ] );
+		}
 
-			responseContainer = overwritten = undefined;
-		} );
+		responseContainer = overwritten = undefined;
+	} );
 
-		// Delegate to script
-		return "script";
-	}
+	// Delegate to script
+	return "script";
 } );
diff --git a/test/data/mock.php b/test/data/mock.php
@@ -70,6 +70,10 @@ protected function json( $req ) {
 			header( 'Content-type: application/json' );
 		}
 
+		if ( isset( $req->query['cors'] ) ) {
+			header( 'Access-Control-Allow-Origin: *' );
+		}
+
 		if ( isset( $req->query['array'] ) ) {
 			echo '[ {"name": "John", "age": 21}, {"name": "Peter", "age": 25 } ]';
 		} else {
diff --git a/test/middleware-mockserver.js b/test/middleware-mockserver.js
@@ -81,6 +81,9 @@ var mocks = {
 		if ( req.query.header ) {
 			resp.writeHead( 200, { "content-type": "application/json" } );
 		}
+		if ( req.query.cors ) {
+			resp.writeHead( 200, { "access-control-allow-origin": "*" } );
+		}
 		if ( req.query.array ) {
 			resp.end( JSON.stringify(
 				[ { name: "John", age: 21 }, { name: "Peter", age: 25 } ]
diff --git a/test/unit/ajax.js b/test/unit/ajax.js
@@ -1239,6 +1239,121 @@ QUnit.module( "ajax", {
 			];
 		} );
 
+		ajaxTest( "jQuery.ajax() - no JSONP auto-promotion" + label, 4, function( assert ) {
+			return [
+				{
+					url: baseURL + "mock.php?action=jsonp",
+					dataType: "json",
+					crossDomain: crossDomain,
+					success: function() {
+						assert.ok( false, "JSON parsing should have failed (no callback)" );
+					},
+					fail: function() {
+						assert.ok( true, "JSON parsing failed, JSONP not used (no callback)" );
+					}
+				},
+				{
+					url: baseURL + "mock.php?action=jsonp&callback=?",
+					dataType: "json",
+					crossDomain: crossDomain,
+					success: function() {
+						assert.ok( false, "JSON parsing should have failed (ULR callback)" );
+					},
+					fail: function() {
+						assert.ok( true, "JSON parsing failed, JSONP not used (URL callback)" );
+					}
+				},
+				{
+					url: baseURL + "mock.php?action=jsonp",
+					dataType: "json",
+					crossDomain: crossDomain,
+					data: "callback=?",
+					success: function() {
+						assert.ok( false, "JSON parsing should have failed (data callback=?)" );
+					},
+					fail: function() {
+						assert.ok( true, "JSON parsing failed, JSONP not used (data callback=?)" );
+					}
+				},
+				{
+					url: baseURL + "mock.php?action=jsonp",
+					dataType: "json",
+					crossDomain: crossDomain,
+					data: "callback=??",
+					success: function() {
+						assert.ok( false, "JSON parsing should have failed (data callback=??)" );
+					},
+					fail: function() {
+						assert.ok( true, "JSON parsing failed, JSONP not used (data callback=??)" );
+					}
+				}
+			];
+		} );
+
+		ajaxTest( "jQuery.ajax() - JSON - no ? replacement" + label, 9, function( assert ) {
+			return [
+				{
+					url: baseURL + "mock.php?action=json&callback=?",
+					dataType: "json",
+					crossDomain: crossDomain,
+					beforeSend: function( _jqXhr, settings ) {
+						var queryString = settings.url.replace( /^[^?]*\?/, "" );
+						assert.ok(
+							queryString.indexOf( "jQuery" ) === -1,
+							"jQuery callback not inserted into the URL (URL callback)"
+						);
+						assert.ok(
+							queryString.indexOf( "callback=?" ) > -1,
+							"\"callback=?\" present in the URL unchanged (URL callback)"
+						);
+					},
+					success: function( data ) {
+						assert.ok( data.data, "JSON results returned (URL callback)" );
+					}
+				},
+				{
+					url: baseURL + "mock.php?action=json",
+					dataType: "json",
+					crossDomain: crossDomain,
+					data: "callback=?",
+					beforeSend: function( _jqXhr, settings ) {
+						var queryString = settings.url.replace( /^[^?]*\?/, "" );
+						assert.ok(
+							queryString.indexOf( "jQuery" ) === -1,
+							"jQuery callback not inserted into the URL (data callback=?)"
+						);
+						assert.ok(
+							queryString.indexOf( "callback=?" ) > -1,
+							"\"callback=?\" present in the URL unchanged (data callback=?)"
+						);
+					},
+					success: function( data ) {
+						assert.ok( data.data, "JSON results returned (data callback=?)" );
+					}
+				},
+				{
+					url: baseURL + "mock.php?action=json",
+					dataType: "json",
+					crossDomain: crossDomain,
+					data: "callback=??",
+					beforeSend: function( _jqXhr, settings ) {
+						var queryString = settings.url.replace( /^[^?]*\?/, "" );
+						assert.ok(
+							queryString.indexOf( "jQuery" ) === -1,
+							"jQuery callback not inserted into the URL (data callback=??)"
+						);
+						assert.ok(
+							queryString.indexOf( "callback=??" ) > -1,
+							"\"callback=?\" present in the URL unchanged (data callback=??)"
+						);
+					},
+					success: function( data ) {
+						assert.ok( data.data, "JSON results returned (data callback=??)" );
+					}
+				}
+			];
+		} );
+
 	} );
 
 	ajaxTest( "jQuery.ajax() - script, Remote", 2, function( assert ) {
