From 2c261abdcead698b11f9f4a09e2c7494ed2d62ee Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Tue, 27 Jun 2017 14:24:25 +0200 Subject: [PATCH] SONAR-8733 harden StaticResourceServlet when response is committed --- .../plugins/StaticResourcesServlet.java | 52 ++++-- .../plugins/StaticResourcesServletTest.java | 159 +++++++++++++++--- .../org/sonar/server/plugins/TestPluginA.java | 29 ++++ .../src/test/resources/static/foo.txt | 0 4 files changed, 198 insertions(+), 42 deletions(-) create mode 100644 server/sonar-server/src/test/java/org/sonar/server/plugins/TestPluginA.java create mode 100644 server/sonar-server/src/test/resources/static/foo.txt 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 d6da860f5c1..b27456ab488 100644 --- 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 12936f78e35..0fe3f9d10ce 100644 --- 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 index 00000000000..02f97bc19ca --- /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 index 00000000000..e69de29bb2d -- 2.39.5