{"action":"created","issue":{"url":"https://api.github.com/repos/tildeio/route-recognizer/issues/91","repository_url":"https://api.github.com/repos/tildeio/route-recognizer","labels_url":"https://api.github.com/repos/tildeio/route-recognizer/issues/91/labels{/name}","comments_url":"https://api.github.com/repos/tildeio/route-recognizer/issues/91/comments","events_url":"https://api.github.com/repos/tildeio/route-recognizer/issues/91/events","html_url":"https://github.com/tildeio/route-recognizer/pull/91","id":153133521,"number":91,"title":"normalize route and path, encoded and decode dynamic path segments","user":{"login":"bantic","id":2023,"avatar_url":"https://avatars.githubusercontent.com/u/2023?v=3","gravatar_id":"","url":"https://api.github.com/users/bantic","html_url":"https://github.com/bantic","followers_url":"https://api.github.com/users/bantic/followers","following_url":"https://api.github.com/users/bantic/following{/other_user}","gists_url":"https://api.github.com/users/bantic/gists{/gist_id}","starred_url":"https://api.github.com/users/bantic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bantic/subscriptions","organizations_url":"https://api.github.com/users/bantic/orgs","repos_url":"https://api.github.com/users/bantic/repos","events_url":"https://api.github.com/users/bantic/events{/privacy}","received_events_url":"https://api.github.com/users/bantic/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":15,"created_at":"2016-05-04T22:53:54Z","updated_at":"2016-05-24T00:12:46Z","closed_at":"2016-05-23T18:23:07Z","pull_request":{"url":"https://api.github.com/repos/tildeio/route-recognizer/pulls/91","html_url":"https://github.com/tildeio/route-recognizer/pull/91","diff_url":"https://github.com/tildeio/route-recognizer/pull/91.diff","patch_url":"https://github.com/tildeio/route-recognizer/pull/91.patch"},"body":"This PR supersedes #55. That PR originally attempted to encode/decode dynamic segments in matched paths, but it grew much larger when I attempted to make the solution robust in the face of dynamic segments with percent-encoded percent characters (`encodeURI(\"%\") === \"%25\"`).\r\n\r\nI'm approaching this PR in the spirit of an RFC, with a very detailed rationale for the changes below. If there is an alternative preferred approach for this large-ish change I'm happy to restructure or break apart this PR as necessary.\r\n\r\ncc @krisselden \r\n\r\n## Changes to route-recognizer\r\n\r\nNote about terminology:\r\n\r\n  * **route** refers to a string that is added to the route recognizer (e.g.: the route in `router.add([{ path: '/foo/bar', ... }])` is `/foo/bar`)\r\n  * **path** refers to a string that will be given to the route recognizer to recognize (e.g. the path in `router.recognize('/boo/baz')` is `/boo/baz`)\r\n  * **match**/**matches** refers to when a path is recognized by the route recognizer (e.g. a route recognizer with route `/foo/:bar` would \"match\" the path `/foo/something`)\r\n  * the route recognizer is also sometimes referred to as, simply, the **router**\r\n\r\n### What is covered\r\n\r\nThis PR addresses the following:\r\n\r\n  * Adds many tests for non-standard routes and paths (with percent-encodings and/or unicode characters)\r\n  * Adds normalization for added routes and recognized paths to improve the matching of non-standard routes and paths\r\n  * Fixes a bug where dynamic segments are not percent-encoded during generation\r\n  * Fixes a bug where dynamic segments are not percent-decoded during recognition\r\n  * Adds tests (and some code) to ensure that star segments in routes do *not* get decoded during recognition\r\n  * Adds a flag that can be turned off by consumers to opt-out of the changes introduced here\r\n\r\n### Who would be affected\r\n\r\nIt may only affect users who have routes with:\r\n\r\n  * percent-encoded or unicode characters\r\n  * dynamic segments that may match paths with percent-encoded characters\r\n  * dynamic segments that are generated using values that have characters that should be encoded (e.g. forward slash: `/`)\r\n\r\nIt **may cause breaking changes** for users who have workarounds in place to encode/decode before generation/recognition. The breakage can be fixed by removing the workaround.\r\n\r\nIt will likely **not cause breaking changes** for users who have static routes with percent-encoded or unicode characters. All static route/path combinations that currently match will *also* match after this PR. Static routes that used to be considered distinct (e.g. `/f%20o` and `/f o`) are now normalized to the same thing. Users with affected static route/path combos likely need to do nothing; they may simply end up with superfluous routes that can be removed.\r\n\r\n### Route and Path Normalization\r\n\r\nThe route-recognizer docs do not have a lot to say about routes with non-ascii and percent-encoded characters, so some of the work of this change is to add tests for many possible routes and expected matches that may exist in the wild.\r\nThe changes in this PR are intended to make the route-recognizer more likely to match routes that are added using either percent-encoded or non-encoded characters, and to recognize paths correctly whether they include percent-encoded characters or not.\r\n\r\nThe normalization added is intended to make the router recognize routes that:\r\n\r\n  * are added with unicode characters (e.g. `/café`)\r\n  * are added as percent-encoded strings (e.g. `/caf%C3%A9`)\r\n  * are added with non-encoded \"special\" url characters (e.g. `/foo bar`, `/foo:bar`)\r\n  * are added with percent-encoded \"special\" url characters (e.g. `/foo%20bar`, `/foo%3Abar`)\r\n  * are recognized with paths containing percent-encoded unicode (e.g. `router.recognize('caf%C3%A9')`)\r\n  * are recognized with paths containing non-percent-encoded unicode (e.g. `router.recognize('café')`)\r\n  * are recognized with paths containing certain non-encoded \"special\" url characters (e.g., `router.recognize('foo bar')`, `router.recognize('foo:bar')`)\r\n  * are recognized with paths containing certain percent-encoded \"special\" url characters (e.g., `router.recognize('foo%20bar')`, `router.recognize('foo%3Abar')`)\r\n\r\nThis adds a normalization step for routes when they are added (before they are parsed into segments) and paths when they are recognized.\r\nSimilar to the way one would lower-case two strings to check for case-insensitive equality, this normalization is intended to improve the recognition of\r\nroutes by ensuring there is a higher degree of similarity between routes and paths. All percent-encoded unicode characters and most percent-encoded special url characters\r\nare normalized into equal strings, whether they are added as routes or recognized as paths.\r\n\r\n### Normalization Steps:\r\n\r\nThe normalization for routes and paths is the same:\r\n  * `decodeURI` the string *ignoring percent-encoded percent (`%`) characters* (i.e., *only* ignore the literal value: `%25`). This prevents an issue with encoded percent characters in dynamic segments, where they may be inadvertently doubly-decoded resulting in a URIError.\r\n  * The decode call decodes any encoded unicode characters, so a route that is added as `/café` or `/caf%C3%A9` will be decoded to `/café` and will recognize the path `/café` or `/caf%C3%A9`\r\n  * upper-case all remaining percent-encoded sequences (e.g. `%3a` -> `%3A`). That way these literal encoded sequences will still be matched. A route added as `\"/fo%3a\"` will be matched by incoming paths `\"/fo%3a\"` and `\"/fo%3A\"`\r\n\r\n#### Normalization of URI-reserved characters\r\n\r\nCharacters in the `uriReserved` set (`; / ? : @ & = + $ ,`) are not encoded by `encodeURI`, and their percent-encodings (which should not be used in URLs, unless they are part of a dynamic segment) are likewise not decoded by `decodeURI` (e.g., `encodeURI(':') === ':'` and `decodeURI('%3A') === '%3A'`).\r\n\r\nSince some of these characters have special meaning to the router dsl (`:` at the start of a segment) and to URLs (`?` indicating query params), they need to be encoded when adding them as routes (e.g., to add a route with the literal value \":\" at the start of a segment it must be encoded: `router.add([{ path: \"/foo/%3Abar\" ...`).\r\n\r\n#### Example affected static routes\r\n\r\nExamples of static routes that *would* be affected by this change:\r\n\r\n| static route | path | Old code matches? | New code matches ? | Note |\r\n|--------------|--------------|-------------------|--------------------|------|\r\n| `/fo%20`     | `/fo `       | N                 | Y                  | old code does not normalize route `/fo%20` |\r\n| `/fo%20`     | `/fo%20`     | N                 | Y                  | old code `decodeURI`s path (to `/fo `) before recognizing against route `/fo%20` |\r\n| `/fo%3a`     | `/fo%3A`     | N                 | Y                  | old code is case-sensitive for percent encodings |\r\n| `/fo%25`     | `/fo%25`     | N                 | Y                  | percent-encoded `%` incorrectly decoded by old code |\r\n| `/caf%C3%A9` | `/café`      | N                 | Y                  | old code does not normalize route |\r\n| `/caf%C3%A9` | `/caf%C3%A9` | N                 | Y                  | old code does not normalize route but it does `decodeURI` path to `/café`, which doesn't match |\r\n\r\n#### Example unaffected static routes\r\n \r\nExamples of static routes that *would not* be affected by this change:\r\n\r\n| static route | path | Old code matches? | New code matches ? | Note |\r\n|-------------|----------|-------------------|--------------------|------|\r\n| `/café`     | `/caf%C3A9`  | Y             | Y                  | old code `decodeURI`s the path (to `/café`) so it will be recognized |\r\n| `/fo:`      | `/fo:`       | Y             | Y                  | exact match |\r\n| `/fo `      | `/fo%20`     | Y             | Y                  | old code `decodeURI`s the path (to `/fo `) so it is recognized |\r\n| `/fo `      | `/fo `       | Y             | Y                  | exact match |\r\n| `/fo%3A`    | `/fo%3A`     | Y             | Y                  | exact match (`decodeURI('%3A') === '%3A'`) |\r\n| `/fo%3A`    | `/fo:`       | N             | N                  | old and new code both don't change the `%3A` and `:` in route and path, respectively |\r\n| `/café`     | `/café`      | Y             | Y                  | exact match |\r\n\r\n### Dynamic Route Generation (bugfix)\r\n\r\nChanges generation to call `encodeURIComponent` on dynamic route segments.\r\nPreviously, the parameter would be interpolated into the generated route *unchanged*.\r\n\r\nThis should be considered a *bugfix* because the previous behavior made it\r\npossible for the router to generate a route that it later wouldn't be able to recognize.\r\n\r\nIt is a *breaking bugfix* for users that have workarounds in place to `encodeURIComponent`\r\ntheir parameters before generating links from them. Those workarounds would lead to\r\ndoubly-encoded parameters in URLs.\r\n\r\n#### Example route generation\r\n\r\nGiven a route with a dynamic segment: `\"/post/:id\"`, the following values of `id` will\r\ngenerate the routes shown.\r\n\r\n| `id` value  | Old route         | New route | Note  |\r\n|-------------|-------------------|-----------|-------|\r\n| `abc/def` | `/post/abc/def` | `/post/abc%2Fdef` | The old value is no longer recognizable by this route |\r\n| `100%` | `/post/100%` | `/post/100%25` | The old value is an invalid URI |\r\n\r\n### Dynamic Route Parameter Decoding (bugfix)\r\n\r\nChanges route recognition to call `decodeURIComponent` on\r\nrecognized parameters from dynamic route segments.\r\n\r\nPreviously, the captured parameters would be returned unmodified. However, before\r\ndynamic parameters are parsed, the `path` passed to `router.recognize` would be\r\n\"normalized\" with `decodeURI` (to handle non-ascii unicode characters), so the\r\ncaptured parameter was not always the same as the value in the `path`. See below.\r\n\r\nThe route-recognizer's docs do not discuss the expected behavior in these scenarios.\r\nThis change brings the behavior into closer alignment with behavior of other well-known\r\nrouters like the Rails router. As such, it could be considered either an *enhancement*\r\nor a *bugfix*.\r\n\r\nThis is a *breaking* change for users that are working around this issue by calling `decodeURIComponent` on the params returned by the route. With the new code, this could result in unexpected values caused by double-decoding the value.\r\n\r\n#### Example dynamic route parameter decoding\r\n\r\nGiven a route with a dynamic segment: `\"/post/:id\"`, the following paths will yield\r\nthe shown values for the `id` param.\r\n\r\n| path              | Old `id` param | New `id` param | Note |\r\n--------------------|----------------|----------------|------|\r\n| `/post/abc%def\"` | `abc%2Fdef`    | `abc/def`      | A user with a workaround would be unaffected. |\r\n| `post/café`      | `café`         | `café`         |      |\r\n| `post/caf%C3%A9` | `café`         | `café`         | the old code's call to `decodeURI(path)` decodes the percent-encoded `%C3A9` to `é`. A user with a workaround would be unaffected. |\r\n| `post/%3A1`      | `%3A1`         | `:1`           | the old code's call to `decodeURI` does not decode `%3A` to `:`. A user with a workaround would be unaffected.     |\r\n| `post/100%25`    | `100%`         | `100%`         | the old code's call to `decodeURI(path)` decodes `%25` to `%`. A user with a workaround in place would get an error when trying to `decodeURIComponent(\"100%\")` |\r\n\r\n#### Special case: doubly-encoded segments\r\n\r\nThe incorrect decoding of encoded percent characters also affects dynamic segments that have been mulitply encoded.\r\nFor example, if the post id is a url that itself has a `encodeURIComponent`-encoded parameter, such as:\r\n````\r\nlet url = \"http://example.com/post/\" + encodeURIComponent(\"http://other-url.com\")\r\nlet encodedUrl = encodeURIComponent(encodedUrl);\r\n```\r\n\r\nThe reserved characters from \"http://other-url.com\" will be doubly-encoded in `encodedUrl`. E.g., the `//` in `http://other-url.com` is first encoded as `%2F%2F`,\r\nand then the `%` characters are encoded again to `%25`: `encodeURIComponent(encodeURIComponent(\"//\")) === \"%252F%252F\"`.\r\n\r\nThe old code would call `decodeURI` once on `encodedUrl`, resulting in the following (incorrect) value for id: `id === encodeURIComponent(\"http://example.com/post/http://other.url\")`.\r\nThe new code avoids decoding the percent characters in the initial path, and it results in the following (correct) value for id: `id === \"http://example.com/post/\" + encodeURIComponent(\"http://other.url\")`.\r\n\r\n### Glob Routes (Star Segments)\r\n\r\nThis PR adds some tests and code to ensure that glob routes (e.g. `/*catchall`) do not decode their matching segments.\r\nE.g., given the catchall route and path `/abc/foo%2Fbar`, the matched param `catchall` is `abc/foo%2Fbar`, not `abc/foo/bar`.\r\n"},"comment":{"url":"https://api.github.com/repos/tildeio/route-recognizer/issues/comments/221132342","html_url":"https://github.com/tildeio/route-recognizer/pull/91#issuecomment-221132342","issue_url":"https://api.github.com/repos/tildeio/route-recognizer/issues/91","id":221132342,"user":{"login":"mixonic","id":8752,"avatar_url":"https://avatars.githubusercontent.com/u/8752?v=3","gravatar_id":"","url":"https://api.github.com/users/mixonic","html_url":"https://github.com/mixonic","followers_url":"https://api.github.com/users/mixonic/followers","following_url":"https://api.github.com/users/mixonic/following{/other_user}","gists_url":"https://api.github.com/users/mixonic/gists{/gist_id}","starred_url":"https://api.github.com/users/mixonic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mixonic/subscriptions","organizations_url":"https://api.github.com/users/mixonic/orgs","repos_url":"https://api.github.com/users/mixonic/repos","events_url":"https://api.github.com/users/mixonic/events{/privacy}","received_events_url":"https://api.github.com/users/mixonic/received_events","type":"User","site_admin":false},"created_at":"2016-05-24T00:12:46Z","updated_at":"2016-05-24T00:12:46Z","body":"(fwiw @bantic is mostly offline for this week)"}}
{"actor":{"display_login":"mixonic"}}