From fa974c7d4e2df273a0bd4ed17b808ed12757870c Mon Sep 17 00:00:00 2001 From: Jason Aden Date: Tue, 20 Feb 2018 15:08:41 -0800 Subject: [PATCH] fix(router): fix URL serialization so special characters are only encoded where needed (#22337) 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 --- packages/router/src/url_tree.ts | 62 ++++++--- packages/router/test/url_serializer.spec.ts | 141 ++++++++++++++++++-- 2 files changed, 175 insertions(+), 28 deletions(-) diff --git a/packages/router/src/url_tree.ts b/packages/router/src/url_tree.ts index 63c3a7ea01..13f0ab46e1 100644 --- 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,9 +326,10 @@ 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 / "-" / "." / "_" / "~" @@ -336,32 +337,61 @@ function serializeSegment(segment: UrlSegmentGroup, root: boolean): string { * 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); } -export function serializePath(path: UrlSegment): string { - return `${encode(path.path)}${serializeParams(path.parameters)}`; +// 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')); } -function serializeParams(params: {[key: string]: string}): string { - return Object.keys(params).map(key => `;${encode(key)}=${encode(params[key])}`).join(''); +export function serializePath(path: UrlSegment): string { + return `${encodeUriSegment(path.path)}${serializeMatrixParams(path.parameters)}`; +} + +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 index a318011b10..f71538cc3a 100644 --- 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}`); }); });