Ajax: Drop the json to jsonp auto-promotion logic #4754
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is this something the migrate plugin could feasilby detect, fix, and warn about without false positives? |
|
@Krinkle I think it should be possible to detect and warn about this. |
dmethvin
approved these changes
Jul 19, 2020
mgol
force-pushed
the
no-jsonp-auto-promotion
branch
2 times, most recently
from
b29bbd0
to
b1bece3
Compare
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 jquerygh-1799 Fixes jquerygh-3376
mgol
force-pushed
the
no-jsonp-auto-promotion
branch
from
b1bece3
to
5f93f8e
Compare
|
I added tests for issue #1799 since the logic will now be skipped for regular JSON requests. |
mgol
added
the
Discuss in Meeting
mgol
added a commit
to mgol/jquery-migrate
that referenced
this pull request
Jul 22, 2020
|
I've also just submitted a Migrate PR: jquery/jquery-migrate#376. |
gibson042
approved these changes
Jul 27, 2020
mgol
commented
Jul 27, 2020
timmywil
removed
the
Discuss in Meeting
timmywil
approved these changes
Jul 27, 2020
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Jul 29, 2020
This aligns the Node.js server with the previous PHP one in accepting `mock.php` as a callback which is triggered by a recently added test. This prevents the request crashing on that Node.js server and printing a JS error: ``` TypeError: Cannot read property '1' of null ``` Ref jquerygh-4754
2 tasks
mgol
added a commit
to mgol/jquery-migrate
that referenced
this pull request
Aug 25, 2020
mgol
added a commit
to jquery/jquery-migrate
that referenced
this pull request
Aug 31, 2020
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Sep 2, 2020
This aligns the Node.js server with the previous PHP one in accepting `mock.php` as a callback which is triggered by a recently added test. This prevents the request crashing on that Node.js server and printing a JS error: ``` TypeError: Cannot read property '1' of null ``` Ref jquerygh-4754
mgol
added a commit
that referenced
this pull request
Sep 2, 2020
This aligns the Node.js server with the previous PHP one in sending `mock.php` as a callback if there's no `callback` parameter in the query string which is triggered by a recently added test. This prevents the request crashing on that Node.js server and printing a JS error: ``` TypeError: Cannot read property '1' of null ``` Closes gh-4764 Ref gh-4754
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Sep 2, 2020
This aligns the Node.js server with the previous PHP one in sending `mock.php` as a callback if there's no `callback` parameter in the query string which is triggered by a recently added test. This prevents the request crashing on that Node.js server and printing a JS error: ``` TypeError: Cannot read property '1' of null ``` Closes jquerygh-4764 Ref jquerygh-4754 (cherry picked from commit df6858d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Previously,
jQuery.ajaxwithdataType: 'json'with a provided callback wasautomatically converted to a jsonp request unless one also specified
jsonp: false. Today the preferred way of interacting with a cross-domainbackend 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 withdataType: "json"wereauto-promoted to JSONP.
Fixes gh-1799
Fixes gh-3376
Checklist