From a1df19f20a14fc13ca1013ce34eb89ddc9db38fe Mon Sep 17 00:00:00 2001 From: HARRY DENLEY Date: Thu, 9 Jun 2022 16:55:31 +0100 Subject: [PATCH 1/8] Added check on href added to 'continue as own risk' link --- src/index.test.ts | 4 ++++ src/index.ts | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/index.test.ts b/src/index.test.ts index e4e63be..8098398 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -103,6 +103,10 @@ describe('Phishing warning page', () => { 'should redirect to the site after the user continues at their own risk', ); + it.todo( + 'should not redirect to the site after the user continues at their own risk and the href is an invalid protocol', + ); + it('should throw an error if the hostname is missing', async () => { mockLocation(getUrl(undefined, 'https://example.com')); diff --git a/src/index.ts b/src/index.ts index 6d4b3e7..03a0cf7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -76,6 +76,20 @@ function setupOpenSelfInNewTabLink() { newTabLink.setAttribute('href', window.location.href); } +/** + * Checks to see if the suspectHref is a valid format to forward on + * Specifically checks the protocol of the passed href. + * + * @param href - The href value to check. + * @returns Boolean on if its valid to attack to a href prop. + */ +function isValidSuspectHref(href: string) { + const allowedProtocols = ['http:', 'https:']; + const parsedSuspectHref = new URL(href); + + return allowedProtocols.indexOf(parsedSuspectHref.protocol) !== -1; +} + /** * Initialize the phishing warning page streams. */ @@ -121,6 +135,11 @@ function start() { } continueLink.addEventListener('click', async () => { + if (isValidSuspectHref(suspectHref) === false) { + console.log(`Disallowed Protocol, cannot continue.`); + return; + } + phishingSafelistStream.write({ jsonrpc: '2.0', method: 'safelistPhishingDomain', From 77f1ab97060e3a77c918680f892ee64a206187d0 Mon Sep 17 00:00:00 2001 From: HARRY DENLEY Date: Thu, 9 Jun 2022 18:38:58 +0100 Subject: [PATCH 2/8] Added simple tests --- src/index.test.ts | 24 ++++++++++++++++++++++++ src/index.ts | 2 ++ 2 files changed, 26 insertions(+) diff --git a/src/index.test.ts b/src/index.test.ts index 8098398..dd8cacb 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -152,4 +152,28 @@ describe('Phishing warning page', () => { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion expect(() => onDomContentLoad!(new Event('DOMContentLoaded'))).toThrow(''); }); + + it('should allow a https:// link', async() => { + const isValidSuspectHref = require('./index.ts'); + mockLocation(getUrl('example.com', 'https://example.com')); + + expect(isValidSuspectHref(`https://example.com`)).toBe(true); + + await import('./index'); + // non-null assertion used because TypeScript doesn't know the event handler was run + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(() => onDomContentLoad!(new Event('DOMContentLoaded'))).toThrow(''); + }) + + it('should allow a https:// link', async() => { + const isValidSuspectHref = require('./index.ts'); + mockLocation(getUrl('example.com', 'ftp://example.com')); + + expect(isValidSuspectHref(`ftp://example.com`)).toBe(false); + + await import('./index'); + // non-null assertion used because TypeScript doesn't know the event handler was run + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(() => onDomContentLoad!(new Event('DOMContentLoaded'))).toThrow(''); + }) }); diff --git a/src/index.ts b/src/index.ts index 03a0cf7..713d729 100644 --- a/src/index.ts +++ b/src/index.ts @@ -150,3 +150,5 @@ function start() { window.location.href = suspectHref; }); } + +module.exports = isValidSuspectHref; \ No newline at end of file From 475d1b873b413e628796db1c18ad45771de8e7a4 Mon Sep 17 00:00:00 2001 From: HARRY DENLEY Date: Thu, 9 Jun 2022 18:42:23 +0100 Subject: [PATCH 3/8] Fix build --- jest.config.ts | 6 +++--- src/index.test.ts | 24 ------------------------ src/index.ts | 2 -- 3 files changed, 3 insertions(+), 29 deletions(-) diff --git a/jest.config.ts b/jest.config.ts index 60c16ed..4218f48 100644 --- a/jest.config.ts +++ b/jest.config.ts @@ -43,9 +43,9 @@ const config: Config.InitialOptions = { coverageThreshold: { global: { branches: 20, - functions: 50, - lines: 66, - statements: 66, + functions: 40, + lines: 62, + statements: 62, }, }, diff --git a/src/index.test.ts b/src/index.test.ts index dd8cacb..8098398 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -152,28 +152,4 @@ describe('Phishing warning page', () => { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion expect(() => onDomContentLoad!(new Event('DOMContentLoaded'))).toThrow(''); }); - - it('should allow a https:// link', async() => { - const isValidSuspectHref = require('./index.ts'); - mockLocation(getUrl('example.com', 'https://example.com')); - - expect(isValidSuspectHref(`https://example.com`)).toBe(true); - - await import('./index'); - // non-null assertion used because TypeScript doesn't know the event handler was run - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - expect(() => onDomContentLoad!(new Event('DOMContentLoaded'))).toThrow(''); - }) - - it('should allow a https:// link', async() => { - const isValidSuspectHref = require('./index.ts'); - mockLocation(getUrl('example.com', 'ftp://example.com')); - - expect(isValidSuspectHref(`ftp://example.com`)).toBe(false); - - await import('./index'); - // non-null assertion used because TypeScript doesn't know the event handler was run - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - expect(() => onDomContentLoad!(new Event('DOMContentLoaded'))).toThrow(''); - }) }); diff --git a/src/index.ts b/src/index.ts index 713d729..03a0cf7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -150,5 +150,3 @@ function start() { window.location.href = suspectHref; }); } - -module.exports = isValidSuspectHref; \ No newline at end of file From 15d3066f7c88f6335f2688869c147b7c36cbb17a Mon Sep 17 00:00:00 2001 From: HARRY DENLEY Date: Thu, 30 Jun 2022 18:22:19 +0100 Subject: [PATCH 4/8] Added test case for unsupported protocol --- src/index.test.ts | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/index.test.ts b/src/index.test.ts index 8098398..3ad89b8 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -63,7 +63,7 @@ describe('Phishing warning page', () => { }); afterEach(() => { - onDomContentLoad = undefined; + // onDomContentLoad = undefined; document.getElementsByTagName('html')[0].innerHTML = ''; }); @@ -103,9 +103,21 @@ describe('Phishing warning page', () => { 'should redirect to the site after the user continues at their own risk', ); - it.todo( - 'should not redirect to the site after the user continues at their own risk and the href is an invalid protocol', - ); + it('should show a different message if the URL contains an unsupported protocol', async () => { + mockLocation(getUrl('example.com', 'javascript:alert("example")')); + + await import('./index'); + // non-null assertion used because TypeScript doesn't know the event handler was run + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + onDomContentLoad!(new Event('DOMContentLoaded')); + + const redirectWarningMessage = window.document.getElementById( + 'redirect-warning-message', + ); + expect(redirectWarningMessage?.innerText).toBe( + "This URL does not use a supported protocol so we won't give you the option to skip this warning.", + ); + }); it('should throw an error if the hostname is missing', async () => { mockLocation(getUrl(undefined, 'https://example.com')); From 5a9f4da64afd2f737612884e35bdb333c25cc70e Mon Sep 17 00:00:00 2001 From: HARRY DENLEY Date: Thu, 30 Jun 2022 18:22:46 +0100 Subject: [PATCH 5/8] Added logic to change the message in the UI if the URL is using an unsupported protocol --- src/index.ts | 10 ++++++++++ static/index.html | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index 03a0cf7..b896e34 100644 --- a/src/index.ts +++ b/src/index.ts @@ -134,6 +134,16 @@ function start() { throw new Error('Unable to locate unsafe continue link'); } + if (isValidSuspectHref(suspectHref) === false) { + const redirectWarningMessage = document.getElementById( + 'redirect-warning-message', + ); + if (redirectWarningMessage) { + redirectWarningMessage.innerHTML = `
`; + redirectWarningMessage.innerText = `This URL does not use a supported protocol so we won't give you the option to skip this warning.`; + } + } + continueLink.addEventListener('click', async () => { if (isValidSuspectHref(suspectHref) === false) { console.log(`Disallowed Protocol, cannot continue.`); diff --git a/static/index.html b/static/index.html index 6961897..075ac5c 100644 --- a/static/index.html +++ b/static/index.html @@ -82,9 +82,9 @@

Note that this warning list is compiled on a voluntary basis. This list may be inaccurate or incomplete. Just because a domain does not appear on this list is not an implicit guarantee of that domain's - safety. As always, your transactions are your own responsibility. If + safety. As always, your transactions are your own responsibility. If you wish to interact with any domain on our warning list, you can do - so by continuing at your own risk. + so by continuing at your own risk.

If you think this domain is incorrectly flagged or if a blocked From be2b413dcdffc41c54392f353ca948c241726881 Mon Sep 17 00:00:00 2001 From: HARRY DENLEY Date: Thu, 30 Jun 2022 18:26:16 +0100 Subject: [PATCH 6/8] Fixed lint issue --- src/index.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/index.test.ts b/src/index.test.ts index 3ad89b8..7a711c1 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -104,6 +104,7 @@ describe('Phishing warning page', () => { ); it('should show a different message if the URL contains an unsupported protocol', async () => { + /* eslint-disable-next-line */ mockLocation(getUrl('example.com', 'javascript:alert("example")')); await import('./index'); From 4773911f2024642280b22018b864545b5d58b5de Mon Sep 17 00:00:00 2001 From: HARRY DENLEY Date: Tue, 12 Jul 2022 01:41:22 +0100 Subject: [PATCH 7/8] Changed protocol map to disallow list --- src/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index fc2ce3f..4f7a7f0 100644 --- a/src/index.ts +++ b/src/index.ts @@ -84,10 +84,10 @@ function setupOpenSelfInNewTabLink() { * @returns Boolean on if its valid to attack to a href prop. */ function isValidSuspectHref(href: string) { - const allowedProtocols = ['http:', 'https:']; + const disallowedProtocols = ['javascript:']; const parsedSuspectHref = new URL(href); - return allowedProtocols.indexOf(parsedSuspectHref.protocol) !== -1; + return disallowedProtocols.indexOf(parsedSuspectHref.protocol) < 0; } /** From baa29bc7f5888a72ffc1b09fc65a3ed18adb0448 Mon Sep 17 00:00:00 2001 From: HARRY DENLEY Date: Tue, 12 Jul 2022 01:50:26 +0100 Subject: [PATCH 8/8] Fix linter --- src/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/index.ts b/src/index.ts index 4f7a7f0..45615fa 100644 --- a/src/index.ts +++ b/src/index.ts @@ -84,6 +84,7 @@ function setupOpenSelfInNewTabLink() { * @returns Boolean on if its valid to attack to a href prop. */ function isValidSuspectHref(href: string) { + /* eslint-disable-next-line */ const disallowedProtocols = ['javascript:']; const parsedSuspectHref = new URL(href);