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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changePage breaks when data hash contains parentheses/apostropes #1383

Closed
konklone opened this Issue Apr 6, 2011 · 23 comments

Comments

Projects
None yet
10 participants
@konklone

konklone commented Apr 6, 2011

The following causes a syntax error in alpha 4:

$.mobile.changePage({url: "index.html", data: {foo: "bar(baz)"}, type: "get"});

In Firefox 4, the error message in Firebug's console is:

"Syntax error, unrecognized expression: data-url='index.html?foo=bar(baz]')"

Attempts to change the page without the parentheses work fine, such as:

$.mobile.changePage({url: "index.html", data: {foo: "bar"}, type: "get"});
@mckamey

This comment has been minimized.

Show comment
Hide comment
@mckamey

mckamey Apr 19, 2011

Note: Apostrope/Single-Quote also break changePage.

Issues #1383, #1408, #1468 are related.

Issue #1468 describes a workaround.

mckamey commented Apr 19, 2011

Note: Apostrope/Single-Quote also break changePage.

Issues #1383, #1408, #1468 are related.

Issue #1468 describes a workaround.

@anthony-clark

This comment has been minimized.

Show comment
Hide comment
@anthony-clark

anthony-clark Jun 17, 2011

On my implementation it's only the closing parenthesis that is causes an error. I have a search from that will pass results to a search page. Queries for "Some search (more" work whereas queries for "Some search (more)" break pageChange.

anthony-clark commented Jun 17, 2011

On my implementation it's only the closing parenthesis that is causes an error. I have a search from that will pass results to a search page. Queries for "Some search (more" work whereas queries for "Some search (more)" break pageChange.

@anthony-clark

This comment has been minimized.

Show comment
Hide comment
@anthony-clark

anthony-clark Jun 17, 2011

I was able to get my forms working by running replacements on the serialized data bound to form submit event using the suggestion here: #1468

My new data looks like:
var newData = $this.serialize().replace(''','%27').replace('(','%28').replace(')','%29');

I updated the data in the function: $( "form" ).live('submit', function( event ) {});

Hope that helps people having a similar problem.

anthony-clark commented Jun 17, 2011

I was able to get my forms working by running replacements on the serialized data bound to form submit event using the suggestion here: #1468

My new data looks like:
var newData = $this.serialize().replace(''','%27').replace('(','%28').replace(')','%29');

I updated the data in the function: $( "form" ).live('submit', function( event ) {});

Hope that helps people having a similar problem.

@ghost ghost assigned johnbender Aug 8, 2011

michaelhull added a commit to michaelhull/jquery-mobile that referenced this issue Sep 3, 2011

Fix for issue jquery#1383 - Avoids infinite loop when dataUrl contain…
…s ) or '. I suggest switching to a hashtable like issue jquery#1727 suggests for a permanent solution.
@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Dec 5, 2011

Contributor

#2384

Additional discussion in that pull request on possible solutions (eg escaping the url).

Contributor

johnbender commented Dec 5, 2011

#2384

Additional discussion in that pull request on possible solutions (eg escaping the url).

@toddparker

This comment has been minimized.

Show comment
Hide comment
@toddparker

toddparker Feb 15, 2012

Contributor

@johnbender - Is this still an open issue?

Contributor

toddparker commented Feb 15, 2012

@johnbender - Is this still an open issue?

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Apr 19, 2012

Contributor

@mrlyons

The pr didn't get merged for the reason cited in the pull request: we need to address this in a couple places by escaping the hashes.

Contributor

johnbender commented Apr 19, 2012

@mrlyons

The pr didn't get merged for the reason cited in the pull request: we need to address this in a couple places by escaping the hashes.

@smjgithub

This comment has been minimized.

Show comment
Hide comment
@smjgithub

smjgithub Jul 10, 2012

I'm currently using jquery.mobile-1.1.0 and I am serving it locally so that I could apply the partial fix from #1383.

Is this fix or a more complete version of the fix likely to make it into any official patch so that I can go back to serving jquery.mobile remotely?

smjgithub commented Jul 10, 2012

I'm currently using jquery.mobile-1.1.0 and I am serving it locally so that I could apply the partial fix from #1383.

Is this fix or a more complete version of the fix likely to make it into any official patch so that I can go back to serving jquery.mobile remotely?

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Sep 24, 2012

Member

Updated the topic of this ticket so it also mentions "apostropes".

Member

jaspermdegroot commented Sep 24, 2012

Updated the topic of this ticket so it also mentions "apostropes".

@SixtoSaez

This comment has been minimized.

Show comment
Hide comment
@SixtoSaez

SixtoSaez Jan 2, 2013

Ran across this issue in a pretty common usage scenario that may justify raising the priority of this issue. Our app passes names and such in the query string of the href attribute of a tags (see sample page: http://pastebin.com/TKybiryG). The name values (i.e. o'reilly) can sometimes contain single quotes which are a valid query string characters. Typical URL encode functions do not escape this character to a % encoding.

SixtoSaez commented Jan 2, 2013

Ran across this issue in a pretty common usage scenario that may justify raising the priority of this issue. Our app passes names and such in the query string of the href attribute of a tags (see sample page: http://pastebin.com/TKybiryG). The name values (i.e. o'reilly) can sometimes contain single quotes which are a valid query string characters. Typical URL encode functions do not escape this character to a % encoding.

@dpolivy

This comment has been minimized.

Show comment
Hide comment
@dpolivy

dpolivy May 29, 2014

This still seems to exist in 1.4.2, and I'm hitting it with apostrophes in my query string portion of the url. Is there any progress on incorporating @SixtoSaez's PR or providing another fix?

dpolivy commented May 29, 2014

This still seems to exist in 1.4.2, and I'm hitting it with apostrophes in my query string portion of the url. Is there any progress on incorporating @SixtoSaez's PR or providing another fix?

@dpolivy

This comment has been minimized.

Show comment
Hide comment
@dpolivy

dpolivy Jun 4, 2014

Borrowing the "fix" from #1468, I've come up with a patch that appears to work (apply it here):

        _createDataUrl: function( absoluteUrl ) {
            return $.mobile.path.convertUrlToDataUrl( absoluteUrl ).replace('\'','%27').replace('(','%28').replace(')','%29');
        },

I haven't run the jQM test suite, and am not so familiar with how the data-url attribute is used, so it'd be great if one of the jQM team members could take a look. I'm happy to submit a PR for this if that would be easier/faster for getting a fix incorporated.

@johnbender / @uGoMobi can you take a quick look?

dpolivy commented Jun 4, 2014

Borrowing the "fix" from #1468, I've come up with a patch that appears to work (apply it here):

        _createDataUrl: function( absoluteUrl ) {
            return $.mobile.path.convertUrlToDataUrl( absoluteUrl ).replace('\'','%27').replace('(','%28').replace(')','%29');
        },

I haven't run the jQM test suite, and am not so familiar with how the data-url attribute is used, so it'd be great if one of the jQM team members could take a look. I'm happy to submit a PR for this if that would be easier/faster for getting a fix incorporated.

@johnbender / @uGoMobi can you take a quick look?

@jaspermdegroot jaspermdegroot added this to the 1.5.0 milestone Jun 5, 2014

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jun 5, 2014

Contributor

The problem is a lot simpler than when you have a query string. Even if the external page you're attempting to reach has special characters, such as ' the selector will fail. The problem is we're assuming a URL can be stored in a DOM attribute and a query can be constructed for that DOM attribute by simple string concatenation. This is wrong.

Contributor

gabrielschulhof commented Jun 5, 2014

The problem is a lot simpler than when you have a query string. Even if the external page you're attempting to reach has special characters, such as ' the selector will fail. The problem is we're assuming a URL can be stored in a DOM attribute and a query can be constructed for that DOM attribute by simple string concatenation. This is wrong.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jun 6, 2014

Contributor

@dpolivy can you please try http://view.jquerymobile.com/1383-escape-urls-in-data-url-selector/dist/jquery.mobile.js? Things should no longer be uri-encoded, even upon form submit.

Contributor

gabrielschulhof commented Jun 6, 2014

@dpolivy can you please try http://view.jquerymobile.com/1383-escape-urls-in-data-url-selector/dist/jquery.mobile.js? Things should no longer be uri-encoded, even upon form submit.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Jun 6, 2014

Contributor

@konklone @mckamey @Drifting could you also please check out the link I gave @dpolivy above and tell me whether this addresses your problems?

Contributor

gabrielschulhof commented Jun 6, 2014

@konklone @mckamey @Drifting could you also please check out the link I gave @dpolivy above and tell me whether this addresses your problems?

@konklone

This comment has been minimized.

Show comment
Hide comment
@konklone

konklone Jun 7, 2014

...it's been 3 years since I reported this. I've moved on with my life.

konklone commented Jun 7, 2014

...it's been 3 years since I reported this. I've moved on with my life.

@dpolivy

This comment has been minimized.

Show comment
Hide comment
@dpolivy

dpolivy Aug 20, 2014

@gabrielschulhof I think there may still be an issue lurking in the code here. When I have a URL that has a valid % in it, and provide that as a data-url in my markup, I get an Uncaught URIError: URI malformed from the last line of $.mobile.path.path.convertUrlToDataUrl.

As per our earlier discussions, and the "correct" behavior, the data-url specified in markup is NOT url encoded -- just HTML entity encoded where necessary. It looks like the code in _loadSuccess is extracting the data-url from the markup -- should it be url encoded at that time for consistency?

https://github.com/jquery/jquery-mobile/blob/master/js/widgets/pagecontainer.js#L526

dpolivy commented Aug 20, 2014

@gabrielschulhof I think there may still be an issue lurking in the code here. When I have a URL that has a valid % in it, and provide that as a data-url in my markup, I get an Uncaught URIError: URI malformed from the last line of $.mobile.path.path.convertUrlToDataUrl.

As per our earlier discussions, and the "correct" behavior, the data-url specified in markup is NOT url encoded -- just HTML entity encoded where necessary. It looks like the code in _loadSuccess is extracting the data-url from the markup -- should it be url encoded at that time for consistency?

https://github.com/jquery/jquery-mobile/blob/master/js/widgets/pagecontainer.js#L526

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Aug 22, 2014

Contributor

@dpolivy can you please describe a specific scenario where you have a "valid %" as part of your initial markup? I've tried the setup below, but that does not result in any JS exceptions.

rootDirectory
    +----- weird-file-name.html
    +----- 100%test/
        +----- index.html

weird-file-name.html:

...
<a href="100%25test/index.html">100%test/index.html</a>
...

When I open weird-file-name.html in the browser and I click the link about, the file 100%tests/index.html is loaded without any problems.

Contributor

gabrielschulhof commented Aug 22, 2014

@dpolivy can you please describe a specific scenario where you have a "valid %" as part of your initial markup? I've tried the setup below, but that does not result in any JS exceptions.

rootDirectory
    +----- weird-file-name.html
    +----- 100%test/
        +----- index.html

weird-file-name.html:

...
<a href="100%25test/index.html">100%test/index.html</a>
...

When I open weird-file-name.html in the browser and I click the link about, the file 100%tests/index.html is loaded without any problems.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Aug 22, 2014

Contributor

It seems that the anchor's href needs to be URL encoded.

Contributor

gabrielschulhof commented Aug 22, 2014

It seems that the anchor's href needs to be URL encoded.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Aug 22, 2014

Contributor

OK. I see. If I repeat the above scenario with the data-url inside 100%test/index.html specified a priori then I get the exception.

Contributor

gabrielschulhof commented Aug 22, 2014

OK. I see. If I repeat the above scenario with the data-url inside 100%test/index.html specified a priori then I get the exception.

@dpolivy

This comment has been minimized.

Show comment
Hide comment
@dpolivy

dpolivy Aug 22, 2014

@gabrielschulhof Exactly; if you specify the data-url on the page, which we determined should NOT be url encoded, then you'll get the exception. I put in a simple fix to call encodeURI on the value returned from text() here, and it seems to fix it. I think it's safe since the proper behavior is for that value to be unencoded?

dpolivy commented Aug 22, 2014

@gabrielschulhof Exactly; if you specify the data-url on the page, which we determined should NOT be url encoded, then you'll get the exception. I put in a simple fix to call encodeURI on the value returned from text() here, and it seems to fix it. I think it's safe since the proper behavior is for that value to be unencoded?

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Aug 22, 2014

Contributor

Well, that's easy enough to fix if I url-encode data-url upon retrieving it from the HTML coming in via Ajax. However, I've discovered a problem with this fix: It causes an unencoded URL to end up in the location when you click on a link to a page behind a URL with a percent sign in it. So, I need to review the whole fix to find the spot where I've introduced that mistake, because on master the correct, url-encoded URL ends up in the location - as it should be.

Contributor

gabrielschulhof commented Aug 22, 2014

Well, that's easy enough to fix if I url-encode data-url upon retrieving it from the HTML coming in via Ajax. However, I've discovered a problem with this fix: It causes an unencoded URL to end up in the location when you click on a link to a page behind a URL with a percent sign in it. So, I need to review the whole fix to find the spot where I've introduced that mistake, because on master the correct, url-encoded URL ends up in the location - as it should be.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Aug 22, 2014

Contributor

@dpolivy Exactly. That's how I fixed it, but, like I said, I found another problem with my fix. A regression I must avoid and against which I need to test before this can land.

Contributor

gabrielschulhof commented Aug 22, 2014

@dpolivy Exactly. That's how I fixed it, but, like I said, I found another problem with my fix. A regression I must avoid and against which I need to test before this can land.

gabrielschulhof added a commit that referenced this issue Sep 4, 2014

Navigation: Correctly (un)escape data-url all throughout the code
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

agcolom added a commit to agcolom/jquery-mobile that referenced this issue Nov 26, 2014

Navigation: Correctly (un)escape data-url all throughout the code
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 jquerygh-7474
Fixes jquerygh-1383

gabrielschulhof added a commit to jquery/api.jquerymobile.com that referenced this issue Dec 17, 2014

gabrielschulhof added a commit to jquery/api.jquerymobile.com that referenced this issue Dec 30, 2014

gabrielschulhof added a commit to jquery/api.jquerymobile.com that referenced this issue Dec 30, 2014

@dpolivy

This comment has been minimized.

Show comment
Hide comment
@dpolivy

dpolivy Mar 17, 2015

@gabrielschulhof What's the correct way to handle ampersand/equals within query parameters in URLs? I've got all of these changes in my build of jQM, but in situations where I have an ampersand within a query string value, the data-url contains just the HTML encoded entity, e.g. data-url="/my/path?q=red+&amp;+blue". Thus, when the URL is updated (via replaceState), it ends up being unencoded, so if you refresh the page, the query string is broken because the & is parsed as another parameter.

Is there a proper way to handle this case?

dpolivy commented Mar 17, 2015

@gabrielschulhof What's the correct way to handle ampersand/equals within query parameters in URLs? I've got all of these changes in my build of jQM, but in situations where I have an ampersand within a query string value, the data-url contains just the HTML encoded entity, e.g. data-url="/my/path?q=red+&amp;+blue". Thus, when the URL is updated (via replaceState), it ends up being unencoded, so if you refresh the page, the query string is broken because the & is parsed as another parameter.

Is there a proper way to handle this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment