Improve hostname and domain retrieval #547

djsmith85 · 2021-11-11T17:13:29Z

Type of change

Objective

Improve the retrieval and validation of hostnames and domains, which affects url matching/auto-fill
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 fails)
Fully up-to-date public-suffix list instead of relying on a regex only including the most common tlds
Support for parsing urls with umlauts

If for any reason the new detection does not work, I've included a fallback to the original parsing logic.

Marking this as draft:
As this affects a core part of the product/clients, I'd like to discuss the points below with the team.

I'd like to discuss the presence of the fallback to the original logic
- If we can get rid of it, we could remove tldjs as a dependency
- Remove tldjs.noop.ts
- Clean up the tsconfig.json in some repos that map tldjs to tldjs.noop.ts
The need for a tldjs.noop.ts or for that matter tldts.noop.ts

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

Closes bitwarden/web#545

Could possibly improve or fix the following issues;

Code changes

spec/common/misc/utils.spec.ts: Added several new test cases
common/package.json: Added new dependency tldts to parse urls
common/package-lock.json: Updates after adding tldts and running npm i
package-lock.json: Updates after adding tldts and running npm i
common/src/misc/utils.ts: Added several new test cases
common/src/misc/tldts.noop.ts: Included method overrides similar to existing tldjs.noop.ts

Testing requirements

For jslib itself there won't be much to test/verify other than the present unit tests.
The following PR's for the affected repos will have a lot of areas to test. Will detail them there once this is ready for merge.

Before you submit

I have checked for linting errors (npm run lint) (required)
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)

…l parsing

eliykat · 2021-11-24T03:57:10Z

May also fix bitwarden/clients#2177. Would need to check what method the Excluded Domains page calls during validation.

eliykat

Nice work @djsmith85. See comments below. In answer to your discussion questions:

let's deprecate tldjs and the old tldjs logic. We seem to have lots of test case coverage here, that lets us be aggressive in our refactoring. There's no point keeping around old stuff "just in case".
let's try to get rid of the noop implementation by moving this to a separate class and only using it in the clients that require it. Utils is supposed to be shared by all Typescript clients, stubbing out dependencies with noop implementation is a bad pattern imo.

eliykat · 2021-11-25T23:19:32Z

common/src/misc/utils.ts

@@ -1,4 +1,5 @@
 import * as tldjs from 'tldjs';
+import * as tldts from 'tldts';


Do we need to import it this way when it's a native Typescript project? We should be able to use the usual import format.

Not necessarily, but I did do it this way, as the getHostname method clashes with ours in utils.ts

eliykat · 2021-11-25T23:20:16Z

common/src/misc/tldts.noop.ts

@@ -0,0 +1,7 @@
+export function parse(url: string, options?: object): null {


I assume we have a noop version so that other clients can use the Utils class without needing this dependency. But this seems like a bad pattern. Can we extract this out to a new class/service instead, so that it's only imported by the clients that need it, it doesn't interfere with Utils, and we don't need the noop?

See comment here: #547 (comment)

common/src/misc/utils.ts

eliykat · 2021-11-25T23:24:24Z

common/src/misc/utils.ts

+            return null;
+        }
+
+        uriString = uriString.trim();


I'd include this in the previous if block just for brevity:
`if (uriString == null || uriString.trim() == '')

Calling trim() was actually important before executing the old parsing logic, as it would throw an exception if it was not trimmed beforehand

eliykat · 2021-11-25T23:26:31Z

common/src/misc/utils.ts

+        if (tldts.getHostname != null) {
+            // Does uriString contain invalid characters
+            // TODO Needs to possibly be extended, although '!' is a reserved character
+            if (uriString.indexOf('!') > 0) {


Can tldts do this kind of validation for us? Crafting regexes here seems like a losing battle.

The only reason I included is, a test would be failing as it's assumed to be an invalid url. The issue is the exclamation mark is actually a reserved character, it's just not commonly used. So I'd prefer to adjust the test and get rid of this check.

eliykat · 2021-11-25T23:28:31Z

common/src/misc/utils.ts

+            return null;
+        }
+
+        if (tldts.getHostname != null) {


I assume this is a holdover pattern from tldjs. I don't know if/why we need this, I suspect it's a holdover from using a js library and/or the noop implementation. If you move this into its own class then I think you can remove this.

Yes, that is just a holdover in case we'd use the tldts.noop. Has been removed in a recent push.

eliykat · 2021-11-25T23:29:20Z

common/src/misc/utils.ts

+                return hostname;
+            }
+        }
+
        const url = Utils.getUrl(uriString);


Probably don't need this if we're removing the if statement on line 201, right? I assume it's a fallback in case tldjs/ts isn't present

Also a fallback and I've removed it.

eliykat · 2021-11-26T00:03:18Z

spec/common/misc/utils.spec.ts

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


I don't think these are valid test cases because domain names do not actually contain unicode characters. If we want the domain name from a string containing unicode, I think it should do the punycode translation in order to get the "real" domain name. e.g.

expect(Utils.getDomain('bütwarden.com')).toBe('xn--btwarden-65a.com');

However, this does depend on what components/services are using this method. There may be some situations where we want to display the unicode in the GUI. But for behind the scenes domain matching logic, we always need the punycode. I suggest doing some investigation here.

spec/common/misc/utils.spec.ts

…ove-hostname-and-domain-retrieval

This reverts commit 6b527b7.

djsmith85 · 2021-12-06T19:23:44Z

@eliykat Thank you for taking the time to go through this and also the great feedback. I've decided to ditch the noop's and also the fallback logic. Pulling this stuff into a separate class/service would be great but it is all static and it's also being used inside our models (loginUriView). So no way to inject that.

I'll need to have a look into for example the web repo and why we are overriding tldjs with the noop. I don't really see any good value/reason for it anymore.

…hostname-and-domain-retrieval

…ove-hostname-and-domain-retrieval

Hinton · 2022-06-03T16:12:07Z

Hi,

We are currently in the process of archiving this repository and moving the code and future development to the bitwarden/clients repository. This PR will be closed but feel free to re-open it again on bitwarden/clients.

djsmith85 added 9 commits November 11, 2021 16:40

Split up localhost and IPv4 tests and added tests for IPv6

e26ad72

Add more inputs to the valid urls tests

765b1f4

Add tests for url with an underscore in the subdomain

96f925a

Removed testcase as 'bitwarden' is a valid hostname

afbf31a

Added parsing of urls with tldts with fallback to tldjs

ecb4ac1

Added tldts.noop.ts which if used will always fallback to the origina…

a9bc646

…l parsing

Added tests to verify handling of localhost with subdomain

cf302d7

Added testcases for urls containing umlauts

0eef358

Added testcases for punycode urls

b777c1d

eliykat suggested changes Nov 26, 2021

View reviewed changes

eliykat mentioned this pull request Nov 28, 2021

when password visibility is toggled the cursor keeps the current sele… #396

Closed

djsmith85 added 7 commits December 6, 2021 15:57

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

21d6739

…ove-hostname-and-domain-retrieval

Remove tldjs, noop's and fallback logic

016e978

Remove validIpAddress method

1e94197

Simplify check of url for null and whitespace

492030d

Make validHosts private

6b527b7

Revert "Make validHosts private"

4240eae

This reverts commit 6b527b7.

Add try/catch around tldts method calls

fde063f

djsmith85 added 10 commits December 16, 2021 15:12

Merge commit '8b2dfc6cdcb8ff5b604364c2ea6d343473aee7cd' into improve-…

3b15f45

…hostname-and-domain-retrieval

Changes after npm run prettier

b73fbf7

Merge commit '193434461dbd9c48fe5dcbad95693470aec422ac' into improve-…

39e36a1

…hostname-and-domain-retrieval

Remove reference to tldjs in jslib-angular

aadc4f1

Fix name of jslib-angular package

15cfc1d

update electron and node package-lock.json files

d687402

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

89792dd

…ove-hostname-and-domain-retrieval

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

7afdd1f

…ove-hostname-and-domain-retrieval

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

b074fd4

…ove-hostname-and-domain-retrieval

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

1f3f7e0

…ove-hostname-and-domain-retrieval

djsmith85 added 3 commits June 2, 2022 11:48

Changes after running npm i

bfca537

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

2141195

…ove-hostname-and-domain-retrieval

Changes after running npm i

b654bf3

Hinton closed this Jun 3, 2022

djsmith85 mentioned this pull request Jul 24, 2022

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

Merged

dyfrgi mentioned this pull request Apr 21, 2023

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

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve hostname and domain retrieval #547

Improve hostname and domain retrieval #547

djsmith85 commented Nov 11, 2021 •

edited

eliykat commented Nov 24, 2021 •

edited

eliykat left a comment •

edited

eliykat Nov 25, 2021

djsmith85 Dec 6, 2021

eliykat Nov 25, 2021

djsmith85 Dec 6, 2021

eliykat Nov 25, 2021

djsmith85 Dec 6, 2021

eliykat Nov 25, 2021

djsmith85 Dec 6, 2021

eliykat Nov 25, 2021

djsmith85 Dec 6, 2021

eliykat Nov 25, 2021

djsmith85 Dec 6, 2021

eliykat Nov 26, 2021

djsmith85 commented Dec 6, 2021

Hinton commented Jun 3, 2022

		@@ -1,4 +1,5 @@
		import * as tldjs from 'tldjs';
		import * as tldts from 'tldts';

		@@ -0,0 +1,7 @@
		export function parse(url: string, options?: object): null {

Improve hostname and domain retrieval #547

Improve hostname and domain retrieval #547

Conversation

djsmith85 commented Nov 11, 2021 • edited

Type of change

Objective

Code changes

Testing requirements

Before you submit

eliykat commented Nov 24, 2021 • edited

eliykat left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djsmith85 commented Dec 6, 2021

Hinton commented Jun 3, 2022

djsmith85 commented Nov 11, 2021 •

edited

eliykat commented Nov 24, 2021 •

edited

eliykat left a comment •

edited