SONAR-8733 harden StaticResourceServlet when response is committed

diff --git a/server/sonar-server/src/main/java/org/sonar/server/plugins/StaticResourcesServlet.java b/server/sonar-server/src/main/java/org/sonar/server/plugins/StaticResourcesServlet.java
index d6da860f5c1d7fb48fadc9f5a741693461498fc2..b27456ab48872844f73d09a6f1c3bf6ce0e6cbfd 100644 (file)
--- a/server/sonar-server/src/main/java/org/sonar/server/plugins/StaticResourcesServlet.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/plugins/StaticResourcesServlet.java
@@ -20,23 +20,27 @@
 package org.sonar.server.plugins;
 
 import com.google.common.annotations.VisibleForTesting;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.catalina.connector.ClientAbortException;
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang.StringUtils;
 import org.sonar.api.utils.log.Logger;
 import org.sonar.api.utils.log.Loggers;
+import org.sonar.core.platform.ComponentContainer;
 import org.sonar.core.platform.PluginRepository;
 import org.sonar.server.platform.Platform;
-
-import javax.servlet.ServletException;
-import javax.servlet.http.HttpServlet;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.OutputStream;
 import org.sonarqube.ws.MediaTypes;
 
+import static java.lang.String.format;
+import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
+import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
+
 public class StaticResourcesServlet extends HttpServlet {
 
   private static final Logger LOG = Loggers.get(StaticResourcesServlet.class);
@@ -49,9 +53,9 @@ public class StaticResourcesServlet extends HttpServlet {
     InputStream in = null;
     OutputStream out = null;
     try {
-      PluginRepository pluginRepository = Platform.getInstance().getContainer().getComponentByType(PluginRepository.class);
+      PluginRepository pluginRepository = getContainer().getComponentByType(PluginRepository.class);
       if (!pluginRepository.hasPlugin(pluginKey)) {
-        response.sendError(HttpServletResponse.SC_NOT_FOUND);
+        silentlySendError(response, SC_NOT_FOUND);
         return;
       }
 
@@ -62,24 +66,42 @@ public class StaticResourcesServlet extends HttpServlet {
         out = response.getOutputStream();
         IOUtils.copy(in, out);
       } else {
-        response.sendError(HttpServletResponse.SC_NOT_FOUND);
+        silentlySendError(response, SC_NOT_FOUND);
       }
     } catch (Exception e) {
-      LOG.error(String.format("Unable to load resource [%s] from plugin [%s]", resource, pluginKey), e);
-      response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+      LOG.error(format("Unable to load resource [%s] from plugin [%s]", resource, pluginKey), e);
+      silentlySendError(response, SC_INTERNAL_SERVER_ERROR);
     } finally {
       IOUtils.closeQuietly(in);
       IOUtils.closeQuietly(out);
     }
   }
 
+  @VisibleForTesting
+  protected ComponentContainer getContainer() {
+    return Platform.getInstance().getContainer();
+  }
+
+  private static void silentlySendError(HttpServletResponse response, int error) {
+    if (response.isCommitted()) {
+      LOG.trace("Response is committed. Cannot send error response code {}", error);
+      return;
+    }
+    try {
+      response.sendError(error);
+    } catch (IOException e) {
+      LOG.trace(format("Failed to send error code %s", error), e);
+    }
+  }
+
   /**
    * @return part of request URL after servlet path
    */
-  protected String getPluginKeyAndResourcePath(HttpServletRequest request) {
+  private String getPluginKeyAndResourcePath(HttpServletRequest request) {
     return StringUtils.substringAfter(request.getRequestURI(), request.getContextPath() + request.getServletPath() + "/");
   }
 
+  @VisibleForTesting
   protected String getPluginKey(HttpServletRequest request) {
     return StringUtils.substringBefore(getPluginKeyAndResourcePath(request), "/");
   }

diff --git a/server/sonar-server/src/test/java/org/sonar/server/plugins/StaticResourcesServletTest.java b/server/sonar-server/src/test/java/org/sonar/server/plugins/StaticResourcesServletTest.java
index 12936f78e35b69e98cb89e1edc0ed10d601d5a2e..0fe3f9d10ce5d9cb38134ec9c41136808643090e 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/plugins/StaticResourcesServletTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/plugins/StaticResourcesServletTest.java
@@ -19,60 +19,165 @@
  */
 package org.sonar.server.plugins;
 
-import org.junit.Before;
-import org.junit.Test;
-
+import java.io.IOException;
+import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import org.junit.Rule;
+import org.junit.Test;
+import org.sonar.api.Plugin;
+import org.sonar.api.utils.log.LogTester;
+import org.sonar.api.utils.log.LoggerLevel;
+import org.sonar.core.platform.ComponentContainer;
+import org.sonar.core.platform.PluginRepository;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Matchers.anyInt;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 public class StaticResourcesServletTest {
 
-  private StaticResourcesServlet servlet;
-  private HttpServletRequest request;
+  @Rule
+  public LogTester logTester = new LogTester();
 
-  @Before
-  public void setUp() {
-    servlet = new StaticResourcesServlet();
-    request = mock(HttpServletRequest.class);
-  }
+  private HttpServletRequest request = mock(HttpServletRequest.class);
+  private HttpServletResponse response = mock(HttpServletResponse.class);
+  private ComponentContainer componentContainer = mock(ComponentContainer.class);
+  private PluginRepository pluginRepository = mock(PluginRepository.class);
+  private StaticResourcesServlet underTest = new StaticResourcesServlet() {
+    @Override
+    protected ComponentContainer getContainer() {
+      return componentContainer;
+    }
+  };
 
   @Test
   public void shouldDeterminePluginKey() {
-    when(request.getContextPath()).thenReturn("/");
-    when(request.getServletPath()).thenReturn("static");
-    when(request.getRequestURI()).thenReturn("/static/myplugin/image.png");
-    assertThat(servlet.getPluginKey(request)).isEqualTo("myplugin");
+    mockRequest("myplugin", "image.png");
+    assertThat(underTest.getPluginKey(request)).isEqualTo("myplugin");
 
-    when(request.getRequestURI()).thenReturn("/static/myplugin/images/image.png");
-    assertThat(servlet.getPluginKey(request)).isEqualTo("myplugin");
+    mockRequest("myplugin", "images/image.png");
+    assertThat(underTest.getPluginKey(request)).isEqualTo("myplugin");
 
-    when(request.getRequestURI()).thenReturn("/static/myplugin/");
-    assertThat(servlet.getPluginKey(request)).isEqualTo("myplugin");
+    mockRequest("myplugin", "");
+    assertThat(underTest.getPluginKey(request)).isEqualTo("myplugin");
   }
 
   @Test
   public void shouldDetermineResourcePath() {
-    when(request.getContextPath()).thenReturn("/");
-    when(request.getServletPath()).thenReturn("static");
-    when(request.getRequestURI()).thenReturn("/static/myplugin/image.png");
-    assertThat(servlet.getResourcePath(request)).isEqualTo("static/image.png");
+    mockRequest("myplugin", "image.png");
+    assertThat(underTest.getResourcePath(request)).isEqualTo("static/image.png");
 
-    when(request.getRequestURI()).thenReturn("/static/myplugin/images/image.png");
-    assertThat(servlet.getResourcePath(request)).isEqualTo("static/images/image.png");
+    mockRequest("myplugin", "images/image.png");
+    assertThat(underTest.getResourcePath(request)).isEqualTo("static/images/image.png");
 
-    when(request.getRequestURI()).thenReturn("/static/myplugin/");
-    assertThat(servlet.getResourcePath(request)).isEqualTo("static/");
+    mockRequest("myplugin", "");
+    assertThat(underTest.getResourcePath(request)).isEqualTo("static/");
   }
 
   @Test
   public void completeMimeType() {
     HttpServletResponse response = mock(HttpServletResponse.class);
-    servlet.completeContentType(response, "static/sqale/sqale.css");
+    underTest.completeContentType(response, "static/sqale/sqale.css");
     verify(response).setContentType("text/css");
   }
+
+  @Test
+  public void does_not_fail_nor_log_ERROR_when_response_is_already_committed_and_plugin_does_not_exist() throws ServletException, IOException {
+    mockPluginForResourceDoesNotExist();
+    mockSendErrorOnCommittedResponse();
+
+    underTest.doGet(request, response);
+
+    assertThat(logTester.logs(LoggerLevel.ERROR)).isEmpty();
+    assertThat(logTester.logs(LoggerLevel.TRACE)).containsOnly("Response is committed. Cannot send error response code 404");
+  }
+
+  @Test
+  public void does_not_fail_nor_log_ERROR_when_sendError_throws_IOException_and_plugin_does_not_exist() throws ServletException, IOException {
+    mockPluginForResourceDoesNotExist();
+    mockSendErrorThrowsIOException();
+
+    underTest.doGet(request, response);
+
+    assertThat(logTester.logs(LoggerLevel.ERROR)).isEmpty();
+    assertThat(logTester.logs(LoggerLevel.TRACE)).containsOnly("Failed to send error code 404");
+  }
+
+  private void mockPluginForResourceDoesNotExist() {
+    String pluginKey = "myplugin";
+    mockRequest(pluginKey, "image.png");
+    when(pluginRepository.hasPlugin(pluginKey)).thenReturn(false);
+    when(componentContainer.getComponentByType(PluginRepository.class)).thenReturn(pluginRepository);
+  }
+
+  @Test
+  public void does_not_fail_nor_log_ERROR_when_response_is_already_committed_and_plugin_exists_but_not_resource() throws ServletException, IOException {
+    mockPluginExistsButNoResource();
+    mockSendErrorOnCommittedResponse();
+
+    underTest.doGet(request, response);
+
+    assertThat(logTester.logs(LoggerLevel.ERROR)).isEmpty();
+    assertThat(logTester.logs(LoggerLevel.TRACE)).containsOnly("Response is committed. Cannot send error response code 404");
+  }
+
+  @Test
+  public void does_not_fail_nor_log_ERROR_when_sendError_throws_IOException_and_plugin_exists_but_not_resource() throws ServletException, IOException {
+    mockPluginExistsButNoResource();
+    mockSendErrorThrowsIOException();
+
+    underTest.doGet(request, response);
+
+    assertThat(logTester.logs(LoggerLevel.ERROR)).isEmpty();
+    assertThat(logTester.logs(LoggerLevel.TRACE)).containsOnly("Failed to send error code 404");
+  }
+
+  private void mockPluginExistsButNoResource() {
+    String pluginKey = "myplugin";
+    mockRequest(pluginKey, "image.png");
+    when(pluginRepository.hasPlugin(pluginKey)).thenReturn(true);
+    when(pluginRepository.getPluginInstance(pluginKey)).thenReturn(mock(Plugin.class));
+    when(componentContainer.getComponentByType(PluginRepository.class)).thenReturn(pluginRepository);
+  }
+
+  @Test
+  public void does_not_fail_when_response_is_committed_after_other_error() throws ServletException, IOException {
+    mockRequest("myplugin", "image.png");
+    when(componentContainer.getComponentByType(PluginRepository.class)).thenThrow(new RuntimeException("Simulating a error"));
+    mockSendErrorOnCommittedResponse();
+
+    underTest.doGet(request, response);
+
+    assertThat(logTester.logs(LoggerLevel.ERROR)).containsOnly("Unable to load resource [static/image.png] from plugin [myplugin]");
+  }
+
+  @Test
+  public void does_not_fail_when_sendError_throws_IOException_after_other_error() throws ServletException, IOException {
+    mockRequest("myplugin", "image.png");
+    when(componentContainer.getComponentByType(PluginRepository.class)).thenThrow(new RuntimeException("Simulating a error"));
+    mockSendErrorThrowsIOException();
+
+    underTest.doGet(request, response);
+
+    assertThat(logTester.logs(LoggerLevel.ERROR)).containsOnly("Unable to load resource [static/image.png] from plugin [myplugin]");
+  }
+
+  private void mockSendErrorThrowsIOException() throws IOException {
+    doThrow(new IOException("Simulating sendError throwing IOException")).when(response).sendError(anyInt());
+  }
+
+  private void mockSendErrorOnCommittedResponse() throws IOException {
+    when(response.isCommitted()).thenReturn(true);
+    doThrow(new IllegalStateException("Simulating sendError call when already committed")).when(response).sendError(anyInt());
+  }
+
+  private void mockRequest(String plugin, String resource) {
+    when(request.getContextPath()).thenReturn("/");
+    when(request.getServletPath()).thenReturn("static");
+    when(request.getRequestURI()).thenReturn("/static/" + plugin + "/" + resource);
+  }
 }

diff --git a/server/sonar-server/src/test/java/org/sonar/server/plugins/TestPluginA.java b/server/sonar-server/src/test/java/org/sonar/server/plugins/TestPluginA.java
new file mode 100644 (file)
index 0000000..02f97bc
--- /dev/null
+++ b/server/sonar-server/src/test/java/org/sonar/server/plugins/TestPluginA.java
@@ -0,0 +1,29 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2017 SonarSource SA
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonar.server.plugins;
+
+import org.sonar.api.Plugin;
+
+public class TestPluginA implements Plugin {
+  @Override
+  public void define(Context context) {
+
+  }
+}

diff --git a/server/sonar-server/src/test/resources/static/foo.txt b/server/sonar-server/src/test/resources/static/foo.txt
new file mode 100644 (file)
index 0000000..e69de29