Correctly store data-url and correctly select for it #7474

gabrielschulhof · 2014-06-05T16:01:00Z

This also removes any lingering references to subPageUrlKey from the code.

coveralls · 2014-06-05T20:27:13Z

Coverage increased (+0.01%) when pulling b29e5bf on 1383-escape-urls-in-data-url-selector into 1ed6f92 on master.

coveralls · 2014-06-06T08:13:13Z

Coverage decreased (-6.17%) when pulling 3899174 on 1383-escape-urls-in-data-url-selector into 1ed6f92 on master.

dpolivy · 2014-06-06T21:08:20Z

js/widgets/pagecontainer.js

 				page, initialContent = this._getInitialContent();

 			// Check to see if the page already exists in the DOM.
 			// 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 ) + "']" );


Shouldn't this use decodedDataUrl too?

No, because we stipulate that dataUrl must not contain a a uri-encoded string. So, when we retrieve the attribute value, we should be able to assume that it's not encoded. If it is, it's not our fault.

This value is created from the passed in absUrl, not the data attribute -- i.e. this is the page we are navigating TO. So this is the first check to see if it is in the DOM.

In my form submission scenario, absUrl contains the serialized form parameters, which includes the encoded characters.

I have now modified convertUrlToDataUrl() to always decode. This fixes your concern here, and it also addresses the potential double-decoding problem.

coveralls · 2014-06-06T22:27:04Z

Coverage decreased (-6.12%) when pulling 5769c0c on 1383-escape-urls-in-data-url-selector into 1ed6f92 on master.

coveralls · 2014-06-06T23:13:20Z

Coverage decreased (-0.06%) when pulling 047db48 on 1383-escape-urls-in-data-url-selector into 2f7d1c6 on master.

coveralls · 2014-06-06T23:13:21Z

Coverage decreased (-0.06%) when pulling 047db48 on 1383-escape-urls-in-data-url-selector into 2f7d1c6 on master.

dpolivy · 2014-06-09T21:41:12Z

Took a quick look at the code and I think the current set of changes address all of the known issues I've found. Nice work, and thanks! Will try to incorporate the changes into my local build and do additional testing.

gabrielschulhof · 2014-06-10T18:48:28Z

@dpolivy Excellent!

dpolivy · 2014-06-11T22:19:33Z

After using this a bit more, I am seeing one change in behavior from before: if one attempts to navigate to a URL which has non-UTF8 encoded characters (e.g. from escape()), then the new $.mobile.path.convertUrlToDataUrl with throw a URIError: URI Malformed exception. Before, this just seemed to work OK (even though the URL was "invalid").

To repro: navigate to a URL with a string like the following in it: Domaine+du+P%E9ga%FC.

Thoughts on the best way to handle this?

arschmitz · 2014-08-21T17:29:17Z

I think this looks good however some of the names of the new files contain spaces can you fix this please.

dpolivy · 2014-08-21T18:42:26Z

Can you also review my comment on the associated issue (#1383 (comment)) -- I think there's a small corner case that needs attention to fully fix this.

arschmitz · 2014-08-22T02:06:05Z

@dpolivy thanks ill take a look at your comment

Closes gh-7474 Fixes gh-1383

This also removes the possibility that a URL gets double-decoded.

gabrielschulhof · 2014-08-26T06:35:19Z

I believe I've addressed @dpolivy's comment.

arschmitz · 2014-09-04T12:40:54Z

You still have file names containing spaces please fix these.

arschmitz · 2014-09-04T17:35:47Z

👍

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

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 Closes jquery-archivegh-7474 Fixes jquery-archivegh-1383

gabrielschulhof added Action: PR needs reviewing labels Jun 5, 2014

gabrielschulhof added this to the 1.4.4 milestone Jun 5, 2014

gabrielschulhof assigned arschmitz Jun 5, 2014

dpolivy reviewed Jun 6, 2014
View reviewed changes

gabrielschulhof force-pushed the 1383-escape-urls-in-data-url-selector branch 2 times, most recently from 13290a4 to c3b1939 Compare August 23, 2014 16:06

Gabriel Schulhof added 3 commits August 23, 2014 19:06

Pagecontainer: Remove code dealing with subPageUrlKey special case

2e71645

Path: Remove code dealing with subPageUrlKey

34e62bc

Path: No longer test for correct stripping of subPageUrlKey

54b5fb8

gabrielschulhof force-pushed the 1383-escape-urls-in-data-url-selector branch 2 times, most recently from 79abe6f to 963c021 Compare August 26, 2014 06:33

Gabriel Schulhof added 6 commits August 26, 2014 09:34

Pagecontainer: Escape dataUrl when trying to find page based on it

9469109

Closes gh-7474 Fixes gh-1383

Navigation: Make sure location is encoded when going to a funky page

c8609e7

Pagecontainer: Test that _find() throws not when looking for weird URLs

59f0999

Init: Correctly assign data-url for initial page

dfc1ba2

Init: Make sure "data-url" attribute for initial page is decoded

2874fa8

Pagecontainer: Decode URI when assigning to "data-url" during _parse()

f9f72a9

Gabriel Schulhof added 7 commits August 26, 2014 09:34

Pagecontainer: Use decoded URL to build selector for data-url

54745c4

Pagecontainer: Call convertUrlToDataUrl() via _createDataUrl()

8b42621

Path: Always uri-decode when computing dataUrl

684b032

Pagecontainer: Remove extra decoding step, now part of _createDataUrl()

e5f0d45

This also removes the possibility that a URL gets double-decoded.

Pagecontainer: Test behavior of _find() in the face of weird URLs

73cfd55

Navigation: Use data-url retrieved from Ajax request in its encoded form

0e85785

Pagecontainer: Pass encoded URL to $.mobile.navigate()

4483140

gabrielschulhof force-pushed the 1383-escape-urls-in-data-url-selector branch from 963c021 to 4483140 Compare August 26, 2014 06:34

arschmitz added Action: PR needs modification and removed Action: PR needs reviewing labels Sep 4, 2014

arschmitz assigned gabrielschulhof and unassigned arschmitz Sep 4, 2014

Init: Rename JS file with spaces in it

6b66496

gabrielschulhof added Action: PR needs reviewing and removed Action: PR needs modification labels Sep 4, 2014

gabrielschulhof assigned arschmitz and unassigned gabrielschulhof Sep 4, 2014

gabrielschulhof closed this in e412102 Sep 4, 2014

gabrielschulhof deleted the 1383-escape-urls-in-data-url-selector branch September 4, 2014 18:22

jaspermdegroot removed the Action: PR needs reviewing label Aug 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly store data-url and correctly select for it #7474

Correctly store data-url and correctly select for it #7474

gabrielschulhof commented Jun 5, 2014

coveralls commented Jun 5, 2014

coveralls commented Jun 6, 2014

dpolivy Jun 6, 2014

gabrielschulhof Jun 6, 2014

dpolivy Jun 6, 2014

gabrielschulhof Jun 6, 2014

coveralls commented Jun 6, 2014

coveralls commented Jun 6, 2014

coveralls commented Jun 6, 2014

dpolivy commented Jun 9, 2014

gabrielschulhof commented Jun 10, 2014

dpolivy commented Jun 11, 2014

arschmitz commented Aug 21, 2014

dpolivy commented Aug 21, 2014

arschmitz commented Aug 22, 2014

gabrielschulhof commented Aug 26, 2014

arschmitz commented Sep 4, 2014

arschmitz commented Sep 4, 2014

Correctly store data-url and correctly select for it #7474

Correctly store data-url and correctly select for it #7474

Conversation

gabrielschulhof commented Jun 5, 2014

coveralls commented Jun 5, 2014

coveralls commented Jun 6, 2014

dpolivy Jun 6, 2014

Choose a reason for hiding this comment

gabrielschulhof Jun 6, 2014

Choose a reason for hiding this comment

dpolivy Jun 6, 2014

Choose a reason for hiding this comment

gabrielschulhof Jun 6, 2014

Choose a reason for hiding this comment

coveralls commented Jun 6, 2014

coveralls commented Jun 6, 2014

coveralls commented Jun 6, 2014

dpolivy commented Jun 9, 2014

gabrielschulhof commented Jun 10, 2014

dpolivy commented Jun 11, 2014

arschmitz commented Aug 21, 2014

dpolivy commented Aug 21, 2014

arschmitz commented Aug 22, 2014

gabrielschulhof commented Aug 26, 2014

arschmitz commented Sep 4, 2014

arschmitz commented Sep 4, 2014