twitchax / AspNetCore.Proxy

ASP.NET Core Proxies made easy.
MIT License
525 stars 83 forks source link

Hang when using a controller to proxy form #57

Closed PreferLinux closed 4 years ago

PreferLinux commented 4 years ago

When proxying through a controller, there is a hang when handling a POST or PUT request with either a Content-Type: application/x-www-form-urlencoded or Content-Type: multipart/form-data header.

I've reproduced it starting with a simple Web API project, with the following changs:

HEADERS

Cache-Control: no-cache Pragma: no-cache Content-Type: application/x-www-form-urlencoded Host: localhost:5000 TE: trailers Request-Id: |b5df5be7-411c1e27a666778b.1. Content-Length: 14 X-Forwarded-For: 127.0.0.1 X-Forwarded-Proto: https X-Forwarded-Host: localhost:5001 Forwarded: proto=https;host=localhost:5001;by=127.0.0.1;for=127.0.0.1;


But everything works correctly when requesting `https://localhost:5001/echo2` (doesn't match a route, so proxies to `localhost:5000/proxied/echo2`).

This must be the controller mucking around with something when handling form data, but I don't know what...
twitchax commented 4 years ago

Hi @PreferLinux, thanks for filing this.

It is almost definitely the controller mucking about. Very interesting, though, so I will have to take a look at some point.

There might be a way to "turn off" whatever it is doing that causes issues.

PreferLinux commented 4 years ago

Just did some more experimenting. Started by using the EchoAsync() method above, checking out the request headers and in the process noticed the HttpRequest.Form – reading Request.Form first meant the body was consumed, and vise versa (expected). Then I tried adding a string route parameter, with Route("echo/{**rest}") and EchoAsync(string rest), to match the ProxyAsync() method. Now Request.Form always had the data, and not the body. Adding [FromRoute] and such didn't help. I had a look to see if I could find something to prevent reading the form, but I didn't come up with anything much. So I recalled that Request does have request path stuff too – and also route values. So checked those out, and they work as expected. Keep the catchall on the route, don't have any parameters / model binding at all, access the remaining path with Request.RouteValues["rest"], and everything seems to work fine.

So long story short, replace Proxy() above with this, and it works.

[Route("proxy/{**rest}")]
public Task ProxyAsync() {
    var rest = Request.RouteValues["rest"];
    return this.HttpProxyAsync($"http://localhost:5000/{rest}");
}

I'd prefer an option, and this seems a little fragile, but it'll do for now.

slaweklatka commented 4 years ago

@PreferLinux @twitchax Hi Guys, First of all I must say it's vary nicely done proxy.

But had exactly same problem when posting form-data, @PreferLinux it seems your solution works fine. Thanks for help.

garyhuntddn commented 4 years ago

This solution worked for me - but I also needed to use

[Route("proxy/{**rest}")]
public Task ProxyAsync() {
    var rest = (String)Request.RouteValues["rest"] + Request.QueryString;
    return this.HttpProxyAsync($"http://localhost:5000/{rest}");
}

so that the query string part was also passed onto the proxied site

PreferLinux commented 4 years ago

Sorry about that. Yes, including the query string absolutely is necessary. I left it out of the initial test for simplicity (eliminating everything reasonably possible while still keeping a relatively complete example), and never put it back.

twitchax commented 4 years ago

@PreferLinux, including the query string fixes the hang? I may be able to detect a form request and throw a meaningful error.

PreferLinux commented 4 years ago

No, the query string has nothing to do with the hang. The hang is basically because, when using model binding (a method parameter) to get the rest catch-all route parameter on a request with a form Content-Type, the form is processed which consumes the request body – even though the form is never accessed. Fortunately there is another way to access route parameters, Request.RouteValues["rest"], which fixes the problem.

The query string came up because it has to be included in the proxy destination URL separately to the route value, and my examples (particularly my working one above) don't include it.

twitchax commented 4 years ago

@PreferLinux, if you ever have some time, it would be awesome if you could PR a failing test for this. Then, I can investigate some fixes.

mot256 commented 4 years ago

I'm getting the same issue with route values and form data. It's not only the "catch-all" route value, but any route value extracted out of the path.

[Route("proxy/{someValue}/{**rest}")] public Task ProxyAsync(string someValue, string rest) { return this.HttpProxyAsync($"http://localhost:5000/{rest}"); }

I'm using version 4.0.1 of package

PreferLinux commented 4 years ago

@PreferLinux, if you ever have some time, it would be awesome if you could PR a failing test for this. Then, I can investigate some fixes.

Sure, I'll try to do that in the next several days or so.

I'm getting the same issue with route values and form data. It's not only the "catch-all" route value, but any route value extracted out of the path.

[Route("proxy/{someValue}/{**rest}")] public Task ProxyAsync(string someValue, string rest) { return this.HttpProxyAsync($"http://localhost:5000/{rest}"); }

I'm using version 4.0.1 of package

Yeah, I'd expect that using any model binding whatsoever is going to cause ASP.NET Core to parse the form. Therefore the method shouldn't have any parameters and needs to get stuff via other means.

twitchax commented 4 years ago

PR above address...closing.

mot256 commented 4 years ago

Thanks for the fix! When will you release it?

twitchax commented 4 years ago

@mot256, I can release today. If you don't mind, it would be great to get real-world confirmation of the fix. Have you had a chance to build from source and test in your application?

mot256 commented 4 years ago

No. But will give it a shot

mot256 commented 4 years ago

@twitchax I have build locally and did some preliminary testes, and problem seems to be solved. At least for my use case.

twitchax commented 4 years ago

Cool. I will release tomorrow.