[PS-1123] Improve hostname and domain retrieval #3168

djsmith85 · 2022-07-24T11:27:29Z

Type of change

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

Objective

Finally got around finishing up bitwarden/jslib#547

Improve the retrieval and validation of hostnames and domains, which affects url matching/auto-fill

The main change is replacing tldjs with tldts

Newly supported scenarios:

IPv6
subdomains with an underscore
Fixes a scenario where IPv4 addresses would not be returned as they got prefixed with http:// (Regex failed)
Fully up-to-date public-suffix list instead of relying on a regex only including the most common tld's (Utils.tldEndingRegex)
Support for parsing urls with umlauts
Better detection if a url is a website and if it's launchable from the UI (Removed Utils.tldEndingRegex).

Asana task: https://app.asana.com/0/1153292148278596/1201431275009049/f

Could possibly improve or fix the following issues:

Auto-Fill / Auto-Suggest not working for Basic Auth (Login Modal) #1191
Autofill isn't working for a specific site #1621
Some subdomains are not matched with Base domain detection option #2304
Autocomplete fails for umlaut domains #910 (Got closed due to inactivity but was still an issue),
Closes Default match detection no longer works for IPV6 URIs #2107
Closes URI match detection doesn't work for underscores in URIs #2108
Closes Some TLD's not recognized as in URLs #2811

Code changes

Improving hostname and domain detection/validation

Uninstall tldjs and install tldts as replacement
- package.json:, apps/cli/package.json:
- package-lock.json: , apps/cli/package-lock.json: Re-built using npm i
libs/common/spec/misc/utils.spec.ts: Added unit tests for Utils.getDomain and Utils.getHostname
libs/common/src/misc/utils.ts:
- Replaced tldjs with tldts
- Removed tldEndingRegex
- Added check for about: protocol
- Removed no longer needed IP4 regex
- getHostname no longer uses Utils.getUrl internally but relies on tldts.getHostname
libs/common/src/misc/tldjs.noop.ts: Deleted tldjs.noop implementation
apps/desktop/tsconfig.json: Removed usage of tldjs.noop
apps/web/tsconfig.json: Removed usage of tldjs.noop

isWebsite/launch detection

libs/common/spec/view/loginUriView.spec.ts: Added unit tests for getters (isWebsite, launchUri, canLaunch) of loginUriView
libs/common/src/models/view/loginUriView.ts: Replaced usage of Utils.tldEndingRegex with improved Utils.getDomain

Fix accessibility-cookie

apps/desktop/src/app/accounts/accessibility-cookie.component.ts: With the removal of the tldjs.noop-impl which previously always returned a hostname instead of the domain when calling tldjs.getDomain(), the call to Utils.getDomain needed to get changed to Utils.getHostName().

Before you submit

- [X] I have checked for **linting** errors (`npm run lint`) (required)
- [X] I have added **unit tests** where it makes sense to do so (encouraged but not required)
- [ ] This change requires a **documentation update** (notify the documentation team)
- [ ] This change has particular **deployment requirements** (notify the DevOps team)

…improve-hostname-and-domain-retrieval

MGibson1

Looks like a straight improvement. One question before approving, though

MGibson1 · 2022-08-01T19:49:45Z

libs/common/src/misc/tldjs.noop.ts

@@ -1,7 +0,0 @@
-export function getDomain(host: string): string | null {


Why is this no longer required?

I can't say why this was initially introduced, but with this in place on web and desktop it would actually always return the hostname instead of the domain.

I just tested this with the 2fa reports on web and it actually produces more hits with my changes. As this now grabs the domain and checks it against the list from 2fa.directory. A hostname would not match with an entry.

…prove-hostname-and-domain-retrieval

MGibson1

Looks good. I'm going to tag this with needs-qa, but it seems like an amorphous "please test autofill" to me, do you have any better qa notes than that?

djsmith85 · 2022-09-26T19:48:21Z

@MGibson1 Thanks for the review, I'll add some more detailed notes for QA on the ticket.

…prove-hostname-and-domain-retrieval

djsmith85 · 2022-10-22T07:47:44Z

Received approval from QA, just need a quick review of the 3 last merge commits pulling in master, before merging.

justindbaur

Fantastic, love the tests. I would love to have a test defining the behavior of a file protocol (i.e file:///C:/something/form.pdf) going through getDomain and getHostname I'd imagine it would be null but mostly just want to define our expectation with something like that.

justindbaur · 2022-10-24T15:17:47Z

libs/common/spec/misc/utils.spec.ts

+      expect(Utils.getDomain("https://my_vault.bitwarden.com/")).toBe("bitwarden.com");
+    });
+
+    it("should support urls containing umlauts", () => {


TIL what that is called.

…prove-hostname-and-domain-retrieval

djsmith85 · 2022-10-24T17:19:44Z

Fantastic, love the tests. I would love to have a test defining the behavior of a file protocol (i.e file:///C:/something/form.pdf) going through getDomain and getHostname I'd imagine it would be null but mostly just want to define our expectation with something like that.

Good catch with the file-schema links. I had to add some exceptions to getHostname as they were returning C as host.

djsmith85 added 6 commits July 23, 2022 17:38

Add test cases from previous PR bitwarden/jslib#547

a235eef

Install tldts as replacement for tldjs

4b53aae

Use tldts for hostname and domain retrieval/validation

ba9910f

Remove usage of old tldjs.noop-implementation

f877bc2

Add handling of about protocol

3bfa6fc

Remove usage of tldEndingRegex and use tldts check instead

029cdb0

djsmith85 requested review from eliykat and a team July 24, 2022 11:28

djsmith85 added 2 commits July 25, 2022 13:26

Merge branch 'master' of https://github.com/bitwarden/bitwarden into …

1bfa22e

…improve-hostname-and-domain-retrieval

Uninstall @types/tldjs and tldjs

4d49584

MGibson1 reviewed Aug 1, 2022

View reviewed changes

djsmith85 added 3 commits September 25, 2022 16:25

Merge branch 'master' of https://github.com/bitwarden/clients into im…

5d641f6

…prove-hostname-and-domain-retrieval

Updated package-lock.json

600e76c

Fix accessibility cookie check

0aff3fe

djsmith85 requested a review from MGibson1 September 25, 2022 17:33

MGibson1 previously approved these changes Sep 26, 2022

View reviewed changes

MGibson1 added the needs-qa Marks a PR as requiring QA approval label Sep 26, 2022

eliykat removed their request for review September 28, 2022 00:16

Merge branch 'master' of https://github.com/bitwarden/clients into im…

c45a56f

…prove-hostname-and-domain-retrieval

djsmith85 dismissed MGibson1’s stale review via c45a56f October 4, 2022 22:13

djsmith85 added 3 commits October 19, 2022 13:46

Merge branch 'master' of https://github.com/bitwarden/clients into im…

3b8d03c

…prove-hostname-and-domain-retrieval

Rename loginUriView.spec to login-uri-view.spec

044c771

Merge branch 'master' of https://github.com/bitwarden/clients into im…

29b6866

…prove-hostname-and-domain-retrieval

djsmith85 requested a review from a team October 22, 2022 07:47

djsmith85 removed the needs-qa Marks a PR as requiring QA approval label Oct 22, 2022

justindbaur requested changes Oct 24, 2022

View reviewed changes

djsmith85 added 2 commits October 24, 2022 18:18

Merge branch 'master' of https://github.com/bitwarden/clients into im…

b8d79a5

…prove-hostname-and-domain-retrieval

Add test for getDomain failing file links

c0db466

getHostName - Return null when given, data, about or file links

18af7d7

djsmith85 requested a review from justindbaur October 24, 2022 17:18

justindbaur approved these changes Oct 24, 2022

View reviewed changes

djsmith85 merged commit 8c59eef into master Oct 24, 2022

djsmith85 deleted the improve-hostname-and-domain-retrieval branch October 24, 2022 17:26

prestonvanloon mentioned this pull request Dec 23, 2022

Browser no longer matches top level domains (2022.12.1) #4315

Closed

1 task

dwbit mentioned this pull request Jan 9, 2023

Default match policy bug - Browser Extension can't match local hostnames without explicit local domain #4247

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PS-1123] Improve hostname and domain retrieval #3168

[PS-1123] Improve hostname and domain retrieval #3168

djsmith85 commented Jul 24, 2022 •

edited

Loading

MGibson1 left a comment

MGibson1 Aug 1, 2022

djsmith85 Sep 25, 2022

MGibson1 left a comment

djsmith85 commented Sep 26, 2022

djsmith85 commented Oct 22, 2022

justindbaur left a comment

justindbaur Oct 24, 2022

djsmith85 commented Oct 24, 2022

		@@ -1,7 +0,0 @@
		export function getDomain(host: string): string \| null {

[PS-1123] Improve hostname and domain retrieval #3168

[PS-1123] Improve hostname and domain retrieval #3168

Conversation

djsmith85 commented Jul 24, 2022 • edited Loading

Type of change

Objective

Code changes

Improving hostname and domain detection/validation

isWebsite/launch detection

Fix accessibility-cookie

Before you submit

MGibson1 left a comment

Choose a reason for hiding this comment

MGibson1 Aug 1, 2022

Choose a reason for hiding this comment

djsmith85 Sep 25, 2022

Choose a reason for hiding this comment

MGibson1 left a comment

Choose a reason for hiding this comment

djsmith85 commented Sep 26, 2022

djsmith85 commented Oct 22, 2022

justindbaur left a comment

Choose a reason for hiding this comment

justindbaur Oct 24, 2022

Choose a reason for hiding this comment

djsmith85 commented Oct 24, 2022

djsmith85 commented Jul 24, 2022 •

edited

Loading