Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Commit

Permalink
Navigation: Correctly (un)escape data-url all throughout the code
Browse files Browse the repository at this point in the history
Also removes code dealing with nested list URL tokens (subPageUrlKey)

The initial bug was regarding the need to use selector-escaping when looking
for pages inside the DOM via their data-url attribute, because the the data-url
may contain "weird" characters like parantheses, single, and double quotes.

During the process of fixing this it became clear that the data-url attribute
can sometimes end up having a URL-encoded value - but not always. This makes
proper string comparison, much less selector-escaping, impossible. So, it
became necessary to require that data-url never be url-encoded.

Conversely, the URL that ends up in the location when using replaceState() must
be URL-encoded, otherwise, upon deep linking, one may end up with an invalid
URL. For example, if the URL contains an actual percent sign, and the URL is
placed in the location via replaceState(), the percent sign must be URL-encoded
in order for deep-linking to work.

Below are the original commit messages that have been squashed:

Pagecontainer: Escape dataUrl when trying to find page based on it
Navigation: Make sure location is encoded when going to a funky page
Pagecontainer: Test that _find() throws not when looking for weird URLs
Init: Correctly assign data-url for initial page
Init: Make sure "data-url" attribute for initial page is decoded
Pagecontainer: Decode URI when assigning to "data-url" during _parse()
Pagecontainer: Use decoded URL to build selector for data-url
Pagecontainer: Call convertUrlToDataUrl() via _createDataUrl()
Path: Always uri-decode when computing dataUrl
Pagecontainer: Remove extra decoding step, now part of _createDataUrl()

This also removes the possibility that a URL gets double-decoded.
Pagecontainer: Test behavior of _find() in the face of weird URLs
Navigation: Use data-url retrieved from Ajax request in its encoded form
Pagecontainer: Pass encoded URL to $.mobile.navigate()
Init: Rename JS file with spaces in it

(cherry picked from commit e412102)

Closes gh-7474
Fixes gh-1383
  • Loading branch information
Gabriel Schulhof committed Sep 4, 2014
1 parent c4623d0 commit d3a412e
Show file tree
Hide file tree
Showing 12 changed files with 180 additions and 25 deletions.
2 changes: 1 addition & 1 deletion js/jquery.mobile.init.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ define([
// unless the data url is already set set it to the pathname
if ( !$this[ 0 ].getAttribute( "data-" + $.mobile.ns + "url" ) ) {
$this.attr( "data-" + $.mobile.ns + "url", $this.attr( "id" ) ||
theLocation.pathname + theLocation.search );
path.convertUrlToDataUrl( theLocation.pathname + theLocation.search ) );
}
});

Expand Down
17 changes: 9 additions & 8 deletions js/navigation/path.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,19 +202,21 @@ define([
},

convertUrlToDataUrl: function( absUrl ) {
var u = path.parseUrl( absUrl );
var result = absUrl,
u = path.parseUrl( absUrl );

if ( path.isEmbeddedPage( u ) ) {
// For embedded pages, remove the dialog hash key as in getFilePath(),
// and remove otherwise the Data Url won't match the id of the embedded Page.
return u.hash
result = u.hash
.split( dialogHashKey )[0]
.replace( /^#/, "" )
.replace( /\?.*$/, "" );
} else if ( path.isSameDomain( u, this.documentBase ) ) {
return u.hrefNoHash.replace( this.documentBase.domain, "" ).split( dialogHashKey )[0];
result = u.hrefNoHash.replace( this.documentBase.domain, "" ).split( dialogHashKey )[0];
}

return window.decodeURIComponent(absUrl);
return window.decodeURIComponent( result );
},

//get path from current hash, or from a file path
Expand Down Expand Up @@ -374,11 +376,10 @@ define([
return ( hasHash ? "#" : "" ) + hash.replace( /([!"#$%&'()*+,./:;<=>?@[\]^`{|}~])/g, "\\$1" );
},

// return the substring of a filepath before the sub-page key, for making
// a server request
// return the substring of a filepath before the dialogHashKey, for making a server
// request
getFilePath: function( path ) {
var splitkey = "&" + $.mobile.subPageUrlKey;
return path && path.split( splitkey )[0].split( dialogHashKey )[0];
return path && path.split( dialogHashKey )[0];
},

// check if the specified url refers to the first page in the main
Expand Down
22 changes: 10 additions & 12 deletions js/widgets/pagecontainer.js
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,8 @@ define( [
// NOTE do _not_ use the :jqmData pseudo selector because parenthesis
// are a valid url char and it breaks on the first occurence
page = this.element
.children( "[data-" + this._getNs() +"url='" + dataUrl + "']" );
.children( "[data-" + this._getNs() +
"url='" + $.mobile.path.hashToSelector( dataUrl ) + "']" );

// If we failed to find the page, check to see if the url is a
// reference to an embedded page. If so, it may have been dynamically
Expand Down Expand Up @@ -442,7 +443,7 @@ define( [
// TODO tagging a page with external to make sure that embedded pages aren't
// removed by the various page handling code is bad. Having page handling code
// in many places is bad. Solutions post 1.0
page.attr( "data-" + this._getNs() + "url", $.mobile.path.convertUrlToDataUrl(fileUrl) )
page.attr( "data-" + this._getNs() + "url", this._createDataUrl( fileUrl ) )
.attr( "data-" + this._getNs() + "external-page", true );

return page;
Expand Down Expand Up @@ -491,8 +492,7 @@ define( [
// or require ordering such that other bits are sprinkled in between parts that
// could be abstracted out as a group
_loadSuccess: function( absUrl, triggerData, settings, deferred ) {
var fileUrl = this._createFileUrl( absUrl ),
dataUrl = this._createDataUrl( absUrl );
var fileUrl = this._createFileUrl( absUrl );

return $.proxy(function( html, textStatus, xhr ) {
//pre-parse html to check for a data-url,
Expand All @@ -512,6 +512,11 @@ define( [
dataUrlRegex.test( RegExp.$1 ) &&
RegExp.$1 ) {
fileUrl = $.mobile.path.getFilePath( $("<div>" + RegExp.$1 + "</div>").text() );

// We specify that, if a data-url attribute is given on the page div, its value
// must be given non-URL-encoded. However, in this part of the code, fileUrl is
// assumed to be URL-encoded, so we URL-encode the retrieved value here
fileUrl = this.window[ 0 ].encodeURIComponent( fileUrl );
}

//dont update the base tag if we are prefetching
Expand Down Expand Up @@ -549,13 +554,6 @@ define( [

this._include( content, settings );

// Enhancing the content may result in new dialogs/sub content being inserted
// into the DOM. If the original absUrl refers to a sub-content, that is the
// real content we are interested in.
if ( absUrl.indexOf( "&" + $.mobile.subPageUrlKey ) > -1 ) {
content = this.element.children( "[data-" + this._getNs() +"url='" + dataUrl + "']" );
}

// Remove loading message.
if ( settings.showLoadMsg ) {
this._hideLoading();
Expand Down Expand Up @@ -1125,7 +1123,7 @@ define( [
};

if ( settings.changeHash !== false && $.mobile.hashListeningEnabled ) {
$.mobile.navigate( url, params, true);
$.mobile.navigate( this.window[ 0 ].encodeURI( url ), params, true);
} else if ( toPage[ 0 ] !== $.mobile.firstPage[ 0 ] ) {
$.mobile.navigate.history.add( url, params );
}
Expand Down
8 changes: 8 additions & 0 deletions tests/integration/navigation/100%test/behind-the-percent.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<!doctype html>
<html>
<head>
</head>
<body>
<div data-nstest-role="page"><div id="percentPageChild"></div></div>
</body>
</html>
1 change: 1 addition & 0 deletions tests/integration/navigation/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

<div id="harmless-default-page" data-nstest-role="page" class="first-page">
<a id="goToGoogle" href="http://www.google.com/#abc">Go to Google</a>
<a id="goToPercentPage" href="100%25test/behind-the-percent.html">Go to percent page</a>
</div>

<div id="foo" data-nstest-role="page" class="foo-class">
Expand Down
43 changes: 43 additions & 0 deletions tests/integration/navigation/navigation_core.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,49 @@ $.testHelper.delayStart();
"Default is not prevented when clicking on external link with hash" );
});

( function() {

var goUrl, originalGo,
navigatorPrototype = $.mobile.Navigator.prototype;

module( "Navigation encoding", {
setup: function() {
goUrl = undefined;
originalGo = navigatorPrototype.go;
navigatorPrototype.go = function( url ) {
goUrl = url;

return originalGo.apply( this, arguments );
};
},
teardown: function() {
navigatorPrototype.go = originalGo;
}
});

asyncTest( "Going to a page requiring url encoding works", function() {
var endingString = $( "#goToPercentPage" ).attr( "href" );

$.testHelper.pageSequence([
function() {
$( "#goToPercentPage" ).click();
},
function() {
deepEqual( $.mobile.activePage.children( "#percentPageChild" ).length, 1,
"Active page is the one loaded from the directory with a percent symbol" );

deepEqual(
goUrl.lastIndexOf( endingString ),
goUrl.length - endingString.length,
"Location ends in '" + endingString + "'" );
$.mobile.back();
},
start
]);
});

})();

module('jquery.mobile.navigation.js', {
setup: function() {
$.mobile.navigate.history.stack = [];
Expand Down
19 changes: 19 additions & 0 deletions tests/unit/content/content_core.js
Original file line number Diff line number Diff line change
Expand Up @@ -512,4 +512,23 @@

proto._loadUrl( "foo", {}, {} );
});

test( "_find() does not throw upon encountering a weird file name", function() {
var errorThrown,
proto = $.mobile.pagecontainer.prototype;

try {
proto._find.call({
_getNs: proto._getNs,
_createFileUrl: proto._createFileUrl,
_createDataUrl: proto._createDataUrl,
_getInitialContent: function() { return $( "<div>" ); },
element: $( "<body>" )
}, "http://localhost/Raison d'être.html" );
} catch( error ) {
errorThrown = error;
}

deepEqual( errorThrown, undefined, "Error was not thrown" );
});
})(jQuery);
33 changes: 33 additions & 0 deletions tests/unit/init/weird file name-tests.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<title>jQuery Mobile Init Test Suite</title>

<script src="../../../external/requirejs/require.js"></script>
<script src="../../../js/requirejs.config.js"></script>
<script src="../../requirejs.config.js"></script>
<script src="../../../js/jquery.tag.inserter.js"></script>
<script src="../../jquery.setNameSpace.js"></script>
<script src="../../jquery.testHelper.js"></script>

<link rel="stylesheet" href="../../../css/themes/default/jquery.mobile.css" />
<link rel="stylesheet" href="../../../external/qunit/qunit.css"/>
<link rel="stylesheet" href="../../jqm-tests.css"/>
<script src="../../../external/qunit/qunit.js"></script>
<script>
$.testHelper.asyncLoad([
[ "weird_file_name_core.js" ],
[ "jquery.mobile.init" ]
]);
</script>

<script src="../../swarminject.js"></script>
</head>
<body>

<div id="qunit"></div>

<div data-nstest-role="page"></div>
</body>
</html>
41 changes: 41 additions & 0 deletions tests/unit/init/weird_file_name_core.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
( function() {

$( document ).on( "mobileinit", function() {
var origParseLocation = $.mobile.path.parseLocation,
origInitializePage = $.mobile.initializePage;

// Overwrite parseLocation with a version that always urlencodes the name of the tests file
$.mobile.path.parseLocation = function() {
var parsedLocation = origParseLocation.apply( this, arguments ),
returnValue = {};

// Make sure the location bits appear urlEncoded
$.each( parsedLocation, function( key, value ) {
returnValue[ key ] =
value.replace( /weird file name-tests/g, "weird%20file%20name-tests" );
});

return returnValue;
};

// Overwrite initializePage with a version that restores both itself and parseLocation after
// one call to itself
$.mobile.initializePage = function() {

$.mobile.intializePage = origInitializePage;
$.mobile.path.parseLocation = origParseLocation;

return origInitializePage.apply( this, arguments );
};
});

test( "data-url for initial page is urldecoded", function() {
deepEqual(
!!$( ":mobile-page" )
.attr( "data-" + $.mobile.ns + "url" )
.match( /weird%20file%20name/ ),
false,
"Value of 'data-url' attribute is not urlencoded" );
});

})();
1 change: 1 addition & 0 deletions tests/unit/pagecontainer/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,6 @@
<a href="page-does-not-exist.html" id="go-to-nonexistent-page">Go to non-existent page</a>
</div>
</div>
<div data-nstest-role="page" data-nstest-url="Raison d'être.html" class="weird-data-url"></div>
</body>
</html>
14 changes: 14 additions & 0 deletions tests/unit/pagecontainer/pagecontainer_core.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
test( "_find() can handle weird data-url attributes", function() {
deepEqual(
$.mobile.pagecontainer.prototype._find.call({
_createFileUrl: $.mobile.pagecontainer.prototype._createFileUrl,
_createDataUrl: $.mobile.pagecontainer.prototype._createDataUrl,
_getInitialContent: $.mobile.pagecontainer.prototype._getInitialContent,
element: $( "body" ),
_getNs: $.mobile.pagecontainer.prototype._getNs,

}, "Raison d'être.html" )[ 0 ],
$( ".weird-data-url" )[ 0 ],
"Correct element is retrieved when the file name is weird" );
});

( function() {
var originalLoad = $.mobile.pagecontainer.prototype._triggerWithDeprecated
module( "load method", {
Expand Down
4 changes: 0 additions & 4 deletions tests/unit/path/path_core.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@
ok($.mobile.path.isPath('/'), "anything with a slash is a path");
});

test( "path.getFilePath method is working properly", function(){
deepEqual($.mobile.path.getFilePath("foo.html" + "&" + $.mobile.subPageUrlKey ), "foo.html", "returns path without sub page key");
});

test( "path.set method is working properly", function(){
$.mobile.navigate.history.ignoreNextHashChange = false;
$.mobile.path.set("foo");
Expand Down

0 comments on commit d3a412e

Please sign in to comment.