commit 786fbaaec9b3a1f09bef552e07eaf94cfaa18b64
author Hugo Arès <hugo.ares@ericsson.com> Thu Jul 14 14:12:17 2016 -0400
committer Hugo Arès <hugo.ares@ericsson.com> Thu Jul 14 14:12:17 2016 -0400
tree b1063a0efabe6b87d849002b8a15c37634479e1b
parent 19d8b4895ae50d1f05ef87035f3388f8df5dcfe8

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

src/main/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServlet.java

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);

src/test/java/com/ericsson/gerrit/plugins/syncindex/SyncIndexRestApiServletTest.java

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);