Update DeflateDecompressionProvider to use ZLibStream instead of DeflateStream

[EatonZ]

Please see this issue, which describes the issue with using DeflateStream, and as the reason ZLibStream was introduced.

In a nutshell, refer to RFC 2616, section 3.5:

deflate The "zlib" format defined in RFC 1950 [31] in combination with the "deflate" compression mechanism described in RFC 1951 [29].

That means deflate, in the context of HTTP, is comprised of the zlib header + deflate compression. When using DeflateStream, it does not include the zlib header. ZLibStream does include the zlib header, and is the correct stream to use in this case.

DeflateDecompressionProvider is using DeflateStream, meaning it is omitting the zlib header, which is required by the RFC.

See also: This comment

Breaking changes possibility: .NET currently does not provide a built-in mechanism to compress HTTP bodies (is it currently suggested here). Any compression would be done by the user in their own code. If they are not using the right stream type, then it's possible the decompression could fail on the server.

[ghost]

Thanks for your PR, @EatonZ. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[davidfowl]

Can you summarize the issue here and explain if this is a breaking change?

[EatonZ]

Hi @davidfowl, I added further info.

[davidfowl]

Breaking changes possibility: .NET currently does not provide a built-in mechanism to compress HTTP bodies (is it currently suggested https://github.com/dotnet/runtime/issues/46944). Any compression would be done by the user in their own code. If they are not using the right stream type, then it's possible the decompression could fail on the server.

What happens if we change this in .NET 8? Will clients that used to work stop working?

[EatonZ]

I will have to check if that will be an issue. If it is, it looks like a compatibility layer was added here that could probably be ported over to aspnetcore somewhere, if you do not want a breaking change.

[EatonZ]

Ok, yes, if the stream is changed, it will be a breaking change, unless a compatibility layer or something is added.

My tests show that the stream you compress with must match the stream used to decompress (in DeflateDecompressionProvider). If you compress using ZLibStream and fire off the request to a .NET 7 API, this exception occurs:

Exception thrown: 'System.IO.InvalidDataException' in System.Private.CoreLib.dll
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware: Error: An unhandled exception has occurred while executing the request.

System.IO.InvalidDataException: The archive entry was compressed using an unsupported compression method.
   at System.IO.Compression.Inflater.Inflate(FlushCode flushCode)
   at System.IO.Compression.Inflater.ReadInflateOutput(Byte* bufPtr, Int32 length, FlushCode flushCode, Int32& bytesRead)
   at System.IO.Compression.Inflater.ReadOutput(Byte* bufPtr, Int32 length, Int32& bytesRead)
   at System.IO.Compression.Inflater.InflateVerified(Byte* bufPtr, Int32 length)
   at System.IO.Compression.DeflateStream.<ReadAsyncMemory>g__Core|50_0(Memory`1 buffer, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.RequestDecompression.SizeLimitedStream.ReadAsync(Memory`1 buffer, CancellationToken cancellationToken)
   at WebApplication2.Controllers.WeatherForecastController.Post() in C:\WebApplication1\Controllers\WeatherForecastController.cs:line 37
   at lambda_method6(Closure, Object)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.AwaitableObjectResultExecutor.Execute(ActionContext actionContext, IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Logged|12_1(ControllerActionInvoker invoker)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.RequestDecompression.RequestDecompressionMiddleware.InvokeCore(HttpContext context, Stream decompressionStream)
   at Microsoft.AspNetCore.RequestDecompression.RequestDecompressionMiddleware.InvokeCore(HttpContext context, Stream decompressionStream)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

[Tratcher]

Do we know if any clients that actually use request body compression and are currently broken?

[Tratcher]

At least the tests fail as expected 😁 (you'll need to update those)

[EatonZ]

Ok, so you agree with my change proposal and it is correct?

[Tratcher]

It's interesting that we avoided this issue in ResponseCompression by not implementing Deflate. https://github.com/dotnet/aspnetcore/blob/83d6c56ab616709ec66381def032ffca0065c0ee/src/Middleware/ResponseCompression/src/ResponseCompressionProvider.cs#L41-L45

HttpClient seems to support both formats by checking the first byte. We could do the same here to avoid a break. https://github.com/dotnet/runtime/blob/4e0195e02fac31032cc8ffd759a6054e3107844a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs#L305-L308

[davidfowl]

We shouldn't make a breaking change, if anything we can do what HttpClient did and sniff the protocol.

[adityamandaleeka]

@EatonZ Would you be interested in modifying this fix to match the HttpClient behavior @Tratcher mentioned above?

[EatonZ]

@davidfowl I'll take a look when I have time.🙂

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

[adityamandaleeka]

Closing this PR. I've just opened https://github.com/dotnet/aspnetcore/pull/47522 with the non-breaking approach discussed.

[adityamandaleeka]

Thanks for raising this @EatonZ

[EatonZ]

@adityamandaleeka Great work! Sorry I took so long - I have been busy lately.

[ghost]

Hi @EatonZ. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.