Skip to content

Ajax: use anchor tag for parsing urls #1880

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
btoews wants to merge 1 commit into from
Closed

Ajax: use anchor tag for parsing urls #1880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions src/ajax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ var
rlocalProtocol = /^(?:about|app|app-storage|.+-extension|file|res|widget):$/,
rnoContent = /^(?:GET|HEAD)$/,
rprotocol = /^\/\//,
rurl = /^([\w.+-]+:)(?:\/\/(?:[^\/?#]*@|)([^\/?#:]*)(?::(\d+)|)|)/,

/* Prefilters
* 1) They are useful to introduce custom dataTypes (see ajax/jsonp.js for an example)
Expand All @@ -40,11 +39,10 @@ var
// Avoid comment-prolog char sequence (#10098); must appease lint and evade compression
allTypes = "*/".concat( "*" ),

// Document location
ajaxLocation = location.href,
// Anchor tag for parsing the document origin.
originAnchor = document.createElement( "a" );

// Segment location into parts
ajaxLocParts = rurl.exec( ajaxLocation.toLowerCase() ) || [];
originAnchor.href = location.href;
Copy link
Member

@dmethvin dmethvin Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the indentation here.


// Base "constructor" for jQuery.ajaxPrefilter and jQuery.ajaxTransport
function addToPrefiltersOrTransports( structure ) {
Expand Down Expand Up @@ -288,9 +286,9 @@ jQuery.extend({
etag: {},

ajaxSettings: {
url: ajaxLocation,
url: location.href,
type: "GET",
isLocal: rlocalProtocol.test( ajaxLocParts[ 1 ] ),
isLocal: rlocalProtocol.test( location.protocol ),
global: true,
processData: true,
async: true,
Expand Down Expand Up @@ -390,8 +388,8 @@ jQuery.extend({
responseHeaders,
// timeout handle
timeoutTimer,
// Cross-domain detection vars
parts,
// Url cleanup var
urlAnchor,
// To know if global events are to be dispatched
fireGlobals,
// Loop variable
Expand Down Expand Up @@ -496,23 +494,28 @@ jQuery.extend({
// Add protocol if not provided (prefilters might expect it)
// Handle falsy url in the settings object (#10093: consistency with old signature)
// We also use the url parameter if available
s.url = ( ( url || s.url || ajaxLocation ) + "" ).replace( rhash, "" )
.replace( rprotocol, ajaxLocParts[ 1 ] + "//" );
s.url = ( ( url || s.url || location.href ) + "" ).replace( rhash, "" )
.replace( rprotocol, location.protocol + "//" );

// Alias method option to type as per ticket #12004
s.type = options.method || options.type || s.method || s.type;

// Extract dataTypes list
s.dataTypes = jQuery.trim( s.dataType || "*" ).toLowerCase().match( rnotwhite ) || [ "" ];

// A cross-domain request is in order when we have a protocol:host:port mismatch
// A cross-domain request is in order when the origin doesn't match the current origin.
if ( s.crossDomain == null ) {
parts = rurl.exec( s.url.toLowerCase() );
s.crossDomain = !!( parts &&
( parts[ 1 ] !== ajaxLocParts[ 1 ] || parts[ 2 ] !== ajaxLocParts[ 2 ] ||
( parts[ 3 ] || ( parts[ 1 ] === "http:" ? "80" : "443" ) ) !==
( ajaxLocParts[ 3 ] || ( ajaxLocParts[ 1 ] === "http:" ? "80" : "443" ) ) )
);
urlAnchor = document.createElement( "a" );

try {
Copy link
Contributor Author

@btoews btoews Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because IE throws exceptions for malformed URLs:

screen shot 2014-12-11 at 12 39 41 pm

urlAnchor.href = s.url;
urlAnchor.href = urlAnchor.href;
Copy link
Contributor Author

@btoews btoews Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This accommodates some weird IE behavior.
screen shot 2014-12-11 at 12 37 47 pm

Copy link
Member

@dmethvin dmethvin Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holy crap. Welcome to our cross-browser hell.

Copy link
Member

@timmywil timmywil Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a support comment about that.

Copy link
Member

@dmethvin dmethvin Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get that.

Copy link
Contributor

@jbedard jbedard Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note sure if this will help, but here's a very similar solution with a giant comment about IE: https://github.com/angular/angular.js/blob/master/src/ng/urlUtils.js#L25

Copy link
Member

@dmethvin dmethvin Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it seems to boil down to almost the same thing.

s.crossDomain = originAnchor.protocol + "//" + originAnchor.host !==
urlAnchor.protocol + "//" + urlAnchor.host;
} catch ( e ) {
// If there is an error parsing the URL, assume it is crossDomain.
s.crossDomain = true;
Copy link
Contributor Author

@btoews btoews Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if this should just fail closed, or cancel the request. If the URL is malformed, the request will end up failing either way...

Copy link
Member

@dmethvin dmethvin Dec 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, did some head-scratching on this one and left it as-is with an enhanced comment.

}
}

// Convert data if not already a string
Expand Down
6 changes: 5 additions & 1 deletion test/unit/ajax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ module( "ajax", {
}
]);

ajaxTest( "jQuery.ajax() - cross-domain detection", 7, function() {
ajaxTest( "jQuery.ajax() - cross-domain detection", 8, function() {
function request( url, title, crossDomainOrOptions ) {
return jQuery.extend( {
dataType: "jsonp",
Expand Down Expand Up @@ -351,6 +351,10 @@ module( "ajax", {
{
crossDomain: true
}
),
request(
" http://otherdomain.com",
"Cross-domain url with leading space is detected as cross-domain"
)
];
});
Expand Down