dotnet / roslyn-analyzers

MIT License
1.59k stars 466 forks source link

CA5359: Apply to HttpClientHandler.ServerCertificateCustomValidationCallback #4542

Open kevinoid opened 3 years ago

kevinoid commented 3 years ago

Analyzer

Diagnostic ID: CA5359

Describe the improvement

Apply the same check to HttpClientHandler.ServerCertificateCustomValidationCallback as is currently applied to HttpWebRequest.ServerCertificateValidationCallback.

Additional context

When adding support for HttpClientHandler.ServerCertificateCustomValidationCallback, it would be nice to check for assignments of HttpClientHandler.DangerousAcceptAnyServerCertificateValidator (which was added in dotnet/corefx#19908) in addition to the existing delegate assignment check.

For example, consider the code:

using System.Net;
using System.Net.Http;

public static class NoServerCertValidationTest
{
    public static void WithHandlerDelegate()
    {
        new HttpClientHandler().ServerCertificateCustomValidationCallback =
            (sender, certificate, chain, sslErrors) => true;
    }

    public static void WithHandlerProp()
    {
        new HttpClientHandler().ServerCertificateCustomValidationCallback =
            HttpClientHandler.DangerousAcceptAnyServerCertificateValidator;
    }

    public static void WithRequest()
    {
        var request = (HttpWebRequest)WebRequest.Create("http://example.com/");
        request.ServerCertificateValidationCallback =
            (message, certificate, chain, sslErrors) => true;
    }

    public static void WithServicePointManager()
    {
        ServicePointManager.ServerCertificateValidationCallback =
            (sender, certificate, chain, sslErrors) => true;
    }
}

Currently CA5359 catches WithRequest and WithServicePointManager, but not WithHandlerDelegate nor WithHandlerProp. I'd suggest it should catch all 4.

Thanks for considering, Kevin

dotpaul commented 3 years ago

Thanks for detailed issue, @kevinoid ! Sounds fine to me.