Support enabling/disabling commentlink sections per-project

Allow project admins to customize which commentlinks apply to their
section of the project hierarchy by selectively setting the enabled
bit on each commentlink section. This allows site admins to create raw
HTML commentlinks with less XSS risk (or at least more auditing) and
not have them apply to every project.

Change-Id: I8dd31aab98379f90253bbfdd9f669a7f5f78c25f
This commit is contained in:
Dave Borowitz
2013-04-08 15:45:12 -07:00
parent 13b38004e9
commit 82d79c0263
6 changed files with 117 additions and 13 deletions

View File

@@ -781,6 +781,21 @@ match expression may be accessed as `$'n'`.
The configuration file eats double quotes, so escaping them as The configuration file eats double quotes, so escaping them as
`\"` is necessary to protect them from the parser. `\"` is necessary to protect them from the parser.
[[commentlink.name.enabled]]commentlink.<name>.enabled::
+
Whether the comment link is enabled. A child project may override a
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.
+
Note that the names and contents of disabled sections are visible even
to anonymous users via the
link:rest-api-projects.html#get-config[REST API].
[[contactstore]]Section contactstore [[contactstore]]Section contactstore
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

View File

@@ -52,6 +52,9 @@ public class ConfigInfo extends JavaScriptObject {
List<FindReplace> commentLinks = new ArrayList<FindReplace>(cls.length()); List<FindReplace> commentLinks = new ArrayList<FindReplace>(cls.length());
for (int i = 0; i < cls.length(); i++) { for (int i = 0; i < cls.length(); i++) {
CommentLinkInfo cl = cls.get(i); CommentLinkInfo cl = cls.get(i);
if (!cl.enabled()) {
continue;
}
if (cl.link() != null) { if (cl.link() != null) {
commentLinks.add(new LinkFindReplace(cl.match(), cl.link())); commentLinks.add(new LinkFindReplace(cl.match(), cl.link()));
} else { } else {
@@ -68,6 +71,9 @@ public class ConfigInfo extends JavaScriptObject {
final native String match() /*-{ return this.match; }-*/; final native String match() /*-{ return this.match; }-*/;
final native String link() /*-{ return this.link; }-*/; final native String link() /*-{ return this.link; }-*/;
final native String html() /*-{ return this.html; }-*/; final native String html() /*-{ return this.html; }-*/;
final native boolean enabled() /*-{
return !this.hasOwnProperty('enabled') || this.enabled;
}-*/;
protected CommentLinkInfo() { protected CommentLinkInfo() {
} }

View File

@@ -75,6 +75,7 @@ public class ProjectConfig extends VersionedMetaData {
private static final String KEY_MATCH = "match"; private static final String KEY_MATCH = "match";
private static final String KEY_HTML = "html"; private static final String KEY_HTML = "html";
private static final String KEY_LINK = "link"; private static final String KEY_LINK = "link";
private static final String KEY_ENABLED = "enabled";
private static final String PROJECT_CONFIG = "project.config"; private static final String PROJECT_CONFIG = "project.config";
private static final String GROUP_LIST = "groups"; private static final String GROUP_LIST = "groups";
@@ -162,19 +163,37 @@ public class ProjectConfig extends VersionedMetaData {
public static CommentLinkInfo buildCommentLink(Config cfg, String name, public static CommentLinkInfo buildCommentLink(Config cfg, String name,
boolean allowRaw) throws IllegalArgumentException { boolean allowRaw) throws IllegalArgumentException {
String match = cfg.getString(COMMENTLINK, name, KEY_MATCH); String match = cfg.getString(COMMENTLINK, name, KEY_MATCH);
if (match != null) {
// Unfortunately this validation isn't entirely complete. Clients // Unfortunately this validation isn't entirely complete. Clients
// can have exceptions trying to evaluate the pattern if they don't // can have exceptions trying to evaluate the pattern if they don't
// support a token used, even if the server does support the token. // support a token used, even if the server does support the token.
// //
// At the minimum, we can trap problems related to unmatched groups. // At the minimum, we can trap problems related to unmatched groups.
Pattern.compile(match); Pattern.compile(match);
}
String link = cfg.getString(COMMENTLINK, name, KEY_LINK); String link = cfg.getString(COMMENTLINK, name, KEY_LINK);
String html = cfg.getString(COMMENTLINK, name, KEY_HTML); String html = cfg.getString(COMMENTLINK, name, KEY_HTML);
checkArgument(allowRaw || Strings.isNullOrEmpty(html), boolean hasHtml = !Strings.isNullOrEmpty(html);
"Raw html replacement not allowed");
return new CommentLinkInfo(name, match, link, html); String rawEnabled = cfg.getString(COMMENTLINK, name, KEY_ENABLED);
Boolean enabled;
if (rawEnabled != null) {
enabled = cfg.getBoolean(COMMENTLINK, name, KEY_ENABLED, true);
} else {
enabled = null;
}
checkArgument(allowRaw || !hasHtml, "Raw html replacement not allowed");
if (Strings.isNullOrEmpty(match) && Strings.isNullOrEmpty(link) && !hasHtml
&& enabled != null) {
if (enabled) {
return new CommentLinkInfo.Enabled(name);
} else {
return new CommentLinkInfo.Disabled(name);
}
}
return new CommentLinkInfo(name, match, link, html, enabled);
} }
public ProjectConfig(Project.NameKey projectName) { public ProjectConfig(Project.NameKey projectName) {

View File

@@ -20,13 +20,37 @@ import com.google.common.base.Strings;
/** Info about a single commentlink section in a config. */ /** Info about a single commentlink section in a config. */
public class CommentLinkInfo { public class CommentLinkInfo {
public static class Enabled extends CommentLinkInfo {
public Enabled(String name) {
super(name, true);
}
@Override
boolean isOverrideOnly() {
return true;
}
}
public static class Disabled extends CommentLinkInfo {
public Disabled(String name) {
super(name, false);
}
@Override
boolean isOverrideOnly() {
return true;
}
}
public final String match; public final String match;
public final String link; public final String link;
public final String html; public final String html;
public final Boolean enabled; // null means true
public transient final String name; public transient final String name;
public CommentLinkInfo(String name, String match, String link, String html) { public CommentLinkInfo(String name, String match, String link, String html,
Boolean enabled) {
checkArgument(name != null, "invalid commentlink.name"); checkArgument(name != null, "invalid commentlink.name");
checkArgument(!Strings.isNullOrEmpty(match), checkArgument(!Strings.isNullOrEmpty(match),
"invalid commentlink.%s.match", name); "invalid commentlink.%s.match", name);
@@ -39,5 +63,30 @@ public class CommentLinkInfo {
this.match = match; this.match = match;
this.link = link; this.link = link;
this.html = html; this.html = html;
this.enabled = enabled;
}
private CommentLinkInfo(CommentLinkInfo src, boolean enabled) {
this.name = src.name;
this.match = src.match;
this.link = src.link;
this.html = src.html;
this.enabled = enabled;
}
private CommentLinkInfo(String name, boolean enabled) {
this.name = name;
this.match = null;
this.link = null;
this.html = null;
this.enabled = enabled;
}
boolean isOverrideOnly() {
return false;
}
CommentLinkInfo inherit(CommentLinkInfo src) {
return new CommentLinkInfo(src, enabled);
} }
} }

View File

@@ -20,6 +20,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.ProjectConfig;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.Provider; import com.google.inject.Provider;
import com.google.inject.ProvisionException;
import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Config;
@@ -40,7 +41,12 @@ public class CommentLinkProvider implements Provider<List<CommentLinkInfo>> {
List<CommentLinkInfo> cls = List<CommentLinkInfo> cls =
Lists.newArrayListWithCapacity(subsections.size()); Lists.newArrayListWithCapacity(subsections.size());
for (String name : subsections) { for (String name : subsections) {
cls.add(ProjectConfig.buildCommentLink(cfg, name, true)); CommentLinkInfo cl = ProjectConfig.buildCommentLink(cfg, name, true);
if (cl.isOverrideOnly()) {
throw new ProvisionException(
"commentlink " + name + " empty except for \"enabled\"");
}
cls.add(cl);
} }
return ImmutableList.copyOf(cls); return ImmutableList.copyOf(cls);
} }

View File

@@ -382,7 +382,16 @@ public class ProjectState {
} }
for (ProjectState s : treeInOrder()) { for (ProjectState s : treeInOrder()) {
for (CommentLinkInfo cl : s.getConfig().getCommentLinkSections()) { for (CommentLinkInfo cl : s.getConfig().getCommentLinkSections()) {
cls.put(cl.name.toLowerCase(), cl); String name = cl.name.toLowerCase();
if (cl.isOverrideOnly()) {
CommentLinkInfo parent = cls.get(name);
if (parent == null) {
continue; // Ignore invalid overrides.
}
cls.put(name, cl.inherit(parent));
} else {
cls.put(name, cl);
}
} }
} }
return ImmutableList.copyOf(cls.values()); return ImmutableList.copyOf(cls.values());