Catch bad commentlink patterns and report them

We now refuse to start the server if a commentlink.match is so
badly malformed that the java.util.regex package can't parse the
expression.  This should catch common errors caused by mismatched
braces or parens in the expression.

Clients now trap expression parse exceptions and try to filter out
the invalid patterns.  Any exceptions are reported back to the
server in the server's error_log, so the site administrator can
take action and correct their server's configuration file.  In the
mean time the patterns are removed from evaulation in the client,
so the site is at least still somewhat useful.

Bug: issue 389
Change-Id: I7df945acc42f74ba48a9e167fecdb20f102522c8
Signed-off-by: Shawn O. Pearce <sop@google.com>
Reviewed-by: Nico Sallembien <nsallembien@google.com>

gerrit-common/src/main/java/com/google/gerrit/common/data/SystemInfoService.java

@@ -20,6 +20,7 @@ import com.google.gwt.user.client.rpc.AsyncCallback;
 import com.google.gwtjsonrpc.client.AllowCrossSiteRequest;
 import com.google.gwtjsonrpc.client.RemoteJsonService;
 import com.google.gwtjsonrpc.client.RpcImpl;
+import com.google.gwtjsonrpc.client.VoidResult;
 import com.google.gwtjsonrpc.client.RpcImpl.Version;
 
 import java.util.List;
@@ -31,4 +32,6 @@ public interface SystemInfoService extends RemoteJsonService {
 
   @SignInRequired
   void contributorAgreements(AsyncCallback<List<ContributorAgreement>> callback);
+
+  void clientError(String message, AsyncCallback<VoidResult> callback);
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeDescriptionBlock.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.client.changes;
 
 import com.google.gerrit.client.Gerrit;
+import com.google.gerrit.client.ui.CommentLinkProcessor;
 import com.google.gerrit.common.data.AccountInfoCache;
 import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.PatchSetInfo;
@@ -45,7 +46,7 @@ public class ChangeDescriptionBlock extends Composite {
 
     SafeHtml msg = new SafeHtmlBuilder().append(info.getMessage());
     msg = msg.linkify();
-    msg = msg.replaceAll(Gerrit.getConfig().getCommentLinks());
+    msg = CommentLinkProcessor.apply(msg);
     msg = new SafeHtmlBuilder().openElement("p").append(msg).closeElement("p");
     msg = msg.replaceAll("\n\n", "</p><p>");
     msg = msg.replaceAll("\n", "<br />");

gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/CommentLinkProcessor.java

@@ -0,0 +1,94 @@
+// Copyright (C) 2010 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.client.ui;
+
+import com.google.gerrit.client.Gerrit;
+import com.google.gwt.user.client.rpc.AsyncCallback;
+import com.google.gwtexpui.safehtml.client.RegexFindReplace;
+import com.google.gwtexpui.safehtml.client.SafeHtml;
+import com.google.gwtjsonrpc.client.VoidResult;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+public class CommentLinkProcessor {
+  public static SafeHtml apply(SafeHtml buf) {
+    try {
+      return buf.replaceAll(Gerrit.getConfig().getCommentLinks());
+
+    } catch (RuntimeException err) {
+      // One or more of the patterns isn't valid on this browser.
+      // Try to filter the list down and remove the invalid ones.
+
+      List<RegexFindReplace> safe = new ArrayList<RegexFindReplace>();
+      List<PatternError> bad = new ArrayList<PatternError>();
+      for (RegexFindReplace r : Gerrit.getConfig().getCommentLinks()) {
+        try {
+          buf.replaceAll(Collections.singletonList(r));
+          safe.add(r);
+        } catch (RuntimeException why) {
+          bad.add(new PatternError(r, why.getMessage()));
+        }
+      }
+
+      if (!bad.isEmpty()) {
+        StringBuilder msg = new StringBuilder();
+        msg.append("Invalid commentlink pattern(s):");
+        for (PatternError e : bad) {
+          msg.append("\n");
+          msg.append("\"");
+          msg.append(e.pattern.find());
+          msg.append("\": ");
+          msg.append(e.errorMessage);
+        }
+        Gerrit.SYSTEM_SVC.clientError(msg.toString(),
+            new AsyncCallback<VoidResult>() {
+              @Override
+              public void onFailure(Throwable caught) {
+              }
+
+              @Override
+              public void onSuccess(VoidResult result) {
+              }
+            });
+      }
+
+      try {
+        Gerrit.getConfig().setCommentLinks(safe);
+        return buf.replaceAll(safe);
+      } catch (RuntimeException err2) {
+        // To heck with it. The patterns passed individually above but
+        // failed as a group? Just drop them all and render without.
+        //
+        Gerrit.getConfig().setCommentLinks(null);
+        return buf;
+      }
+    }
+  }
+
+  private static class PatternError {
+    RegexFindReplace pattern;
+    String errorMessage;
+
+    PatternError(RegexFindReplace r, String w) {
+      pattern = r;
+      errorMessage = w;
+    }
+  }
+
+  private CommentLinkProcessor() {
+  }
+}

gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/CommentPanel.java

@@ -107,8 +107,9 @@ public class CommentPanel extends Composite implements HasDoubleClickHandlers {
     }
 
     messageSummary.setText(summarize(message));
-    SafeHtml.set(messageText, new SafeHtmlBuilder().append(message).wikify()
-        .replaceAll(Gerrit.getConfig().getCommentLinks()));
+    SafeHtml buf = new SafeHtmlBuilder().append(message).wikify();
+    buf = CommentLinkProcessor.apply(buf);
+    SafeHtml.set(messageText, buf);
   }
 
   public void setAuthorNameText(final String nameText) {

gerrit-httpd/src/main/java/com/google/gerrit/httpd/GerritConfigProvider.java

@@ -29,6 +29,7 @@ import com.google.gerrit.server.ssh.SshInfo;
 import com.google.gwtexpui.safehtml.client.RegexFindReplace;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.ProvisionException;
 
 import org.eclipse.jgit.lib.Config;
 
@@ -36,6 +37,8 @@ import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.regex.Pattern;
+import java.util.regex.PatternSyntaxException;
 
 class GerritConfigProvider implements Provider<GerritConfig> {
   private final Realm realm;
@@ -109,6 +112,19 @@ class GerritConfigProvider implements Provider<GerritConfig> {
     List<RegexFindReplace> links = new ArrayList<RegexFindReplace>();
     for (String name : cfg.getSubsections("commentlink")) {
       String match = cfg.getString("commentlink", name, "match");
+
+      // Unfortunately this validation isn't entirely complete. Clients
+      // can have exceptions trying to evaluate the pattern if they don't
+      // support a token used, even if the server does support the token.
+      //
+      // At the minimum, we can trap problems related to unmatched groups.
+      try {
+        Pattern.compile(match);
+      } catch (PatternSyntaxException e) {
+        throw new ProvisionException("Invalid pattern \"" + match
+            + "\" in commentlink." + name + ".match: " + e.getMessage());
+      }
+
       String link = cfg.getString("commentlink", name, "link");
       String html = cfg.getString("commentlink", name, "html");
       if (html == null || html.isEmpty()) {

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SystemInfoServiceImpl.java

@@ -20,6 +20,7 @@ import com.google.gerrit.reviewdb.ContributorAgreement;
 import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.ssh.SshInfo;
 import com.google.gwt.user.client.rpc.AsyncCallback;
+import com.google.gwtjsonrpc.client.VoidResult;
 import com.google.gwtorm.client.OrmException;
 import com.google.gwtorm.client.SchemaFactory;
 import com.google.inject.Inject;
@@ -28,12 +29,18 @@ import com.google.inject.Provider;
 import com.jcraft.jsch.HostKey;
 import com.jcraft.jsch.JSch;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.ArrayList;
 import java.util.List;
 
 import javax.servlet.http.HttpServletRequest;
 
 class SystemInfoServiceImpl implements SystemInfoService {
+  private static final Logger log =
+      LoggerFactory.getLogger(SystemInfoServiceImpl.class);
+
   private static final JSch JSCH = new JSch();
 
   private final SchemaFactory<ReviewDb> schema;
@@ -75,4 +82,13 @@ class SystemInfoServiceImpl implements SystemInfoService {
     }
     callback.onSuccess(r);
   }
+
+  @Override
+  public void clientError(String message, AsyncCallback<VoidResult> callback) {
+    HttpServletRequest r = httpRequest.get();
+    String ua = r.getHeader("User-Agent");
+    message = message.replaceAll("\n", "\n  ");
+    log.error("Client UI JavaScript error: User-Agent=" + ua + ": " + message);
+    callback.onSuccess(VoidResult.INSTANCE);
+  }
 }