Stop double encoding in the URL Also stop encoding @. This will make URLs more readable. Consistently don't encode /. This was enabled by earlier changes that make sure that all route pattern are regular expressions. The special :param matching of page.js did not work with `/` chars. Release-Notes: skip Change-Id: I15f864505811fa2c04cfb5bbb00b643464f40f3c
diff --git a/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list.ts b/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list.ts index 944cd7a..a2de840 100644 --- a/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list.ts +++ b/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list.ts
@@ -173,7 +173,7 @@ } private computePluginUrl(id: string) { - return getBaseUrl() + '/' + encodeURL(id, true); + return getBaseUrl() + '/' + encodeURL(id); } }
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts index 981cbe4..41ad3f8 100644 --- a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts +++ b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
@@ -438,7 +438,7 @@ // TODO: Replace with `createRepoUrl()`, but be aware that `encodeURL()` // gets `false` as a second parameter here. The router pattern in gr-router // does not handle the filter URLs, if the repo is not encoded! - return `/admin/repos/${encodeURL(repo ?? '', false)},${detailType}`; + return `/admin/repos/${encodeURL(repo ?? '')},${detailType}`; } private computeWeblink(repo: ProjectInfo | BranchInfo | TagInfo) {
diff --git a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.ts b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.ts index 975dd3b..5f0a171 100644 --- a/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.ts +++ b/polygerrit-ui/app/elements/admin/gr-rule-editor/gr-rule-editor.ts
@@ -343,7 +343,7 @@ // private but used in test computeGroupPath(groupId?: string) { if (!groupId) return; - return `${getBaseUrl()}/admin/groups/${encodeURL(groupId, true)}`; + return `${getBaseUrl()}/admin/groups/${encodeURL(groupId)}`; } // private but used in test
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts index 94467b5..a841958 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -35,6 +35,7 @@ import {fireAlert, firePageError} from '../../../utils/event-util'; import {windowLocationReload} from '../../../utils/dom-util'; import { + encodeURL, getBaseUrl, PatchRangeParams, toPath, @@ -969,7 +970,7 @@ if (ctx.params[0].toLowerCase() === 'self') { this.redirectToLogin(ctx.canonicalPath); } else { - this.redirect('/q/owner:' + encodeURIComponent(ctx.params[0])); + this.redirect('/q/owner:' + encodeURL(ctx.params[0])); } } else { const state: DashboardViewState = { @@ -1275,7 +1276,7 @@ } handleChangeNumberLegacyRoute(ctx: PageContext) { - this.redirect('/c/' + encodeURIComponent(ctx.params[0])); + this.redirect('/c/' + ctx.params[0]); } handleChangeRoute(ctx: PageContext) { @@ -1578,9 +1579,7 @@ } handleDocumentationSearchRedirectRoute(ctx: PageContext) { - this.redirect( - '/Documentation/q/filter:' + encodeURIComponent(ctx.params[0]) - ); + this.redirect('/Documentation/q/filter:' + encodeURL(ctx.params[0])); } handleDocumentationRedirectRoute(ctx: PageContext) {
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts index 6cad45c..3ca7bdc 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -373,9 +373,9 @@ view: GerritView.SETTINGS, emailToken: 'asdf', }); - await checkUrlToState('/settings/VE/asdf%2520qwer', { + await checkUrlToState('/settings/VE/asdf%40qwer', { view: GerritView.SETTINGS, - emailToken: 'asdf+qwer', + emailToken: 'asdf@qwer', }); }); @@ -1008,8 +1008,8 @@ '/Documentation/q/filter:asdf' ); await checkRedirect( - '/Documentation/q/as%20df', - '/Documentation/q/filter:as%20df' + '/Documentation/q/as%3Fdf', + '/Documentation/q/filter:as%3Fdf' ); await checkUrlToState('/Documentation/q/filter:', {
diff --git a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label_test.ts b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label_test.ts index e7c0536..71f8391 100644 --- a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label_test.ts +++ b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label_test.ts
@@ -88,7 +88,7 @@ /* HTML */ ` <div class="container"> <gr-hovercard-account for="hovercardTarget"></gr-hovercard-account> - <a class="ownerLink" href="/q/owner:user-31%2540" tabindex="-1"> + <a class="ownerLink" href="/q/owner:user-31@" tabindex="-1"> <span class="hovercardTargetWrapper"> <gr-avatar hidden="" imagesize="32"> </gr-avatar> <span
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents_test.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents_test.ts index b217562..a1ac781 100644 --- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents_test.ts +++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents_test.ts
@@ -79,7 +79,7 @@ </div> <div class="links"> <gr-icon icon="link" class="linkIcon"></gr-icon> - <a href="/q/owner:kermit%2540gmail.com">Changes</a> + <a href="/q/owner:kermit@gmail.com">Changes</a> · <a href="/dashboard/31415926535">Dashboard</a> </div> @@ -114,7 +114,7 @@ </div> <div class="links"> <gr-icon class="linkIcon" icon="link"> </gr-icon> - <a href="/q/owner:kermit%2540gmail.com"> Changes </a> + <a href="/q/owner:kermit@gmail.com"> Changes </a> · <a href="/dashboard/31415926535"> Dashboard </a> </div>
diff --git a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts index f5837e3..7e90c8d 100644 --- a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts +++ b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts
@@ -169,7 +169,7 @@ if (!this.isConnected || !this.path) return; if (filter) { this.getNavigation().setUrl( - `${this.path}/q/filter:${encodeURL(filter, false)}` + `${this.path}/q/filter:${encodeURL(filter)}` ); return; } @@ -190,7 +190,7 @@ const newOffset = Math.max(0, offset + this.itemsPerPage * direction); let href = getBaseUrl() + (this.path ?? ''); if (this.filter) { - href += '/q/filter:' + encodeURL(this.filter, false); + href += '/q/filter:' + encodeURL(this.filter); } if (newOffset > 0) { href += `,${newOffset}`;
diff --git a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view_test.ts b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view_test.ts index ecc1c25..cce2840 100644 --- a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view_test.ts +++ b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view_test.ts
@@ -77,7 +77,7 @@ element.filter = 'plugins/'; assert.equal( element.computeNavLink(1), - '/admin/projects/q/filter:plugins%252F,50' + '/admin/projects/q/filter:plugins/,50' ); });
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts index a206037..153777f 100644 --- a/polygerrit-ui/app/models/views/change.ts +++ b/polygerrit-ui/app/models/views/change.ts
@@ -196,7 +196,7 @@ childView: ChangeChildView.DIFF, }); - const path = `/${encodeURL(state.diffView?.path ?? '', true)}`; + const path = `/${encodeURL(state.diffView?.path ?? '')}`; let suffix = ''; // TODO: Move creating of comment URLs to a separate function. We are @@ -226,7 +226,7 @@ patchNum: obj.patchNum ?? EDIT, }); - const path = `/${encodeURL(state.editView?.path ?? '', true)}`; + const path = `/${encodeURL(state.editView?.path ?? '')}`; const line = state.editView?.lineNum; const suffix = line ? `#${line}` : ''; @@ -242,7 +242,7 @@ if (range.length) range = '/' + range; let repo = ''; - if (state.repo) repo = `${encodeURL(state.repo, true)}/+/`; + if (state.repo) repo = `${encodeURL(state.repo)}/+/`; return `${getBaseUrl()}/c/${repo}${state.changeNum}${range}`; }
diff --git a/polygerrit-ui/app/models/views/change_test.ts b/polygerrit-ui/app/models/views/change_test.ts index 837e362..1e71bbd 100644 --- a/polygerrit-ui/app/models/views/change_test.ts +++ b/polygerrit-ui/app/models/views/change_test.ts
@@ -74,7 +74,7 @@ ...createChangeViewState(), repo: 'x+/y+/z+/w' as RepoName, }; - assert.equal(createChangeUrl(state), '/c/x%252B/y%252B/z%252B/w/+/42'); + assert.equal(createChangeUrl(state), '/c/x%2B/y%2B/z%2B/w/+/42'); }); test('createDiffUrl', () => { @@ -85,7 +85,7 @@ }; assert.equal( createDiffUrl(params), - '/c/test-project/+/42/12/x%252By/path.cpp' + '/c/test-project/+/42/12/x%2By/path.cpp' ); window.CANONICAL_PATH = '/base'; @@ -93,10 +93,10 @@ window.CANONICAL_PATH = undefined; params.repo = 'test' as RepoName; - assert.equal(createDiffUrl(params), '/c/test/+/42/12/x%252By/path.cpp'); + assert.equal(createDiffUrl(params), '/c/test/+/42/12/x%2By/path.cpp'); params.basePatchNum = 6 as BasePatchSetNum; - assert.equal(createDiffUrl(params), '/c/test/+/42/6..12/x%252By/path.cpp'); + assert.equal(createDiffUrl(params), '/c/test/+/42/6..12/x%2By/path.cpp'); params.diffView = { path: 'foo bar/my+file.txt%', @@ -105,7 +105,7 @@ delete params.basePatchNum; assert.equal( createDiffUrl(params), - '/c/test/+/42/2/foo+bar/my%252Bfile.txt%2525' + '/c/test/+/42/2/foo+bar/my%2Bfile.txt%2525' ); params.diffView = { @@ -129,7 +129,7 @@ repo: 'x+/y' as RepoName, diffView: {path: 'x+y/path.cpp'}, }; - assert.equal(createDiffUrl(params), '/c/x%252B/y/+/42/12/x%252By/path.cpp'); + assert.equal(createDiffUrl(params), '/c/x%2B/y/+/42/12/x%2By/path.cpp'); }); test('createEditUrl', () => { @@ -140,7 +140,7 @@ }; assert.equal( createEditUrl(params), - '/c/test-project/+/42/12/x%252By/path.cpp,edit#31' + '/c/test-project/+/42/12/x%2By/path.cpp,edit#31' ); window.CANONICAL_PATH = '/base';
diff --git a/polygerrit-ui/app/models/views/dashboard.ts b/polygerrit-ui/app/models/views/dashboard.ts index d9ff2d2..74523db 100644 --- a/polygerrit-ui/app/models/views/dashboard.ts +++ b/polygerrit-ui/app/models/views/dashboard.ts
@@ -33,7 +33,7 @@ const query = repoName ? section.query.replace(REPO_TOKEN_PATTERN, repoName) : section.query; - return encodeURIComponent(section.name) + '=' + encodeURIComponent(query); + return encodeURL(section.name) + '=' + encodeURL(query); }); } @@ -43,13 +43,13 @@ // Custom dashboard. const queryParams = sectionsToEncodedParams(state.sections, repoName); if (state.title) { - queryParams.push('title=' + encodeURIComponent(state.title)); + queryParams.push('title=' + encodeURL(state.title)); } const user = state.user ? state.user : ''; return `${getBaseUrl()}/dashboard/${user}?${queryParams.join('&')}`; } else if (repoName) { // Project dashboard. - const encodedRepo = encodeURL(repoName, true); + const encodedRepo = encodeURL(repoName); return `${getBaseUrl()}/p/${encodedRepo}/+/dashboard/${state.dashboard}`; } else { // User dashboard.
diff --git a/polygerrit-ui/app/models/views/dashboard_test.ts b/polygerrit-ui/app/models/views/dashboard_test.ts index 86bb5c0..a7620dd 100644 --- a/polygerrit-ui/app/models/views/dashboard_test.ts +++ b/polygerrit-ui/app/models/views/dashboard_test.ts
@@ -34,7 +34,7 @@ }; assert.equal( createDashboardUrl(state), - '/dashboard/?section%201=query%201§ion%202=query%202' + '/dashboard/?section+1=query+1§ion+2=query+2' ); }); @@ -48,8 +48,8 @@ }; assert.equal( createDashboardUrl(state), - '/dashboard/?section%201=query%201%20repo-name&' + - 'section%202=query%202%20repo-name' + '/dashboard/?section+1=query+1+repo-name&' + + 'section+2=query+2+repo-name' ); }); @@ -61,7 +61,7 @@ }; assert.equal( createDashboardUrl(state), - '/dashboard/user?name=query&title=custom%20dashboard' + '/dashboard/user?name=query&title=custom+dashboard' ); });
diff --git a/polygerrit-ui/app/models/views/group.ts b/polygerrit-ui/app/models/views/group.ts index 277bcff..2ab3735 100644 --- a/polygerrit-ui/app/models/views/group.ts +++ b/polygerrit-ui/app/models/views/group.ts
@@ -22,7 +22,7 @@ } export function createGroupUrl(state: Omit<GroupViewState, 'view'>) { - let url = `/admin/groups/${encodeURL(`${state.groupId}`, true)}`; + let url = `/admin/groups/${encodeURL(`${state.groupId}`)}`; if (state.detail === GroupDetailView.MEMBERS) { url += ',members'; } else if (state.detail === GroupDetailView.LOG) {
diff --git a/polygerrit-ui/app/models/views/repo.ts b/polygerrit-ui/app/models/views/repo.ts index ec65ca1..66bf5bf 100644 --- a/polygerrit-ui/app/models/views/repo.ts +++ b/polygerrit-ui/app/models/views/repo.ts
@@ -36,7 +36,7 @@ } export function createRepoUrl(state: Omit<RepoViewState, 'view'>) { - let url = `/admin/repos/${encodeURL(`${state.repo}`, true)}`; + let url = `/admin/repos/${encodeURL(`${state.repo}`)}`; if (state.detail === RepoDetailView.GENERAL) { url += ',general'; } else if (state.detail === RepoDetailView.ACCESS) {
diff --git a/polygerrit-ui/app/models/views/search.ts b/polygerrit-ui/app/models/views/search.ts index c5d394d..48775ce 100644 --- a/polygerrit-ui/app/models/views/search.ts +++ b/polygerrit-ui/app/models/views/search.ts
@@ -86,42 +86,39 @@ } if (params.query) { - return `${getBaseUrl()}/q/${encodeURL(params.query, true)}${offsetExpr}`; + return `${getBaseUrl()}/q/${encodeURL(params.query)}${offsetExpr}`; } const operators: string[] = []; if (params.owner) { - operators.push('owner:' + encodeURL(params.owner, false)); + operators.push('owner:' + encodeURL(params.owner)); } if (params.repo) { - operators.push('project:' + encodeURL(params.repo, false)); + operators.push('project:' + encodeURL(params.repo)); } if (params.branch) { - operators.push('branch:' + encodeURL(params.branch, false)); + operators.push('branch:' + encodeURL(params.branch)); } if (params.topic) { operators.push( - 'topic:' + - escapeAndWrapSearchOperatorValue(encodeURL(params.topic, false)) + 'topic:' + escapeAndWrapSearchOperatorValue(encodeURL(params.topic)) ); } if (params.hashtag) { operators.push( 'hashtag:' + escapeAndWrapSearchOperatorValue( - encodeURL(params.hashtag.toLowerCase(), false) + encodeURL(params.hashtag.toLowerCase()) ) ); } if (params.statuses) { if (params.statuses.length === 1) { - operators.push('status:' + encodeURL(params.statuses[0], false)); + operators.push('status:' + encodeURL(params.statuses[0])); } else if (params.statuses.length > 1) { operators.push( '(' + - params.statuses - .map(s => `status:${encodeURL(s, false)}`) - .join(' OR ') + + params.statuses.map(s => `status:${encodeURL(s)}`).join(' OR ') + ')' ); }
diff --git a/polygerrit-ui/app/models/views/search_test.ts b/polygerrit-ui/app/models/views/search_test.ts index 6809225..e2e02f5 100644 --- a/polygerrit-ui/app/models/views/search_test.ts +++ b/polygerrit-ui/app/models/views/search_test.ts
@@ -56,10 +56,10 @@ // The presence of the query param overrides other options. options.query = 'foo$bar'; - assert.equal(createSearchUrl(options), '/q/foo%2524bar'); + assert.equal(createSearchUrl(options), '/q/foo%24bar'); options.offset = 100; - assert.equal(createSearchUrl(options), '/q/foo%2524bar,100'); + assert.equal(createSearchUrl(options), '/q/foo%24bar,100'); options = {statuses: ['a', 'b', 'c']}; assert.equal(
diff --git a/polygerrit-ui/app/utils/url-util.ts b/polygerrit-ui/app/utils/url-util.ts index 8564c3f..22a9721 100644 --- a/polygerrit-ui/app/utils/url-util.ts +++ b/polygerrit-ui/app/utils/url-util.ts
@@ -81,18 +81,56 @@ } /** - * Pretty-encodes a URL. Double-encodes the string, and then replaces - * benevolent characters for legibility. + * Encodes *parts* of a URL. See inline comments below for the details. + * Note specifically that ? & = # are encoded. So this is very close to + * encodeURIComponent() with some tweaks. */ -export function encodeURL(url: string, replaceSlashes?: boolean): string { - // @see Issue 4255 regarding double-encoding. - let output = encodeURIComponent(encodeURIComponent(url)); - // @see Issue 4577 regarding more readable URLs. - output = output.replace(/%253A/g, ':'); - output = output.replace(/%2520/g, '+'); - if (replaceSlashes) { - output = output.replace(/%252F/g, '/'); - } +export function encodeURL(url: string): string { + // page.js decodes the entire URL, and then decodes once more the + // individual regex matching groups. It uses `decodeURIComponent()`, which + // will choke on singular `%` chars without two trailing digits. We prefer + // to not double encode *everything* (just for readaiblity and simplicity), + // but `%` *must* be double encoded. + let output = url.replaceAll('%', '%25'); + + // This escapes ALL characters EXCEPT: + // A–Z a–z 0–9 - _ . ! ~ * ' ( ) + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent + output = encodeURIComponent(output); + + // If we would use `encodeURI()` instead of `encodeURIComponent()`, then we + // would also NOT encode: + // ; / ? : @ & = + $ , # + // + // That would be more readable, but for example ? and & have special meaning + // in the URL, so they must be encoded. Let's discuss all these chars and + // decide whether we have to encode them or not. + // + // ? & = # have to be encoded. Otherwise we might mess up the URL. + // + // : @ do not have to be encoded, because we are only dealing with path, + // query and fragment of the URL, not with scheme, user, host, port. + // For search queries it is much nicer to not encode those chars, think of + // searching for `owner:spearce@spearce.org`. + // + // / does not have to be encoded, because we don't care about individual path + // components. File path and repo names are so much nicer to read without / + // being encoded! + // + // + must be encoded, because we want to use it instead of %20 for spaces, see + // below. + // + // ; $ , probably don't have to be encoded, but we don't bother about them + // much, so we don't reverse the encoding here, but we don't think it would + // cause any harm, if we did. + output = output.replace(/%3A/g, ':'); + output = output.replace(/%40/g, '@'); + output = output.replace(/%2F/g, '/'); + + // page.js replaces `+` by ` ` in addition to calling `decodeURIComponent()`. + // So we can use `+` to increase readability. + output = output.replace(/%20/g, '+'); + return output; }
diff --git a/polygerrit-ui/app/utils/url-util_test.ts b/polygerrit-ui/app/utils/url-util_test.ts index a014dc2..16f85dd 100644 --- a/polygerrit-ui/app/utils/url-util_test.ts +++ b/polygerrit-ui/app/utils/url-util_test.ts
@@ -102,28 +102,21 @@ suite('url encoding and decoding tests', () => { suite('encodeURL', () => { - test('double encodes', () => { - assert.equal(encodeURL('abc?123'), 'abc%253F123'); - assert.equal(encodeURL('def/ghi'), 'def%252Fghi'); - assert.equal(encodeURL('jkl'), 'jkl'); - assert.equal(encodeURL(''), ''); + test('does not encode alphanumeric chars', () => { + assert.equal(encodeURL("AZaz09-_.!~*'()"), "AZaz09-_.!~*'()"); }); - test('does not convert colons', () => { - assert.equal(encodeURL('mno:pqr'), 'mno:pqr'); + test('double encodes %', () => { + assert.equal(encodeURL('abc%def'), 'abc%2525def'); }); - test('converts spaces to +', () => { + test('does not encode colon and slash', () => { + assert.equal(encodeURL(':/'), ':/'); + }); + + test('encodes spaces as +', () => { assert.equal(encodeURL('words with spaces'), 'words+with+spaces'); }); - - test('does not convert slashes when configured', () => { - assert.equal(encodeURL('stu/vwx', true), 'stu/vwx'); - }); - - test('does not convert slashes when configured', () => { - assert.equal(encodeURL('stu/vwx', true), 'stu/vwx'); - }); }); suite('singleDecodeUrl', () => {