fix(router): fix URL serialization so special characters are only encoded where needed (#22337)

jasonaden · alexeagle · commit 789a47ec44c2 · 2018-03-06T06:58:08.000-08:00
This change brings Angular largely in line with how AngularJS previously serialized URLs. This is based on RFC 3986 and resolves issues such as the above #10280 where URLs could be parsed, re-serialized, then parsed again producing a different result on the second parsing. Adjustments to be aware of in this commit: * URI fragments will now serialize the same as query strings * In the URI path or segments (portion prior to query string and/or fragment), the plus sign (`+`) and ampersand (`&`) will appear decoded * In the URL path or segments, parentheses values (`(` and `)`) will now appear percent encoded as `%28` and `%29` respectively * In the URL path or segments, semicolons will be encoded in their percent encoding `%3B` NOTE: Parentheses and semicolons denoting auxillary routes or matrix params will still appear in their decoded form -- only parentheses and semicolons used as values in a segment or key/value pair for matrix params will be encoded. While these changes are not considered breaking because applications should be decoding URLs and key/value pairs, it is possible that some unit tests will break if comparing hard-coded URLs in tests since that hard coded string will represent the old encoding. Therefore we are releasing this fix in the upcoming Angular v6 rather than adding it to a patch for v5. Fixes: #10280 PR Close #22337
diff --git a/packages/router/src/url_tree.ts b/packages/router/src/url_tree.ts
@@ -280,7 +280,7 @@ export class DefaultUrlSerializer implements UrlSerializer {
   serialize(tree: UrlTree): string {
     const segment = `/${serializeSegment(tree.root, true)}`;
     const query = serializeQueryParams(tree.queryParams);
-    const fragment = typeof tree.fragment === `string` ? `#${encodeURI(tree.fragment !)}` : '';
+    const fragment = typeof tree.fragment === `string` ? `#${encodeUriQuery(tree.fragment !)}` : '';
 
     return `${segment}${query}${fragment}`;
   }
@@ -326,42 +326,72 @@ function serializeSegment(segment: UrlSegmentGroup, root: boolean): string {
 }
 
 /**
- * This method is intended for encoding *key* or *value* parts of query component. We need a custom
- * method because encodeURIComponent is too aggressive and encodes stuff that doesn't have to be
- * encoded per http://tools.ietf.org/html/rfc3986:
+ * Encodes a URI string with the default encoding. This function will only ever be called from
+ * `encodeUriQuery` or `encodeUriSegment` as it's the base set of encodings to be used. We need
+ * a custom encoding because encodeURIComponent is too aggressive and encodes stuff that doesn't
+ * have to be encoded per http://tools.ietf.org/html/rfc3986:
  *    query         = *( pchar / "/" / "?" )
  *    pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
  *    unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
  *    pct-encoded   = "%" HEXDIG HEXDIG
  *    sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
  *                     / "*" / "+" / "," / ";" / "="
  */
-export function encode(s: string): string {
+function encodeUriString(s: string): string {
   return encodeURIComponent(s)
       .replace(/%40/g, '@')
       .replace(/%3A/gi, ':')
       .replace(/%24/g, '$')
-      .replace(/%2C/gi, ',')
-      .replace(/%3B/gi, ';');
+      .replace(/%2C/gi, ',');
+}
+
+/**
+ * This function should be used to encode both keys and values in a query string key/value or the
+ * URL fragment. In the following URL, you need to call encodeUriQuery on "k", "v" and "f":
+ *
+ * http://www.site.org/html;mk=mv?k=v#f
+ */
+export function encodeUriQuery(s: string): string {
+  return encodeUriString(s).replace(/%3B/gi, ';');
+}
+
+/**
+ * This function should be run on any URI segment as well as the key and value in a key/value
+ * pair for matrix params. In the following URL, you need to call encodeUriSegment on "html",
+ * "mk", and "mv":
+ *
+ * http://www.site.org/html;mk=mv?k=v#f
+ */
+export function encodeUriSegment(s: string): string {
+  return encodeUriString(s).replace(/\(/g, '%28').replace(/\)/g, '%29').replace(/%26/gi, '&');
 }
 
 export function decode(s: string): string {
   return decodeURIComponent(s);
 }
 
+// Query keys/values should have the "+" replaced first, as "+" in a query string is " ".
+// decodeURIComponent function will not decode "+" as a space.
+export function decodeQuery(s: string): string {
+  return decode(s.replace(/\+/g, '%20'));
+}
+
 export function serializePath(path: UrlSegment): string {
-  return `${encode(path.path)}${serializeParams(path.parameters)}`;
+  return `${encodeUriSegment(path.path)}${serializeMatrixParams(path.parameters)}`;
 }
 
-function serializeParams(params: {[key: string]: string}): string {
-  return Object.keys(params).map(key => `;${encode(key)}=${encode(params[key])}`).join('');
+function serializeMatrixParams(params: {[key: string]: string}): string {
+  return Object.keys(params)
+      .map(key => `;${encodeUriSegment(key)}=${encodeUriSegment(params[key])}`)
+      .join('');
 }
 
 function serializeQueryParams(params: {[key: string]: any}): string {
   const strParams: string[] = Object.keys(params).map((name) => {
     const value = params[name];
-    return Array.isArray(value) ? value.map(v => `${encode(name)}=${encode(v)}`).join('&') :
-                                  `${encode(name)}=${encode(value)}`;
+    return Array.isArray(value) ?
+        value.map(v => `${encodeUriQuery(name)}=${encodeUriQuery(v)}`).join('&') :
+        `${encodeUriQuery(name)}=${encodeUriQuery(value)}`;
   });
 
   return strParams.length ? `?${strParams.join("&")}` : '';
@@ -414,7 +444,7 @@ class UrlParser {
   }
 
   parseFragment(): string|null {
-    return this.consumeOptional('#') ? decodeURI(this.remaining) : null;
+    return this.consumeOptional('#') ? decodeURIComponent(this.remaining) : null;
   }
 
   private parseChildren(): {[outlet: string]: UrlSegmentGroup} {
@@ -506,8 +536,8 @@ class UrlParser {
       }
     }
 
-    const decodedKey = decode(key);
-    const decodedVal = decode(value);
+    const decodedKey = decodeQuery(key);
+    const decodedVal = decodeQuery(value);
 
     if (params.hasOwnProperty(decodedKey)) {
       // Append to existing values
diff --git a/packages/router/test/url_serializer.spec.ts b/packages/router/test/url_serializer.spec.ts
@@ -7,7 +7,7 @@
  */
 
 import {PRIMARY_OUTLET} from '../src/shared';
-import {DefaultUrlSerializer, UrlSegmentGroup, encode, serializePath} from '../src/url_tree';
+import {DefaultUrlSerializer, UrlSegmentGroup, encodeUriQuery, encodeUriSegment, serializePath} from '../src/url_tree';
 
 describe('url serializer', () => {
   const url = new DefaultUrlSerializer();
@@ -189,7 +189,7 @@ describe('url serializer', () => {
   describe('encoding/decoding', () => {
     it('should encode/decode path segments and parameters', () => {
       const u =
-          `/${encode("one two")};${encode("p 1")}=${encode("v 1")};${encode("p 2")}=${encode("v 2")}`;
+          `/${encodeUriSegment("one two")};${encodeUriSegment("p 1")}=${encodeUriSegment("v 1")};${encodeUriSegment("p 2")}=${encodeUriSegment("v 2")}`;
       const tree = url.parse(u);
 
       expect(tree.root.children[PRIMARY_OUTLET].segments[0].path).toEqual('one two');
@@ -199,7 +199,8 @@ describe('url serializer', () => {
     });
 
     it('should encode/decode "slash" in path segments and parameters', () => {
-      const u = `/${encode("one/two")};${encode("p/1")}=${encode("v/1")}/three`;
+      const u =
+          `/${encodeUriSegment("one/two")};${encodeUriSegment("p/1")}=${encodeUriSegment("v/1")}/three`;
       const tree = url.parse(u);
       const segment = tree.root.children[PRIMARY_OUTLET].segments[0];
       expect(segment.path).toEqual('one/two');
@@ -210,7 +211,8 @@ describe('url serializer', () => {
     });
 
     it('should encode/decode query params', () => {
-      const u = `/one?${encode("p 1")}=${encode("v 1")}&${encode("p 2")}=${encode("v 2")}`;
+      const u =
+          `/one?${encodeUriQuery("p 1")}=${encodeUriQuery("v 1")}&${encodeUriQuery("p 2")}=${encodeUriQuery("v 2")}`;
       const tree = url.parse(u);
 
       expect(tree.queryParams).toEqual({'p 1': 'v 1', 'p 2': 'v 2'});
@@ -219,28 +221,143 @@ describe('url serializer', () => {
       expect(url.serialize(tree)).toEqual(u);
     });
 
+    it('should decode spaces in query as %20 or +', () => {
+      const u1 = `/one?foo=bar baz`;
+      const u2 = `/one?foo=bar+baz`;
+      const u3 = `/one?foo=bar%20baz`;
+
+      const u1p = url.parse(u1);
+      const u2p = url.parse(u2);
+      const u3p = url.parse(u3);
+
+      expect(url.serialize(u1p)).toBe(url.serialize(u2p));
+      expect(url.serialize(u2p)).toBe(url.serialize(u3p));
+      expect(u1p.queryParamMap.get('foo')).toBe('bar baz');
+      expect(u2p.queryParamMap.get('foo')).toBe('bar baz');
+      expect(u3p.queryParamMap.get('foo')).toBe('bar baz');
+    });
+
     it('should encode query params leaving sub-delimiters intact', () => {
-      const percentChars = '/?#[]&+= ';
-      const percentCharsEncoded = '%2F%3F%23%5B%5D%26%2B%3D%20';
+      const percentChars = '/?#&+=[] ';
+      const percentCharsEncoded = '%2F%3F%23%26%2B%3D%5B%5D%20';
       const intactChars = '!$\'()*,;:';
       const params = percentChars + intactChars;
       const paramsEncoded = percentCharsEncoded + intactChars;
       const mixedCaseString = 'sTrInG';
 
-      expect(percentCharsEncoded).toEqual(encode(percentChars));
-      expect(intactChars).toEqual(encode(intactChars));
+      expect(percentCharsEncoded).toEqual(encodeUriQuery(percentChars));
+      expect(intactChars).toEqual(encodeUriQuery(intactChars));
       // Verify it replaces repeated characters correctly
-      expect(paramsEncoded + paramsEncoded).toEqual(encode(params + params));
+      expect(paramsEncoded + paramsEncoded).toEqual(encodeUriQuery(params + params));
       // Verify it doesn't change the case of alpha characters
-      expect(mixedCaseString + paramsEncoded).toEqual(encode(mixedCaseString + params));
+      expect(mixedCaseString + paramsEncoded).toEqual(encodeUriQuery(mixedCaseString + params));
     });
 
     it('should encode/decode fragment', () => {
-      const u = `/one#${encodeURI("one two=three four")}`;
+      const u = `/one#${encodeUriQuery('one two=three four')}`;
       const tree = url.parse(u);
 
       expect(tree.fragment).toEqual('one two=three four');
-      expect(url.serialize(tree)).toEqual(u);
+      expect(url.serialize(tree)).toEqual('/one#one%20two%3Dthree%20four');
+    });
+  });
+
+  describe('special character encoding/decoding', () => {
+
+    // Tests specific to https://github.com/angular/angular/issues/10280
+    it('should parse encoded parens in matrix params', () => {
+      const auxRoutesUrl = '/abc;foo=(other:val)';
+      const fooValueUrl = '/abc;foo=%28other:val%29';
+
+      const auxParsed = url.parse(auxRoutesUrl).root;
+      const fooParsed = url.parse(fooValueUrl).root;
+
+
+      // Test base case
+      expect(auxParsed.children[PRIMARY_OUTLET].segments.length).toBe(1);
+      expect(auxParsed.children[PRIMARY_OUTLET].segments[0].path).toBe('abc');
+      expect(auxParsed.children[PRIMARY_OUTLET].segments[0].parameters).toEqual({foo: ''});
+      expect(auxParsed.children['other'].segments.length).toBe(1);
+      expect(auxParsed.children['other'].segments[0].path).toBe('val');
+
+      // Confirm matrix params are URL decoded
+      expect(fooParsed.children[PRIMARY_OUTLET].segments.length).toBe(1);
+      expect(fooParsed.children[PRIMARY_OUTLET].segments[0].path).toBe('abc');
+      expect(fooParsed.children[PRIMARY_OUTLET].segments[0].parameters).toEqual({
+        foo: '(other:val)'
+      });
+    });
+
+    it('should serialize encoded parens in matrix params', () => {
+      const testUrl = '/abc;foo=%28one%29';
+
+      const parsed = url.parse(testUrl);
+
+      expect(url.serialize(parsed)).toBe('/abc;foo=%28one%29');
+    });
+
+    it('should not serialize encoded parens in query params', () => {
+      const testUrl = '/abc?foo=%28one%29';
+
+      const parsed = url.parse(testUrl);
+
+      expect(parsed.queryParams).toEqual({foo: '(one)'});
+
+      expect(url.serialize(parsed)).toBe('/abc?foo=(one)');
+    });
+
+    // Test special characters in general
+
+    // From http://www.ietf.org/rfc/rfc3986.txt
+    const unreserved = `abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-._~`;
+
+    it('should encode a minimal set of special characters in queryParams and fragment', () => {
+      const notEncoded = unreserved + `:@!$'*,();`;
+      const encode = ` +%&=#[]/?`;
+      const encoded = `%20%2B%25%26%3D%23%5B%5D%2F%3F`;
+
+      const parsed = url.parse('/foo');
+
+      parsed.queryParams = {notEncoded, encode};
+
+      expect(url.serialize(parsed)).toBe(`/foo?notEncoded=${notEncoded}&encode=${encoded}`);
+    });
+
+    it('should encode a minimal set of special characters in fragment', () => {
+      const notEncoded = unreserved + `:@!$'*,();`;
+      const encode = ` +%&=#[]/?`;
+      const encoded = `%20%2B%25%26%3D%23%5B%5D%2F%3F`;
+
+      const parsed = url.parse('/foo');
+
+      parsed.fragment = notEncoded + encode;
+
+      expect(url.serialize(parsed)).toBe(`/foo#${notEncoded}${encoded}`);
+    });
+
+    it('should encode minimal special characters plus parens and semi-colon in matrix params',
+       () => {
+         const notEncoded = unreserved + `:@!$'*,&`;
+         const encode = ` /%=#()[];?+`;
+         const encoded = `%20%2F%25%3D%23%28%29%5B%5D%3B%3F%2B`;
+
+         const parsed = url.parse('/foo');
+
+         parsed.root.children[PRIMARY_OUTLET].segments[0].parameters = {notEncoded, encode};
+
+         expect(url.serialize(parsed)).toBe(`/foo;notEncoded=${notEncoded};encode=${encoded}`);
+       });
+
+    it('should encode special characters in the path the same as matrix params', () => {
+      const notEncoded = unreserved + `:@!$'*,&`;
+      const encode = ` /%=#()[];?+`;
+      const encoded = `%20%2F%25%3D%23%28%29%5B%5D%3B%3F%2B`;
+
+      const parsed = url.parse('/foo');
+
+      parsed.root.children[PRIMARY_OUTLET].segments[0].path = notEncoded + encode;
+
+      expect(url.serialize(parsed)).toBe(`/${notEncoded}${encoded}`);
     });
   });
 
