microsoft / reverse-proxy

A toolkit for developing high-performance HTTP reverse proxy applications.
https://microsoft.github.io/reverse-proxy
MIT License
8.6k stars 843 forks source link

How to change the request body correctly in middleware #2617

Closed inforly closed 1 month ago

inforly commented 2 months ago

Describe the bug

We are developing a middleware to change the request (transform is not suitable here, since we want to the change happens in the 1st middleware): if a GET request has the q query parameter, the middleware will change it to a POST request and set the value of the q query parameter as the body of the request, the code like:

        public Task Invoke(HttpContext context)
        {
            // convert GET to POST if with q query parameter
            if (HttpMethods.IsGet(context.Request.Method) && context.Request.Query.TryGetValue("q", out var query))
            {
                context.Request.Method = HttpMethods.Post;
                var queryBytes = Encoding.UTF8.GetBytes(query);
                context.Request.ContentLength = queryBytes.Length;
                context.Request.Body = new MemoryStream(queryBytes);
            }
            return _next(context);
        }

But when we send the request like: curl http://localhost:5000/?q=test, it will throw the error:

warn: Yarp.ReverseProxy.Forwarder.HttpForwarder[48]
      Request: An error was encountered before receiving a response.
      **System.Net.Http.HttpRequestException: Sent 0 request content bytes, but Content-Length promised 4.**
         at System.Net.Http.HttpConnection.SendRequestContentAsync(HttpRequestMessage request, HttpContentWriteStream stream, Boolean async, CancellationToken cancellationToken)
         at System.Net.Http.HttpConnection.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
         at System.Net.Http.HttpConnection.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
         at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
         at System.Net.Http.DiagnosticsHandler.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
         at Yarp.ReverseProxy.Forwarder.HttpForwarder.SendAsync(HttpContext context, String destinationPrefix, HttpMessageInvoker httpClient, ForwarderRequestConfig requestConfig, HttpTransformer transformer, CancellationToken cancellationToken)

Could you please take a look of this, how to change the body correctly for such scenario?

To Reproduce

Further technical details

MihaZupan commented 2 months ago

I actually expected your code to work as-is.

The way we're detecting the presence of a request body isn't accounting for the body being created for a request that originally had none. You can "fix" that with this if you do so:

 context.Request.Body = new MemoryStream(queryBytes);
+context.Features.Set<IHttpRequestBodyDetectionFeature>(null);

@Tratcher does that seem like something we'd want to change in ASP.NET (IHttpRequestBodyDetectionFeature.CanHaveBody)?

As-is, this case hits a debug assert in YARP here: https://github.com/microsoft/reverse-proxy/blob/181efd33d77c7ffe07a096aa682fc8d0e8239b99/src/ReverseProxy/Forwarder/RequestUtilities.cs#L326-L331

inforly commented 2 months ago

I actually expected your code to work as-is.

The way we're detecting the presence of a request body isn't accounting for the body being created for a request that originally had none. You can "fix" that with this if you do so:

 context.Request.Body = new MemoryStream(queryBytes);
+context.Features.Set<IHttpRequestBodyDetectionFeature>(null);

@Tratcher does that seem like something we'd want to change in ASP.NET (IHttpRequestBodyDetectionFeature.CanHaveBody)?

As-is, this case hits a debug assert in YARP here:

https://github.com/microsoft/reverse-proxy/blob/181efd33d77c7ffe07a096aa682fc8d0e8239b99/src/ReverseProxy/Forwarder/RequestUtilities.cs#L326-L331

It works, thanks a lot!

Tratcher commented 1 month ago

Changing a GET to a POST is an unusual transformation. Rather than dropping IHttpRequestBodyDetectionFeature I'd suggest replacing it with one that returns true.

@inforly, you'll also want to remove that parameter from the query to avoid duplication.

adityamandaleeka commented 1 month ago

Glad this is working for you now @inforly. Since this is the only issue we've seen about this (and as @tratcher pointed out, it's an unusual transformation), I'm closing this issue now.

inforly commented 1 month ago

Changing a GET to a POST is an unusual transformation. Rather than dropping IHttpRequestBodyDetectionFeature I'd suggest replacing it with one that returns true.

@inforly, you'll also want to remove that parameter from the query to avoid duplication.

@Tratcher could you please give the details for this?

Tratcher commented 1 month ago

@inforly something like:

class MyBodyDetectionFeature : IHttpBodyDetectionFeature {
 CanHaveBody => true;
}
context.Features.Set<IHttpRequestBodyDetectionFeature>(new MyBodyDetectionFeature());