Merge "Remove html commentlink functionality."
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index d50e69d..3e82254 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1727,9 +1727,7 @@
will match typical Gerrit Change-Id values and create a hyperlink
to changes which reference it. The second configuration 'bugzilla'
will hyperlink terms such as 'bug 42' to an external bug tracker,
-supplying the argument record number '42' for display. The third
-configuration 'tracker' uses raw HTML to more precisely control
-how the replacement is displayed to the user.
+supplying the argument record number '42' for display.
commentlinks supports link:#reloadConfig[configuration reloads]. Though a
link:cmd-flush-caches.html[flush-caches] of "projects" is needed for the
@@ -1746,16 +1744,10 @@
prefix = $1
suffix = $4
text = $2$3
-
-[commentlink "tracker"]
- match = ([Bb]ug:\\s+)(\\d+)
- html = $1<a href=\"http://trak.example.com/$2\">$2</a>
----
Comment links can also be specified in `project.config` and sections in
-children override those in parents. The only restriction is that to
-avoid injecting arbitrary user-supplied HTML in the page, comment links
-defined in `project.config` may only supply `link`, not `html`.
+children override those in parents.
[[commentlink.name.match]]commentlink.<name>.match::
+
@@ -1789,38 +1781,23 @@
+
The URL to direct the user to whenever the regular expression is
matched. Groups in the match expression may be accessed as `$'n'`.
-+
-The link property is used only when the html property is not present.
[[commentlink.name.prefix]]commentlink.<name>.prefix::
+
The text inserted before the link. Groups in the match expression may be
accessed as `$'n'`.
-+
-The link property is used only when the html property is not present.
[[commentlink.name.suffix]]commentlink.<name>.suffix::
+
The text inserted after the link. Groups in the match expression may be
accessed as `$'n'`.
-+
-The link property is used only when the html property is not present.
[[commentlink.name.text]]commentlink.<name>.text::
+
The text content of the link. Groups in the match expression may be
accessed as `$'n'`.
+
-The link property is used only when the html property is not present.
-
-[[commentlink.name.html]]commentlink.<name>.html::
-+
-HTML to replace the entire matched string with. If present,
-this property overrides the link property above. Groups in the
-match expression may be accessed as `$'n'`.
-+
-The configuration file eats double quotes, so escaping them as
-`\"` is necessary to protect them from the parser.
+If not specified defaults to `$&` (the matched text).
[[commentlink.name.enabled]]commentlink.<name>.enabled::
+
@@ -1828,11 +1805,6 @@
section in a parent or the site-wide config that is disabled by
specifying `enabled = true`.
+
-Disabling sections in `gerrit.config` can be used by site administrators
-to create a library of comment links with `html` set that are not
-user-supplied and thus can be verified to be XSS-free, but are only
-enabled for a subset of projects.
-+
By default, true.
+
Note that the names and contents of disabled sections are visible even
diff --git a/java/com/google/gerrit/entities/StoredCommentLinkInfo.java b/java/com/google/gerrit/entities/StoredCommentLinkInfo.java
index 75bc034..5ee76da 100644
--- a/java/com/google/gerrit/entities/StoredCommentLinkInfo.java
+++ b/java/com/google/gerrit/entities/StoredCommentLinkInfo.java
@@ -31,7 +31,7 @@
public abstract String getMatch();
/**
- * The link to replace the match with. This can only be set if html is {@code null}.
+ * The link to replace the match with.
*
* <p>The constructed link is using {@link #getLink()} {@link #getPrefix()} {@link #getSuffix()}
* and {@link #getText()}, and has the shape of
@@ -41,31 +41,18 @@
@Nullable
public abstract String getLink();
- /**
- * The text before the link tag that the match is replaced with. This can only be set if link is
- * not {@code null}.
- */
+ /** The optional text before the link tag that the match is replaced with. */
@Nullable
public abstract String getPrefix();
- /**
- * The text after the link tag that the match is replaced with. This can only be set if link is
- * not {@code null}.
- */
+ /** The optional text after the link tag that the match is replaced with. */
@Nullable
public abstract String getSuffix();
- /**
- * The content of the link tag that the match is replaced with. This can only be set if link is
- * not {@code null}.
- */
+ /** The content of the link tag that the match is replaced with. If not set full match is used. */
@Nullable
public abstract String getText();
- /** The html to replace the match with. This can only be set if link is {@code null}. */
- @Nullable
- public abstract String getHtml();
-
/** Weather this comment link is active. {@code null} means true. */
@Nullable
public abstract Boolean getEnabled();
@@ -103,7 +90,6 @@
.setPrefix(src.prefix)
.setSuffix(src.suffix)
.setText(src.text)
- .setHtml(src.html)
.setEnabled(enabled)
.setOverrideOnly(false)
.build();
@@ -118,7 +104,6 @@
info.prefix = getPrefix();
info.suffix = getSuffix();
info.text = getText();
- info.html = getHtml();
info.enabled = getEnabled();
return info;
}
@@ -137,25 +122,21 @@
public abstract Builder setText(@Nullable String value);
- public abstract Builder setHtml(@Nullable String value);
-
public abstract Builder setEnabled(@Nullable Boolean value);
public abstract Builder setOverrideOnly(boolean value);
public StoredCommentLinkInfo build() {
checkArgument(getName() != null, "invalid commentlink.name");
- setLink(Strings.emptyToNull(getLink()));
setPrefix(Strings.emptyToNull(getPrefix()));
setSuffix(Strings.emptyToNull(getSuffix()));
setText(Strings.emptyToNull(getText()));
- setHtml(Strings.emptyToNull(getHtml()));
if (!getOverrideOnly()) {
checkArgument(
!Strings.isNullOrEmpty(getMatch()), "invalid commentlink.%s.match", getName());
checkArgument(
- (getLink() != null && getHtml() == null) || (getLink() == null && getHtml() != null),
- "commentlink.%s must have either link or html",
+ !Strings.isNullOrEmpty(getLink()),
+ "commentlink.%s must have link specified",
getName());
}
return autoBuild();
@@ -175,8 +156,6 @@
protected abstract String getText();
- protected abstract String getHtml();
-
protected abstract boolean getOverrideOnly();
}
}
diff --git a/java/com/google/gerrit/extensions/api/projects/CommentLinkInfo.java b/java/com/google/gerrit/extensions/api/projects/CommentLinkInfo.java
index b45fcee..5c47ac3 100644
--- a/java/com/google/gerrit/extensions/api/projects/CommentLinkInfo.java
+++ b/java/com/google/gerrit/extensions/api/projects/CommentLinkInfo.java
@@ -24,7 +24,6 @@
public String prefix;
public String suffix;
public String text;
- public String html;
public Boolean enabled; // null means true
public transient String name;
@@ -41,7 +40,6 @@
&& Objects.equals(this.prefix, that.prefix)
&& Objects.equals(this.suffix, that.suffix)
&& Objects.equals(this.text, that.text)
- && Objects.equals(this.html, that.html)
&& Objects.equals(this.enabled, that.enabled);
}
return false;
@@ -49,7 +47,7 @@
@Override
public int hashCode() {
- return Objects.hash(match, link, html, enabled);
+ return Objects.hash(match, link, prefix, suffix, text, enabled);
}
@Override
@@ -61,7 +59,6 @@
.add("prefix", prefix)
.add("suffix", suffix)
.add("text", text)
- .add("html", html)
.add("enabled", enabled)
.toString();
}
diff --git a/java/com/google/gerrit/server/cache/serialize/entities/StoredCommentLinkInfoSerializer.java b/java/com/google/gerrit/server/cache/serialize/entities/StoredCommentLinkInfoSerializer.java
index 5aa7a2a..5ac9ac4 100644
--- a/java/com/google/gerrit/server/cache/serialize/entities/StoredCommentLinkInfoSerializer.java
+++ b/java/com/google/gerrit/server/cache/serialize/entities/StoredCommentLinkInfoSerializer.java
@@ -30,7 +30,6 @@
.setPrefix(emptyToNull(proto.getPrefix()))
.setSuffix(emptyToNull(proto.getSuffix()))
.setText(emptyToNull(proto.getText()))
- .setHtml(emptyToNull(proto.getHtml()))
.setEnabled(proto.getEnabled())
.setOverrideOnly(proto.getOverrideOnly())
.build();
@@ -44,7 +43,6 @@
.setPrefix(nullToEmpty(autoValue.getPrefix()))
.setSuffix(nullToEmpty(autoValue.getSuffix()))
.setText(nullToEmpty(autoValue.getText()))
- .setHtml(nullToEmpty(autoValue.getHtml()))
.setEnabled(Optional.ofNullable(autoValue.getEnabled()).orElse(true))
.setOverrideOnly(autoValue.getOverrideOnly())
.build();
diff --git a/java/com/google/gerrit/server/project/CommentLinkProvider.java b/java/com/google/gerrit/server/project/CommentLinkProvider.java
index c2ac68a..88f045e 100644
--- a/java/com/google/gerrit/server/project/CommentLinkProvider.java
+++ b/java/com/google/gerrit/server/project/CommentLinkProvider.java
@@ -48,7 +48,7 @@
ImmutableList.builderWithExpectedSize(subsections.size());
for (String name : subsections) {
try {
- StoredCommentLinkInfo cl = ProjectConfig.buildCommentLink(cfg, name, true);
+ StoredCommentLinkInfo cl = ProjectConfig.buildCommentLink(cfg, name);
if (cl.getOverrideOnly()) {
logger.atWarning().log("commentlink %s empty except for \"enabled\"", name);
continue;
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 2dd7970..a964ee1 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.project;
-import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.entities.Permission.isPermission;
@@ -131,7 +130,6 @@
KEY_SR_OVERRIDE_IN_CHILD_PROJECTS);
public static final String KEY_MATCH = "match";
- private static final String KEY_HTML = "html";
public static final String KEY_LINK = "link";
public static final String KEY_PREFIX = "prefix";
public static final String KEY_SUFFIX = "suffix";
@@ -318,7 +316,7 @@
return builder.build();
}
- public static StoredCommentLinkInfo buildCommentLink(Config cfg, String name, boolean allowRaw)
+ public static StoredCommentLinkInfo buildCommentLink(Config cfg, String name)
throws IllegalArgumentException {
String match = cfg.getString(COMMENTLINK, name, KEY_MATCH);
if (match != null) {
@@ -335,9 +333,6 @@
String linkSuffix = cfg.getString(COMMENTLINK, name, KEY_SUFFIX);
String linkText = cfg.getString(COMMENTLINK, name, KEY_TEXT);
- String html = cfg.getString(COMMENTLINK, name, KEY_HTML);
- boolean hasHtml = !Strings.isNullOrEmpty(html);
-
String rawEnabled = cfg.getString(COMMENTLINK, name, KEY_ENABLED);
Boolean enabled;
if (rawEnabled != null) {
@@ -345,12 +340,8 @@
} else {
enabled = null;
}
- checkArgument(allowRaw || !hasHtml, "Raw html replacement not allowed");
- if (Strings.isNullOrEmpty(match)
- && Strings.isNullOrEmpty(link)
- && !hasHtml
- && enabled != null) {
+ if (Strings.isNullOrEmpty(match) && Strings.isNullOrEmpty(link) && enabled != null) {
if (enabled) {
return StoredCommentLinkInfo.enabled(name);
}
@@ -362,7 +353,6 @@
.setPrefix(linkPrefix)
.setSuffix(linkSuffix)
.setText(linkText)
- .setHtml(html)
.setEnabled(enabled)
.setOverrideOnly(false)
.build();
@@ -1215,7 +1205,7 @@
commentLinkSections = new LinkedHashMap<>(subsections.size());
for (String name : subsections) {
try {
- commentLinkSections.put(name, buildCommentLink(rc, name, false));
+ commentLinkSections.put(name, buildCommentLink(rc, name));
} catch (PatternSyntaxException e) {
error(
String.format(
@@ -1417,9 +1407,9 @@
unsetSection(rc, COMMENTLINK);
if (commentLinkSections != null) {
for (StoredCommentLinkInfo cm : commentLinkSections.values()) {
- rc.setString(COMMENTLINK, cm.getName(), KEY_MATCH, cm.getMatch());
- if (!Strings.isNullOrEmpty(cm.getHtml())) {
- rc.setString(COMMENTLINK, cm.getName(), KEY_HTML, cm.getHtml());
+ // Match and Link can be empty if the commentlink is override only.
+ if (!Strings.isNullOrEmpty(cm.getMatch())) {
+ rc.setString(COMMENTLINK, cm.getName(), KEY_MATCH, cm.getMatch());
}
if (!Strings.isNullOrEmpty(cm.getLink())) {
rc.setString(COMMENTLINK, cm.getName(), KEY_LINK, cm.getLink());
diff --git a/java/com/google/gerrit/server/restapi/project/PutConfig.java b/java/com/google/gerrit/server/restapi/project/PutConfig.java
index d4077c8..d4b30c2 100644
--- a/java/com/google/gerrit/server/restapi/project/PutConfig.java
+++ b/java/com/google/gerrit/server/restapi/project/PutConfig.java
@@ -313,7 +313,7 @@
cfg.setString(COMMENTLINK, name, KEY_TEXT, value.text);
}
cfg.setBoolean(COMMENTLINK, name, KEY_ENABLED, value.enabled == null || value.enabled);
- projectConfig.addCommentLinkSection(ProjectConfig.buildCommentLink(cfg, name, false));
+ projectConfig.addCommentLinkSection(ProjectConfig.buildCommentLink(cfg, name));
} else {
// Delete the commentlink section
projectConfig.removeCommentLinkSection(name);
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/CachedProjectConfigSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/CachedProjectConfigSerializerTest.java
index c7e09dc..bf8a071 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/CachedProjectConfigSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/CachedProjectConfigSerializerTest.java
@@ -52,7 +52,7 @@
.addNotifySection(NotifyConfigSerializerTest.ALL_VALUES_SET)
.addLabelSection(LabelTypeSerializerTest.ALL_VALUES_SET)
.addSubscribeSection(SubscribeSectionSerializerTest.ALL_VALUES_SET)
- .addCommentLinkSection(StoredCommentLinkInfoSerializerTest.HTML_ONLY)
+ .addCommentLinkSection(StoredCommentLinkInfoSerializerTest.LINK_ONLY)
.setRevision(Optional.of(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")))
.setRulesId(Optional.of(ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")))
.setExtensionPanelSections(ImmutableMap.of("key1", ImmutableList.of("val1", "val2")))
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/StoredCommentLinkInfoSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/StoredCommentLinkInfoSerializerTest.java
index e293493..aa6cfef 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/StoredCommentLinkInfoSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/StoredCommentLinkInfoSerializerTest.java
@@ -22,27 +22,16 @@
import org.junit.Test;
public class StoredCommentLinkInfoSerializerTest {
- static final StoredCommentLinkInfo HTML_ONLY =
+ static final StoredCommentLinkInfo LINK_ONLY =
StoredCommentLinkInfo.builder("name")
.setEnabled(true)
- .setHtml("<p>html")
+ .setLink("a.com/b.html")
.setMatch("*")
.build();
@Test
- public void htmlOnly_roundTrip() {
- assertThat(deserialize(serialize(HTML_ONLY))).isEqualTo(HTML_ONLY);
- }
-
- @Test
public void linkOnly_roundTrip() {
- StoredCommentLinkInfo autoValue =
- StoredCommentLinkInfo.builder("name")
- .setEnabled(true)
- .setLink("<p>html")
- .setMatch("*")
- .build();
- assertThat(deserialize(serialize(autoValue))).isEqualTo(autoValue);
+ assertThat(deserialize(serialize(LINK_ONLY))).isEqualTo(LINK_ONLY);
}
@Test
diff --git a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
index ef92139..3b7ad1e 100644
--- a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
+++ b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
@@ -537,14 +537,14 @@
StoredCommentLinkInfo cm =
StoredCommentLinkInfo.builder("Test")
.setMatch("abc.*")
- .setHtml("<a>link</a>")
+ .setLink("link")
.setEnabled(true)
.setOverrideOnly(false)
.build();
cfg.addCommentLinkSection(cm);
rev = commit(cfg);
assertThat(text(rev, "project.config"))
- .isEqualTo("[commentlink \"Test\"]\n\tmatch = abc.*\n\thtml = <a>link</a>\n");
+ .isEqualTo("[commentlink \"Test\"]\n\tmatch = abc.*\n\tlink = link\n");
}
@Test
@@ -722,7 +722,7 @@
}
@Test
- public void readCommentLinksNoHtmlOrLinkButEnabled() throws Exception {
+ public void readCommentLinksNoLinkButEnabled() throws Exception {
RevCommit rev =
tr.commit().add("project.config", "[commentlink \"bugzilla\"]\n \tenabled = true").create();
ProjectConfig cfg = read(rev);
@@ -732,7 +732,7 @@
}
@Test
- public void readCommentLinksNoHtmlOrLinkAndDisabled() throws Exception {
+ public void readCommentLinksNoLinkAndDisabled() throws Exception {
RevCommit rev =
tr.commit()
.add("project.config", "[commentlink \"bugzilla\"]\n \tenabled = false")
@@ -744,7 +744,7 @@
}
@Test
- public void readCommentLinksNoHtmlOrLinkAndMissingEnabled() throws Exception {
+ public void readCommentLinksMissingEnabled() throws Exception {
RevCommit rev =
tr.commit()
.add(
@@ -792,26 +792,7 @@
}
@Test
- public void readCommentLinkRawHtml() throws Exception {
- RevCommit rev =
- tr.commit()
- .add(
- "project.config",
- "[commentlink \"bugzilla\"]\n"
- + "\tmatch = \"(bugs#?)(d+)\"\n"
- + "\thtml = http://bugs.example.com/show_bug.cgi?id=$2")
- .create();
- ProjectConfig cfg = read(rev);
- assertThat(cfg.getCommentLinkSections()).isEmpty();
- assertThat(cfg.getValidationErrors())
- .containsExactly(
- ValidationError.create(
- "project.config: Error in pattern \"(bugs#?)(d+)\" in commentlink.bugzilla.match: "
- + "Raw html replacement not allowed"));
- }
-
- @Test
- public void readCommentLinkMatchButNoHtmlOrLink() throws Exception {
+ public void readCommentLinkMatchButNoLink() throws Exception {
RevCommit rev =
tr.commit()
.add("project.config", "[commentlink \"bugzilla\"]\n" + "\tmatch = \"(bugs#?)(d+)\"\n")
@@ -822,7 +803,7 @@
.containsExactly(
ValidationError.create(
"project.config: Error in pattern \"(bugs#?)(d+)\" in commentlink.bugzilla.match: "
- + "commentlink.bugzilla must have either link or html"));
+ + "commentlink.bugzilla must have link specified"));
}
@Test
diff --git a/polygerrit-ui/app/api/rest-api.ts b/polygerrit-ui/app/api/rest-api.ts
index 1768552..1e8f78f 100644
--- a/polygerrit-ui/app/api/rest-api.ts
+++ b/polygerrit-ui/app/api/rest-api.ts
@@ -452,12 +452,11 @@
*/
export declare interface CommentLinkInfo {
match: string;
- link?: string;
+ link: string;
prefix?: string;
suffix?: string;
text?: string;
enabled?: boolean;
- html?: string;
}
export declare interface CommentLinks {
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
index 218c04e..61ea5a1 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
@@ -47,10 +47,6 @@
match: '(LinkRewriteMe)',
link: 'http://google.com/$1',
},
- customHtmlRewrite: {
- match: 'HTMLRewriteMe',
- html: '<div>HTMLRewritten</div>',
- },
complexLinkRewrite: {
match: '(^|\\s)A Link (\\d+)($|\\s)',
link: '/page?id=$2',
@@ -101,11 +97,13 @@
await setCommentLinks({
capitalizeFoo: {
match: 'foo',
- html: 'FOO',
+ prefix: 'FOO',
+ link: 'a.b.c',
},
lowercaseFoo: {
match: 'FOO',
- html: 'foo',
+ prefix: 'foo',
+ link: 'c.d.e',
},
});
element.content = 'foo';
@@ -115,9 +113,8 @@
element,
/* HTML */ `
<pre class="plaintext">
- FOO
- </pre
- >
+ FOO<a href="a.b.c" rel="noopener" target="_blank">foo</a>
+ </pre>
`
);
});
@@ -126,11 +123,15 @@
await setCommentLinks({
bracketNum: {
match: '(Start:) ([0-9]+)',
- html: '$1 [$2]',
+ prefix: '$1 ',
+ link: 'bug/$2',
+ text: 'bug/$2',
},
bracketNum2: {
match: '(Start: [0-9]+) ([0-9]+)',
- html: '$1 [$2]',
+ prefix: '$1 ',
+ link: 'bug/$2',
+ text: 'bug/$2',
},
});
element.content = 'Start: 123 456';
@@ -140,9 +141,14 @@
element,
/* HTML */ `
<pre class="plaintext">
- Start: [123] [456]
- </pre
- >
+ Start:
+ <a href="bug/123" rel="noopener" target="_blank">
+ bug/123
+ </a>
+ <a href="bug/456" rel="noopener" target="_blank">
+ bug/456
+ </a>
+ </pre>
`
);
});
@@ -150,8 +156,7 @@
test('renders text with links and rewrites', async () => {
element.content = `text with plain link: google.com
\ntext with config link: LinkRewriteMe
- \ntext with complex link: A Link 12
- \ntext with config html: HTMLRewriteMe`;
+ \ntext with complex link: A Link 12`;
await element.updateComplete;
assert.shadowDom.equal(
@@ -178,8 +183,6 @@
>
Link 12
</a>
- text with config html:
- <div>HTMLRewritten</div>
</pre>
`
);
@@ -217,8 +220,7 @@
\ntext with plain link: google.com
\ntext with config link: LinkRewriteMe
\ntext without a link: NotA Link 15 cats
- \ntext with complex link: A Link 12
- \ntext with config html: HTMLRewriteMe`;
+ \ntext with complex link: A Link 12`;
await element.updateComplete;
assert.shadowDom.equal(
@@ -254,9 +256,6 @@
Link 12
</a>
</p>
- <p>text with config html:</p>
- <div>HTMLRewritten</div>
- <p></p>
</div>
</marked-element>
`
@@ -271,8 +270,7 @@
\n##### h5-heading
\n###### h6-heading
\n# heading with plain link: google.com
- \n# heading with config link: LinkRewriteMe
- \n# heading with config html: HTMLRewriteMe`;
+ \n# heading with config link: LinkRewriteMe`;
await element.updateComplete;
assert.shadowDom.equal(
@@ -302,10 +300,6 @@
LinkRewriteMe
</a>
</h1>
- <h1>
- heading with config html:
- <div>HTMLRewritten</div>
- </h1>
</div>
</marked-element>
`
@@ -315,8 +309,7 @@
test('renders inline-code without linking or rewriting', async () => {
element.content = `\`inline code\`
\n\`inline code with plain link: google.com\`
- \n\`inline code with config link: LinkRewriteMe\`
- \n\`inline code with config html: HTMLRewriteMe\``;
+ \n\`inline code with config link: LinkRewriteMe\``;
await element.updateComplete;
assert.shadowDom.equal(
@@ -333,9 +326,6 @@
<p>
<code>inline code with config link: LinkRewriteMe</code>
</p>
- <p>
- <code>inline code with config html: HTMLRewriteMe</code>
- </p>
</div>
</marked-element>
`
@@ -345,8 +335,7 @@
test('renders multiline-code without linking or rewriting', async () => {
element.content = `\`\`\`\nmultiline code\n\`\`\`
\n\`\`\`\nmultiline code with plain link: google.com\n\`\`\`
- \n\`\`\`\nmultiline code with config link: LinkRewriteMe\n\`\`\`
- \n\`\`\`\nmultiline code with config html: HTMLRewriteMe\n\`\`\``;
+ \n\`\`\`\nmultiline code with config link: LinkRewriteMe\n\`\`\``;
await element.updateComplete;
assert.shadowDom.equal(
@@ -363,9 +352,6 @@
<pre>
<code>multiline code with config link: LinkRewriteMe</code>
</pre>
- <pre>
- <code>multiline code with config html: HTMLRewriteMe</code>
- </pre>
</div>
</marked-element>
`
@@ -496,8 +482,7 @@
test('renders block quotes with links and rewrites', async () => {
element.content = `> block quote
\n> block quote with plain link: google.com
- \n> block quote with config link: LinkRewriteMe
- \n> block quote with config html: HTMLRewriteMe`;
+ \n> block quote with config link: LinkRewriteMe`;
await element.updateComplete;
assert.shadowDom.equal(
@@ -528,11 +513,6 @@
</a>
</p>
</blockquote>
- <blockquote>
- <p>block quote with config html:</p>
- <div>HTMLRewritten</div>
- <p></p>
- </blockquote>
</div>
</marked-element>
`
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index 08ac65f..23a5794 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -136,9 +136,13 @@
nanosecondSuffix) as Timestamp;
}
-export function createCommentLink(match = 'test'): CommentLinkInfo {
+export function createCommentLink(
+ match = 'test',
+ link = 'http://test.com'
+): CommentLinkInfo {
return {
match,
+ link,
};
}
diff --git a/polygerrit-ui/app/utils/link-util.ts b/polygerrit-ui/app/utils/link-util.ts
index b5b9025..ec1e7e7 100644
--- a/polygerrit-ui/app/utils/link-util.ts
+++ b/polygerrit-ui/app/utils/link-util.ts
@@ -45,8 +45,7 @@
): RewriteResult[] {
const enabledRewrites = Object.values(repoCommentLinks).filter(
commentLinkInfo =>
- commentLinkInfo.enabled !== false &&
- (commentLinkInfo.link !== undefined || commentLinkInfo.html !== undefined)
+ commentLinkInfo.enabled !== false && commentLinkInfo.link !== undefined
);
return enabledRewrites.flatMap(rewrite => {
const regexp = new RegExp(rewrite.match, 'g');
@@ -127,25 +126,19 @@
matchedText: string,
rewrite: CommentLinkInfo
): string {
- if (rewrite.link !== undefined) {
- const replacementHref = rewrite.link.startsWith('/')
- ? `${getBaseUrl()}${rewrite.link}`
- : rewrite.link;
- const regexp = new RegExp(rewrite.match, 'g');
- return matchedText.replace(
- regexp,
- createLinkTemplate(
- replacementHref,
- rewrite.text ?? '$&',
- rewrite.prefix,
- rewrite.suffix
- )
- );
- } else if (rewrite.html !== undefined) {
- return matchedText.replace(new RegExp(rewrite.match, 'g'), rewrite.html);
- } else {
- throw new Error('commentLinkInfo is not a link or html rewrite');
- }
+ const replacementHref = rewrite.link.startsWith('/')
+ ? `${getBaseUrl()}${rewrite.link}`
+ : rewrite.link;
+ const regexp = new RegExp(rewrite.match, 'g');
+ return matchedText.replace(
+ regexp,
+ createLinkTemplate(
+ replacementHref,
+ rewrite.text ?? '$&',
+ rewrite.prefix,
+ rewrite.suffix
+ )
+ );
}
/**
diff --git a/polygerrit-ui/app/utils/link-util_test.ts b/polygerrit-ui/app/utils/link-util_test.ts
index 61d6bff..f5c13e8 100644
--- a/polygerrit-ui/app/utils/link-util_test.ts
+++ b/polygerrit-ui/app/utils/link-util_test.ts
@@ -76,67 +76,6 @@
);
});
});
- suite('html rewrites', () => {
- test('basic case', () => {
- assert.equal(
- linkifyUrlsAndApplyRewrite('foo', {
- foo: {
- match: '(foo)',
- html: '<div>$1</div>',
- },
- }),
- '<div>foo</div>'
- );
- });
-
- test('only inserts', () => {
- assert.equal(
- linkifyUrlsAndApplyRewrite('foo', {
- foo: {
- match: 'foo',
- html: 'foo bar',
- },
- }),
- 'foo bar'
- );
- });
-
- test('only deletes', () => {
- assert.equal(
- linkifyUrlsAndApplyRewrite('foo bar baz', {
- bar: {
- match: 'bar',
- html: '',
- },
- }),
- 'foo baz'
- );
- });
-
- test('multiple matches', () => {
- assert.equal(
- linkifyUrlsAndApplyRewrite('foo foo', {
- foo: {
- match: '(foo)',
- html: '<div>$1</div>',
- },
- }),
- '<div>foo</div> <div>foo</div>'
- );
- });
-
- test('does not apply within normal links', () => {
- assert.equal(
- linkifyUrlsAndApplyRewrite('google.com', {
- ogle: {
- match: 'ogle',
- html: '<div>gerritcodereview.com<div>',
- },
- }),
- link('google.com', 'http://google.com')
- );
- });
- });
test('for overlapping rewrites prefer the latest ending', () => {
assert.equal(
@@ -147,14 +86,14 @@
},
foobarbaz: {
match: 'foobarbaz',
- html: '<div>foobarbaz.gov</div>',
+ link: 'foobarbaz.gov',
},
foobar: {
match: 'foobar',
link: 'foobar.gov',
},
}),
- '<div>foobarbaz.gov</div>'
+ link('foobarbaz', 'foobarbaz.gov')
);
});
@@ -167,14 +106,14 @@
},
foobarbaz: {
match: 'foobarbaz',
- html: '<div>FooBarBaz.gov</div>',
+ link: 'FooBarBaz.gov',
},
foobar: {
match: 'barbaz',
link: 'BarBaz.gov',
},
}),
- '<div>FooBarBaz.gov</div>'
+ link('foobarbaz', 'FooBarBaz.gov')
);
});
@@ -183,18 +122,18 @@
linkifyUrlsAndApplyRewrite('foobarbaz', {
foo: {
match: 'foo',
- html: 'FOO',
+ link: 'FOO',
},
oobarba: {
match: 'oobarba',
- html: 'OOBARBA',
+ link: 'OOBARBA',
},
baz: {
match: 'baz',
- html: 'BAZ',
+ link: 'BAZ',
},
}),
- 'FOObarBAZ'
+ `${link('foo', 'FOO')}bar${link('baz', 'BAZ')}`
);
});
@@ -203,18 +142,27 @@
linkifyUrlsAndApplyRewrite('bugs: 123 234 345', {
bug1: {
match: '(bugs:) (\\d+)',
- html: '$1 <div>bug/$2</div>',
+ prefix: '$1 ',
+ link: 'bug/$2',
+ text: 'bug/$2',
},
bug2: {
match: '(bugs:) (\\d+) (\\d+)',
- html: '$1 $2 <div>bug/$3</div>',
+ prefix: '$1 $2 ',
+ link: 'bug/$3',
+ text: 'bug/$3',
},
bug3: {
match: '(bugs:) (\\d+) (\\d+) (\\d+)',
- html: '$1 $2 $3 <div>bug/$4</div>',
+ prefix: '$1 $2 $3 ',
+ link: 'bug/$4',
+ text: 'bug/$4',
},
}),
- 'bugs: <div>bug/123</div> <div>bug/234</div> <div>bug/345</div>'
+ `bugs: ${link('bug/123', 'bug/123')} ${link('bug/234', 'bug/234')} ${link(
+ 'bug/345',
+ 'bug/345'
+ )}`
);
});
diff --git a/proto/cache.proto b/proto/cache.proto
index 9aadf0f..0cecdea 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -535,9 +535,10 @@
// Serialized form of com.google.gerrit.entities.StoredCommentLinkInfo.
// Next ID: 10
message StoredCommentLinkInfoProto {
+ reserved 4; // html
+
string name = 1;
string match = 2;
- string html = 4;
bool enabled = 5;
bool override_only = 6;
string link = 3;
diff --git a/tools/migration/html_to_link_commentlink.md b/tools/migration/html_to_link_commentlink.md
new file mode 100644
index 0000000..45570ac
--- /dev/null
+++ b/tools/migration/html_to_link_commentlink.md
@@ -0,0 +1,47 @@
+# Overview
+
+**Raw html substitution will no longer be an option for comment links.**
+
+The raw-html option for commentlink sections is deprecated and removed.
+Example:
+
+```
+[commentlink "issue b/"]
+ match = (^|\\s)b/(\\d+)
+ html = $1<a href=\"http://b/issue?id=$2&query=$2\" target=\"_blank\">b/$2</a>
+```
+
+Before it allowed to find and replace text matches in commit messages and
+comments with arbitrary html. When misconfigured this has in the past enabled
+injecting undesired html code and XSS attacks by writing a comment.
+
+Even though the sanitization of the resulting html has improved. This feature is
+more powerful than needed. In almost all cases across host configurations html
+is only used to either configure text of the link, or limit the link to wrap
+only a portion of the matched text.
+
+To fill the gap in functionality from deprecating the option additional optional
+parameters (prefix, suffix and text) have been added. They allow to generate
+links that look like:
+```
+ PREFIX<a href="LINK">TEXT</a>SUFFIX
+```
+With substitution being strictly plaintext and all html escaped.
+
+The comment link section in project configs (in refs/meta/config) never
+supported the raw-html option and don't need to be updated.
+
+# Config migration command
+
+```
+CONFIG_FILE=<path to gerrit.config file>
+perl -0pe 's/([ \t]*)html\s*=\s*\"(.*)<a.* href=(?:\\\"(\S+)\\\"|(\S+)(?=\s|>))(?: .*)?>(.*)<\/a>(.*)(?<!\\)\"/$1link = \"$3$4\"\n$1prefix = \"$2\"\n$1text = \"$5\"\n$1suffix = \"$6\"/g' $CONFIG_FILE |
+perl -0pe 's/([ \t]*)html\s*=\s*(\S.*)?<a.* href=(?:\\\"(\S+)\\\"|(\S+)(?=\s|>))(?: .*)?>(.*)<\/a>(.*\S)?/$1link = \"$3$4\"\n$1prefix = \"$2\"\n$1text = \"$5\"\n$1suffix = \"$6\"/g' |
+perl -ne 'print if !/\s*(prefix|suffix|text)\s*=\s*\"\"/'
+```
+
+The command does 3 simple string replace passes:
+
+1. Replace `html=<value>` with quote-escaped value.
+2. Replace `html=<value>` with value without quotes.
+3. Remove empty `prefix`, `suffix`, `text` fields.
