Skip to content
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

Follow vs return redirects #56

Open
PreferLinux opened this issue Jun 15, 2020 · 6 comments
Open

Follow vs return redirects #56

PreferLinux opened this issue Jun 15, 2020 · 6 comments

Comments

@PreferLinux
Copy link
Contributor

Right now, if proxying to somewhere that returns a redirect, the HttpClient follows the redirect, and the original requester never sees it – so if the original request is from a web browser, the browser still shows the original URL. I just had this and wanted to return the redirects, so I figured out how to get that to work. I realise that both behaviours are valid, and the existing behaviour is probably better for many situations, but here's how I got it to work for me in case it helps someone else.

There are two parts: telling the HttpClient not to follow the redirect, and editing the Location: response header so that it still goes through the proxy if needed.

As far as I can see, the first has to be handled when configuring a named client, like this:

services.AddHttpClient("client_name").ConfigurePrimaryHttpMessageHandler(() => new HttpClientHandler() {
   AllowAutoRedirect = false
});

The second uses WithAfterReceive() like this:

var httpOpts = HttpProxyOptionsBuilder.Instance
.WithHttpClientName("client_name")
.WithAfterReceive((ctx, resp) => {
	if (resp.Headers.Contains("Location")) {
		var relUri = destUri.MakeRelativeUri(resp.Headers.Location);
		if (!relUri.IsAbsoluteUri) {
			resp.Headers.Location = new Uri(ourUrli, relUri);
		}
	}
	return Task.CompletedTask;
})
.Build();

With both in place, the proxy returns valid redirects.

@twitchax
Copy link
Owner

Ahhh, cool. I could add this as part of the options.

@PreferLinux
Copy link
Contributor Author

That would be good. I see that you're adding and using a named client already, so you'd be able to do that part there easily with the option. However if anyone wanted to use their own named client they'd have to do it themselves unless you have them pass in the result of the services.AddHttpClient() call.

The location editing I did there is OK, but it doesn't account for everything it probably should. For instance, if the response Uri's Host is private it really needs to edit it, but it doesn't necessarily know how to do that. And even when the actual correct return URL can be determined, the proxy might not be listening for requests from there, and may well not send the requests to the new location. But for basic stuff like redirecting to or from a login page it should work fine.

My use case, in case it is helpful, was proxying pgAdmin (running under Apache!) behind an Asp.Net Core internet-facing site. However I ran into a issue which seems to be a problem proxying a POSTed form (request times out while reading data), which I should really open another issue for...

@twitchax
Copy link
Owner

Yeah, there will definitely be some edge cases that I will need to consider. It might make sense to just cover most cases, and then add improvements over time.

@shoe-diamente
Copy link

shoe-diamente commented Sep 21, 2020

Stumbled upon this one myself.
My use case was reverse proxying PhpMyAdmin.

Basically I have a https://example.com server which proxies PhpMyAdmin via https://example.com/phpmyadmin/ to a http://localhost:8083 phpmyadmin local server instance.
At some point in the authentication flow phpmyadmin returns a 302 redirect with Location: /phpmyadmin/index.php and AspNetCore.Proxy by default short circuits that to http://localhost:8083/phpmyadmin/index.php which gives 404 as opposed to returning it back and letting the client request https://example.com/phpmyadmin/index.php.

I was able to follow @PreferLinux's advice, but I would recommend having auto redirect disabled by default instead in the library.
Just my 2 cents 😄

Peace ✌️

@twitchax
Copy link
Owner

Yeah, makes sense. I will take a look when I get a chance. 👍

A PR for a failing test case wouldn't hurt. :)

@jl-beast
Copy link

Hey, I ran into this same case myself, I was wondering if this could be provided in by default?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants