Rewrite Icon fetching (#3023)

* Rewrite Icon fetching

* Move validation to IconUri, Uri, or UriBuilder

* `dotnet format` 🤖

* PR suggestions

* Add not null compiler hint

* Add twitter to test case

* Move Uri manipulation to UriService

* Implement MockedHttpClient

Presents better, fluent handling of message matching and response
building.

* Add redirect handling tests

* Add testing to models

* More aggressively dispose content in icon link

* Format 🤖

* Update icon lockfile

* Convert to cloned stream for HttpResponseBuilder

Content was being disposed when HttResponseMessage was being disposed.
This avoids losing our reference to our content and allows multiple
usages of the same `MockedHttpMessageResponse`

* Move services to extension

Extension is shared by testing and allows access to services from
our service tests

* Remove unused `using`

* Prefer awaiting asyncs for better exception handling

* `dotnet format` 🤖

* Await async

* Update tests to use test TLD and ip ranges

* Remove unused interfaces

* Make assignments static when possible

* Prefer invariant comparer to downcasing

* Prefer injecting interface services to implementations

* Prefer comparer set in HashSet initialization

* Allow SVG icons

* Filter out icons with unknown formats

* Seek to beginning of MemoryStream after writing it

* More appropriate to not return icon if it's invalid

* Add svg icon test

test/Common/Helpers/HtmlBuilder.cs

@@ -0,0 +1,59 @@
+using System.Text;
+
+namespace Bit.Test.Common.Helpers;
+
+public class HtmlBuilder
+{
+    private string _topLevelNode;
+    private readonly StringBuilder _builder = new();
+
+    public HtmlBuilder(string topLevelNode = "html")
+    {
+        _topLevelNode = CoerceTopLevelNode(topLevelNode);
+    }
+
+    public HtmlBuilder Append(string node)
+    {
+        _builder.Append(node);
+        return this;
+    }
+
+    public HtmlBuilder Append(HtmlBuilder builder)
+    {
+        _builder.Append(builder.ToString());
+        return this;
+    }
+
+    public HtmlBuilder WithAttribute(string name, string value)
+    {
+        _topLevelNode = $"{_topLevelNode} {name}=\"{value}\"";
+        return this;
+    }
+
+    public override string ToString()
+    {
+        _builder.Insert(0, $"<{_topLevelNode}>");
+        _builder.Append($"</{_topLevelNode}>");
+        return _builder.ToString();
+    }
+
+    private static string CoerceTopLevelNode(string topLevelNode)
+    {
+        var result = topLevelNode;
+        if (topLevelNode.StartsWith("<"))
+        {
+            result = topLevelNode[1..];
+        }
+        if (topLevelNode.EndsWith(">"))
+        {
+            result = result[..^1];
+        }
+
+        if (topLevelNode.IndexOf(">") != -1)
+        {
+            throw new ArgumentException("Top level nodes cannot contain '>' characters.");
+        }
+
+        return result;
+    }
+}

test/Common/MockedHttpClient/HttpRequestMatcher.cs

@@ -0,0 +1,104 @@
+#nullable enable
+
+using System.Net;
+
+namespace Bit.Test.Common.MockedHttpClient;
+
+public class HttpRequestMatcher : IHttpRequestMatcher
+{
+    private readonly Func<HttpRequestMessage, bool> _matcher;
+    private HttpRequestMatcher? _childMatcher;
+    private MockedHttpResponse _mockedResponse = new(HttpStatusCode.OK);
+    private bool _responseSpecified = false;
+
+    public int NumberOfMatches { get; private set; }
+
+    /// <summary>
+    /// Returns whether or not the provided request can be handled by this matcher chain.
+    /// </summary>
+    /// <param name="request"></param>
+    /// <returns></returns>
+    public bool Matches(HttpRequestMessage request) => _matcher(request) && (_childMatcher == null || _childMatcher.Matches(request));
+
+    public HttpRequestMatcher(HttpMethod method)
+    {
+        _matcher = request => request.Method == method;
+    }
+
+    public HttpRequestMatcher(string uri)
+    {
+        _matcher = request => request.RequestUri == new Uri(uri);
+    }
+
+    public HttpRequestMatcher(Uri uri)
+    {
+        _matcher = request => request.RequestUri == uri;
+    }
+
+    public HttpRequestMatcher(HttpMethod method, string uri)
+    {
+        _matcher = request => request.Method == method && request.RequestUri == new Uri(uri);
+    }
+
+    public HttpRequestMatcher(Func<HttpRequestMessage, bool> matcher)
+    {
+        _matcher = matcher;
+    }
+
+    public HttpRequestMatcher WithHeader(string name, string value)
+    {
+        return AddChild(request => request.Headers.TryGetValues(name, out var values) && values.Contains(value));
+    }
+
+    public HttpRequestMatcher WithQueryParameters(Dictionary<string, string> requiredQueryParameters) =>
+        WithQueryParameters(requiredQueryParameters.Select(x => $"{x.Key}={x.Value}").ToArray());
+    public HttpRequestMatcher WithQueryParameters(string name, string value) =>
+        WithQueryParameters($"{name}={value}");
+    public HttpRequestMatcher WithQueryParameters(params string[] queryKeyValues)
+    {
+        bool matcher(HttpRequestMessage request)
+        {
+            var query = request.RequestUri?.Query;
+            if (query == null)
+            {
+                return false;
+            }
+
+            return queryKeyValues.All(queryKeyValue => query.Contains(queryKeyValue));
+        }
+        return AddChild(matcher);
+    }
+
+    /// <summary>
+    /// Configure how this matcher should respond to matching HttpRequestMessages.
+    /// Note, after specifying a response, you can no longer further specify match criteria.
+    /// </summary>
+    /// <param name="statusCode"></param>
+    /// <returns></returns>
+    public MockedHttpResponse RespondWith(HttpStatusCode statusCode)
+    {
+        _responseSpecified = true;
+        _mockedResponse = new MockedHttpResponse(statusCode);
+        return _mockedResponse;
+    }
+
+    /// <summary>
+    /// Called to produce an HttpResponseMessage for the given request. This is probably something you want to leave alone
+    /// </summary>
+    /// <param name="request"></param>
+    public async Task<HttpResponseMessage> RespondToAsync(HttpRequestMessage request)
+    {
+        NumberOfMatches++;
+        return await (_childMatcher == null ? _mockedResponse.RespondToAsync(request) : _childMatcher.RespondToAsync(request));
+    }
+
+    private HttpRequestMatcher AddChild(Func<HttpRequestMessage, bool> matcher)
+    {
+        if (_responseSpecified)
+        {
+            throw new Exception("Cannot continue to configure a matcher after a response has been specified");
+        }
+        _childMatcher = new HttpRequestMatcher(matcher);
+        return _childMatcher;
+    }
+}

test/Common/MockedHttpClient/HttpResponseBuilder.cs

@@ -0,0 +1,84 @@
+using System.Net;
+
+namespace Bit.Test.Common.MockedHttpClient;
+
+public class HttpResponseBuilder : IDisposable
+{
+    private bool _disposedValue;
+
+    public HttpStatusCode StatusCode { get; set; }
+    public IEnumerable<KeyValuePair<string, string>> Headers { get; set; } = new List<KeyValuePair<string, string>>();
+    public IEnumerable<string> HeadersToRemove { get; set; } = new List<string>();
+    public HttpContent Content { get; set; }
+
+    public async Task<HttpResponseMessage> ToHttpResponseAsync()
+    {
+        var copiedContentStream = new MemoryStream();
+        await Content.CopyToAsync(copiedContentStream); // This is important, otherwise the content stream will be disposed when the response is disposed.
+        copiedContentStream.Seek(0, SeekOrigin.Begin);
+        var message = new HttpResponseMessage(StatusCode)
+        {
+            Content = new StreamContent(copiedContentStream),
+        };
+
+        foreach (var header in Headers)
+        {
+            message.Headers.TryAddWithoutValidation(header.Key, header.Value);
+        }
+
+        return message;
+    }
+
+    public HttpResponseBuilder WithStatusCode(HttpStatusCode statusCode)
+    {
+        return new()
+        {
+            StatusCode = statusCode,
+            Headers = Headers,
+            HeadersToRemove = HeadersToRemove,
+            Content = Content,
+        };
+    }
+
+    public HttpResponseBuilder WithHeader(string name, string value)
+    {
+        return new()
+        {
+            StatusCode = StatusCode,
+            Headers = Headers.Append(new KeyValuePair<string, string>(name, value)),
+            HeadersToRemove = HeadersToRemove,
+            Content = Content,
+        };
+    }
+
+    public HttpResponseBuilder WithContent(HttpContent content)
+    {
+        return new()
+        {
+            StatusCode = StatusCode,
+            Headers = Headers,
+            HeadersToRemove = HeadersToRemove,
+            Content = content,
+        };
+    }
+
+    protected virtual void Dispose(bool disposing)
+    {
+        if (!_disposedValue)
+        {
+            if (disposing)
+            {
+                Content?.Dispose();
+            }
+
+            _disposedValue = true;
+        }
+    }
+
+    public void Dispose()
+    {
+        // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
+        Dispose(disposing: true);
+        GC.SuppressFinalize(this);
+    }
+}

test/Common/MockedHttpClient/IHttpRequestMatcher.cs

@@ -0,0 +1,10 @@
+#nullable enable
+
+namespace Bit.Test.Common.MockedHttpClient;
+
+public interface IHttpRequestMatcher
+{
+    int NumberOfMatches { get; }
+    bool Matches(HttpRequestMessage request);
+    Task<HttpResponseMessage> RespondToAsync(HttpRequestMessage request);
+}

test/Common/MockedHttpClient/IMockedHttpResponse.cs

@@ -0,0 +1,7 @@
+namespace Bit.Test.Common.MockedHttpClient;
+
+public interface IMockedHttpResponse
+{
+    int NumberOfResponses { get; }
+    Task<HttpResponseMessage> RespondToAsync(HttpRequestMessage request);
+}

test/Common/MockedHttpClient/MockedHttpMessageHandler.cs

@@ -0,0 +1,113 @@
+#nullable enable
+
+using System.Net;
+
+namespace Bit.Test.Common.MockedHttpClient;
+
+public class MockedHttpMessageHandler : HttpMessageHandler
+{
+    private readonly List<IHttpRequestMatcher> _matchers = new();
+
+    /// <summary>
+    /// The fallback handler to use when the request does not match any of the provided matchers.
+    /// </summary>
+    /// <returns>A Matcher that responds with 404 Not Found</returns>
+    public MockedHttpResponse Fallback { get; set; } = new(HttpStatusCode.NotFound);
+
+    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
+    {
+        var matcher = _matchers.FirstOrDefault(x => x.Matches(request));
+        if (matcher == null)
+        {
+            return await Fallback.RespondToAsync(request);
+        }
+
+        return await matcher.RespondToAsync(request);
+    }
+
+    /// <summary>
+    /// Instantiates a new HttpRequestMessage matcher that will handle requests in fitting with the returned matcher. Configuration can be chained.
+    /// </summary>
+    /// <param name="requestMatcher"></param>
+    /// <typeparam name="T"></typeparam>
+    /// <returns></returns>
+    public T When<T>(T requestMatcher) where T : IHttpRequestMatcher
+    {
+        _matchers.Add(requestMatcher);
+        return requestMatcher;
+    }
+
+    /// <summary>
+    /// Instantiates a new HttpRequestMessage matcher that will handle requests in fitting with the returned matcher. Configuration can be chained.
+    /// </summary>
+    /// <param name="requestMatcher"></param>
+    /// <typeparam name="T"></typeparam>
+    /// <returns></returns>
+    public HttpRequestMatcher When(string uri)
+    {
+        var matcher = new HttpRequestMatcher(uri);
+        _matchers.Add(matcher);
+        return matcher;
+    }
+
+    /// <summary>
+    /// Instantiates a new HttpRequestMessage matcher that will handle requests in fitting with the returned matcher. Configuration can be chained.
+    /// </summary>
+    /// <param name="requestMatcher"></param>
+    /// <typeparam name="T"></typeparam>
+    /// <returns></returns>
+    public HttpRequestMatcher When(Uri uri)
+    {
+        var matcher = new HttpRequestMatcher(uri);
+        _matchers.Add(matcher);
+        return matcher;
+    }
+
+    /// <summary>
+    /// Instantiates a new HttpRequestMessage matcher that will handle requests in fitting with the returned matcher. Configuration can be chained.
+    /// </summary>
+    /// <param name="requestMatcher"></param>
+    /// <typeparam name="T"></typeparam>
+    /// <returns></returns>
+    public HttpRequestMatcher When(HttpMethod method)
+    {
+        var matcher = new HttpRequestMatcher(method);
+        _matchers.Add(matcher);
+        return matcher;
+    }
+
+    /// <summary>
+    /// Instantiates a new HttpRequestMessage matcher that will handle requests in fitting with the returned matcher. Configuration can be chained.
+    /// </summary>
+    /// <param name="requestMatcher"></param>
+    /// <typeparam name="T"></typeparam>
+    /// <returns></returns>
+    public HttpRequestMatcher When(HttpMethod method, string uri)
+    {
+        var matcher = new HttpRequestMatcher(method, uri);
+        _matchers.Add(matcher);
+        return matcher;
+    }
+
+    /// <summary>
+    /// Instantiates a new HttpRequestMessage matcher that will handle requests in fitting with the returned matcher. Configuration can be chained.
+    /// </summary>
+    /// <param name="requestMatcher"></param>
+    /// <typeparam name="T"></typeparam>
+    /// <returns></returns>
+    public HttpRequestMatcher When(Func<HttpRequestMessage, bool> matcher)
+    {
+        var requestMatcher = new HttpRequestMatcher(matcher);
+        _matchers.Add(requestMatcher);
+        return requestMatcher;
+    }
+
+    /// <summary>
+    /// Converts the MockedHttpMessageHandler to a HttpClient that can be used in your tests after setup.
+    /// </summary>
+    /// <returns></returns>
+    public HttpClient ToHttpClient()
+    {
+        return new HttpClient(this);
+    }
+}

test/Common/MockedHttpClient/MockedHttpResponse.cs

@@ -0,0 +1,68 @@
+using System.Net;
+using System.Net.Http.Headers;
+using System.Text;
+
+namespace Bit.Test.Common.MockedHttpClient;
+
+public class MockedHttpResponse : IMockedHttpResponse
+{
+    private MockedHttpResponse _childResponse;
+    private readonly Func<HttpRequestMessage, HttpResponseBuilder, HttpResponseBuilder> _responder;
+
+    public int NumberOfResponses { get; private set; }
+
+    public MockedHttpResponse(HttpStatusCode statusCode)
+    {
+        _responder = (_, builder) => builder.WithStatusCode(statusCode);
+    }
+
+    private MockedHttpResponse(Func<HttpRequestMessage, HttpResponseBuilder, HttpResponseBuilder> responder)
+    {
+        _responder = responder;
+    }
+
+    public MockedHttpResponse WithStatusCode(HttpStatusCode statusCode)
+    {
+        return AddChild((_, builder) => builder.WithStatusCode(statusCode));
+    }
+
+    public MockedHttpResponse WithHeader(string name, string value)
+    {
+        return AddChild((_, builder) => builder.WithHeader(name, value));
+    }
+    public MockedHttpResponse WithHeaders(params KeyValuePair<string, string>[] headers)
+    {
+        return AddChild((_, builder) => headers.Aggregate(builder, (b, header) => b.WithHeader(header.Key, header.Value)));
+    }
+
+    public MockedHttpResponse WithContent(string mediaType, string content)
+    {
+        return WithContent(new StringContent(content, Encoding.UTF8, mediaType));
+    }
+    public MockedHttpResponse WithContent(string mediaType, byte[] content)
+    {
+        return WithContent(new ByteArrayContent(content) { Headers = { ContentType = new MediaTypeHeaderValue(mediaType) } });
+    }
+    public MockedHttpResponse WithContent(HttpContent content)
+    {
+        return AddChild((_, builder) => builder.WithContent(content));
+    }
+
+    public async Task<HttpResponseMessage> RespondToAsync(HttpRequestMessage request)
+    {
+        return await RespondToAsync(request, new HttpResponseBuilder());
+    }
+
+    private async Task<HttpResponseMessage> RespondToAsync(HttpRequestMessage request, HttpResponseBuilder currentBuilder)
+    {
+        NumberOfResponses++;
+        var nextBuilder = _responder(request, currentBuilder);
+        return await (_childResponse == null ? nextBuilder.ToHttpResponseAsync() : _childResponse.RespondToAsync(request, nextBuilder));
+    }
+
+    private MockedHttpResponse AddChild(Func<HttpRequestMessage, HttpResponseBuilder, HttpResponseBuilder> responder)
+    {
+        _childResponse = new MockedHttpResponse(responder);
+        return _childResponse;
+    }
+}

test/Icons.Test/Icons.Test.csproj

@@ -20,6 +20,7 @@
 
   <ItemGroup>
     <ProjectReference Include="..\..\src\Icons\Icons.csproj" />
+    <ProjectReference Include="..\Common\Common.csproj" />
   </ItemGroup>
 
 </Project>

test/Icons.Test/Models/IconHttpRequestTests.cs

@@ -0,0 +1,38 @@
+using System.Net;
+using Bit.Icons.Models;
+using Bit.Icons.Services;
+using Bit.Test.Common.MockedHttpClient;
+using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.Net.Http.Headers;
+using NSubstitute;
+using Xunit;
+
+namespace Bit.Icons.Test.Models;
+
+public class IconHttpRequestTests
+{
+    [Fact]
+    public async Task FetchAsync_FollowsTwoRedirectsAsync()
+    {
+        var handler = new MockedHttpMessageHandler();
+
+        var request = handler
+            .Fallback
+            .WithStatusCode(HttpStatusCode.Redirect)
+            .WithContent("text/html", "<html><head><title>Redirect 2</title></head><body><a href=\"https://icon.test\">Redirect 3</a></body></html>")
+            .WithHeader(HeaderNames.Location, "https://icon.test");
+
+        var clientFactory = Substitute.For<IHttpClientFactory>();
+        clientFactory.CreateClient("Icons").Returns(handler.ToHttpClient());
+
+        var uriService = Substitute.For<IUriService>();
+        uriService.TryGetUri(Arg.Any<Uri>(), out Arg.Any<IconUri>()).Returns(x =>
+        {
+            x[1] = new IconUri(new Uri("https://icon.test"), IPAddress.Parse("192.0.2.1"));
+            return true;
+        });
+        var result = await IconHttpRequest.FetchAsync(new Uri("https://icon.test"), NullLogger<IIconFetchingService>.Instance, clientFactory, uriService);
+
+        Assert.Equal(3, request.NumberOfResponses); // Initial + 2 redirects
+    }
+}

test/Icons.Test/Models/IconHttpResponseTests.cs

@@ -0,0 +1,101 @@
+using System.Net;
+using AngleSharp.Html.Parser;
+using Bit.Icons.Models;
+using Bit.Icons.Services;
+using Bit.Test.Common.Helpers;
+using Bit.Test.Common.MockedHttpClient;
+using Microsoft.Extensions.Logging.Abstractions;
+using NSubstitute;
+using Xunit;
+
+namespace Bit.Icons.Test.Models;
+
+public class IconHttpResponseTests
+{
+    private readonly IUriService _mockedUriService;
+    private static readonly IHtmlParser _parser = new HtmlParser();
+
+    public IconHttpResponseTests()
+    {
+        _mockedUriService = Substitute.For<IUriService>();
+        _mockedUriService.TryGetUri(Arg.Any<Uri>(), out Arg.Any<IconUri>()).Returns(x =>
+        {
+            x[1] = new IconUri(new Uri("https://icon.test"), IPAddress.Parse("192.0.2.1"));
+            return true;
+        });
+    }
+
+    [Fact]
+    public async Task RetrieveIconsAsync_Processes200LinksAsync()
+    {
+        var htmlBuilder = new HtmlBuilder();
+        var headBuilder = new HtmlBuilder("head");
+        for (var i = 0; i < 200; i++)
+        {
+            headBuilder.Append(UnusableLinkNode());
+        }
+        headBuilder.Append(UsableLinkNode());
+        htmlBuilder.Append(headBuilder);
+        var response = GetHttpResponseMessage(htmlBuilder.ToString());
+        var sut = CurriedIconHttpResponse()(response);
+
+        var result = await sut.RetrieveIconsAsync(new Uri("https://icon.test"), "icon.test", _parser);
+
+        Assert.Empty(result);
+    }
+
+    [Fact]
+    public async Task RetrieveIconsAsync_Processes10IconsAsync()
+    {
+        var htmlBuilder = new HtmlBuilder();
+        var headBuilder = new HtmlBuilder("head");
+        for (var i = 0; i < 11; i++)
+        {
+            headBuilder.Append(UsableLinkNode());
+        }
+        htmlBuilder.Append(headBuilder);
+        var response = GetHttpResponseMessage(htmlBuilder.ToString());
+        var sut = CurriedIconHttpResponse()(response);
+
+        var result = await sut.RetrieveIconsAsync(new Uri("https://icon.test"), "icon.test", _parser);
+
+        Assert.Equal(10, result.Count());
+    }
+
+    private static string UsableLinkNode()
+    {
+        return "<link rel=\"icon\" href=\"https://icon.test/favicon.ico\" />";
+    }
+
+    private static string UnusableLinkNode()
+    {
+        // Empty href links are not usable
+        return "<link rel=\"icon\" href=\"\" />";
+    }
+
+    private static HttpResponseMessage GetHttpResponseMessage(string content)
+    {
+        return new HttpResponseMessage(HttpStatusCode.OK)
+        {
+            RequestMessage = new HttpRequestMessage(HttpMethod.Get, "https://icon.test"),
+            Content = new StringContent(content)
+        };
+    }
+
+    private Func<HttpResponseMessage, IconHttpResponse> CurriedIconHttpResponse()
+    {
+        return (HttpResponseMessage response) => new IconHttpResponse(response, NullLogger<IIconFetchingService>.Instance, UsableIconHttpClientFactory(), _mockedUriService);
+    }
+
+    private static IHttpClientFactory UsableIconHttpClientFactory()
+    {
+        var substitute = Substitute.For<IHttpClientFactory>();
+        var handler = new MockedHttpMessageHandler();
+        handler.Fallback
+            .WithStatusCode(HttpStatusCode.OK)
+            .WithContent("image/png", new byte[] { 137, 80, 78, 71 });
+
+        substitute.CreateClient("Icons").Returns(handler.ToHttpClient());
+        return substitute;
+    }
+}

test/Icons.Test/Models/IconLinkTests.cs

@@ -0,0 +1,85 @@
+using System.Net;
+using AngleSharp.Dom;
+using Bit.Icons.Models;
+using Bit.Icons.Services;
+using Microsoft.Extensions.Logging;
+using NSubstitute;
+using Xunit;
+
+namespace Bit.Icons.Test.Models;
+
+public class IconLinkTests
+{
+    private readonly IElement _element;
+    private readonly Uri _uri = new("https://icon.test");
+    private readonly ILogger<IIconFetchingService> _logger = Substitute.For<ILogger<IIconFetchingService>>();
+    private readonly IHttpClientFactory _httpClientFactory;
+    private readonly IUriService _uriService;
+    private readonly string _baseUrlPath = "/";
+
+    public IconLinkTests()
+    {
+        _element = Substitute.For<IElement>();
+        _httpClientFactory = Substitute.For<IHttpClientFactory>();
+        _uriService = Substitute.For<IUriService>();
+        _uriService.TryGetUri(Arg.Any<Uri>(), out Arg.Any<IconUri>()).Returns(x =>
+        {
+            x[1] = new IconUri(new Uri("https://icon.test"), IPAddress.Parse("192.0.2.1"));
+            return true;
+        });
+    }
+
+    [Fact]
+    public void WithNoHref_IsNotUsable()
+    {
+        _element.GetAttribute("href").Returns(string.Empty);
+
+        var result = new IconLink(_element, _uri, _baseUrlPath).IsUsable();
+
+        Assert.False(result);
+    }
+
+    [Theory]
+    [InlineData(null, false)]
+    [InlineData("", false)]
+    [InlineData(" ", false)]
+    [InlineData("unusable", false)]
+    [InlineData("ico", true)]
+    public void WithNoRel_IsUsable(string extension, bool expectedResult)
+    {
+        SetAttributeValue("href", $"/favicon.{extension}");
+
+        var result = new IconLink(_element, _uri, _baseUrlPath).IsUsable();
+
+        Assert.Equal(expectedResult, result);
+    }
+
+    [Theory]
+    [InlineData("icon", true)]
+    [InlineData("stylesheet", false)]
+    public void WithRel_IsUsable(string rel, bool expectedResult)
+    {
+        SetAttributeValue("href", "/favicon.ico");
+        SetAttributeValue("rel", rel);
+
+        var result = new IconLink(_element, _uri, _baseUrlPath).IsUsable();
+
+        Assert.Equal(expectedResult, result);
+    }
+
+    [Fact]
+    public void FetchAsync_Unvalidated_ReturnsNull()
+    {
+        var result = new IconLink(_element, _uri, _baseUrlPath).FetchAsync(_logger, _httpClientFactory, _uriService);
+
+        Assert.Null(result.Result);
+    }
+
+    private void SetAttributeValue(string attribute, string value)
+    {
+        var attr = Substitute.For<IAttr>();
+        attr.Value.Returns(value);
+
+        _element.Attributes[attribute].Returns(attr);
+    }
+}

test/Icons.Test/Models/IconUriTests.cs

@@ -0,0 +1,22 @@
+using System.Net;
+using Bit.Icons.Models;
+using Xunit;
+
+namespace Bit.Icons.Test.Models;
+
+public class IconUriTests
+{
+    [Theory]
+    [InlineData("https://icon.test", "1.1.1.1", true)]
+    [InlineData("https://icon.test:4443", "1.1.1.1", false)] // Non standard port
+    [InlineData("http://test", "1.1.1.1", false)] // top level domain
+    [InlineData("https://icon.test", "127.0.0.1", false)] // IP is internal
+    [InlineData("https://icon.test", "::1", false)] // IP is internal
+    [InlineData("https://1.1.1.1", "::1", false)] // host is IP
+    public void IsValid(string uri, string ip, bool expectedResult)
+    {
+        var result = new IconUri(new Uri(uri), IPAddress.Parse(ip)).IsValid;
+
+        Assert.Equal(expectedResult, result);
+    }
+}

test/Icons.Test/Services/IconFetchingServiceTests.cs

@@ -1,25 +1,25 @@
 using Bit.Icons.Services;
-using Microsoft.Extensions.DependencyInjection;
-using Microsoft.Extensions.Logging;
 using Xunit;
 
 namespace Bit.Icons.Test.Services;
 
-public class IconFetchingServiceTests
+public class IconFetchingServiceTests : ServiceTestBase<IconFetchingService>
 {
     [Theory]
+    [InlineData("www.twitter.com")] // https site
     [InlineData("www.google.com")] // https site
     [InlineData("neverssl.com")] // http site
-    [InlineData("ameritrade.com")]
+    [InlineData("neopets.com")] // uses favicon.ico
+    [InlineData("hopin.com")] // uses svg+xml format
+    [InlineData("ameritrade.com")] // redirects to tdameritrace.com
     [InlineData("icloud.com")]
     [InlineData("bofa.com", Skip = "Broken in pipeline for .NET 6. Tracking link: https://bitwarden.atlassian.net/browse/PS-982")]
     public async Task GetIconAsync_Success(string domain)
     {
-        var sut = new IconFetchingService(GetLogger());
+        var sut = BuildSut();
         var result = await sut.GetIconAsync(domain);
 
         Assert.NotNull(result);
-        Assert.NotNull(result.Icon);
     }
 
     [Theory]
@@ -28,23 +28,12 @@ public class IconFetchingServiceTests
     [InlineData("localhost")]
     public async Task GetIconAsync_ReturnsNull(string domain)
     {
-        var sut = new IconFetchingService(GetLogger());
+        var sut = BuildSut();
         var result = await sut.GetIconAsync(domain);
 
         Assert.Null(result);
     }
 
-    private static ILogger<IconFetchingService> GetLogger()
-    {
-        var services = new ServiceCollection();
-        services.AddLogging(b =>
-        {
-            b.ClearProviders();
-            b.AddDebug();
-        });
-
-        var provider = services.BuildServiceProvider();
-
-        return provider.GetRequiredService<ILogger<IconFetchingService>>();
-    }
+    private IconFetchingService BuildSut() =>
+        GetService<IconFetchingService>();
 }

test/Icons.Test/Services/ServiceTestBase.cs

@@ -0,0 +1,41 @@
+using Bit.Icons.Extensions;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+
+namespace Bit.Icons.Test.Services;
+
+public class ServiceTestBase
+{
+    internal ServiceCollection _services = new();
+    internal ServiceProvider _provider;
+
+    public ServiceTestBase()
+    {
+        _services = new ServiceCollection();
+        _services.AddLogging(b =>
+        {
+            b.ClearProviders();
+            b.AddDebug();
+        });
+
+        _services.ConfigureHttpClients();
+        _services.AddHtmlParsing();
+        _services.AddServices();
+
+        _provider = _services.BuildServiceProvider();
+    }
+
+    public T GetService<T>() =>
+        _provider.GetRequiredService<T>();
+}
+
+public class ServiceTestBase<TSut> : ServiceTestBase where TSut : class
+{
+    public ServiceTestBase() : base()
+    {
+        _services.AddTransient<TSut>();
+        _provider = _services.BuildServiceProvider();
+    }
+
+    public TSut Sut => GetService<TSut>();
+}

test/Icons.Test/packages.lock.json

@@ -73,6 +73,33 @@
           "StackExchange.Redis": "2.5.43"
         }
       },
+      "AutoFixture": {
+        "type": "Transitive",
+        "resolved": "4.17.0",
+        "contentHash": "efMRCG3Epc4QDELwdmQGf6/caQUleRXPRCnLAq5gLMpTuOTcOQWV12vEJ8qo678Rj97/TjjxHYu/34rGkXdVAA==",
+        "dependencies": {
+          "Fare": "[2.1.1, 3.0.0)",
+          "System.ComponentModel.Annotations": "4.3.0"
+        }
+      },
+      "AutoFixture.AutoNSubstitute": {
+        "type": "Transitive",
+        "resolved": "4.17.0",
+        "contentHash": "iWsRiDQ7T8s6F4mvYbSvPTq0GDtxJD6D+E1Fu9gVbHUvJiNikC1yIDNTH+3tQF7RK864HH/3R8ETj9m2X8UXvg==",
+        "dependencies": {
+          "AutoFixture": "4.17.0",
+          "NSubstitute": "[2.0.3, 5.0.0)"
+        }
+      },
+      "AutoFixture.Xunit2": {
+        "type": "Transitive",
+        "resolved": "4.17.0",
+        "contentHash": "lrURL/LhJLPkn2tSPUEW8Wscr5LoV2Mr8A+ikn5gwkofex3o7qWUsBswlLw+KCA7EOpeqwZOldp3k91zDF+48Q==",
+        "dependencies": {
+          "AutoFixture": "4.17.0",
+          "xunit.extensibility.core": "[2.2.0, 3.0.0)"
+        }
+      },
       "AutoMapper": {
         "type": "Transitive",
         "resolved": "12.0.1",
@@ -246,6 +273,14 @@
           "Microsoft.Win32.Registry": "5.0.0"
         }
       },
+      "Fare": {
+        "type": "Transitive",
+        "resolved": "2.1.1",
+        "contentHash": "HaI8puqA66YU7/9cK4Sgbs1taUTP1Ssa4QT2PIzqJ7GvAbN1QgkjbRsjH+FSbMh1MJdvS0CIwQNLtFT+KF6KpA==",
+        "dependencies": {
+          "NETStandard.Library": "1.6.1"
+        }
+      },
       "Fido2": {
         "type": "Transitive",
         "resolved": "3.0.1",
@@ -326,6 +361,15 @@
           "IdentityModel": "4.4.0"
         }
       },
+      "Kralizek.AutoFixture.Extensions.MockHttp": {
+        "type": "Transitive",
+        "resolved": "1.2.0",
+        "contentHash": "6zmks7/5mVczazv910N7V2EdiU6B+rY61lwdgVO0o2iZuTI6KI3T+Hgkrjv0eGOKYucq2OMC+gnAc5Ej2ajoTQ==",
+        "dependencies": {
+          "AutoFixture": "4.11.0",
+          "RichardSzalay.MockHttp": "6.0.0"
+        }
+      },
       "LaunchDarkly.Cache": {
         "type": "Transitive",
         "resolved": "1.0.2",
@@ -1148,6 +1192,11 @@
           "System.Diagnostics.DiagnosticSource": "4.7.1"
         }
       },
+      "RichardSzalay.MockHttp": {
+        "type": "Transitive",
+        "resolved": "6.0.0",
+        "contentHash": "bStGNqIX/MGYtML7K3EzdsE/k5HGVAcg7XgN23TQXGXqxNC9fvYFR94fA0sGM5hAT36R+BBGet6ZDQxXL/IPxg=="
+      },
       "runtime.debian.8-x64.runtime.native.System.Security.Cryptography.OpenSsl": {
         "type": "Transitive",
         "resolved": "4.3.2",
@@ -1568,6 +1617,24 @@
           "System.Runtime": "4.3.0"
         }
       },
+      "System.ComponentModel.Annotations": {
+        "type": "Transitive",
+        "resolved": "4.3.0",
+        "contentHash": "SY2RLItHt43rd8J9D8M8e8NM4m+9WLN2uUd9G0n1I4hj/7w+v3pzK6ZBjexlG1/2xvLKQsqir3UGVSyBTXMLWA==",
+        "dependencies": {
+          "System.Collections": "4.3.0",
+          "System.ComponentModel": "4.3.0",
+          "System.Globalization": "4.3.0",
+          "System.Linq": "4.3.0",
+          "System.Reflection": "4.3.0",
+          "System.Reflection.Extensions": "4.3.0",
+          "System.Resources.ResourceManager": "4.3.0",
+          "System.Runtime": "4.3.0",
+          "System.Runtime.Extensions": "4.3.0",
+          "System.Text.RegularExpressions": "4.3.0",
+          "System.Threading": "4.3.0"
+        }
+      },
       "System.ComponentModel.Primitives": {
         "type": "Transitive",
         "resolved": "4.3.0",
@@ -2770,6 +2837,18 @@
           "NETStandard.Library": "1.6.1"
         }
       },
+      "common": {
+        "type": "Project",
+        "dependencies": {
+          "AutoFixture.AutoNSubstitute": "[4.17.0, )",
+          "AutoFixture.Xunit2": "[4.17.0, )",
+          "Core": "[2023.5.1, )",
+          "Kralizek.AutoFixture.Extensions.MockHttp": "[1.2.0, )",
+          "Microsoft.NET.Test.Sdk": "[17.1.0, )",
+          "NSubstitute": "[4.3.0, )",
+          "xunit": "[2.4.1, )"
+        }
+      },
       "core": {
         "type": "Project",
         "dependencies": {
@@ -2850,4 +2929,4 @@
       }
     }
   }
-}
+}