Setting the read timeout in the RequestConfig is not enough. (mitre#150)

* Setting the read timeout in the RequectConfig is not enough.

The read timeout must be set in the SocketConfig as well.

Setting the read timeout only in the RequestConfig can cause hangs which
could block the whole proxy forever.

* SoTimeout cannot be negative -> -1 must be interpreted as 0

* add SocketConfig parameter to getProxyClient() Javadoc

* refactor createHttpClient(RequestConfig, SocketConfig) to
createHttpClient() and do not create a customized socket configuration
if no read timeout is set

* add change description for mitre#150

Author: Martin-Wegner

CHANGES.md

@@ -4,6 +4,16 @@
 \#151: Copy `HttpOnly` flag of proxy coookie to request clients, for fixing security vulnerabilities in cookies.
 This also updates `javax.servlet-api` to `v3.0.1`.
 
+\#150 Setting the read timeout in the `RequestConfig` is not enough.
+The read timeout must be set in the `SocketConfig` as well.
+Setting the read timeout only in the `RequestConfig` can cause hangs which could
+block the whole proxy forever.
+Attention: Method signature of `createHttpClient(RequestConfig)` changed to
+`createHttpClient()`.
+Please override `buildRequestConfig()` and `buildSocketConfig()` to configure the
+Apache HttpClient.
+Thanks Martin Wegner.
+
 \#139: Use Java system properties for http proxy (and other settings) by default.
 This is a regression; it used to work this way in 1.8 and prior.
 Thanks Thorsten Möller.

src/main/java/org/mitre/dsmiley/httpproxy/ProxyServlet.java

@@ -28,6 +28,7 @@
 import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.AbortableHttpRequest;
 import org.apache.http.client.utils.URIUtils;
+import org.apache.http.config.SocketConfig;
 import org.apache.http.entity.InputStreamEntity;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.message.BasicHeader;
@@ -49,7 +50,6 @@
 import java.util.BitSet;
 import java.util.Enumeration;
 import java.util.Formatter;
-import java.util.List;
 
 /**
  * An HTTP reverse proxy/gateway servlet. It is designed to be extended for customization
@@ -91,10 +91,10 @@ public class ProxyServlet extends HttpServlet {
 
   /** A integer parameter name to set the socket read timeout (millis) */
   public static final String P_READTIMEOUT = "http.read.timeout";
-  
+
   /** A boolean parameter whether to use JVM-defined system properties to configure various networking aspects. */
   public static final String P_USESYSTEMPROPERTIES = "useSystemProperties";
-  
+
   /** The parameter name for the target (destination) URI to proxy to. */
   protected static final String P_TARGET_URI = "targetUri";
   protected static final String ATTR_TARGET_URI =
@@ -177,7 +177,7 @@ public void init() throws ServletException {
     if (connectTimeoutString != null) {
       this.connectTimeout = Integer.parseInt(connectTimeoutString);
     }
-    
+
     String readTimeoutString = getConfigParam(P_READTIMEOUT);
     if (readTimeoutString != null) {
       this.readTimeout = Integer.parseInt(readTimeoutString);
@@ -190,7 +190,7 @@ public void init() throws ServletException {
 
     initTarget();//sets target*
 
-    proxyClient = createHttpClient(buildRequestConfig());
+    proxyClient = createHttpClient();
   }
 
   /**
@@ -205,6 +205,20 @@ protected RequestConfig buildRequestConfig() {
             .build();
   }
 
+  /**
+   * Sub-classes can override specific behaviour of {@link org.apache.http.config.SocketConfig}.
+   */
+  protected SocketConfig buildSocketConfig() {
+
+    if (readTimeout < 1) {
+      return null;
+    }
+
+    return SocketConfig.custom()
+            .setSoTimeout(readTimeout)
+            .build();
+  }
+
   protected void initTarget() throws ServletException {
     targetUri = getConfigParam(P_TARGET_URI);
     if (targetUri == null)
@@ -223,16 +237,18 @@ protected void initTarget() throws ServletException {
    * HttpClient offers many opportunities for customization.
    * In any case, it should be thread-safe.
    */
-  protected HttpClient createHttpClient(final RequestConfig requestConfig) {
-    HttpClientBuilder clientBuilder = HttpClientBuilder.create().setDefaultRequestConfig(requestConfig);
+  protected HttpClient createHttpClient() {
+    HttpClientBuilder clientBuilder = HttpClientBuilder.create()
+                                        .setDefaultRequestConfig(buildRequestConfig())
+                                        .setDefaultSocketConfig(buildSocketConfig());
     if (useSystemProperties)
       clientBuilder = clientBuilder.useSystemProperties();
     return clientBuilder.build();
   }
 
   /**
    * The http client used.
-   * @see #createHttpClient(RequestConfig)
+   * @see #createHttpClient()
    */
   protected HttpClient getProxyClient() {
     return proxyClient;
@@ -392,8 +408,8 @@ protected void closeQuietly(Closeable closeable) {
     }
   }
 
-  /** 
-   * Copy request headers from the servlet client to the proxy request. 
+  /**
+   * Copy request headers from the servlet client to the proxy request.
    * This is easily overridden to add your own.
    */
   protected void copyRequestHeaders(HttpServletRequest servletRequest, HttpRequest proxyRequest) {