fix dynamic segments with URL-encoding #72
Conversation
| @@ -497,7 +497,7 @@ RouteRecognizer.prototype = { | |||
| queryParams = this.parseQueryString(queryString); | |||
| } | |||
|
|
|||
| path = decodeURI(path); | |||
| path = decodeURIComponent(path); | |||
There was a problem hiding this comment.
Alternatively, we could leave this as decodeURI and change findHandler to decode the individual components:
// findHandler:
var currentCaptureValue = captures[currentCapture++];
if (currentCaptureValue != null) {
currentCaptureValue = decodeURIComponent(currentCaptureValue);
}
params[names[j]] = currentCaptureValue;but that doesn't fix the problem with adding /foo%20bar as a static segment.
There was a problem hiding this comment.
@mmun this is the fix, but you leave the decodeURI, decodeURI only decodes characters that are not required for URI structure, it will not touch structural escapes, so it can be done before parsing. The above would handle currentCaptureValue.
```js
decodeURI('/foo%20bar') // '/foo bar'
decodeURI('/foo%23bar') // '/foo%23bar'
decodeURI('/foo%3fbar') // '/foo%3fbar'
decodeURIComponent('/foo%20bar') // '/foo bar'
decodeURIComponent('/foo%23bar') // '/foo#bar'
decodeURIComponent('/foo%3fbar') // '/foo?bar'
```
This fixes two separate, but related problems:
1. `router.add([{ path: '/foo bar?baz', }])` did not previously
match `/foo%20%3fbaz`
2. `router.add([{ path: '/foo/:bar', ... }])`, when given
`/foo/a%20b%23c%3fd`, would return params `{ bar: 'a b%23c%3fd' }`
| test("A route with URL-encoded parts recognizes", function() { | ||
| var handler = {}; | ||
| var router = new RouteRecognizer(); | ||
| router.add([{ path: "/foo bar?baz", handler: handler }]); |
There was a problem hiding this comment.
I'd prefer to use another character besides ? or # (how about $?)
In fact we should be asserting that these two character do not appear in a path (we may want to reserve them for some DSL later on) as they have very explicit URL parsing behaviors.
There was a problem hiding this comment.
I think url special characters should have to be encoded if they are meant to be matched as part of the path.
|
We need to parse structure, basically all of the path component should be parsed then within each part, we should decode the path, I'm not a fan of the DSL recognizing structural chars. a dynamic segment should be able to contain a url if you wanted it too. |
|
Superseded by #55 which is fixing all of the encoding issues in a more holistic manner. |
Addresses, at least in part, #71. I'd like to have people run these tests on more platforms to make sure it's consistent. This is a replacement for #70 and #67 and possibly #55