Ajax: Warn against automatic JSON-to-JSONP promotion

Fixes jquerygh-372
Ref jquery/jquery#1799
Ref jquery/jquery#3376
Ref jquery/jquery#4754

Author: mgol

CONTRIBUTING.md

@@ -64,7 +64,7 @@ See: [jQuery Core Style Guide](http://docs.jquery.com/JQuery_Core_Style_Guidelin
 ## Tips For Bug Patching
 
 
-### Environment: localhost 
+### Environment: localhost
 
 To test the plugin you will need:
 
@@ -188,7 +188,7 @@ $ git checkout master
 
 ### Test Suite Tips...
 
-By default the plugin runs against the current (jquery-git WIP) version of jQuery. You can select a different version by specifying it in the URL. Files are always retrieved from code.jquery.com. 
+By default the plugin runs against the current (jquery-3.x-git WIP) version of jQuery. You can select a different version by specifying it in the URL. Files are always retrieved from code.jquery.com.
 
 Example:
 

Gruntfile.js

@@ -26,7 +26,7 @@ module.exports = function( grunt ) {
 		"test/traversing.js",
 
 		{ pattern: "dist/jquery-migrate.js", included: false, served: true },
-		{ pattern: "test/**/*.@(js|css|jpg|html|xml)", included: false, served: true }
+		{ pattern: "test/**/*.@(js|json|css|jpg|html|xml)", included: false, served: true }
 	];
 
 	// Project configuration.

src/jquery/ajax.js

@@ -1,9 +1,11 @@
-import { migrateWarnFunc } from "../main.js";
+import { jQueryVersionSince } from "../compareVersions.js";
+import { migrateWarn, migrateWarnFunc } from "../main.js";
 
 // Support jQuery slim which excludes the ajax module
 if ( jQuery.ajax ) {
 
-var oldAjax = jQuery.ajax;
+var oldAjax = jQuery.ajax,
+	rjsonp = /(=)\?(?=&|$)|\?\?/;
 
 jQuery.ajax = function( ) {
 	var jQXHR = oldAjax.apply( this, arguments );
@@ -21,4 +23,26 @@ jQuery.ajax = function( ) {
 	return jQXHR;
 };
 
+// Only trigger the logic in jQuery <4 as the JSON-to-JSONP auto-promotion
+// behavior is gone in jQuery 4.0 and as it has security implications, we don't
+// want to restore the legacy behavior.
+if ( !jQueryVersionSince( "4.0.0" ) ) {
+
+	// Register this prefilter before the jQuery one. Otherwise, a promoted
+	// request is transformed into one with the script dataType and we can't
+	// catch it anymore.
+	jQuery.ajaxPrefilter( "+json", function( s ) {
+
+		// Warn if JSON-to-JSONP auto-promotion happens.
+		if ( s.jsonp !== false && ( rjsonp.test( s.url ) ||
+				typeof s.data === "string" &&
+				( s.contentType || "" )
+					.indexOf( "application/x-www-form-urlencoded" ) === 0 &&
+				rjsonp.test( s.data )
+		) ) {
+			migrateWarn( "JSON-to-JSONP auto-promotion is deprecated" );
+		}
+	} );
+}
+
 }

test/.eslintrc.json

@@ -12,17 +12,9 @@
 
 	"rules": {
 		// Too many errors
-//		"max-len": "off",
-//		"brace-style": "off",
-//		"key-spacing": "off",
-//		"camelcase": "off",
 		"no-unused-vars": "off",
 		"one-var": "off",
 		"strict": "off"
-//
-//		// Not really too many - waiting for autofix features for these rules
-//		"lines-around-comment": "off",
-//		"dot-notation": "off"
 	},
 
 	"globals": {
@@ -31,22 +23,12 @@
 		"compareVersions": true,
 		"jQueryVersionSince": true,
 		"TestManager": true,
+		"url": true,
 
 		"Symbol": false,
 
 		"jQuery": true,
 		"QUnit": true,
-		"module": true,
-		"ok": true,
-		"equal": true,
-		"test": true,
-		"asyncTest": true,
-		"notEqual": true,
-		"deepEqual": true,
-		"strictEqual": true,
-		"notStrictEqual": true,
-		"start": true,
-		"stop": true,
-		"expect": true
+		"module": true
 	}
 }

test/ajax.js

@@ -10,7 +10,7 @@ QUnit.test( "jQuery.ajax() deprecations on jqXHR", function( assert ) {
 
 	expectWarning( assert, ".success(), .error(), .compete() calls", 3, function() {
 
-		jQuery.ajax( "/not-found.404" )
+		return jQuery.ajax( url( "not-found.404" ) )
 			.success( jQuery.noop )
 			.error( function( jQXHR ) {
 
@@ -19,12 +19,104 @@ QUnit.test( "jQuery.ajax() deprecations on jqXHR", function( assert ) {
 			} )
 			.complete( function() {
 				assert.ok( true, "ajax complete" );
+			} )
+			.catch( jQuery.noop );
+	} ).then( function() {
+		done();
+	} );
+
+} );
+
+[ " - Same Domain", " - Cross Domain" ].forEach( function( label, crossDomain ) {
+
+	// The JSON-to-JSONP auto-promotion behavior is gone in jQuery 4.0 and as
+	// it has security implications, we don't want to restore the legacy behavior.
+	QUnit[ jQueryVersionSince( "4.0.0" ) ? "skip" : "test" ](
+			"jQuery.ajax() JSON-to-JSONP auto-promotion" + label, function( assert ) {
+
+		assert.expect( 5 );
+
+		var done = assert.async(),
+			tests = [
+				function() {
+					return expectNoWarning( assert, "dataType: \"json\"",
+						function() {
+							return jQuery.ajax( {
+								url: url( "data/null.json" ),
+								crossDomain: crossDomain,
+								dataType: "json"
+							} ).catch( jQuery.noop );
+						}
+					);
+				},
 
-				// Wait for expectWarning to complete
-				setTimeout( done, 1 );
+				function() {
+					return expectWarning( assert, "dataType: \"json\", URL callback", 1,
+						function() {
+							return jQuery.ajax( {
+								url: url( "data/null.json?callback=?" ),
+								crossDomain: crossDomain,
+								dataType: "json"
+							} ).catch( jQuery.noop );
+						}
+					);
+				},
+
+				function() {
+					return expectWarning( assert, "dataType: \"json\", data callback", 1,
+						function() {
+							return jQuery.ajax( {
+								url: url( "data/null.json" ),
+								crossDomain: crossDomain,
+								data: "callback=?",
+								dataType: "json"
+							} ).catch( jQuery.noop );
+						}
+					);
+				},
+
+				function() {
+					return expectNoWarning( assert, "dataType: \"jsonp\", URL callback",
+						function() {
+							return jQuery.ajax( {
+								url: url( "data/null.json?callback=?" ),
+								crossDomain: crossDomain,
+								dataType: "jsonp"
+							} ).catch( jQuery.noop );
+						}
+					);
+				},
+
+				function() {
+					return expectNoWarning( assert, "dataType: \"jsonp\", data callback",
+						function() {
+							return jQuery.ajax( {
+								url: url( "data/null.json" ),
+								crossDomain: crossDomain,
+								data: "callback=?",
+								dataType: "jsonp"
+							} ).catch( jQuery.noop );
+						}
+					);
+				}
+			];
+
+		// Invoke tests sequentially as they're async and early tests could get warnings
+		// from later ones.
+		function run( tests ) {
+			var test = tests[ 0 ];
+			return test().then( function() {
+				if ( tests.length > 1 ) {
+					return run( tests.slice( 1 ) );
+				}
 			} );
-	} );
+		}
 
+		run( tests )
+			.then( function() {
+				done();
+			} );
+	} );
 } );
 
 }

test/index.html

@@ -14,7 +14,7 @@
 	<!-- Load a jQuery and jquery-migrate plugin file based on URL -->
 	<script src="testinit.js"></script>
 	<script>
-		TestManager.loadProject( "jquery", "git" );
+		TestManager.loadProject( "jquery", "3.x-git" );
 		// Close this script tag so file will load
 	</script>
 	<script>

test/migrate.js

@@ -1,30 +1,45 @@
 /* exported expectWarning, expectNoWarning */
 
 function expectWarning( assert, name, expected, fn ) {
+	var result;
 	if ( !fn ) {
 		fn = expected;
 		expected = null;
 	}
 	jQuery.migrateReset();
-	fn();
+	result = fn();
 
-	// Special-case for 0 warnings expected
-	if ( expected === 0 ) {
-		assert.deepEqual( jQuery.migrateWarnings, [], name + ": did not warn" );
+	function check() {
 
-	// Simple numeric equality assertion for warnings matching an explicit count
-	} else if ( expected && jQuery.migrateWarnings.length === expected ) {
-		assert.equal( jQuery.migrateWarnings.length, expected, name + ": warned" );
+		// Special-case for 0 warnings expected
+		if ( expected === 0 ) {
+			assert.deepEqual( jQuery.migrateWarnings, [], name + ": did not warn" );
 
-	// Simple ok assertion when we saw at least one warning and weren't looking for an explict count
-	} else if ( !expected && jQuery.migrateWarnings.length ) {
-		assert.ok( true, name + ": warned" );
+		// Simple numeric equality assertion for warnings matching an explicit count
+		} else if ( expected && jQuery.migrateWarnings.length === expected ) {
+			assert.equal( jQuery.migrateWarnings.length, expected, name + ": warned" );
 
-	// Failure; use deepEqual to show the warnings that *were* generated and the expectation
-	} else {
-		assert.deepEqual( jQuery.migrateWarnings,
-			"<warnings: " + ( expected || "1+" ) + ">", name + ": warned"
+		// Simple ok assertion when we saw at least one warning and weren't looking for an explict count
+		} else if ( !expected && jQuery.migrateWarnings.length ) {
+			assert.ok( true, name + ": warned" );
+
+		// Failure; use deepEqual to show the warnings that *were* generated and the expectation
+		} else {
+			assert.deepEqual( jQuery.migrateWarnings,
+				"<warnings: " + ( expected || "1+" ) + ">", name + ": warned"
+			);
+		}
+	}
+
+	if ( result && result.then ) {
+		return jQuery.when(
+			result.then( function() {
+				check();
+			} )
 		);
+	} else {
+		check();
+		return jQuery.when();
 	}
 }
 

test/testinit.js

@@ -108,7 +108,13 @@ TestManager = {
 	iframeCallback: undefined,
 	baseURL: window.__karma__ ? "base/test/" : "./",
 	init: function( projects ) {
-		var p, project, originalDeduplicateWarnings;
+		var p, project, originalDeduplicateWarnings,
+			FILEPATH = "/test/testinit.js",
+			activeScript = [].slice.call( document.getElementsByTagName( "script" ), -1 )[ 0 ],
+			parentUrl = activeScript && activeScript.src ?
+				activeScript.src.replace( /[?#].*/, "" ) + FILEPATH.replace( /[^/]+/g, ".." ) + "/" :
+				"../",
+			baseURL = parentUrl + "test/";
 
 		this.projects = projects;
 		this.loaded = [];
@@ -135,6 +141,21 @@ TestManager = {
 			} );
 		}
 
+		/**
+		 * Add random number to url to stop caching
+		 *
+		 * Also prefixes with baseURL automatically.
+		 *
+		 * @example url("index.html")
+		 * @result "data/index.html?10538358428943"
+		 *
+		 * @example url("mock.php?foo=bar")
+		 * @result "data/mock.php?foo=bar&10538358345554"
+		 */
+		window.url = function url( value ) {
+			return baseURL + value + ( /\?/.test( value ) ? "&" : "?" ) +
+				new Date().getTime() + "" + parseInt( Math.random() * 100000, 10 );
+		};
 
 		QUnit.begin( function( details ) {
 			originalDeduplicateWarnings = jQuery.migrateDeduplicateWarnings;