tmenier / Flurl

Fluent URL builder and testable HTTP client for .NET
https://flurl.dev
MIT License
4.23k stars 387 forks source link

`OnRedirect` does not work with custom `HttpClient` #831

Closed DanielHabenicht closed 3 months ago

DanielHabenicht commented 4 months ago

If one needs a custom HttpClient, e.g. for certificate issues during testing the redirect functionalities of Flurl do not work.

var handler = new HttpClientHandler()
{
    ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator
};

// Works if the HttpClient is not given as argument
var client = new FlurlClient(new HttpClient(handler)).OnRedirect(
    call =>
    {
        if (call.Redirect.Url.Host == "customHost")
        {
            call.Redirect.Url.Host = "test.domain.internal";
        }

        call.Redirect.Follow = true;
    });
client.Settings.Redirects.Enabled = false;

using var session = new CookieSession(client);
var result = await session
    .Request("http://google.com/")
    .GetAsync();

// result.StatusCode is 200 instead of the expected 301, Enabled = false should not have redirected.

This is unexpected behaviour or not documented. Furthermore the OnRedirect Callback never gets called, as well as .WithAutoRedirect(false)

tmenier commented 4 months ago

Flurl needs to disable redirects on the HttpClientHandler in order to implement its own redirects features, and that's impossible to do after the HttpClient has been created. So not a bug, but you're right that it could be documented better.

DanielHabenicht commented 4 months ago

Alright, maybe apart from the documentation side, one could throw an error or warn if it is unsupported? (or sort it out via custom typing, so that these Extension Methods are not callable if configured with a custom HttpClient)

Another thing to note is, that this breaks functionality for a use case that needs to ignore SSL Certificates, but also needs the Redirect Features.

Should I make a PR to the https://github.com/tmenier/flurl-site/ Repository with remarks on incompatibility?

tmenier commented 4 months ago

one could throw an error or warn if it is unsupported?

Here's the problem: If you provide your own HttpClient whose inner message handler has AllowAutoRedirect set to false, then Flurl's redirect features will work fine and there's no reason to throw or warn. But Flurl has no way to detect that - the handler isn't exposed via any property of HttpClient.

This gotcha is the main reason that 4.0 provides new hooks to configure the inner handler, which is preferred over providing your own client whenever possible. I think it would be preferable in your case.

Feel free to submit a PR for the docs if you want. The link above (under "a few words of caution") warns against tampering with AllowAutoRedirect and UseCookies, but not specific to the scenario of providing your own HttpClient.

tmenier commented 3 months ago

Closing, but hopefully my answer made sense. In short, Flurl can't know when to throw or warn because it can't see the internals of your provided HttpClient.