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', () => {
