jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.1k stars 5.38k forks source link

URIError parsing url Params encoded with encodeURIComponent #3426

Closed asparagino closed 9 years ago

asparagino commented 9 years ago

It appears that decodeURI is called on urls before _extractParameters calls decodeURIComponent on params extracted from the url. This can cause a URIError when the param has already been partially decoded to contain a character that wouldn't be legal in a URI.

Example url: 'http://foo.com/bar/'+encodeURIComponent('foo%/bar')

Here's a fix that works for us, though I'm not confident I've tested with router and backbone history options we're not using: https://github.com/jashkenas/backbone/pull/3425

braddunbar commented 9 years ago

Hey @asparagino, thanks for reporting this! I've always wondered if there were edge cases in which decodeURI would bite us. But now I know! :smiley:

We actually use decodeURI to deal with unicode escapes in Firefox (and maybe other browsers). I'll do some research to see what we can do here instead.

macgyver commented 9 years ago

+1 for this - I work for a newsy site with "seo friendly" urls, and pages like: https://trove.com/a/1-Tax-Move-Most-Retirees-Must-Make-by-Dec-31-or-Pay-a-50%25-Penalty.cMdE1 will fail, because decodeURIComponent("1-Tax-Move-Most-Retirees-Must-Make-by-Dec-31-or-Pay-a-50%-Penalty") raises an exception, whereas the same URL without the url-encoded percent sign will succeed: https://trove.com/a/1-Tax-Move-Most-Retirees-Must-Make-by-Dec-31-or-Pay-a-50-Penalty.cMdE1

Ideally any valid url would work, regardless of whether the fragment can be url-decoded twice without error.

braddunbar commented 9 years ago

FYI, the decodeURI call was added in #2949 to deal with the fact that some browsers (i.e. Firefox) returns window.location with unicode characters percent encoded.

braddunbar commented 9 years ago

I think this is fixed by #3434. Mind giving that a try?

macgyver commented 9 years ago

I might be making some leaps here but I think the correct behavior is for the browser to return a valid URI or URI components in the Location object attributes, The value of the href attribute MUST be the absolute URI reference

and the percent character by itself (not as part of a uri-encoded octet) is not a valid character in a uri Because the percent "%" character always has the reserved purpose of being the escape indicator, it must be escaped as "%25" in order to be used as data within a URI. {page 9}

I wouldn't expect this to be solved anytime soon so the fix in Backbone is still a great idea, but do you think there would be some value in following up with browser vendors besides Firefox about clarifying and implementing the standard behavior?

braddunbar commented 9 years ago

do you think there would be some value in following up with browser vendors besides Firefox about clarifying and implementing the standard behavior?

I hope so! :smiley:

macgyver commented 9 years ago

bug filed for WebKit: https://bugs.webkit.org/show_bug.cgi?id=140080 and Internet Explorer: https://connect.microsoft.com/IE/feedback/details/1076886

I don't really expect much action on such an old component of the browser but maybe some folks with a lot of experience will at least chime in on the question.

asparagino commented 9 years ago

@macgyver - from debugging this, it looks the problem here is the url decoding process within backbone. Even if you construct a valid url part, by calling encodeURIComponent on each param as you construct a url, decodeURI is called on location.href first, then the url is split apart and decodeURIComponent is called on each param.

So it's not that the browser returns an invalid URI in the location object, backbone has partially decoded and de-escaped %25 to % already with a decodeURI call, and the second attempt with decodeURIComponent is what throws the URIError.

@braddunbar - #3434 looks good. I think it's a better fix than my hack in #3425 and I'll let you know if I pick up any further edge cases in testing.

braddunbar commented 9 years ago

Thanks for taking a look @asparagino!