Merge pull request adamfisk#343 from danielkyu/bug-format-accept-enco…

…ding-remove-sdch

Fixed bug where the 'Accept-Encoding' header could be improperly form…

src/main/java/org/littleshoot/proxy/impl/ClientToProxyConnection.java

@@ -1091,7 +1091,8 @@ private void modifyRequestHeadersToReflectProxying(HttpRequest httpRequest) {
 
             HttpHeaders headers = httpRequest.headers();
 
-            removeSDCHEncoding(headers);
+            // Remove sdch from encodings we accept since we can't decode it.
+            ProxyUtils.removeSdchEncoding(headers);
             switchProxyConnectionHeader(headers);
             stripConnectionTokens(headers);
             stripHopByHopHeaders(headers);
@@ -1128,22 +1129,6 @@ private void modifyResponseHeadersToReflectProxying(
         }
     }
 
-    /**
-     * Remove sdch from encodings we accept since we can't decode it.
-     * 
-     * @param headers
-     *            The headers to modify
-     */
-    private void removeSDCHEncoding(HttpHeaders headers) {
-        String ae = headers.get(HttpHeaders.Names.ACCEPT_ENCODING);
-        if (StringUtils.isNotBlank(ae)) {
-            //
-            String noSdch = ae.replace(",sdch", "").replace("sdch", "");
-            headers.set(HttpHeaders.Names.ACCEPT_ENCODING, noSdch);
-            LOG.debug("Removed sdch and inserted: {}", noSdch);
-        }
-    }
-
     /**
      * Switch the de-facto standard "Proxy-Connection" header to "Connection"
      * when we pass it along to the remote host. This is largely undocumented

src/main/java/org/littleshoot/proxy/impl/ProxyUtils.java

@@ -637,4 +637,30 @@ public static FullHttpResponse createFullHttpResponse(HttpVersion httpVersion,
 
         return response;
     }
+
+    /**
+     * Given an HttpHeaders instance, removes 'sdch' from the 'Accept-Encoding'
+     * header list (if it exists) and returns the modified instance.
+     *
+     * Removes all occurrences of 'sdch' from the 'Accept-Encoding' header.
+     * @param headers The headers to modify.
+     */
+    public static void removeSdchEncoding(HttpHeaders headers) {
+        List<String> encodings = headers.getAll(HttpHeaders.Names.ACCEPT_ENCODING);
+        headers.remove(HttpHeaders.Names.ACCEPT_ENCODING);
+
+        for (String encoding : encodings) {
+            if (encoding != null) {
+                // The former regex should remove occurrences of 'sdch' while the
+                // latter regex should take care of the dangling comma case when
+                // 'sdch' was the first element in the list and there are other
+                // encodings.
+                encoding = encoding.replaceAll(",? *(sdch|SDCH)", "").replaceFirst("^ *, *", "");
+
+                if (StringUtils.isNotBlank(encoding)) {
+                    headers.add(HttpHeaders.Names.ACCEPT_ENCODING, encoding);
+                }
+            }
+        }
+    }
 }

src/test/java/org/littleshoot/proxy/impl/ProxyUtilsTest.java

@@ -1,6 +1,7 @@
 package org.littleshoot.proxy.impl;
 
 import io.netty.handler.codec.http.DefaultFullHttpRequest;
+import io.netty.handler.codec.http.DefaultHttpHeaders;
 import io.netty.handler.codec.http.DefaultHttpMessage;
 import io.netty.handler.codec.http.DefaultHttpResponse;
 import io.netty.handler.codec.http.HttpHeaders;
@@ -11,6 +12,8 @@
 import io.netty.handler.codec.http.HttpVersion;
 import org.junit.Test;
 
+import java.util.Arrays;
+import java.util.ArrayList;
 import java.util.List;
 
 import static org.hamcrest.Matchers.contains;
@@ -235,4 +238,57 @@ public void testSplitCommaSeparatedHeaderValues() {
         assertThat("Expected no header tokens", ProxyUtils.splitCommaSeparatedHeaderValues("  \t  \t  "), empty());
         assertThat("Expected no header tokens", ProxyUtils.splitCommaSeparatedHeaderValues(" ,  ,\t, "), empty());
     }
+
+    /**
+     * Verifies that 'sdch' is removed from the 'Accept-Encoding' header list.
+     */
+    @Test
+    public void testRemoveSdchEncoding() {
+        final List<String> emptyList = new ArrayList<>();
+        // Various cases where 'sdch' is not present within the accepted
+        // encodings list
+        assertRemoveSdchEncoding(Arrays.asList(""), emptyList);
+        assertRemoveSdchEncoding(Arrays.asList("gzip"), Arrays.asList("gzip"));
+
+        assertRemoveSdchEncoding(Arrays.asList("gzip", "deflate", "br"), Arrays.asList("gzip", "deflate", "br"));
+        assertRemoveSdchEncoding(Arrays.asList("gzip, deflate, br"), Arrays.asList("gzip, deflate, br"));
+
+        // Various cases where 'sdch' is present within the accepted encodings
+        // list
+        assertRemoveSdchEncoding(Arrays.asList("sdch"), emptyList);
+        assertRemoveSdchEncoding(Arrays.asList("SDCH"), emptyList);
+
+        assertRemoveSdchEncoding(Arrays.asList("sdch", "gzip"), Arrays.asList("gzip"));
+        assertRemoveSdchEncoding(Arrays.asList("sdch, gzip"), Arrays.asList("gzip"));
+
+        assertRemoveSdchEncoding(Arrays.asList("gzip", "sdch", "deflate"), Arrays.asList("gzip", "deflate"));
+        assertRemoveSdchEncoding(Arrays.asList("gzip, sdch, deflate"), Arrays.asList("gzip, deflate"));
+        assertRemoveSdchEncoding(Arrays.asList("gzip,deflate,sdch"), Arrays.asList("gzip,deflate"));
+
+        assertRemoveSdchEncoding(Arrays.asList("gzip", "deflate, sdch", "br"), Arrays.asList("gzip", "deflate", "br"));
+    }
+
+    /**
+     * Helper method that asserts that 'sdch' is removed from the
+     * 'Accept-Encoding' header.
+     *
+     * @param inputEncodings The input value of the 'Accept-Encoding' header
+     *        that should be used as the basis for the assertion check.
+
+     * @param inputEncodings The input list that maps to the values of the
+     *        'Accept-Encoding' header that should be used as the basis for the
+     *        assertion check.
+     * @param expectedEncodings The list containing the expected values of the
+     *        'Accept-Encoding' header after the 'sdch' encoding is removed.
+     */
+    private void assertRemoveSdchEncoding(List<String> inputEncodings, List<String> expectedEncodings) {
+        HttpHeaders headers = new DefaultHttpHeaders();
+
+        for (String encoding : inputEncodings) {
+            headers.add(HttpHeaders.Names.ACCEPT_ENCODING, encoding);
+        }
+
+        ProxyUtils.removeSdchEncoding(headers);
+        assertEquals(expectedEncodings, headers.getAll(HttpHeaders.Names.ACCEPT_ENCODING));
+    }
 }