Handle exceptions when sending error message in servlet According to Sonarqube, even though the signatures for methods in a servlet include throws IOException, ServletException, it's a bad idea to let such exceptions be thrown. Failure to catch exceptions in a servlet could leave a system in a vulnerable state, possibly resulting in denial-of-service attacks, or the exposure of sensitive information because when a servlet throws an exception, the servlet container typically sends debugging information back to the user. Change-Id: Ice7f9fddeaa701a0a1d5869abad1c29bdf589bc7
diff --git a/src/main/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServlet.java b/src/main/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServlet.java index 0a7eb2c..b066628 100644 --- a/src/main/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServlet.java +++ b/src/main/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServlet.java
@@ -70,7 +70,7 @@ } private void process(HttpServletRequest req, HttpServletResponse rsp, - String operation) throws IOException { + String operation) { rsp.setContentType("text/plain"); rsp.setCharacterEncoding("UTF-8"); String path = req.getPathInfo(); @@ -81,17 +81,26 @@ index(id, operation); rsp.setStatus(SC_NO_CONTENT); } catch (IOException e) { - rsp.sendError(SC_CONFLICT, e.getMessage()); + sendError(rsp,SC_CONFLICT, e.getMessage()); logger.error("Unable to update index", e); } catch (OrmException e) { String msg = "Error trying to find a change \n"; - rsp.sendError(SC_NOT_FOUND, msg); + sendError(rsp,SC_NOT_FOUND, msg); logger.debug(msg, e); } finally { Context.unsetForwardedEvent(); } } + private static void sendError(HttpServletResponse rsp, int statusCode, + String message) { + try { + rsp.sendError(statusCode, message); + } catch (IOException e) { + logger.error("Failed to send error messsage: " + e.getMessage(), e); + } + } + private void index(Change.Id id, String operation) throws IOException, OrmException { AtomicInteger changeIdLock = getAndIncrementChangeIdLock(id);
diff --git a/src/test/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServletTest.java b/src/test/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServletTest.java index 48c8e2d..3522e77 100644 --- a/src/test/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServletTest.java +++ b/src/test/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServletTest.java
@@ -14,6 +14,7 @@ package com.ericsson.gerrit.plugins.syncindex; +import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND; import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.expectLastCall; @@ -110,6 +111,14 @@ verifyDelete(); } + @Test + public void sendErrorThrowsIOException() throws Exception { + rsp.sendError(SC_NOT_FOUND, "Error trying to find a change \n"); + expectLastCall().andThrow(new IOException("someError")); + setupPostMocks(CHANGE_EXISTS, THROW_ORM_EXCEPTION); + verifyPost(); + } + private void setupPostMocks(boolean changeExist) throws Exception { setupPostMocks(changeExist, DO_NOT_THROW_ORM_EXCEPTION, DO_NOT_THROW_IO_EXCEPTION);