fix(UrlMatcher): Properly encode/decode slashes in parameters

Closes #2339 Closes #2172 Closes #2250 Closes #1119
angular-ui · Jan 23, 2016 · 02e9866 · nateabele · Jan 23, 2016 · christopherthielen
1 parent 20d6e24
commit 02e9866
Show file tree

Hide file tree

Showing 4 changed files with 130 additions and 8 deletions.
diff --git a/package.json b/package.json
@@ -58,6 +58,7 @@
     "karma-coffee-preprocessor": "~0.1.0",
     "karma": "~0.10.4",
     "karma-phantomjs-launcher": "~0.1.0",
+    "requirejs": "^2.1.22",
     "load-grunt-tasks": "~0.4.0",
     "grunt-conventional-changelog": "~1.1.0",
     "grunt-ngdocs": "~0.2.5"

diff --git a/src/urlMatcherFactory.js b/src/urlMatcherFactory.js
@@ -256,20 +256,29 @@ UrlMatcher.prototype.exec = function (path, searchParams) {
     return map(allReversed, unquoteDashes).reverse();
   }
 
+  var param, paramVal;
   for (i = 0; i < nPath; i++) {
     paramName = paramNames[i];
-    var param = this.params[paramName];
-    var paramVal = m[i+1];
+    param = this.params[paramName];
+    paramVal = m[i+1];
     // if the param value matches a pre-replace pair, replace the value before decoding.
     for (j = 0; j < param.replace.length; j++) {
       if (param.replace[j].from === paramVal) paramVal = param.replace[j].to;
     }
     if (paramVal && param.array === true) paramVal = decodePathArray(paramVal);
+    if (isDefined(paramVal)) paramVal = param.type.decode(paramVal);
     values[paramName] = param.value(paramVal);
   }
   for (/**/; i < nTotal; i++) {
     paramName = paramNames[i];
     values[paramName] = this.params[paramName].value(searchParams[paramName]);
+    param = this.params[paramName];
+    paramVal = searchParams[paramName];
+    for (j = 0; j < param.replace.length; j++) {
+      if (param.replace[j].from === paramVal) paramVal = param.replace[j].to;
+    }
+    if (isDefined(paramVal)) paramVal = param.type.decode(paramVal);
+    values[paramName] = param.value(paramVal);
   }
 
   return values;
@@ -582,8 +591,12 @@ function $UrlMatcherFactory() {
 
   var isCaseInsensitive = false, isStrictMode = true, defaultSquashPolicy = false;
 
-  function valToString(val) { return val != null ? val.toString().replace(/\//g, "%2F") : val; }
-  function valFromString(val) { return val != null ? val.toString().replace(/%2F/g, "/") : val; }
+  // Use tildes to pre-encode slashes.
+  // If the slashes are simply URLEncoded, the browser can choose to pre-decode them,
+  // and bidirectional encoding/decoding fails.
+  // Tilde was chosen because it's not a RFC 3986 section 2.2 Reserved Character
+  function valToString(val) { return val != null ? val.toString().replace(/~/g, "~~").replace(/\//g, "~2F") : val; }
+  function valFromString(val) { return val != null ? val.toString().replace(/~2F/g, "/").replace(/~~/g, "~") : val; }
 
   var $types = {}, enqueue = true, typeQueue = [], injector, defaultTypes = {
     string: {

diff --git a/test/stateSpec.js b/test/stateSpec.js
@@ -1169,6 +1169,82 @@ describe('state', function () {
       expect($state.current.name).toBe('');
     }));
 
+
+    // Tests for issue #2339
+    describe("slashes in parameter values", function() {
+
+      var $rootScope, $state, $compile;
+      beforeEach(function () {
+
+        stateProvider.state('myState', {
+          url: '/my-state?:previous',
+          controller: function () {
+            log += 'myController;';
+          }
+        });
+
+        inject(function (_$rootScope_, _$state_, _$compile_) {
+          $rootScope = _$rootScope_;
+          $state = _$state_;
+          $compile = _$compile_;
+        });
+        spyOn($state, 'go').andCallThrough();
+        spyOn($state, 'transitionTo').andCallThrough();
+        $compile('<div><div ui-view/></div>')($rootScope);
+        log = '';
+      });
+
+      describe('with no "/" in the params', function () {
+        beforeEach(function () {
+          $state.go('myState',{previous: 'last'});
+          $rootScope.$digest();
+        });
+        it('should call $state.go once', function() {
+          expect($state.go.calls.length).toBe(1);
+        });
+        it('should call $state.transitionTo once', function() {
+          expect($state.transitionTo.calls.length).toBe(1);
+        });
+        it('should call myController once', function() {
+          expect(log).toBe('myController;');
+        });
+      });
+
+      describe('with a "/" in the params', function () {
+        beforeEach(function () {
+          $state.go('myState',{previous: '/last'});
+          $rootScope.$digest();
+        });
+        it('should call $state.go once', function() {
+          expect($state.go.calls.length).toBe(1);
+        });
+        it('should call $state.transitionTo once', function() {
+          expect($state.transitionTo.calls.length).toBe(1);
+        });
+        it('should call myController once', function() {
+          expect(log).toBe('myController;');
+        });
+      });
+
+      describe('with an encoded "/" in the params', function () {
+        beforeEach(function () {
+          $state.go('myState',{previous: encodeURIComponent('/last')});
+          $rootScope.$digest();
+        });
+        it('should call $state.go once', function() {
+          expect($state.go.calls.length).toBe(1);
+        });
+        it('should call $state.transitionTo once', function() {
+          expect($state.transitionTo.calls.length).toBe(1);
+        });
+        it('should call myController once', function() {
+          expect(log).toBe('myController;');
+        });
+      });
+    });
+
+
+
     describe("typed parameter handling", function() {
       beforeEach(function () {
         stateProvider.state({

diff --git a/test/urlMatcherFactorySpec.js b/test/urlMatcherFactorySpec.js
@@ -61,10 +61,40 @@ describe("UrlMatcher", function () {
     expect(matcher.format(array)).toBe('/?foo=bar&foo=baz');
   });
 
-  it("should encode and decode slashes in parameter values", function () {
-    var matcher = new UrlMatcher('/:foo');
-    expect(matcher.format({ foo: "/" })).toBe('/%252F');
-    expect(matcher.format({ foo: "//" })).toBe('/%252F%252F');
+  it("should encode and decode slashes in parameter values as ~2F", function () {
+    var matcher1 = new UrlMatcher('/:foo');
+
+    expect(matcher1.format({ foo: "/" })).toBe('/~2F');
+    expect(matcher1.format({ foo: "//" })).toBe('/~2F~2F');
+
+    expect(matcher1.exec('/')).toBeTruthy();
+    expect(matcher1.exec('//')).not.toBeTruthy();
+
+    expect(matcher1.exec('/').foo).toBe("");
+    expect(matcher1.exec('/123').foo).toBe("123");
+    expect(matcher1.exec('/~2F').foo).toBe("/");
+    expect(matcher1.exec('/123~2F').foo).toBe("123/");
+
+    // param :foo should match between two slashes
+    var matcher2 = new UrlMatcher('/:foo/');
+
+    expect(matcher2.exec('/')).not.toBeTruthy();
+    expect(matcher2.exec('//')).toBeTruthy();
+
+    expect(matcher2.exec('//').foo).toBe("");
+    expect(matcher2.exec('/123/').foo).toBe("123");
+    expect(matcher2.exec('/~2F/').foo).toBe("/");
+    expect(matcher2.exec('/123~2F/').foo).toBe("123/");
+  });
+
+  it("should encode and decode tildes in parameter values as ~~", function () {
+    var matcher1 = new UrlMatcher('/:foo');
+
+    expect(matcher1.format({ foo: "abc" })).toBe('/abc');
+    expect(matcher1.format({ foo: "~abc" })).toBe('/~~abc');
+
+    expect(matcher1.exec('/abc').foo).toBe("abc");
+    expect(matcher1.exec('/~~abc').foo).toBe("~abc");
   });
 
   describe("snake-case parameters", function() {
@@ -306,6 +336,8 @@ describe("UrlMatcher", function () {
 
       $location.url("/foo");
       expect(m.exec($location.path(), $location.search())).toEqual( { param1: undefined } );
+      $location.url("/foo?param1=");
+      expect(m.exec($location.path(), $location.search())).toEqual( { param1: undefined } );
       $location.url("/foo?param1=bar");
       expect(m.exec($location.path(), $location.search())).toEqual( { param1: [ 'bar' ] } );
       $location.url("/foo?param1=bar&param1=baz");