Rewrite Icon fetching by MGibson1 · Pull Request #3023 · bitwarden/server

MGibson1 · 2023-06-16T02:18:45Z

Type of change

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

Objective

Rewrites icon fetching to be more object oriented.

Code changes

DomainIcons: Object controlling all icons retrieved for a given domain. Handles creation of potential uri locations for a given domain
IconHttpRequest: Simplified HttpRequest class for Icon fetching. Handles limited redirect following and ensures use of validated IP addresses only.
IconHttpResponse: HttpResponseMessage wrapper. Parses responses for Icon links, and retrieves them, collating into a list of Icons from a response.
IconLink: Known link to an Icon. Retrives content and parses it into an Icon
IconUri: Handles Icon validation and swap to IP
Startup: Swap over to using HttpClientFactory with the same settings as the old static HttpClient
IPAddressExtension & UriExtension: Useful validation and manipulation extensions
Tests: Not much testing here, but I did update to match the new DI requirements and added a test that still uses /favicon.ico as the resolved icon location.

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
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

bw-ephemeral-env-bot · 2023-06-16T02:18:47Z

Ephemeral Environment

Server Branch/Tag	Web Branch/Tag	Status
`platform-pm-2122-use-validated-ip-in-icon-retrieval`	`latest`	`-`
`-`	`-`	`-`

Instructions

Details

Creating a new environment

Edit this comment and change the server and web branches/tags to your desired environment.
Add the ee label to this PR.
The bot will begin to create a new environment.

Updating an environment

As you push new commits, the bot will automatically update your environment.

Deletion of an environment

NOTE: Environments will be automatically deleted on merge/close of a PR.

Remove the ee label from the PR.
The bot will destroy the environment.

Additional information can be found here.

justindbaur

Thoughts on passing through the CancellationToken from HttpContext.RequestAborted to all the async calls, this way if the connection gets closed we can stop doing all this work.

With something that has no database side effects I'd normally really want to do that but I can also see an argument against it because we can let the request continue and let it make it into our cache so that we can return the cached version the next time.

withinfocus

Great suggestions from Justin. I see a couple ⛏️ s beyond that but will skip calling them out until this settles.

withinfocus · 2023-06-16T16:35:52Z

+        _httpClientFactory = httpClientFactory;
+    }
+
+    public async Task<IEnumerable<Icon>> RetrieveIconsAsync(Uri requestUri, string domain)


💭 Does it make sense to just have this return one? Is there a use case I'm not seeing for providing all of what's found?

hmm this is pretty interesting. We used to prioritize icon display after pulling, but now that prioritization occurs prior to navigating to a link. That does mean that we could pull only the highest priority and cut down on network requests.

However, if the first link fails, we'd have to go on down the line iteratively. Currently, we just fire off all 10 requests simultaneously and wait for them all to get back to us. Effectively we're discussing changing this from an eager load to a lazy one.

I don't have the data to say which would be better, presumably most links work, but those that don't would suffer in load times.

bitwarden-bot · 2023-07-06T16:26:43Z

Checkmarx One – Scan Summary & Details – 8639c76c-8843-45f7-8cb4-fb88ca1ea26c

No New Or Fixed Issues Found

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

…e-validated-ip-in-icon-retrieval

MGibson1 · 2023-07-11T15:30:08Z

Looking in to test failures. I swear they weren't happening locally a half hour ago... 😵

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`

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

withinfocus

Ton of changes here, but I think it's making sense to me and has a lot of test coverage. Few questions.

withinfocus · 2023-07-11T18:12:30Z

 public class IconFetchingServiceTests : ServiceTestBase<IconFetchingService>
 {
    [Theory]
+    [InlineData("www.twitter.com")] // https site


❓ Anything special with how this test was handled? Seems the same as the line below.

From when I stepped through the retrieval it's the same as google, we have just been getting reports that twitter in particular is having problems.

Because this work is so pervasive I have no clue if this test passing represents a functionality change or not, but I wanted this as a canary. If it proves to be flaky on twitter's end I want to remove.

justindbaur

Great stuff!

…retrieval

…e-validated-ip-in-icon-retrieval

withinfocus · 2023-08-07T18:44:52Z

    private const string _jpegMediaType = "image/jpeg";
    private static readonly byte[] _jpegHeader = new byte[] { 255, 216, 255 };

+    private const string _svgXmlMediaType = "image/svg+xml";


💭 Does there need to be a code expansion below to check the header? I see this done for the other types.

I don't think we need to, those are only for formats that we try to infer if we don't get a Media-Type header back or we don't support the media type we got back. I also don't think there are generally "marker" bytes for svg.

Throwing in an additional test theory with a site that uses an SVG icon would be a nice and quick addition.

there doesn't need to be mime type detection on the file level. That's just a fallback if the mimetype doesn't exist.

I don't believe svg+xml has a mime type file header (I couldn't find one) so it's not added.

…retrieval

* 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 (cherry picked from commit 4377c7a)

MGibson1 added 3 commits June 16, 2023 08:24

Rewrite Icon fetching

57b5e63

Move validation to IconUri, Uri, or UriBuilder

7f158ab

dotnet format 🤖

817c3d9

MGibson1 force-pushed the platform/pm-2122/use-validated-ip-in-icon-retrieval branch from ef80a5f to 817c3d9 Compare June 16, 2023 12:29

justindbaur requested changes Jun 16, 2023

View reviewed changes

withinfocus reviewed Jun 16, 2023

View reviewed changes

MGibson1 added 2 commits June 16, 2023 15:59

PR suggestions

78b211e

Add not null compiler hint

41de3fa

MGibson1 added 9 commits July 10, 2023 13:28

Add twitter to test case

f5d93b7

Move Uri manipulation to UriService

d40e8a8

Implement MockedHttpClient

3727a5a

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

Add redirect handling tests

0752ff4

Add testing to models

4258da7

More aggressively dispose content in icon link

759b020

Format 🤖

8446d2e

Merge remote-tracking branch 'origin/master' into platform/pm-2122/us…

0654eb9

…e-validated-ip-in-icon-retrieval

Update icon lockfile

0365355

MGibson1 requested review from justindbaur and withinfocus July 11, 2023 15:01

MGibson1 added 2 commits July 11, 2023 12:23

Convert to cloned stream for HttpResponseBuilder

d36183e

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

0ae19fd

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

MGibson1 added the needs-qa label Jul 11, 2023

Remove unused using

c597856

MGibson1 marked this pull request as ready for review July 11, 2023 17:06

withinfocus reviewed Jul 11, 2023

View reviewed changes

Prefer awaiting asyncs for better exception handling

ec68e19

MGibson1 requested a review from withinfocus July 11, 2023 21:41

Prefer injecting interface services to implementations

53f9306

MGibson1 dismissed withinfocus’s stale review via 53f9306 July 17, 2023 14:32

MGibson1 requested a review from justindbaur July 17, 2023 14:54

justindbaur previously approved these changes Jul 17, 2023

View reviewed changes

Comment thread src/Icons/Models/IconLink.cs Outdated

Prefer comparer set in HashSet initialization

078402e

MGibson1 dismissed justindbaur’s stale review via 078402e July 17, 2023 16:44

MGibson1 enabled auto-merge (squash) July 17, 2023 16:46

justindbaur previously approved these changes Jul 17, 2023

View reviewed changes

justindbaur disabled auto-merge July 18, 2023 14:22

justindbaur enabled auto-merge (squash) July 18, 2023 14:22

MGibson1 added 5 commits July 27, 2023 08:45

Merge branch 'master' into platform/pm-2122/use-validated-ip-in-icon-…

7a03ca0

…retrieval

Merge branch 'master' into platform/pm-2122/use-validated-ip-in-icon-…

2f409d0

…retrieval

Merge remote-tracking branch 'origin/master' into platform/pm-2122/us…

7cd263b

…e-validated-ip-in-icon-retrieval

Allow SVG icons

d5ed549

Filter out icons with unknown formats

18091d9

MGibson1 dismissed justindbaur’s stale review via 18091d9 August 7, 2023 18:38

MGibson1 requested a review from justindbaur August 7, 2023 18:39

withinfocus reviewed Aug 7, 2023

View reviewed changes

Seek to beginning of MemoryStream after writing it

21781f8

justindbaur previously approved these changes Aug 7, 2023

View reviewed changes

More appropriate to not return icon if it's invalid

5fa8867

MGibson1 dismissed justindbaur’s stale review via 5fa8867 August 8, 2023 15:57

Add svg icon test

ba197ce

withinfocus approved these changes Aug 8, 2023

View reviewed changes

Merge branch 'master' into platform/pm-2122/use-validated-ip-in-icon-…

5e172fc

…retrieval

MGibson1 removed the needs-qa label Aug 8, 2023

justindbaur merged commit 4377c7a into master Aug 8, 2023

justindbaur deleted the platform/pm-2122/use-validated-ip-in-icon-retrieval branch August 8, 2023 19:29

m4r1k mentioned this pull request Jun 26, 2026

Icons service drops non-default ports, so self-hosted sites on custom ports get the wrong favicon #7873

Open

Uh oh!

Conversation

MGibson1 commented Jun 16, 2023

Type of change

Objective

Code changes

Before you submit

Uh oh!

bw-ephemeral-env-bot Bot commented Jun 16, 2023

Ephemeral Environment

Instructions

Creating a new environment

Updating an environment

Deletion of an environment

Uh oh!

justindbaur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

withinfocus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitwarden-bot commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No New Or Fixed Issues Found

Uh oh!

MGibson1 commented Jul 11, 2023

Uh oh!

withinfocus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justindbaur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bitwarden-bot commented Jul 6, 2023 •

edited

Loading