Decode unicode escapes without decoding `%25`. #3434

braddunbar · 2015-01-04T16:13:04Z

The Problem

By using decodeURI to decode unicode escapes, %25 is also decoded. When included in a url parameter, this creates an invalid escape sequence and causes decodeURIComponent to throw.

Sidebar: Should invalid sequences cause the router to throw in the first place? What else can it do?

The Solution

Double escape %25 as %2525 before using decodeURI.

Is this another step down a crazy rabbit hole of URI escaping?

Maybe? I'm pretty happy with the outcome though. I like that we can continue to remove escapes if we come across more problems of this nature. Further, adding it as History#decodeFragment means users can customize it to patch things easily when needed.

I'm going to do a bit more research and try to figure out why the browser escapes unicode in the first place. It effectively prevents us from knowing what was escaped by the user and what was escaped by the browser. 😕

Fixes #3426.

braddunbar · 2015-01-04T16:36:31Z

For some added fun, PhantomJS displays a behavior not found in any actual browser (the tests pass in all the browsers I could find to test). 😒

akre54 · 2015-01-05T04:25:02Z

backbone.js

@@ -1440,6 +1440,11 @@
      return path === this.root && !this.getSearch();
    },

+    // Decode unicode escapes without decoding `%25`.


Can you add a bit more prose here for future-usses who might look at this quizzically? It's always nice to have a bit more context in comments than just looking at the code would give us.

Also, any reason this has to be on the Router prototype instead of living as a standalone helper function?

edit: duh, missed the sentence about overriding. Necessary though? What else would need to override this?

Can you add a bit more prose here for future-usses who might look at this quizzically?

Happy to!

Also, any reason this has to be on the Router prototype instead of living as a standalone helper function?

I started out with a standalone helper but then I started to think what would need to be done if someone finds another problematic escape sequence. With the helper, you'd need to override #getPath and #navigate, which could get ugly really fast. This way, it's a one liner to replace #decodeFragment.

Updated in faea842. It's hard to strike a balance between succinctness and completeness so let me know if I should tweak it! 😃

excellent. thanks!

macgyver · 2015-01-05T14:22:03Z

Works in Chrome 41, Firefox 33, Safari 8, Safari for iOS 7.1, and IE8-10 (the results of extractParams are properly decoded strings when the url is a percent-encoded string, even when including %25)

jashkenas · 2015-01-05T16:33:05Z

Looks fine to me -- but it would be nice to get someone familiar with Browser-unicode-escaping to chime in ... if such a unicorn exists.

akre54 · 2015-01-05T16:50:30Z

praps @mathiasbynens can help per usual?

braddunbar · 2015-01-07T17:09:36Z

FYI, PhantomJS is failing because it decodes the entire url, including the fragment and all the percent-encoded characters. This is not behavior exhibited by any browser and I'm reticent to add a fix for it. If we can't trust it to behave like a browser then what good does it do us?

akre54 · 2015-02-17T22:42:50Z

I'd like to merge this but I'm weary of breaking every Travis build from now on. Do we have a plan around this?

braddunbar · 2015-02-17T22:45:15Z

My thoughts exactly! I'd love some feedback regarding that. PhantomJS is a nice smoke test but I'm not sure what good it does if it's giving false negatives via behavior not exhibited in any real browser.

akre54 · 2015-02-17T22:47:41Z

We've had numerous problems with PhantomJS in the past, and it seems due to slow development most of these bugs are here to stay. @megawac created a pull removing PhantomJS from Underscore in jashkenas/underscore#2054, I wonder if he'll be kind enough to do us the favor here too.

megawac · 2015-02-17T22:50:06Z

Was planning on filing an issue with it later today actually about some of
the things that would be involved

On Tue, Feb 17, 2015 at 5:48 PM, Adam Krebs notifications@github.com
wrote:

We've had numerous problems with PhantomJS in the past, and it seems due
to slow development most of these bugs are here to stay. @megawac
https://github.com/megawac created a pull removing PhantomJS from
Underscore in jashkenas/underscore#2054
jashkenas/underscore#2054, I wonder if he'll be
kind enough to do us the favor here too.

—
Reply to this email directly or view it on GitHub
#3434 (comment).

akre54 · 2015-02-17T22:50:49Z

Excellent. Looking forward.

jridgewell · 2015-02-17T22:56:24Z

I was working on an alternative solution in my url-encoding branch, but never got polished it into a PR. It's working on Chrome, Safari, Firefox, and IE6 (only IE vm I have). I can have it ready tonight, if you'd like.

jridgewell · 2015-02-18T05:40:41Z

Also: this is failing in IE6

I've found two issues:

The percent encoded "?", and turned that into a percent decoded location.search:

location = {
  href: "http://example.com/myyjä/foo%20%25%3F%2f%40%25%20bar",
  hash: "",
  host: "example.com:80",
  search: "?/@% bar",
  fragment: "",
  pathname: "/myyjä/foo%20%",
  protocol: "http:"
}

New issue related to Handle incorrect hash/search values in IE6. #3152?

The pathname is already decoded, so trying to fragment.replace(/%25/g, '%2525') never finds anything.

jridgewell · 2015-02-18T22:47:08Z

Follow up: it's because the anchor tag parser will decode the pathname. Putting it into the hash, then starting up with {pushState: true}, prevents the decode, but requires a bit of boilerplate.

That's not how IE6's actual location behaves. If you were to start a server that responds to any path and navigate to myyjä/foo%20%25%3F%2f%40%25%20bar, location.pathname would not be decoded.

jashkenas · 2015-02-23T15:41:22Z

@braddunbar Any thoughts on @jridgewell's last couple of comments?

jridgewell · 2015-02-23T15:49:00Z

I fixed it by setting location.pathname directly. IE6's actual location object doesn't decode the pathname.

jashkenas · 2015-02-23T15:54:36Z

@jridgewell So this is good to merge in your opinion?

jridgewell · 2015-02-23T16:00:19Z

I think so.

jashkenas · 2015-02-23T16:48:53Z

I wouldn't bet money on this change not invoking some hellacious old-IE or old-Firefox decoding problem down the road. But hey — here's to optimism.

Decode unicode escapes without decoding `%25`.

akre54 · 2015-02-24T02:08:07Z

It looks like this test is failing in phantomjs, Android 4.0.4 and Safari 5 on Win7.

https://travis-ci.org/jashkenas/backbone/builds/51918113

jashkenas · 2015-02-24T04:19:21Z

I wouldn't bet money on this change not invoking some hellacious old-IE or old-Firefox decoding problem down the road.

Do I win a prize? ;)

akre54 · 2015-02-24T14:50:52Z

I dunno. Is 12 hours enough to count as "down the road"?

braddunbar · 2015-02-24T15:09:48Z

Haha! PhantomJS also failed so my first guess is that it's the same as IE6 and we can get around it by setting pathname directly. I'll take a look later on!

jashkenas · 2015-05-13T21:42:56Z

For what it's worth, this patch and new test are currently failing in IE9.

braddunbar · 2015-05-13T21:48:08Z

Hmm…looks ok on BrowserStack.

jashkenas · 2015-05-13T21:53:03Z

It does, but it breaks on my local VM. Go figure.

jridgewell · 2015-05-13T22:51:12Z

All tests are passing on mine.

Decode unicode escapes without decoding %25.

7ed58e4

Fixes #3426.

braddunbar mentioned this pull request Jan 4, 2015

URIError parsing url Params encoded with encodeURIComponent #3426

Closed

akre54 reviewed Jan 5, 2015
View reviewed changes

Elaborate on History#decodeFragment.

faea842

jashkenas mentioned this pull request Jan 6, 2015

Make it possible to decode a url containing an encodeURIComponent encode... #3425

Closed

braddunbar mentioned this pull request Jan 7, 2015

Invalid parameters #3440

Open

jridgewell mentioned this pull request Feb 19, 2015

Fragment Encoding #3506

Closed

akre54 mentioned this pull request Feb 20, 2015

Run test suite using karma #3505

Merged

jridgewell mentioned this pull request Feb 20, 2015

Draft changelog for Backbone 1.2.0 #3285

Merged

3 tasks

jashkenas added the change label Feb 23, 2015

jashkenas added a commit that referenced this pull request Feb 23, 2015

Merge pull request #3434 from braddunbar/decode-fragment

e109f6d

Decode unicode escapes without decoding `%25`.

jashkenas merged commit e109f6d into jashkenas:master Feb 23, 2015

jashkenas added the fixed label Feb 23, 2015

braddunbar deleted the decode-fragment branch February 23, 2015 16:49

jridgewell mentioned this pull request Apr 16, 2015

Uncaught Router crashes at decodeURIComponent #3569

Closed

akre54 mentioned this pull request Jan 29, 2016

Router.navigate executes although trigger:false with encoded hash fragment in Firefox #3941

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode unicode escapes without decoding `%25`. #3434

Decode unicode escapes without decoding `%25`. #3434

braddunbar commented Jan 4, 2015

braddunbar commented Jan 4, 2015

akre54 Jan 5, 2015

akre54 Jan 5, 2015

braddunbar Jan 5, 2015

braddunbar Jan 5, 2015

akre54 Jan 5, 2015

macgyver commented Jan 5, 2015

jashkenas commented Jan 5, 2015

akre54 commented Jan 5, 2015

braddunbar commented Jan 7, 2015

akre54 commented Feb 17, 2015

braddunbar commented Feb 17, 2015

akre54 commented Feb 17, 2015

megawac commented Feb 17, 2015

akre54 commented Feb 17, 2015

jridgewell commented Feb 17, 2015

jridgewell commented Feb 18, 2015

jridgewell commented Feb 18, 2015

jashkenas commented Feb 23, 2015

jridgewell commented Feb 23, 2015

jashkenas commented Feb 23, 2015

jridgewell commented Feb 23, 2015

jashkenas commented Feb 23, 2015

akre54 commented Feb 24, 2015

jashkenas commented Feb 24, 2015

akre54 commented Feb 24, 2015

braddunbar commented Feb 24, 2015

jashkenas commented May 13, 2015

braddunbar commented May 13, 2015

jashkenas commented May 13, 2015

jridgewell commented May 13, 2015

Decode unicode escapes without decoding %25. #3434

Decode unicode escapes without decoding %25. #3434

Conversation

braddunbar commented Jan 4, 2015

The Problem

The Solution

Is this another step down a crazy rabbit hole of URI escaping?

braddunbar commented Jan 4, 2015

akre54 Jan 5, 2015

Choose a reason for hiding this comment

akre54 Jan 5, 2015

Choose a reason for hiding this comment

braddunbar Jan 5, 2015

Choose a reason for hiding this comment

braddunbar Jan 5, 2015

Choose a reason for hiding this comment

akre54 Jan 5, 2015

Choose a reason for hiding this comment

macgyver commented Jan 5, 2015

jashkenas commented Jan 5, 2015

akre54 commented Jan 5, 2015

braddunbar commented Jan 7, 2015

akre54 commented Feb 17, 2015

braddunbar commented Feb 17, 2015

akre54 commented Feb 17, 2015

megawac commented Feb 17, 2015

akre54 commented Feb 17, 2015

jridgewell commented Feb 17, 2015

jridgewell commented Feb 18, 2015

jridgewell commented Feb 18, 2015

jashkenas commented Feb 23, 2015

jridgewell commented Feb 23, 2015

jashkenas commented Feb 23, 2015

jridgewell commented Feb 23, 2015

jashkenas commented Feb 23, 2015

akre54 commented Feb 24, 2015

jashkenas commented Feb 24, 2015

akre54 commented Feb 24, 2015

braddunbar commented Feb 24, 2015

jashkenas commented May 13, 2015

braddunbar commented May 13, 2015

jashkenas commented May 13, 2015

jridgewell commented May 13, 2015

Decode unicode escapes without decoding `%25`. #3434

Decode unicode escapes without decoding `%25`. #3434