Skip to content

Exception in chuncked response #68581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
riksking opened this issue Apr 25, 2022 · 12 comments · Fixed by #69016
Closed

Exception in chuncked response #68581

riksking opened this issue Apr 25, 2022 · 12 comments · Fixed by #69016
Assignees
Labels
area-System.Net.Http bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Milestone

Comments

@riksking
Copy link

riksking commented Apr 25, 2022

Hi there.

Describe the bug
I am migrating a project from .Net Framework 4.7.2 to .NET 6.
And I am getting error in response when it is serialized.

System.ServiceModel.ProtocolException: There is a problem with the XML data received from the network. See the description of the inner exception for details. ---> System.Xml.XmlException: The data at the root level is invalid. Line 1, position 1.

I think this is due to the fact that the response does not support chunking.

I got response like this

20d
<soap:Envelope xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/"><soap:Body><ns2:searchClientRs xmlns:ns2="........">
    <messageHeader>
        <messageId>689b1900-ef74-4352-90fd-d180d59c1a37</messageId>
        <correlationId>689b1900-ef74-4352-90fd-d180d59c1a37</correlationId>
        <responseStatus>
            <code>0</code>
            <textMessage/>
            <warning xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:nil="true"/>
        </responseStatus>
    </messageHead
33f
er>
    <clients>
        <client>
            <bankId>1</bankId>
...

Symbols "20d" and "33f" it's special symbol in chunked response, as far as I understand))

I tried using the TransferMode setting to enable chunking.
But neither the TransferMode.Stream setting nor the TransferMode.StreamResponse setting helped and returned the same error.

I found this code in source

namespace System.ServiceModel.Channels
{
    internal class HttpResponseMessageHelper
    {
    ...
        private Task<Message> ReadStreamAsMessageAsync(TimeoutHelper timeoutHelper)
        {
            var content = _httpResponseMessage.Content;
            Task<Stream> contentStreamTask = GetStreamAsync();
 
            if (TransferModeHelper.IsResponseStreamed(_factory.TransferMode))
            {
                return ReadStreamedMessageAsync(contentStreamTask);
            }
            if (!content.Headers.ContentLength.HasValue)
            {
                return ReadChunkedBufferedMessageAsync(contentStreamTask, timeoutHelper);
            }
            return ReadBufferedMessageAsync(contentStreamTask);
        }

And I checked all three read methods.
They all throw the same exception.

To Reproduce
I'm working with a SOAP service that I don't have access to and it's written in Java.
So I don't know how to reproduce this code.
I can attach the full text of the response with headers.

Expected behavior
I expect that everything will work just as well as on the .NET Framework 4.7.2.

Additional context
Soap UI correctly parses this response.

image

@mconnew
Copy link
Member

mconnew commented Apr 27, 2022

The Streamed mode for responses doesn't change how the response is parsed. Basically Streamed vs Buffered is just about do you consume the entire request as a single buffer or consume what's needed for the next output to the application (eg when the return value of the operation is a Stream, reading 100 bytes from the stream will only read 100 bytes worth of encoded data from the SOAP message). So with WCF you can have a client send buffered, and the server receive it streamed, or vice versa. For HTTP it's only about memory consumption patterns.

So now to your problem. it looks like this is a bug in the HttpClient. There's a Content-Length and Transfer-Encoding header. According to rfc7230 section 3.3.3, this isn't valid but does give precedence to the Transfer-Encoding header implying the client should handle this by ignoring the Content-Length. The fact that .NET Framework handles this as well as does Soap UI suggests that HttpClient should too:

   If a message is received with both a Transfer-Encoding and a
   Content-Length header field, the Transfer-Encoding overrides the
   Content-Length.  Such a message might indicate an attempt to
   perform request smuggling ([Section 9.5](https://datatracker.ietf.org/doc/html/rfc7230#section-9.5)) or response splitting
   ([Section 9.4](https://datatracker.ietf.org/doc/html/rfc7230#section-9.4)) and ought to be handled as an error.  A sender MUST
   remove the received Content-Length field prior to forwarding such
   a message downstream.

The behavior that's happening is that the body isn't being treated as though it's been chunked so WCF is getting passed the XML with those extra lines in it which is understandably causing the XmlReader to claim the XML is invalid.

I found where the bug is. In the code here the presence of a Content-Length value is checked before the presence of the Transfer-Encoding chunked header. I'm going to transfer this issue to the runtime repo as that's where the bug is.

@mconnew mconnew transferred this issue from dotnet/wcf Apr 27, 2022
@ghost
Copy link

ghost commented Apr 27, 2022

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 27, 2022
@ghost
Copy link

ghost commented Apr 27, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Hi there.

Describe the bug
I am migrating a project from .Net Framework 4.7.2 to .NET 6.
And I am getting error in response when it is serialized.

System.ServiceModel.ProtocolException: There is a problem with the XML data received from the network. See the description of the inner exception for details. ---> System.Xml.XmlException: The data at the root level is invalid. Line 1, position 1.

I think this is due to the fact that the response does not support chunking.

I got response like this

20d
<soap:Envelope xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/"><soap:Body><ns2:searchClientRs xmlns:ns2="........">
    <messageHeader>
        <messageId>689b1900-ef74-4352-90fd-d180d59c1a37</messageId>
        <correlationId>689b1900-ef74-4352-90fd-d180d59c1a37</correlationId>
        <responseStatus>
            <code>0</code>
            <textMessage/>
            <warning xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:nil="true"/>
        </responseStatus>
    </messageHead
33f
er>
    <clients>
        <client>
            <bankId>1</bankId>
...

Symbols "20d" and "33f" it's special symbol in chunked response, as far as I understand))

I tried using the TransferMode setting to enable chunking.
But neither the TransferMode.Stream setting nor the TransferMode.StreamResponse setting helped and returned the same error.

I found this code in source

namespace System.ServiceModel.Channels
{
    internal class HttpResponseMessageHelper
    {
    ...
        private Task<Message> ReadStreamAsMessageAsync(TimeoutHelper timeoutHelper)
        {
            var content = _httpResponseMessage.Content;
            Task<Stream> contentStreamTask = GetStreamAsync();
 
            if (TransferModeHelper.IsResponseStreamed(_factory.TransferMode))
            {
                return ReadStreamedMessageAsync(contentStreamTask);
            }
            if (!content.Headers.ContentLength.HasValue)
            {
                return ReadChunkedBufferedMessageAsync(contentStreamTask, timeoutHelper);
            }
            return ReadBufferedMessageAsync(contentStreamTask);
        }

And I checked all three read methods.
They all throw the same exception.

To Reproduce
I'm working with a SOAP service that I don't have access to and it's written in Java.
So I don't know how to reproduce this code.
I can attach the full text of the response with headers.

Expected behavior
I expect that everything will work just as well as on the .NET Framework 4.7.2.

Additional context
Soap UI correctly parses this response.

image

Author: riksking
Assignees: -
Labels:

area-System.Net.Http, untriaged

Milestone: -

@MihaZupan
Copy link
Member

You are correct that this is a bug and we should give priority to Transfer-Encoding over Content-Length.
The exact issue has been mentioned in the past in #63053 (comment) and #62906 (comment).
Not closing this as a duplicate yet so we can retriage priority (7.0 vs future).

@mconnew
Copy link
Member

mconnew commented Apr 27, 2022

I would like to call out that there is no workaround for this bug. The person who reported the issue doesn't own the service, so this is a blocker to being able to adopt .NET Core. The server in question is the Java Jetty server which is a mainstream and popular server and not some obscure unusual implementation.

@MihaZupan MihaZupan added tenet-compatibility Incompatibility with previous versions or .NET Framework and removed Regression labels May 5, 2022
@karelz karelz added this to the 7.0.0 milestone May 5, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 5, 2022
@karelz karelz added the help wanted [up-for-grabs] Good issue for external contributors label May 5, 2022
@karelz
Copy link
Member

karelz commented May 5, 2022

Triage: We are going against RFC recommendation. We should follow it instead.
It will be technical breaking change, which should be ok.

The fix is simple - swapping 2 lines. Any takers?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 7, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 9, 2022
@wfurt
Copy link
Member

wfurt commented May 9, 2022

should we reopen this for servicing @karelz since there is no workaround and this violates RFC?
(if so, it may be worth of validating fix with daily builds)

@mconnew
Copy link
Member

mconnew commented May 11, 2022

I vote to back port this to .NET 6 as it's the LTS version.

@MihaZupan MihaZupan added the untriaged New issue has not been triaged by the area owner label May 11, 2022
@MihaZupan
Copy link
Member

MihaZupan commented May 11, 2022

Reopening for backporting consideration.

A technically possible workaround is for the user to check for both headers, and manually unwrap the chunk framing (e.g. wrapping the content in a copy of ChunkedEncodingReadStream), but that's quite horrible.

@MihaZupan MihaZupan reopened this May 11, 2022
@pedrobsaila
Copy link
Contributor

pedrobsaila commented May 11, 2022

#69085 should also be backported with it, because the test that I added in this breaks with NodeJs

@MihaZupan MihaZupan removed the help wanted [up-for-grabs] Good issue for external contributors label May 12, 2022
@antonfirsov antonfirsov modified the milestones: 7.0.0, 6.0.x May 12, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 12, 2022
@antonfirsov
Copy link
Member

antonfirsov commented May 12, 2022

Triage: blocking migration to Core, we should backport the fix to 6.0. The only risk is if someone is relying on the current non RFC-compliant 6.0 behavior.

@MihaZupan MihaZupan self-assigned this May 17, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 14, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2022
@MihaZupan MihaZupan modified the milestones: 6.0.x, 6.0.8 Jul 12, 2022
@MihaZupan
Copy link
Member

MihaZupan commented Jul 12, 2022

Fixed in main (7.0) in PR #69016 and in 6.0.8 in PR #70744.

@karelz karelz modified the milestones: 6.0.8, 6.0.x Jul 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2022
@MihaZupan MihaZupan modified the milestones: 6.0.x, 6.0.8 Oct 21, 2022
@karelz karelz modified the milestones: 6.0.8, 6.0.x Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants