Skip to content

Commit

Permalink
Clarify and fix handling of urlencoding
Browse files Browse the repository at this point in the history
Previously, '%2B' (aka '+') was incorrectly converted to space.  Other
urlencoded characters where affected, too.  The reason was that pagejs's
`Context()` calls `decodeURLEncodedURIComponent()` when initializing
`pathname` and `Route.match()` calls `decodeURLEncodedURIComponent()`
again on the matched `params` after splitting `pathname`.  Since
`decodeURLEncodedURIComponent()` replaces plus with space and the path
is already decoded when calling it on the `params`, pluses were
incorrectly converted to spaces; other urlencoded values were effected
if the second urldecoding was not idempotent.

pagejs's `Route.match()` calls `decodeURIComponent()` to decode
`pathname` before applying the path regex, so there is no need to decode
the entire path when initializing the `Context()`.  pagejs's behavior
seems odd at least; maybe it is a bug.

This commit fixes the problem by disabling pagejs's
`decodeURLEncodedURIComponent()` via the config object.  There is no
reasonable scenario in which the old behavior was correct and is
modified by fixing the duplicate decoding.  params are now correctly
handled, and `FlowRouter` uses `qs.parse()` to handle query strings,
which handles urldecoding, so pagejs should leave the query string
alone.

The semantic of `FlowRouter.path(pathDef, params, queryParams)` is
clarified to urlencode `params` and `queryParams`, but not the
`pathDef`.

The tests are updated to exercise the encoding by using a few special
characters.
  • Loading branch information
sprohaska committed Jun 18, 2015
1 parent db4e960 commit 4a792fd
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 12 deletions.
8 changes: 5 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -398,13 +398,15 @@ console.log(color); // prints "red"

Generate a path from a path definition. Both `params` and `queryParams` are optional.

Special characters in `params` and `queryParams` will be urlencoded.

~~~js
var pathDef = "/blog/:cat/:id";
var params = {cat: "meteor", id: "abc"};
var queryParams = {show: "yes", color: "black"};
var params = {cat: "met eor", id: "abc"};
var queryParams = {show: "y+e=s", color: "black"};

var path = FlowRouter.path(pathDef, params, queryParams);
console.log(path); // prints "/blog/meteor/abc?show=yes&color=black"
console.log(path); // prints "/blog/met%20eor/abc?show=y%2Be%3Ds&color=black"
~~~

If there are no params or queryParams, this will simply return the pathDef as it is.
Expand Down
4 changes: 2 additions & 2 deletions client/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ Router.prototype.path = function(pathDef, fields, queryParams) {
// remove +?*
key = key.replace(/[\+\*\?]+/g, "");

return fields[key] || "";
return encodeURIComponent(fields[key] || "");
});

var strQueryParams = this._qs.stringify(queryParams || {});
Expand Down Expand Up @@ -326,7 +326,7 @@ Router.prototype.initialize = function() {
};

// initialize
this._page();
this._page({decodeURLComponents: false});
};

Router.prototype._buildTracker = function() {
Expand Down
14 changes: 7 additions & 7 deletions test/client/router.core.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ function (test, next) {

FlowRouter.route(pathDef, {
action: function(params) {
test.equal(params.key, "abc");
test.equal(params.key, "abc +@%");
rendered++;
}
});

FlowRouter.go(pathDef, {key: "abc"});
FlowRouter.go(pathDef, {key: "abc +@%"});

setTimeout(function() {
test.equal(rendered, 1);
Expand Down Expand Up @@ -240,10 +240,10 @@ Tinytest.addAsync('Client - Router - setParams - preserve query strings', functi
}
});

FlowRouter.go(pathDef, {cat: "meteor", id: "200"}, {aa: "20"});
FlowRouter.go(pathDef, {cat: "meteor", id: "200 +%"}, {aa: "20 +%"});
setTimeout(function() {
// return done();
var success = FlowRouter.setParams({id: "700"});
var success = FlowRouter.setParams({id: "700 +%"});
test.isTrue(success);
setTimeout(validate, 50);
}, 50);
Expand All @@ -252,9 +252,9 @@ Tinytest.addAsync('Client - Router - setParams - preserve query strings', functi
test.equal(paramsList.length, 2);
test.equal(queryParamsList.length, 2);

test.equal(_.pick(paramsList[0], "id", "cat"), {cat: "meteor", id: "200"});
test.equal(_.pick(paramsList[1], "id", "cat"), {cat: "meteor", id: "700"});
test.equal(queryParamsList, [{aa: "20"}, {aa: "20"}]);
test.equal(_.pick(paramsList[0], "id", "cat"), {cat: "meteor", id: "200 +%"});
test.equal(_.pick(paramsList[1], "id", "cat"), {cat: "meteor", id: "700 +%"});
test.equal(queryParamsList, [{aa: "20 +%"}, {aa: "20 +%"}]);
done();
}
});
Expand Down

0 comments on commit 4a792fd

Please sign in to comment.