HttpCodecFilter should remove Content-Encoding header entries when handled

glassfishrobot commented 7 years ago

Enabling handling the Content-Encoding filter for incoming data in grizzly 4.1.1.164 revealed a problem in the HttpCodecFilter. The story that revealed this:

https://github.com/payara/Payara/issues/1186

I ran the sample and indeed the jersey GZIPContentEncodingFilter tries to do a second decode of the incoming data. The filter checks the Content-Encoding header and decides based on its presence and value whether or not it should kick in.

The HttpContentFilter of grizzly does not touch the Content-Encoding header, so the header remains and leads to this problem.

I'll attach a proposed patch to fix this problem.

Environment

Payara 4.1.1.164

Affected Versions

[2.3.28]

glassfishrobot commented 7 years ago

Reported by matthiasblaesing

glassfishrobot commented 7 years ago

matthiasblaesing said: The patch is added inline, as I can't attach files.

The idea: HttpCodecFilter extracts the encoding filters it should apply when decoding the data (org.glassfish.grizzly.http.HttpCodecFilter#setContentEncodingsOnParsing). For all following filters this means from their perspective the data is not encoded anymore and having no other information than the header value, this needs to be adjusted.

The unittest CompressionSemanticsTest is adjusted based on the above reasoning. The filters behind the HttpCodedFilter don't see the Content-Encoding anymore and so the header must not be present.

diff --git a/modules/http/src/main/java/org/glassfish/grizzly/http/HttpCodecFilter.java b/modules/http/src/main/java/org/glassfish/grizzly/http/HttpCodecFilter.java
index 2bbf1e2..c7fc9c6 100644
--- a/modules/http/src/main/java/org/glassfish/grizzly/http/HttpCodecFilter.java
+++ b/modules/http/src/main/java/org/glassfish/grizzly/http/HttpCodecFilter.java
@@ -1785,21 +1785,24 @@

         if (bc != null) {
             final List<ContentEncoding> encodings = httpHeader.getContentEncodings(true);
-            int currentIdx = 0;
-
             int commaIdx;
             do {
-commaIdx = bc.indexOf(',', currentIdx);
-final ContentEncoding ce = lookupContentEncoding(bc, currentIdx,
+commaIdx = bc.indexOf(',', 0);
+final ContentEncoding ce = lookupContentEncoding(bc, 0,
         commaIdx >= 0 ? commaIdx : bc.getLength());
 if (ce != null && ce.wantDecode(httpHeader)) {
     encodings.add(0, ce);
+    if(commaIdx > -1) {
+        String remainingEncoding = bc.toString(commaIdx + 1, bc.getLength()).trim();
+        httpHeader.setHeader(Header.ContentEncoding, remainingEncoding);
+    } else {
+        httpHeader.getHeaders().removeHeader(Header.ContentEncoding);
+    }
 } else {
     // if encoding was not found or doesn't want to decode -
     // following could not be applied
     return;
 }
-currentIdx = commaIdx + 1;
             } while (commaIdx >= 0);
         }
     }
diff --git a/modules/http/src/test/java/org/glassfish/grizzly/http/CompressionSemanticsTest.java b/modules/http/src/test/java/org/glassfish/grizzly/http/CompressionSemanticsTest.java
index 9c43e3e..7a1b85a 100644
--- a/modules/http/src/test/java/org/glassfish/grizzly/http/CompressionSemanticsTest.java
+++ b/modules/http/src/test/java/org/glassfish/grizzly/http/CompressionSemanticsTest.java
@@ -101,7 +101,7 @@
         ExpectedResult result = new ExpectedResult();
         result.setProtocol("HTTP/1.1");
         result.setStatusCode(200);
-        result.addHeader("content-encoding", "gzip");
+        result.addHeader("!content-encoding", "gzip");

         final MemoryManager mm = MemoryManager.DEFAULT_MEMORY_MANAGER;
         result.setContent(Buffers.wrap(mm, "Echo: <nothing>"));
@@ -162,7 +162,7 @@
         ExpectedResult result = new ExpectedResult();
         result.setProtocol("HTTP/1.1");
         result.setStatusCode(200);
-        result.addHeader("content-encoding", "gzip");
+        result.addHeader("!content-encoding", "gzip");

         final MemoryManager mm = MemoryManager.DEFAULT_MEMORY_MANAGER;
         result.setContent(Buffers.wrap(mm, "Echo: <nothing>"));
@@ -236,7 +236,7 @@
         ExpectedResult result = new ExpectedResult();
         result.setProtocol("HTTP/1.1");
         result.setStatusCode(200);
-        result.addHeader("content-encoding", "gzip,lzma");
+        result.addHeader("!content-encoding", "gzip,lzma");

         final MemoryManager mm = MemoryManager.DEFAULT_MEMORY_MANAGER;
         result.setContent(Buffers.wrap(mm, "Echo: <nothing>"));
@@ -333,6 +333,10 @@
             clientFilterChainBuilder.add(new ChunkingFilter(2));

             final HttpClientFilter httpClientFilter = new HttpClientFilter();
+            // Cleanup Default Filters +            for(ContentEncoding ce: httpClientFilter.getContentEncodings()) {
+httpClientFilter.removeContentEncoding(ce);
+            }
             if (clientContentEncoding != null) {
 for (ContentEncoding ce : clientContentEncoding) {
     httpClientFilter.addContentEncoding(ce);

glassfishrobot commented 7 years ago

@rlubke said: Thanks for the taking the time to log an issue and provide a patch. I'l spend some time reviewing ASAP.

glassfishrobot commented 7 years ago

lprimak said: I reviewed the patch, and it looks good for what it does, but I agree about the comfort level of changing the input headers. I will try something on Payra side and will get back

glassfishrobot commented 7 years ago

lprimak said: Ryan,

I have extensively tested many scenarios, and have come to the conclusion that it's best to follow what Matthias suggested and remove the encoding header. The issue is that all filters in the filter chain look at that header and if the content has been de-compressed, and the header is still there, any and all filters will think it's still in gzip format, and try to decompress it. There is no other good way to prevent this from happening, aside from removing the header.

I tried many, many different combinations, including Jersey interceptor like this:

@Provider @Priority(Priorities.HEADER_DECORATOR)
public class GzipStripperInterceptor implements ReaderInterceptor {
    @Override
    public Object aroundReadFrom(ReaderInterceptorContext context) throws IOException, WebApplicationException {
        context.getHeaders().get(HttpHeaders.CONTENT_ENCODING).remove("gzip");
        return context.proceed();
    }
}

All of this seems to be a "hacky" workaround and not a real solution. The real solution does seem to be to remove the gzip header.

Thank you!

glassfishrobot commented 7 years ago

lprimak said: Also, this patch:

--- a/modules/http-server/src/main/java/org/glassfish/grizzly/http/server/CompressionEncodingFilter.java
+++ b/modules/http-server/src/main/java/org/glassfish/grizzly/http/server/CompressionEncodingFilter.java
@@ -114,6 +114,7 @@

         assert httpPacket instanceof HttpRequestPacket;
         return canDecompressHttpRequest((HttpRequestPacket) httpPacket, 
+compressionConfig,
 aliases);
     }

@@ -177,8 +178,13 @@
      */
     protected static boolean canDecompressHttpRequest(
             final HttpRequestPacket request,
+            final CompressionConfig config,
             final String[] aliases) {

+        if(config.getCompressionMode() == CompressionMode.OFF) {
+            return false;
+        }
+        
         String contentEncoding = request.getHeader(Header.ContentEncoding);

         // If no header is present, assume request is not encoded, so don't decode

should also be applied because there is no way to turn off Grizzly decompression code anywhere.

(this is Matthias' patch, not mine, so credit attribution stays where it belongs) as seen here: https://github.com/payara/Payara/issues/1186#issuecomment-266105446

glassfishrobot commented 7 years ago

@rlubke said: Perhaps we can compromise on this.

What if we applied this logic on requests only? This sort of issue never manifest itself with response processing. I would feel better about not breaking clients that may rely on that header being present for some reason. However, with the request, this functionality is relatively new, and may be less impactful to remove the header.

glassfishrobot commented 7 years ago

lprimak said: I don't see any harm in that. Thanks, Ryan!

glassfishrobot commented 7 years ago

matthiasblaesing said: Sorry! I missed the client use-case (in review it is clear, but...). I thought a bit more about the problem and would like what you think about this approach (this needs a go from both of you!):

Primary assumption: it is preferred if the general grizzly behavior is not changed and compression becomes opt-in.

On the grizzly side:

Enhance #1669") part1: Consider compression config for decompression as suggested here: https://java.net/jira/browse/GRIZZLY-1875?focusedCommentId=393784&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-393784. Assumption: If compression is disabled decompression should also be disabled. (Modifies CompressionEncodingFilter)

Enhance #1669") part2: Make decompression opt-in. Even if compression is enabled, only enable decompression if the filter is explictly enabled, defaulting to false. Assumption: The status quo should not be changed, but users that know about this can opt-in. (Modifies CompressionEncodingFilter)

Modify HttpCodecFilter and add a property "removeHandledContentEncodingHeaders" (feel free to bikeshed this). The intention: This property would be false by default (again not changing the status quo). If this is set to true, HttpCodecFilter#setContentEncodingsOnParsing will remove the content header elements, that were handled (as currently suggested unconditionally).

In general: Give users, that know what they want the option to enable and control decompression and control about header modifications, without affecting the "normal" users.

On the glassfish/payara side:

GenericGrizzlyListener#configureCompressionEncodings: Enable decompression for the two configured compression filters (gzipContentEncoding and lzmaEncoding) using the grizzly modifications suggested above (this ties payara to the updated grizzly version)

GenericGrizzlyListener#configureHttpProtocol: Enabled "removeHandledContentEncodingHeaders" by setting the property on webServerFilter to true.

When all these changes come together: Grizzly users can enable decompression and are not forced to use it. If they opt in, they can still opt-in to pass headers unmodified of modified. Glassfish will be the first user that uses these enhancements and if a client user wants to opt-in, he/she can decide to do so.

Please give your thoughts.

glassfishrobot commented 7 years ago

@rlubke said: +1. I'll work on this as time permits over the new year. I have no idea what your vacation schedules look like, but I start tomorrow and won't officially be back until the second.

Hopefully, that's not too much of an issue.

glassfishrobot commented 7 years ago

lprimak said: Agree with everything, except one point: When compression is enabled, removal of the compression headers should be turned "on" by default. This is more consistent with "works out-of-the-box" principle

glassfishrobot commented 7 years ago

matthiasblaesing said: Ok - I did a implementation of the discussed changes. I split the patch as described above:

http://www.doppel-helix.eu/2016-12-26-grizzly.patch (patch was done against the 2.3.x branch)

Add option to CompressionConfig to enable/disable decompression (disabled by default) - named: decompressionEnabled
Modify CompressionEncodingFilter to allow specifying enableDecompression via a new constructor variant
Add option to HttpCodecFilter to enable the removal of handled content encoding headers (disabled by default, to stay compatible with current users) - named: removeHandledContentEncodingHeaders
Adjusted CompressionSemanticsTest to verify the adjustment

http://www.doppel-helix.eu/2016-12-26-payara.patch (patch was done against payara master branch, would be good to see this in glassfish too)

Companion to the grizzly patch, grizzly patch is needed prior to this one
Adjust grizzly-spdy to be compatible with grizzly 2.3.39 (ALL_SPDY_VERSIONS was made private)
bump version of grizzly to the 2.3.39-SNAPSHOT version (after release this should of course be 2.3.39)
enable "removeHandledContentEncodingHeaders" for HttpServerFilter (this is one of the main points of this changeset)
enable decompression if compression is enabled (at a later stage glassfish/payara might want to expose this as a seperate configuration element)

A critical look would be appreciated.

I decided against the "on by default" behavior. If there are users out there relying on getting the content-encoding header at this point through the HttpServerFilter, they will have to adjust. As this should IMHO be only a minor version bump I'd like to keep behavioral changes out. Having said this, in general I agree with your argument, so it might be good to add a "enhancement" issue for 3.0.0 to change the default for removeHandledContentEncodingHeaders to true.

glassfishrobot commented 7 years ago

@rlubke said: Reviewing today...

glassfishrobot commented 7 years ago

@rlubke said: I made some minor changes to the Grizzly patch:

fix a typo in the comments
Removed String creation when adjusting the value of the content-encoding header. Instead, set the new start of the Chunk and call a new API added before the break to trim it.

All tests are passing. The changes look good to me.

I'll review the Payara changes and make the necessary changes to GlassFish.

Lastly, I agree with having it disabled by default to preserve backwards compatibility. Please do log an enhancement for 3.0.

glassfishrobot commented 7 years ago

@rlubke said: Changes applied:

2.3.x: 196a756adfd8b01e83b1884dbc44ac18498b1d11 master: aaa9165390a0feadd9b1030d101720c716ff0086

glassfishrobot commented 7 years ago

matthiasblaesing said: Thank you Ryan! I'll follow up with the mention enhancement for Grizzly 3.

Can you give a release date for grizzly 2.3.29? I'd like to try to get this into the next payara version.

glassfishrobot commented 7 years ago

@rlubke said: I'm working on a couple of issues I'd like to get in for 2.3.29. I hope I can have them wrapped up by the end of the week. So potentially next week.

glassfishrobot commented 7 years ago

This issue was imported from java.net JIRA GRIZZLY-1875

glassfishrobot commented 7 years ago

Marked as fixed on Tuesday, January 3rd 2017, 11:58:11 am

javaee / grizzly

HttpCodecFilter should remove Content-Encoding header entries when handled #1875

Environment

Affected Versions