Skip to content

Commit e35cd85

Browse files
mgolmgol
committed
Ajax: Warn against automatic JSON-to-JSONP promotion
Fixes jquerygh-372 Ref jquery/jquery#1799 Ref jquery/jquery#3376 Ref jquery/jquery#4754
1 parent 31ea893 commit e35cd85

File tree

8 files changed

+179
-45
lines changed

8 files changed

+179
-45
lines changed

‎CONTRIBUTING.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ See: [jQuery Core Style Guide](http://docs.jquery.com/JQuery_Core_Style_Guidelin
6464
## Tips For Bug Patching
6565

6666

67-
### Environment: localhost
67+
### Environment: localhost
6868

6969
To test the plugin you will need:
7070

@@ -188,7 +188,7 @@ $ git checkout master
188188

189189
### Test Suite Tips...
190190

191-
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.
191+
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.
192192

193193
Example:
194194

‎Gruntfile.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ module.exports = function( grunt ) {
2626
"test/traversing.js",
2727

2828
{ pattern: "dist/jquery-migrate.js", included: false, served: true },
29-
{ pattern: "test/**/*.@(js|css|jpg|html|xml)", included: false, served: true }
29+
{ pattern: "test/**/*.@(js|json|css|jpg|html|xml)", included: false, served: true }
3030
];
3131

3232
// Project configuration.

‎src/jquery/ajax.js‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
1-
import { migrateWarnFunc } from "../main.js";
1+
import { jQueryVersionSince } from "../compareVersions.js";
2+
import { migrateWarn, migrateWarnFunc } from "../main.js";
23

34
// Support jQuery slim which excludes the ajax module
45
if ( jQuery.ajax ) {
56

6-
var oldAjax = jQuery.ajax;
7+
var oldAjax = jQuery.ajax,
8+
rjsonp = /(=)\?(?=&|$)|\?\?/;
79

810
jQuery.ajax = function( ) {
911
var jQXHR = oldAjax.apply( this, arguments );
@@ -21,4 +23,26 @@ jQuery.ajax = function( ) {
2123
return jQXHR;
2224
};
2325

26+
// Only trigger the logic in jQuery <4 as the JSON-to-JSONP auto-promotion
27+
// behavior is gone in jQuery 4.0 and as it has security implications, we don't
28+
// want to restore the legacy behavior.
29+
if ( !jQueryVersionSince( "4.0.0" ) ) {
30+
31+
// Register this prefilter before the jQuery one. Otherwise, a promoted
32+
// request is transformed into one with the script dataType and we can't
33+
// catch it anymore.
34+
jQuery.ajaxPrefilter( "+json", function( s ) {
35+
36+
// Warn if JSON-to-JSONP auto-promotion happens.
37+
if ( s.jsonp !== false && ( rjsonp.test( s.url ) ||
38+
typeof s.data === "string" &&
39+
( s.contentType || "" )
40+
.indexOf( "application/x-www-form-urlencoded" ) === 0 &&
41+
rjsonp.test( s.data )
42+
) ) {
43+
migrateWarn( "JSON-to-JSONP auto-promotion is deprecated" );
44+
}
45+
} );
46+
}
47+
2448
}

‎test/.eslintrc.json‎

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,9 @@
1212

1313
"rules": {
1414
// Too many errors
15-
// "max-len": "off",
16-
// "brace-style": "off",
17-
// "key-spacing": "off",
18-
// "camelcase": "off",
1915
"no-unused-vars": "off",
2016
"one-var": "off",
2117
"strict": "off"
22-
//
23-
// // Not really too many - waiting for autofix features for these rules
24-
// "lines-around-comment": "off",
25-
// "dot-notation": "off"
2618
},
2719

2820
"globals": {
@@ -31,22 +23,12 @@
3123
"compareVersions": true,
3224
"jQueryVersionSince": true,
3325
"TestManager": true,
26+
"url": true,
3427

3528
"Symbol": false,
3629

3730
"jQuery": true,
3831
"QUnit": true,
39-
"module": true,
40-
"ok": true,
41-
"equal": true,
42-
"test": true,
43-
"asyncTest": true,
44-
"notEqual": true,
45-
"deepEqual": true,
46-
"strictEqual": true,
47-
"notStrictEqual": true,
48-
"start": true,
49-
"stop": true,
50-
"expect": true
32+
"module": true
5133
}
5234
}

‎test/ajax.js‎

Lines changed: 96 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ QUnit.test( "jQuery.ajax() deprecations on jqXHR", function( assert ) {
1010

1111
expectWarning( assert, ".success(), .error(), .compete() calls", 3, function() {
1212

13-
jQuery.ajax( "/not-found.404" )
13+
return jQuery.ajax( url( "not-found.404" ) )
1414
.success( jQuery.noop )
1515
.error( function( jQXHR ) {
1616

@@ -19,12 +19,104 @@ QUnit.test( "jQuery.ajax() deprecations on jqXHR", function( assert ) {
1919
} )
2020
.complete( function() {
2121
assert.ok( true, "ajax complete" );
22+
} )
23+
.catch( jQuery.noop );
24+
} ).then( function() {
25+
done();
26+
} );
27+
28+
} );
29+
30+
[ " - Same Domain", " - Cross Domain" ].forEach( function( label, crossDomain ) {
31+
32+
// The JSON-to-JSONP auto-promotion behavior is gone in jQuery 4.0 and as
33+
// it has security implications, we don't want to restore the legacy behavior.
34+
QUnit[ jQueryVersionSince( "4.0.0" ) ? "skip" : "test" ](
35+
"jQuery.ajax() JSON-to-JSONP auto-promotion" + label, function( assert ) {
36+
37+
assert.expect( 5 );
38+
39+
var done = assert.async(),
40+
tests = [
41+
function() {
42+
return expectNoWarning( assert, "dataType: \"json\"",
43+
function() {
44+
return jQuery.ajax( {
45+
url: url( "data/null.json" ),
46+
crossDomain: crossDomain,
47+
dataType: "json"
48+
} ).catch( jQuery.noop );
49+
}
50+
);
51+
},
2252

23-
// Wait for expectWarning to complete
24-
setTimeout( done, 1 );
53+
function() {
54+
return expectWarning( assert, "dataType: \"json\", URL callback", 1,
55+
function() {
56+
return jQuery.ajax( {
57+
url: url( "data/null.json?callback=?" ),
58+
crossDomain: crossDomain,
59+
dataType: "json"
60+
} ).catch( jQuery.noop );
61+
}
62+
);
63+
},
64+
65+
function() {
66+
return expectWarning( assert, "dataType: \"json\", data callback", 1,
67+
function() {
68+
return jQuery.ajax( {
69+
url: url( "data/null.json" ),
70+
crossDomain: crossDomain,
71+
data: "callback=?",
72+
dataType: "json"
73+
} ).catch( jQuery.noop );
74+
}
75+
);
76+
},
77+
78+
function() {
79+
return expectNoWarning( assert, "dataType: \"jsonp\", URL callback",
80+
function() {
81+
return jQuery.ajax( {
82+
url: url( "data/null.json?callback=?" ),
83+
crossDomain: crossDomain,
84+
dataType: "jsonp"
85+
} ).catch( jQuery.noop );
86+
}
87+
);
88+
},
89+
90+
function() {
91+
return expectNoWarning( assert, "dataType: \"jsonp\", data callback",
92+
function() {
93+
return jQuery.ajax( {
94+
url: url( "data/null.json" ),
95+
crossDomain: crossDomain,
96+
data: "callback=?",
97+
dataType: "jsonp"
98+
} ).catch( jQuery.noop );
99+
}
100+
);
101+
}
102+
];
103+
104+
// Invoke tests sequentially as they're async and early tests could get warnings
105+
// from later ones.
106+
function run( tests ) {
107+
var test = tests[ 0 ];
108+
return test().then( function() {
109+
if ( tests.length > 1 ) {
110+
return run( tests.slice( 1 ) );
111+
}
25112
} );
26-
} );
113+
}
27114

115+
run( tests )
116+
.then( function() {
117+
done();
118+
} );
119+
} );
28120
} );
29121

30122
}

‎test/index.html‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
<!-- Load a jQuery and jquery-migrate plugin file based on URL -->
1515
<script src="testinit.js"></script>
1616
<script>
17-
TestManager.loadProject( "jquery", "git" );
17+
TestManager.loadProject( "jquery", "3.x-git" );
1818
// Close this script tag so file will load
1919
</script>
2020
<script>

‎test/migrate.js‎

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,45 @@
11
/* exported expectWarning, expectNoWarning */
22

33
function expectWarning( assert, name, expected, fn ) {
4+
var result;
45
if ( !fn ) {
56
fn = expected;
67
expected = null;
78
}
89
jQuery.migrateReset();
9-
fn();
10+
result = fn();
1011

11-
// Special-case for 0 warnings expected
12-
if ( expected === 0 ) {
13-
assert.deepEqual( jQuery.migrateWarnings, [], name + ": did not warn" );
12+
function check() {
1413

15-
// Simple numeric equality assertion for warnings matching an explicit count
16-
} else if ( expected && jQuery.migrateWarnings.length === expected ) {
17-
assert.equal( jQuery.migrateWarnings.length, expected, name + ": warned" );
14+
// Special-case for 0 warnings expected
15+
if ( expected === 0 ) {
16+
assert.deepEqual( jQuery.migrateWarnings, [], name + ": did not warn" );
1817

19-
// Simple ok assertion when we saw at least one warning and weren't looking for an explict count
20-
} else if ( !expected && jQuery.migrateWarnings.length ) {
21-
assert.ok( true, name + ": warned" );
18+
// Simple numeric equality assertion for warnings matching an explicit count
19+
} else if ( expected && jQuery.migrateWarnings.length === expected ) {
20+
assert.equal( jQuery.migrateWarnings.length, expected, name + ": warned" );
2221

23-
// Failure; use deepEqual to show the warnings that *were* generated and the expectation
24-
} else {
25-
assert.deepEqual( jQuery.migrateWarnings,
26-
"<warnings: " + ( expected || "1+" ) + ">", name + ": warned"
22+
// Simple ok assertion when we saw at least one warning and weren't looking for an explict count
23+
} else if ( !expected && jQuery.migrateWarnings.length ) {
24+
assert.ok( true, name + ": warned" );
25+
26+
// Failure; use deepEqual to show the warnings that *were* generated and the expectation
27+
} else {
28+
assert.deepEqual( jQuery.migrateWarnings,
29+
"<warnings: " + ( expected || "1+" ) + ">", name + ": warned"
30+
);
31+
}
32+
}
33+
34+
if ( result && result.then ) {
35+
return jQuery.when(
36+
result.then( function() {
37+
check();
38+
} )
2739
);
40+
} else {
41+
check();
42+
return jQuery.when();
2843
}
2944
}
3045

‎test/testinit.js‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,13 @@ TestManager = {
108108
iframeCallback: undefined,
109109
baseURL: window.__karma__ ? "base/test/" : "./",
110110
init: function( projects ) {
111-
var p, project, originalDeduplicateWarnings;
111+
var p, project, originalDeduplicateWarnings,
112+
FILEPATH = "/test/testinit.js",
113+
activeScript = [].slice.call( document.getElementsByTagName( "script" ), -1 )[ 0 ],
114+
parentUrl = activeScript && activeScript.src ?
115+
activeScript.src.replace( /[?#].*/, "" ) + FILEPATH.replace( /[^/]+/g, ".." ) + "/" :
116+
"../",
117+
baseURL = parentUrl + "test/";
112118

113119
this.projects = projects;
114120
this.loaded = [];
@@ -135,6 +141,21 @@ TestManager = {
135141
} );
136142
}
137143

144+
/**
145+
* Add random number to url to stop caching
146+
*
147+
* Also prefixes with baseURL automatically.
148+
*
149+
* @example url("index.html")
150+
* @result "data/index.html?10538358428943"
151+
*
152+
* @example url("mock.php?foo=bar")
153+
* @result "data/mock.php?foo=bar&10538358345554"
154+
*/
155+
window.url = function url( value ) {
156+
return baseURL + value + ( /\?/.test( value ) ? "&" : "?" ) +
157+
new Date().getTime() + "" + parseInt( Math.random() * 100000, 10 );
158+
};
138159

139160
QUnit.begin( function( details ) {
140161
originalDeduplicateWarnings = jQuery.migrateDeduplicateWarnings;

0 commit comments

Comments
 (0)