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

[PM-8029] Icon loading fixes and uri parsing improvements #3232

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

1fexd
Copy link

@1fexd 1fexd commented May 9, 2024

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

  • Avoid loading icons when the user has chosen not to display them
  • Remove abitrary TLD restriction for link open / favicon loading check by adding Nager.PublicSuffix
  • Only try to load favicons for valid domains (via publicsuffix)
  • Never attempt to load favicons for .onion / .i2p domains
  • Improve uri parsing util without changing the existing behavior

I've also added some suggestions on what changes to the parsing behavior should be made in my opinion.

Code changes

  • file.ext: Description of what was changed and why

  • src/Core/Core.csproj: Add NuGet package to load publicsuffixlist, add asset

  • src/Core/Pages/Vault/CipherItemViewModel.cs: Don't load favicon if user has disabled favicons

  • src/Core/Utilities/CoreHelpers.cs: Uri parsing improvements with suggested breaking changes

  • src/Core/Utilities/IconImageConverter.cs: Favicon loading fixes and improvements

  • src/Core/Utilities/MauiAssetRuleProvider.cs: Maui asset rule provider

  • src/Core/Utilities/ServiceContainer.cs: Register rule provider

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@1fexd 1fexd requested a review from a team as a code owner May 9, 2024 23:14
@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-8029

@bitwarden-bot bitwarden-bot changed the title Icon loading fixes and uri parsing improvements [PM-8029] Icon loading fixes and uri parsing improvements May 9, 2024
@1fexd
Copy link
Author

1fexd commented May 10, 2024

public class Tests
{

    /// <summary>
    /// Returns the second and top level domain of the given uri.
    /// Does not support plain hostnames without
    ///
    /// Input => Output examples:
    /// <para>https://bitwarden.com => bitwarden.com</para>
    /// <para>https://login.bitwarden.com:1337 => bitwarden.com</para>
    /// <para>https://sub.login.bitwarden.com:1337 => bitwarden.com</para>
    /// <para>https://localhost:8080 => localhost</para>
    /// <para>localhost => null</para>
    /// <para>bitwarden => null</para>
    /// <para>127.0.0.1 => 127.0.0.1</para>
    /// </summary>
    [Test]
    public async Task Test_GetDomain()
    {
        var coreHelpers = await Create();

        Assert.That(coreHelpers.GetDomain("https://bitwarden.com"), Is.EqualTo("bitwarden.com"));
        Assert.That(coreHelpers.GetDomain("https://login.bitwarden.com:1337"), Is.EqualTo("bitwarden.com"));
        Assert.That(coreHelpers.GetDomain("https://sub.login.bitwarden.com:1337"), Is.EqualTo("bitwarden.com"));
        Assert.That(coreHelpers.GetDomain("https://localhost:8080"), Is.EqualTo("localhost"));
        Assert.That(coreHelpers.GetDomain("localhost"), Is.EqualTo(null));
        Assert.That(coreHelpers.GetDomain("bitwarden"), Is.EqualTo(null));
        Assert.That(coreHelpers.GetDomain("127.0.0.1"), Is.EqualTo("127.0.0.1"));
        Assert.That(coreHelpers.GetDomain("linksheet.local"), Is.EqualTo("linksheet.local"));
    }

    /// <summary>
    /// Returns the host and port of the given uri.
    /// Does not support plain hostnames without
    ///
    /// Input => Output examples:
    /// <para>https://bitwarden.com => bitwarden.com</para>
    /// <para>https://login.bitwarden.com:1337 => login.bitwarden.com:1337</para>
    /// <para>https://sub.login.bitwarden.com:1337 => sub.login.bitwarden.com:1337</para>
    /// <para>https://localhost:8080 => localhost:8080</para>
    /// <para>localhost => null</para>
    /// <para>bitwarden => null</para>
    /// <para>127.0.0.1 => 127.0.0.1</para>
    /// </summary>
    [Test]
    public async Task Test_GetHost()
    {
        var coreHelpers = await Create();

        Assert.That(coreHelpers.GetHost("https://bitwarden.com"), Is.EqualTo("bitwarden.com"));
        Assert.That(coreHelpers.GetHost("https://login.bitwarden.com:1337"), Is.EqualTo("login.bitwarden.com:1337"));
        Assert.That(coreHelpers.GetHost("https://sub.login.bitwarden.com:1337"), Is.EqualTo("sub.login.bitwarden.com:1337"));
        Assert.That(coreHelpers.GetHost("https://localhost:8080"), Is.EqualTo("localhost:8080"));
        Assert.That(coreHelpers.GetHost("localhost"), Is.EqualTo(null));
        Assert.That(coreHelpers.GetHost("bitwarden"), Is.EqualTo(null));
        Assert.That(coreHelpers.GetHost("127.0.0.1"), Is.EqualTo("127.0.0.1"));
    }

    /// <summary>
    /// Returns the host (and not port) of the given uri.
    /// Does not support plain hostnames without a protocol.
    ///
    /// Input => Output examples:
    /// <para>https://bitwarden.com => bitwarden.com</para>
    /// <para>https://login.bitwarden.com:1337 => login.bitwarden.com</para>
    /// <para>https://sub.login.bitwarden.com:1337 => sub.login.bitwarden.com</para>
    /// <para>https://localhost:8080 => localhost</para>
    /// <para>localhost => null</para>
    /// <para>bitwarden => null</para>
    /// <para>127.0.0.1 => 127.0.0.1</para>
    /// </summary>
    [Test]
    public async Task Test_GetHostname()
    {
        var coreHelpers = await Create();

        Assert.That(coreHelpers.GetHostname("https://bitwarden.com"), Is.EqualTo("bitwarden.com"));
        Assert.That(coreHelpers.GetHostname("https://login.bitwarden.com:1337"), Is.EqualTo("login.bitwarden.com"));
        Assert.That(coreHelpers.GetHostname("https://sub.login.bitwarden.com:1337"), Is.EqualTo("sub.login.bitwarden.com"));
        Assert.That(coreHelpers.GetHostname("https://localhost:8080"), Is.EqualTo("localhost"));
        Assert.That(coreHelpers.GetHostname("localhost"), Is.EqualTo(null));
        Assert.That(coreHelpers.GetHostname("bitwarden"), Is.EqualTo(null));
        Assert.That(coreHelpers.GetHostname("127.0.0.1"), Is.EqualTo("127.0.0.1"));

        Assert.That(CoreHelpersOld.GetHostname("google.com"), Is.EqualTo("google.com"));
    }


    /// <summary>
    /// No test data specified
    /// </summary>
    [Test]
    public async Task Test_GetUri()
    {
        var coreHelpers = await Create();

        Assert.That(coreHelpers.GetHttpUri("https://bitwarden.com")?.ToString(), Is.EqualTo("https://bitwarden.com/"));
        Assert.That(coreHelpers.GetHttpUri("https://login.bitwarden.com:1337")?.ToString(), Is.EqualTo("https://login.bitwarden.com:1337/"));
        Assert.That(coreHelpers.GetHttpUri("https://sub.login.bitwarden.com:1337")?.ToString(), Is.EqualTo("https://sub.login.bitwarden.com:1337/"));
        Assert.That(coreHelpers.GetHttpUri("https://localhost:8080")?.ToString(), Is.EqualTo("https://localhost:8080/"));
        Assert.That(coreHelpers.GetHttpUri("localhost"), Is.EqualTo(null));
        Assert.That(coreHelpers.GetHttpUri("bitwarden"), Is.EqualTo(null));
        Assert.That(coreHelpers.GetHttpUri("127.0.0.1")?.ToString(), Is.EqualTo("http://127.0.0.1/"));
        Assert.That(coreHelpers.GetHttpUri("."), Is.EqualTo(null));
    }

    public async Task<CoreHelpers> Create()
    {
        var ruleProvider = new LocalFileRuleProvider("path_to/Resources/public_suffix_list.dat");
        await ruleProvider.BuildAsync();

        return new CoreHelpers(new DomainParser(ruleProvider));
    }
}

I used this Unit test class to check if my changes work as intended

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

Successfully merging this pull request may close these issues.

None yet

2 participants