AMBARI-14984 - Script Alert Parameters With Shell Characters Are Not Properly Escaped (jonathanhurley)

ambari-funtest/pom.xml

@@ -495,11 +495,6 @@
       <artifactId>httpclient</artifactId>
       <version>4.2.5</version>
     </dependency>
-    <dependency>
-      <groupId>com.google.guava</groupId>
-      <artifactId>guava</artifactId>
-      <version>14.0.1</version>
-    </dependency>
     <dependency>
       <groupId>com.google.code.findbugs</groupId>
       <artifactId>jsr305</artifactId>

ambari-project/pom.xml

@@ -220,7 +220,7 @@
       <dependency>
         <groupId>com.google.guava</groupId>
         <artifactId>guava</artifactId>
-        <version>14.0.1</version>
+        <version>16.0</version>
       </dependency>
       <dependency>
         <groupId>com.google.code.findbugs</groupId>

ambari-server/pom.xml

@@ -1178,11 +1178,6 @@
       <artifactId>httpclient</artifactId>
       <version>4.2.5</version>
     </dependency>
-    <dependency>
-      <groupId>com.google.guava</groupId>
-      <artifactId>guava</artifactId>
-      <version>14.0.1</version>
-    </dependency>
     <dependency>
       <groupId>com.google.code.findbugs</groupId>
       <artifactId>jsr305</artifactId>

ambari-server/src/main/java/org/apache/ambari/server/notifications/dispatchers/AlertScriptDispatcher.java

@@ -40,6 +40,8 @@ import org.apache.commons.lang.SystemUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.google.common.escape.Escaper;
+import com.google.common.escape.Escapers;
 import com.google.inject.Inject;
 
 /**
@@ -81,6 +83,18 @@ public class AlertScriptDispatcher implements NotificationDispatcher {
    */
   private static final long DEFAULT_SCRIPT_TIMEOUT = 5000L;
 
+  /**
+   * Used to escape text being passed into the shell command.
+   */
+  public static final Escaper SHELL_ESCAPE;
+
+  static {
+    final Escapers.Builder builder = Escapers.builder();
+    builder.addEscape('\"', "\\\"");
+    builder.addEscape('!', "\\!");
+    SHELL_ESCAPE = builder.build();
+  }
+
   /**
    * Configuration data from the ambari.properties file.
    */
@@ -242,8 +256,8 @@ public class AlertScriptDispatcher implements NotificationDispatcher {
 
     // these could have spaces in them, so quote them so they don't mess up the
     // command line
-    String alertLabel = "\"" + definition.getLabel() + "\"";
-    String alertText = "\"" + alertInfo.getAlertText() + "\"";
+    String alertLabel = "\"" + SHELL_ESCAPE.escape(definition.getLabel()) + "\"";
+    String alertText = "\"" + SHELL_ESCAPE.escape(alertInfo.getAlertText()) + "\"";
 
     Object[] params = new Object[] { script, definitionName, alertLabel, serviceName,
         alertState.name(), alertText };

ambari-server/src/main/java/org/apache/ambari/server/state/services/AlertNoticeDispatchService.java

@@ -712,7 +712,7 @@ public class AlertNoticeDispatchService extends AbstractScheduledService {
      *
      * @param history
      */
-    protected AlertInfo(AlertHistoryEntity history) {
+    public AlertInfo(AlertHistoryEntity history) {
       m_history = history;
     }
 

ambari-server/src/main/java/org/apache/ambari/server/state/services/CachedAlertFlushService.java

@@ -76,7 +76,7 @@ public class CachedAlertFlushService extends AbstractScheduledService {
   protected void startUp() throws Exception {
     boolean enabled = m_configuration.isAlertCacheEnabled();
     if (!enabled) {
-      stop();
+      stopAsync();
     }
   }
 

ambari-server/src/test/java/org/apache/ambari/server/notifications/dispatchers/AlertScriptDispatcherTest.java

@@ -20,6 +20,7 @@ package org.apache.ambari.server.notifications.dispatchers;
 import java.lang.reflect.Field;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.UUID;
 import java.util.concurrent.Executor;
 
@@ -28,8 +29,12 @@ import org.apache.ambari.server.notifications.DispatchCallback;
 import org.apache.ambari.server.notifications.DispatchFactory;
 import org.apache.ambari.server.notifications.Notification;
 import org.apache.ambari.server.notifications.NotificationDispatcher;
+import org.apache.ambari.server.orm.entities.AlertDefinitionEntity;
+import org.apache.ambari.server.orm.entities.AlertHistoryEntity;
+import org.apache.ambari.server.state.AlertState;
 import org.apache.ambari.server.state.alert.AlertNotification;
 import org.apache.ambari.server.state.alert.TargetType;
+import org.apache.ambari.server.state.services.AlertNoticeDispatchService.AlertInfo;
 import org.apache.ambari.server.state.stack.OsFamily;
 import org.easymock.EasyMock;
 import org.junit.Before;
@@ -44,6 +49,8 @@ import com.google.inject.Guice;
 import com.google.inject.Inject;
 import com.google.inject.Injector;
 
+import junit.framework.Assert;
+
 /**
  * Tests {@link AlertScriptDispatcher}.
  */
@@ -216,6 +223,60 @@ public class AlertScriptDispatcherTest {
     PowerMock.verifyAll();
   }
 
+  /**
+   * Tests that arguments given to the {@link ProcessBuilder} are properly
+   * escaped.
+   *
+   * @throws Exception
+   */
+  @Test
+  public void testArgumentEscaping() throws Exception {
+    final String ALERT_DEFINITION_NAME = "mock_alert_with_quotes";
+    final String ALERT_DEFINITION_LABEL = "Mock alert with Quotes";
+    final String ALERT_LABEL = "Alert Label";
+    final String ALERT_SERVICE_NAME = "FOO_SERVICE";
+    final String ALERT_TEXT = "Did you know, \"Quotes are hard!!!\"";
+    final String ALERT_TEXT_ESCAPED = "Did you know, \\\"Quotes are hard\\!\\!\\!\\\"";
+
+    DispatchCallback callback = EasyMock.createNiceMock(DispatchCallback.class);
+    AlertNotification notification = new AlertNotification();
+    notification.Callback = callback;
+    notification.CallbackIds = Collections.singletonList(UUID.randomUUID().toString());
+
+    AlertDefinitionEntity definition = new AlertDefinitionEntity();
+    definition.setDefinitionName(ALERT_DEFINITION_NAME);
+    definition.setLabel(ALERT_DEFINITION_LABEL);
+
+    AlertHistoryEntity history = new AlertHistoryEntity();
+    history.setAlertDefinition(definition);
+    history.setAlertLabel(ALERT_LABEL);
+    history.setAlertText(ALERT_TEXT);
+    history.setAlertState(AlertState.OK);
+    history.setServiceName(ALERT_SERVICE_NAME);
+
+    AlertInfo alertInfo = new AlertInfo(history);
+    notification.setAlertInfo(alertInfo);
+
+    AlertScriptDispatcher dispatcher = new AlertScriptDispatcher();
+    m_injector.injectMembers(dispatcher);
+
+    ProcessBuilder processBuilder = dispatcher.getProcessBuilder(SCRIPT_CONFIG_VALUE, notification);
+    List<String> commands = processBuilder.command();
+    Assert.assertEquals(3, commands.size());
+    Assert.assertEquals("sh", commands.get(0));
+    Assert.assertEquals("-c", commands.get(1));
+
+    StringBuilder buffer = new StringBuilder();
+    buffer.append(SCRIPT_CONFIG_VALUE).append(" ");
+    buffer.append(ALERT_DEFINITION_NAME).append(" ");
+    buffer.append("\"").append(ALERT_DEFINITION_LABEL).append("\"").append(" ");
+    buffer.append(ALERT_SERVICE_NAME).append(" ");
+    buffer.append(AlertState.OK).append(" ");
+    buffer.append("\"").append(ALERT_TEXT_ESCAPED).append("\"");
+
+    Assert.assertEquals(buffer.toString(), commands.get(2));
+  }
+
   /**
    *
    */