- Notifications
You must be signed in to change notification settings - Fork20.6k
Ajax: Drop the json to jsonp auto-promotion logic#4754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or 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 changesJul 19, 2020
b29bbd0 tob1bece3ComparePreviously, `jQuery.ajax` with `dataType: '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 thedeveloper may be unaware they're not just downloading data but executing codefrom 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"` wereauto-promoted to JSONP.Fixesjquerygh-1799Fixesjquerygh-3376
I added tests for issue#1799 since the logic will now be skipped for regular JSON requests. |
mgol added a commit to mgol/jquery-migrate that referenced this pull requestJul 22, 2020
I've also just submitted a Migrate PR:jquery/jquery-migrate#376. |
gibson042 approved these changesJul 27, 2020
mgol commentedJul 27, 2020
Uh oh!
There was an error while loading.Please reload this page.
timmywil approved these changesJul 27, 2020
mgol added a commit to mgol/jquery that referenced this pull requestJul 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 therequest crashing on that Node.js server and printing a JS error:```TypeError: Cannot read property '1' of null```Refjquerygh-4754
2 tasks
mgol added a commit to mgol/jquery-migrate that referenced this pull requestAug 25, 2020
mgol added a commit to jquery/jquery-migrate that referenced this pull requestAug 31, 2020
mgol added a commit to mgol/jquery that referenced this pull requestSep 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 therequest crashing on that Node.js server and printing a JS error:```TypeError: Cannot read property '1' of null```Refjquerygh-4754
mgol added a commit that referenced this pull requestSep 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 istriggered by a recently added test. This prevents the request crashing on thatNode.js server and printing a JS error:```TypeError: Cannot read property '1' of null```Closesgh-4764Refgh-4754
mgol added a commit to mgol/jquery that referenced this pull requestSep 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 istriggered by a recently added test. This prevents the request crashing on thatNode.js server and printing a JS error:```TypeError: Cannot read property '1' of null```Closesjquerygh-4764Refjquerygh-4754(cherry picked from commitdf6858d)
Sign up for freeto 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.
Uh oh!
There was an error while loading.Please reload this page.
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.
Fixesgh-1799
Fixesgh-3376
Checklist