From e227c6bf1d8d859392c9972437d323d766ae3012 Mon Sep 17 00:00:00 2001 From: Devin Binnie <52460000+devinbinnie@users.noreply.github.com> Date: Wed, 3 May 2023 08:48:41 -0400 Subject: [PATCH] [MM-52587] Clean up URL utils, use isInternalURL when possible (#2702) --- src/common/Validator.ts | 8 +- src/common/communication.ts | 1 - src/common/servers/MattermostServer.ts | 4 +- src/common/servers/serverManager.test.js | 8 +- src/common/servers/serverManager.ts | 8 +- src/common/utils/url.test.js | 450 ++++++++---------- src/common/utils/url.ts | 246 +++------- src/main/app/app.test.js | 4 +- src/main/app/app.ts | 27 +- src/main/app/initialize.test.js | 8 +- src/main/app/initialize.ts | 10 +- src/main/app/utils.ts | 4 +- src/main/authManager.test.js | 2 +- src/main/authManager.ts | 12 +- src/main/certificateStore.test.js | 16 +- src/main/certificateStore.ts | 24 +- src/main/contextMenu.ts | 4 +- src/main/trustedOrigins.test.js | 14 +- src/main/trustedOrigins.ts | 37 +- src/main/utils.test.js | 4 +- src/main/utils.ts | 12 +- src/main/views/MattermostView.ts | 30 +- src/main/views/viewManager.ts | 18 +- src/main/views/webContentEvents.test.js | 48 +- src/main/views/webContentEvents.ts | 62 ++- src/main/windows/callsWidgetWindow.ts | 12 +- src/renderer/components/ConfigureServer.tsx | 26 +- src/renderer/components/NewTeamModal.tsx | 4 +- src/renderer/modals/login/loginModal.tsx | 4 +- .../modals/permission/permissionModal.tsx | 8 +- 30 files changed, 481 insertions(+), 634 deletions(-) diff --git a/src/common/Validator.ts b/src/common/Validator.ts index c70d82c0..649aa60f 100644 --- a/src/common/Validator.ts +++ b/src/common/Validator.ts @@ -13,7 +13,7 @@ import {PermissionType, TrustedOrigin} from 'types/trustedOrigin'; import {Logger} from 'common/log'; import {TAB_MESSAGING} from 'common/tabs/TabView'; -import urlUtils from 'common/utils/url'; +import {isValidURL} from 'common/utils/url'; const log = new Logger('Validator'); const defaultOptions = { @@ -232,7 +232,7 @@ function cleanTeams(teams: T[], func: (te newTeams = newTeams.map((team) => func(team)); // next filter out urls that are still invalid so all is not lost - newTeams = newTeams.filter(({url}) => urlUtils.isValidURL(url)); + newTeams = newTeams.filter(({url}) => isValidURL(url)); } return newTeams; } @@ -245,7 +245,7 @@ export function validateV1ConfigData(data: ConfigV1) { export function validateV2ConfigData(data: ConfigV2) { data.teams = cleanTeams(data.teams, cleanTeam); - if (data.spellCheckerURL && !urlUtils.isValidURL(data.spellCheckerURL)) { + if (data.spellCheckerURL && !isValidURL(data.spellCheckerURL)) { log.error('Invalid download location for spellchecker dictionary, removing from config'); delete data.spellCheckerURL; } @@ -254,7 +254,7 @@ export function validateV2ConfigData(data: ConfigV2) { export function validateV3ConfigData(data: ConfigV3) { data.teams = cleanTeams(data.teams, cleanTeamWithTabs); - if (data.spellCheckerURL && !urlUtils.isValidURL(data.spellCheckerURL)) { + if (data.spellCheckerURL && !isValidURL(data.spellCheckerURL)) { log.error('Invalid download location for spellchecker dictionary, removing from config'); delete data.spellCheckerURL; } diff --git a/src/common/communication.ts b/src/common/communication.ts index 26c76139..15f80bfb 100644 --- a/src/common/communication.ts +++ b/src/common/communication.ts @@ -102,7 +102,6 @@ export const GET_AVAILABLE_SPELL_CHECKER_LANGUAGES = 'get-available-spell-checke export const GET_VIEW_INFO_FOR_TEST = 'get-view-info-for-test'; -export const RESIZE_MODAL = 'resize-modal'; export const GET_MODAL_UNCLOSEABLE = 'get-modal-uncloseable'; export const UPDATE_PATHS = 'update-paths'; diff --git a/src/common/servers/MattermostServer.ts b/src/common/servers/MattermostServer.ts index 061bc70e..1ed72d19 100644 --- a/src/common/servers/MattermostServer.ts +++ b/src/common/servers/MattermostServer.ts @@ -5,7 +5,7 @@ import {v4 as uuid} from 'uuid'; import {MattermostTeam, Team} from 'types/config'; -import urlUtils from 'common/utils/url'; +import {parseURL} from 'common/utils/url'; export class MattermostServer { id: string; @@ -23,7 +23,7 @@ export class MattermostServer { } updateURL = (url: string) => { - this.url = urlUtils.parseURL(url)!; + this.url = parseURL(url)!; if (!this.url) { throw new Error('Invalid url for creating a server'); } diff --git a/src/common/servers/serverManager.test.js b/src/common/servers/serverManager.test.js index aa3bc882..f980243b 100644 --- a/src/common/servers/serverManager.test.js +++ b/src/common/servers/serverManager.test.js @@ -2,7 +2,7 @@ // See LICENSE.txt for license information. import {TAB_MESSAGING, TAB_FOCALBOARD, TAB_PLAYBOOKS} from 'common/tabs/TabView'; -import urlUtils, {equalUrlsIgnoringSubpath} from 'common/utils/url'; +import {parseURL, isInternalURL} from 'common/utils/url'; import Utils from 'common/utils/util'; import {ServerManager} from './serverManager'; @@ -12,7 +12,7 @@ jest.mock('common/config', () => ({ })); jest.mock('common/utils/url', () => ({ parseURL: jest.fn(), - equalUrlsIgnoringSubpath: jest.fn(), + isInternalURL: jest.fn(), })); jest.mock('common/utils/util', () => ({ isVersionGreaterThanOrEqualTo: jest.fn(), @@ -125,8 +125,8 @@ describe('common/servers/serverManager', () => { }; beforeEach(() => { - urlUtils.parseURL.mockImplementation((url) => new URL(url)); - equalUrlsIgnoringSubpath.mockImplementation((url1, url2) => `${url1}`.startsWith(`${url2}`)); + parseURL.mockImplementation((url) => new URL(url)); + isInternalURL.mockImplementation((url1, url2) => `${url1}`.startsWith(`${url2}`)); }); afterEach(() => { diff --git a/src/common/servers/serverManager.ts b/src/common/servers/serverManager.ts index 4abc7215..146550a1 100644 --- a/src/common/servers/serverManager.ts +++ b/src/common/servers/serverManager.ts @@ -17,7 +17,7 @@ import {TAB_FOCALBOARD, TAB_MESSAGING, TAB_PLAYBOOKS, TabView, getDefaultTabs} f import MessagingTabView from 'common/tabs/MessagingTabView'; import FocalboardTabView from 'common/tabs/FocalboardTabView'; import PlaybooksTabView from 'common/tabs/PlaybooksTabView'; -import urlUtils, {equalUrlsIgnoringSubpath} from 'common/utils/url'; +import {isInternalURL, parseURL} from 'common/utils/url'; import Utils from 'common/utils/util'; const log = new Logger('ServerManager'); @@ -133,12 +133,12 @@ export class ServerManager extends EventEmitter { lookupTabByURL = (inputURL: URL | string, ignoreScheme = false) => { log.silly('lookupTabByURL', `${inputURL}`, ignoreScheme); - const parsedURL = urlUtils.parseURL(inputURL); + const parsedURL = parseURL(inputURL); if (!parsedURL) { return undefined; } const server = this.getAllServers().find((server) => { - return equalUrlsIgnoringSubpath(parsedURL, server.url, ignoreScheme) && parsedURL.pathname.match(new RegExp(`^${server.url.pathname}(.+)?(/(.+))?$`)); + return isInternalURL(parsedURL, server.url, ignoreScheme) && parsedURL.pathname.match(new RegExp(`^${server.url.pathname}(.+)?(/(.+))?$`)); }); if (!server) { return undefined; @@ -204,7 +204,7 @@ export class ServerManager extends EventEmitter { } let urlModified; - if (existingServer.url.toString() !== urlUtils.parseURL(server.url)?.toString()) { + if (existingServer.url.toString() !== parseURL(server.url)?.toString()) { // Emit this event whenever we update a server URL to ensure remote info is fetched urlModified = () => this.emit(SERVERS_URL_MODIFIED, [serverId]); } diff --git a/src/common/utils/url.test.js b/src/common/utils/url.test.js index f10ce0d4..40930461 100644 --- a/src/common/utils/url.test.js +++ b/src/common/utils/url.test.js @@ -2,7 +2,17 @@ // See LICENSE.txt for license information. 'use strict'; -import urlUtils, {getFormattedPathName, isUrlType, equalUrlsIgnoringSubpath, equalUrlsWithSubpath} from 'common/utils/url'; +import { + getFormattedPathName, + isUrlType, + isValidURL, + isValidURI, + parseURL, + isInternalURL, + isCustomLoginURL, + isCallsPopOutURL, + isTrustedURL, +} from 'common/utils/url'; jest.mock('common/tabs/TabView', () => ({ getServerView: (srv, tab) => { @@ -14,132 +24,6 @@ jest.mock('common/tabs/TabView', () => ({ })); describe('common/utils/url', () => { - describe('isValidURL', () => { - it('should be true for a valid web url', () => { - const testURL = 'https://developers.mattermost.com/'; - expect(urlUtils.isValidURL(testURL)).toBe(true); - }); - it('should be true for a valid, non-https web url', () => { - const testURL = 'http://developers.mattermost.com/'; - expect(urlUtils.isValidURL(testURL)).toBe(true); - }); - it('should be true for an invalid, self-defined, top-level domain', () => { - const testURL = 'https://www.example.x'; - expect(urlUtils.isValidURL(testURL)).toBe(true); - }); - it('should be true for a file download url', () => { - const testURL = 'https://community.mattermost.com/api/v4/files/ka3xbfmb3ffnmgdmww8otkidfw?download=1'; - expect(urlUtils.isValidURL(testURL)).toBe(true); - }); - it('should be true for a permalink url', () => { - const testURL = 'https://community.mattermost.com/test-channel/pl/pdqowkij47rmbyk78m5hwc7r6r'; - expect(urlUtils.isValidURL(testURL)).toBe(true); - }); - it('should be true for a valid, internal domain', () => { - const testURL = 'https://mattermost.company-internal'; - expect(urlUtils.isValidURL(testURL)).toBe(true); - }); - it('should be true for a second, valid internal domain', () => { - const testURL = 'https://serverXY/mattermost'; - expect(urlUtils.isValidURL(testURL)).toBe(true); - }); - it('should be true for a valid, non-https internal domain', () => { - const testURL = 'http://mattermost.local'; - expect(urlUtils.isValidURL(testURL)).toBe(true); - }); - it('should be true for a valid, non-https, ip address with port number', () => { - const testURL = 'http://localhost:8065'; - expect(urlUtils.isValidURL(testURL)).toBe(true); - }); - }); - describe('isValidURI', () => { - it('should be true for a deeplink url', () => { - const testURL = 'mattermost://community-release.mattermost.com/core/channels/developers'; - expect(urlUtils.isValidURI(testURL)).toBe(true); - }); - it('should be false for a malicious url', () => { - const testURL = String.raw`mattermost:///" --data-dir "\\deans-mbp\mattermost`; - expect(urlUtils.isValidURI(testURL)).toBe(false); - }); - }); - - describe('getHost', () => { - it('should return the origin of a well formed url', () => { - const myurl = 'https://mattermost.com/download'; - expect(urlUtils.getHost(myurl)).toBe('https://mattermost.com'); - }); - - it('shoud raise an error on malformed urls', () => { - const myurl = 'http://example.com:-80/'; - expect(() => { - urlUtils.getHost(myurl); - }).toThrow(SyntaxError); - }); - }); - - describe('parseURL', () => { - it('should return the URL if it is already a URL', () => { - const url = new URL('http://mattermost.com'); - expect(urlUtils.parseURL(url)).toBe(url); - }); - - it('should return undefined when a bad url is passed', () => { - const badURL = 'not-a-real-url-at-all'; - expect(urlUtils.parseURL(badURL)).toBe(undefined); - }); - - it('should remove duplicate slashes in a URL when parsing', () => { - const urlWithExtraSlashes = 'https://mattermost.com//sub//path//example'; - const parsedURL = urlUtils.parseURL(urlWithExtraSlashes); - - expect(parsedURL.toString()).toBe('https://mattermost.com/sub/path/example'); - }); - }); - - describe('isInternalURL', () => { - it('should return false on different hosts', () => { - const baseURL = new URL('http://mattermost.com'); - const externalURL = new URL('http://google.com'); - - expect(urlUtils.isInternalURL(externalURL, baseURL)).toBe(false); - }); - - it('should return false on different ports', () => { - const baseURL = new URL('http://mattermost.com:8080'); - const externalURL = new URL('http://mattermost.com:9001'); - - expect(urlUtils.isInternalURL(externalURL, baseURL)).toBe(false); - }); - - it('should return false on different subpaths', () => { - const baseURL = new URL('http://mattermost.com/sub/path/'); - const externalURL = new URL('http://mattermost.com/different/sub/path'); - - expect(urlUtils.isInternalURL(externalURL, baseURL)).toBe(false); - }); - - it('should return true if matching', () => { - const baseURL = new URL('http://mattermost.com/'); - const externalURL = new URL('http://mattermost.com'); - - expect(urlUtils.isInternalURL(externalURL, baseURL)).toBe(true); - }); - - it('should return true if matching with subpath', () => { - const baseURL = new URL('http://mattermost.com/sub/path/'); - const externalURL = new URL('http://mattermost.com/sub/path'); - - expect(urlUtils.isInternalURL(externalURL, baseURL)).toBe(true); - }); - - it('should return true if subpath of', () => { - const baseURL = new URL('http://mattermost.com/'); - const externalURL = new URL('http://mattermost.com/sub/path'); - - expect(urlUtils.isInternalURL(externalURL, baseURL)).toBe(true); - }); - }); - describe('getFormattedPathName', () => { it('should format all to lower case', () => { const unformattedPathName = '/aAbBbB/cC/DdeR/'; @@ -151,6 +35,159 @@ describe('common/utils/url', () => { expect(getFormattedPathName(unformattedPathName)).toBe('/aabbbb/cc/dder/'); }); }); + describe('parseURL', () => { + it('should return the URL if it is already a URL', () => { + const url = new URL('http://mattermost.com'); + expect(parseURL(url)).toBe(url); + }); + + it('should return undefined when a bad url is passed', () => { + const badURL = 'not-a-real-url-at-all'; + expect(parseURL(badURL)).toBe(undefined); + }); + + it('should remove duplicate slashes in a URL when parsing', () => { + const urlWithExtraSlashes = 'https://mattermost.com//sub//path//example'; + const parsedURL = parseURL(urlWithExtraSlashes); + + expect(parsedURL.toString()).toBe('https://mattermost.com/sub/path/example'); + }); + }); + + describe('isValidURL', () => { + it('should be true for a valid web url', () => { + const testURL = 'https://developers.mattermost.com/'; + expect(isValidURL(testURL)).toBe(true); + }); + it('should be true for a valid, non-https web url', () => { + const testURL = 'http://developers.mattermost.com/'; + expect(isValidURL(testURL)).toBe(true); + }); + it('should be true for an invalid, self-defined, top-level domain', () => { + const testURL = 'https://www.example.x'; + expect(isValidURL(testURL)).toBe(true); + }); + it('should be true for a file download url', () => { + const testURL = 'https://community.mattermost.com/api/v4/files/ka3xbfmb3ffnmgdmww8otkidfw?download=1'; + expect(isValidURL(testURL)).toBe(true); + }); + it('should be true for a permalink url', () => { + const testURL = 'https://community.mattermost.com/test-channel/pl/pdqowkij47rmbyk78m5hwc7r6r'; + expect(isValidURL(testURL)).toBe(true); + }); + it('should be true for a valid, internal domain', () => { + const testURL = 'https://mattermost.company-internal'; + expect(isValidURL(testURL)).toBe(true); + }); + it('should be true for a second, valid internal domain', () => { + const testURL = 'https://serverXY/mattermost'; + expect(isValidURL(testURL)).toBe(true); + }); + it('should be true for a valid, non-https internal domain', () => { + const testURL = 'http://mattermost.local'; + expect(isValidURL(testURL)).toBe(true); + }); + it('should be true for a valid, non-https, ip address with port number', () => { + const testURL = 'http://localhost:8065'; + expect(isValidURL(testURL)).toBe(true); + }); + }); + describe('isValidURI', () => { + it('should be true for a deeplink url', () => { + const testURL = 'mattermost://community-release.mattermost.com/core/channels/developers'; + expect(isValidURI(testURL)).toBe(true); + }); + it('should be false for a malicious url', () => { + const testURL = String.raw`mattermost:///" --data-dir "\\deans-mbp\mattermost`; + expect(isValidURI(testURL)).toBe(false); + }); + }); + describe('isInternalURL', () => { + it('should return false on different hosts', () => { + const baseURL = new URL('http://mattermost.com'); + const externalURL = new URL('http://google.com'); + + expect(isInternalURL(externalURL, baseURL)).toBe(false); + }); + + it('should return false on different ports', () => { + const baseURL = new URL('http://mattermost.com:8080'); + const externalURL = new URL('http://mattermost.com:9001'); + + expect(isInternalURL(externalURL, baseURL)).toBe(false); + }); + + it('should return false on different subpaths', () => { + const baseURL = new URL('http://mattermost.com/sub/path/'); + const externalURL = new URL('http://mattermost.com/different/sub/path'); + + expect(isInternalURL(externalURL, baseURL)).toBe(false); + }); + + it('should return true if matching', () => { + const baseURL = new URL('http://mattermost.com/'); + const externalURL = new URL('http://mattermost.com'); + + expect(isInternalURL(externalURL, baseURL)).toBe(true); + }); + + it('should return true if matching with subpath', () => { + const baseURL = new URL('http://mattermost.com/sub/path/'); + const externalURL = new URL('http://mattermost.com/sub/path'); + + expect(isInternalURL(externalURL, baseURL)).toBe(true); + }); + + it('should return true if subpath of', () => { + const baseURL = new URL('http://mattermost.com/'); + const externalURL = new URL('http://mattermost.com/sub/path'); + + expect(isInternalURL(externalURL, baseURL)).toBe(true); + }); + + it('same host, different URL scheme, with ignore scheme', () => { + const url1 = new URL('http://server-1.com'); + const url2 = new URL('mattermost://server-1.com'); + expect(isInternalURL(url1, url2, true)).toBe(true); + }); + }); + describe('isTrustedURL', () => { + it('base urls', () => { + const url1 = new URL('http://server-1.com'); + const url2 = new URL('http://server-1.com'); + expect(isTrustedURL(url1, url2)).toBe(true); + }); + + it('different urls', () => { + const url1 = new URL('http://server-1.com'); + const url2 = new URL('http://server-2.com'); + expect(isTrustedURL(url1, url2)).toBe(false); + }); + + it('same host, different subpath', () => { + const url1 = new URL('http://server-1.com/subpath'); + const url2 = new URL('http://server-1.com'); + expect(isTrustedURL(url1, url2)).toBe(true); + }); + + it('same host and subpath', () => { + const url1 = new URL('http://server-1.com/subpath'); + const url2 = new URL('http://server-1.com/subpath'); + expect(isTrustedURL(url1, url2)).toBe(true); + }); + + it('same host, different URL scheme', () => { + const url1 = new URL('http://server-1.com'); + const url2 = new URL('mattermost://server-1.com'); + expect(isTrustedURL(url1, url2)).toBe(false); + }); + + it('same host, different ports', () => { + const url1 = new URL('http://server-1.com:8080'); + const url2 = new URL('http://server-1.com'); + expect(isTrustedURL(url1, url2)).toBe(false); + }); + }); describe('isUrlType', () => { const serverURL = new URL('http://mattermost.com'); @@ -172,175 +209,100 @@ describe('common/utils/url', () => { }); }); - describe('equalUrls', () => { - it('base urls', () => { - const url1 = new URL('http://server-1.com'); - const url2 = new URL('http://server-1.com'); - expect(equalUrlsIgnoringSubpath(url1, url2)).toBe(true); - expect(equalUrlsWithSubpath(url1, url2)).toBe(true); - }); - - it('different urls', () => { - const url1 = new URL('http://server-1.com'); - const url2 = new URL('http://server-2.com'); - expect(equalUrlsIgnoringSubpath(url1, url2)).toBe(false); - expect(equalUrlsWithSubpath(url1, url2)).toBe(false); - }); - - it('same host, different subpath', () => { - const url1 = new URL('http://server-1.com/subpath'); - const url2 = new URL('http://server-1.com'); - expect(equalUrlsIgnoringSubpath(url1, url2)).toBe(true); - expect(equalUrlsWithSubpath(url1, url2)).toBe(false); - }); - - it('same host and subpath', () => { - const url1 = new URL('http://server-1.com/subpath'); - const url2 = new URL('http://server-1.com/subpath'); - expect(equalUrlsIgnoringSubpath(url1, url2)).toBe(true); - expect(equalUrlsWithSubpath(url1, url2)).toBe(true); - }); - - it('same host, different URL scheme', () => { - const url1 = new URL('http://server-1.com'); - const url2 = new URL('mattermost://server-1.com'); - expect(equalUrlsIgnoringSubpath(url1, url2)).toBe(false); - expect(equalUrlsWithSubpath(url1, url2)).toBe(false); - }); - - it('same host, different URL scheme, with ignore scheme', () => { - const url1 = new URL('http://server-1.com'); - const url2 = new URL('mattermost://server-1.com'); - expect(equalUrlsIgnoringSubpath(url1, url2, true)).toBe(true); - expect(equalUrlsWithSubpath(url1, url2, true)).toBe(true); - }); - - it('same host, different ports', () => { - const url1 = new URL('http://server-1.com:8080'); - const url2 = new URL('http://server-1.com'); - expect(equalUrlsIgnoringSubpath(url1, url2, true)).toBe(false); - expect(equalUrlsWithSubpath(url1, url2, true)).toBe(false); - }); - }); - - describe('cleanPathName', () => { - it('should not clean path name if it occurs other than the beginning', () => { - expect(urlUtils.cleanPathName('/mattermost', '/home/channels/mattermost/test')).toBe('/home/channels/mattermost/test'); - }); - - it('should clean path name if it occurs at the beginning', () => { - expect(urlUtils.cleanPathName('/mattermost', '/mattermost/channels/home/test')).toBe('/channels/home/test'); - }); - - it('should do nothing if it doesnt occur', () => { - expect(urlUtils.cleanPathName('/mattermost', '/channels/home/test')).toBe('/channels/home/test'); - }); - }); - describe('isCustomLoginURL', () => { it('should match correct URL', () => { - expect(urlUtils.isCustomLoginURL( - 'http://server.com/oauth/authorize', - 'http://server.com', + expect(isCustomLoginURL( + new URL('http://server.com/oauth/authorize'), + new URL('http://server.com'), )).toBe(true); }); it('should not match incorrect URL', () => { - expect(urlUtils.isCustomLoginURL( - 'http://server.com/oauth/notauthorize', - 'http://server.com', + expect(isCustomLoginURL( + new URL('http://server.com/oauth/notauthorize'), + new URL('http://server.com'), )).toBe(false); }); it('should not match base URL', () => { - expect(urlUtils.isCustomLoginURL( - 'http://server.com/', - 'http://server.com', + expect(isCustomLoginURL( + new URL('http://server.com/'), + new URL('http://server.com'), )).toBe(false); }); it('should match with subpath', () => { - expect(urlUtils.isCustomLoginURL( - 'http://server.com/subpath/oauth/authorize', - 'http://server.com/subpath', + expect(isCustomLoginURL( + new URL('http://server.com/subpath/oauth/authorize'), + new URL('http://server.com/subpath'), )).toBe(true); }); it('should not match with different subpath', () => { - expect(urlUtils.isCustomLoginURL( - 'http://server.com/subpath/oauth/authorize', - 'http://server.com/different/subpath', + expect(isCustomLoginURL( + new URL('http://server.com/subpath/oauth/authorize'), + new URL('http://server.com/different/subpath'), )).toBe(false); }); it('should not match with oauth subpath', () => { - expect(urlUtils.isCustomLoginURL( - 'http://server.com/oauth/authorize', - 'http://server.com/oauth/authorize', + expect(isCustomLoginURL( + new URL('http://server.com/oauth/authorize'), + new URL('http://server.com/oauth/authorize'), )).toBe(false); }); }); describe('isCallsPopOutURL', () => { it('should match correct URL', () => { - expect(urlUtils.isCallsPopOutURL( - 'http://example.org', - 'http://example.org/team/com.mattermost.calls/expanded/callid', + expect(isCallsPopOutURL( + new URL('http://example.org'), + new URL('http://example.org/team/com.mattermost.calls/expanded/callid'), 'callid', )).toBe(true); }); it('should match with subpath', () => { - expect(urlUtils.isCallsPopOutURL( - 'http://example.org/subpath', - 'http://example.org/subpath/team/com.mattermost.calls/expanded/callid', + expect(isCallsPopOutURL( + new URL('http://example.org/subpath'), + new URL('http://example.org/subpath/team/com.mattermost.calls/expanded/callid'), 'callid', )).toBe(true); }); it('should match with teamname with dash', () => { - expect(urlUtils.isCallsPopOutURL( - 'http://example.org', - 'http://example.org/team-name/com.mattermost.calls/expanded/callid', + expect(isCallsPopOutURL( + new URL('http://example.org'), + new URL('http://example.org/team-name/com.mattermost.calls/expanded/callid'), 'callid', )).toBe(true); }); it('should not match with invalid team name', () => { - expect(urlUtils.isCallsPopOutURL( - 'http://example.org', - 'http://example.org/invalid$team/com.mattermost.calls/expanded/othercallid', + expect(isCallsPopOutURL( + new URL('http://example.org'), + new URL('http://example.org/invalid$team/com.mattermost.calls/expanded/othercallid'), 'callid', )).toBe(false); }); it('should not match with incorrect callid', () => { - expect(urlUtils.isCallsPopOutURL( - 'http://example.org', - 'http://example.org/team/com.mattermost.calls/expanded/othercallid', + expect(isCallsPopOutURL( + new URL('http://example.org'), + new URL('http://example.org/team/com.mattermost.calls/expanded/othercallid'), 'callid', )).toBe(false); }); it('should not match with incorrect origin', () => { - expect(urlUtils.isCallsPopOutURL( - 'http://example.com', - 'http://example.org/team/com.mattermost.calls/expanded/callid', + expect(isCallsPopOutURL( + new URL('http://example.com'), + new URL('http://example.org/team/com.mattermost.calls/expanded/callid'), 'callid', )).toBe(false); }); - it('should not match with missing arguments', () => { - expect(urlUtils.isCallsPopOutURL()).toBe(false); - }); - }); - - describe('escapeRegExp', () => { - it('simple', () => { - expect(urlUtils.escapeRegExp('simple')).toBe('simple'); - }); - - it('path', () => { - expect(urlUtils.escapeRegExp('/path/')).toBe('/path/'); - }); - - it('regexp', () => { - expect(urlUtils.escapeRegExp('/path(a+)+')).toBe('/path\\(a\\+\\)\\+'); + it('should match with regex path embedded', () => { + expect(isCallsPopOutURL( + new URL('http://example.com/path(a+)+'), + new URL('http://example.org//path\\(a\\+\\)\\+/team/com.mattermost.calls/expanded/callid'), + 'callid', + )).toBe(false); }); }); }); diff --git a/src/common/utils/url.ts b/src/common/utils/url.ts index b4d6c32b..53f7934d 100644 --- a/src/common/utils/url.ts +++ b/src/common/utils/url.ts @@ -6,137 +6,40 @@ import {isHttpsUri, isHttpUri, isUri} from 'valid-url'; import buildConfig from 'common/config/buildConfig'; import {customLoginRegexPaths, nonTeamUrlPaths, CALLS_PLUGIN_ID} from 'common/utils/constants'; -function isValidURL(testURL: string) { - return Boolean(isHttpUri(testURL) || isHttpsUri(testURL)) && Boolean(parseURL(testURL)); -} - -function isValidURI(testURL: string) { - return Boolean(isUri(testURL)); -} - -function startsWithProtocol(testURL: string) { - return Boolean((/^https?:\/\/.*/).test(testURL.trim())); -} - -function parseURL(inputURL: URL | string) { +export const getFormattedPathName = (pn: string) => (pn.endsWith('/') ? pn.toLowerCase() : `${pn.toLowerCase()}/`); +export const parseURL = (inputURL: string | URL) => { if (inputURL instanceof URL) { return inputURL; } try { - return new URL(inputURL.replace(/([^:]\/)\/+/g, '$1')); + return new URL(inputURL.replace(/([^:]\/)\/+/g, '$1')); // Regex here to remove extra slashes } catch (e) { return undefined; } -} +}; -function getHost(inputURL: URL | string) { - const parsedURL = parseURL(inputURL); - if (parsedURL) { - return parsedURL.origin; - } - throw new SyntaxError(`Couldn't parse url: ${inputURL}`); -} +/** + * URL form checks + */ + +export const isValidURL = (testURL: string) => Boolean(isHttpUri(testURL) || isHttpsUri(testURL)) && Boolean(parseURL(testURL)); +export const isValidURI = (testURL: string) => Boolean(isUri(testURL)); // isInternalURL determines if the target url is internal to the application. // - currentURL is the current url inside the webview -function isInternalURL(targetURL: URL | undefined, currentURL: URL) { - if (!targetURL) { - return false; - } - +export const isInternalURL = (targetURL: URL, currentURL: URL, ignoreScheme?: boolean) => { if (targetURL.host !== currentURL.host) { return false; } - if (!equalUrlsWithSubpath(targetURL, currentURL) && !(targetURL.pathname || '/').startsWith(currentURL.pathname)) { + if (!equalUrlsWithSubpath(targetURL, currentURL, ignoreScheme) && !(targetURL.pathname || '/').startsWith(currentURL.pathname)) { return false; } return true; -} +}; -function getServerInfo(serverUrl: URL | string) { - const parsedServer = parseURL(serverUrl); - if (!parsedServer) { - return undefined; - } - - // does the server have a subpath? - const pn = parsedServer.pathname.toLowerCase(); - const subpath = getFormattedPathName(pn); - return {subpath, url: parsedServer}; -} - -export function getFormattedPathName(pn: string) { - return pn.endsWith('/') ? pn.toLowerCase() : `${pn.toLowerCase()}/`; -} - -function getManagedResources() { - if (!buildConfig) { - return []; - } - - return buildConfig.managedResources || []; -} - -export function isUrlType(urlType: string, serverUrl: URL | string, inputURL: URL | string) { - if (!serverUrl || !inputURL) { - return false; - } - - const parsedURL = parseURL(inputURL); - const server = getServerInfo(serverUrl); - if (!parsedURL || !server || (!equalUrlsIgnoringSubpath(server.url, parsedURL))) { - return false; - } - return (getFormattedPathName(parsedURL.pathname).startsWith(`${server.subpath}${urlType}/`) || - getFormattedPathName(parsedURL.pathname).startsWith(`/${urlType}/`)); -} - -function isAdminUrl(serverUrl: URL | string, inputURL: URL | string) { - return isUrlType('admin_console', serverUrl, inputURL); -} - -function isTeamUrl(serverUrl: URL | string, inputURL: URL | string, withApi?: boolean) { - const parsedURL = parseURL(inputURL); - const server = getServerInfo(serverUrl); - if (!parsedURL || !server || (!equalUrlsIgnoringSubpath(server.url, parsedURL))) { - return false; - } - - const paths = [...getManagedResources(), ...nonTeamUrlPaths]; - - if (withApi) { - paths.push('api'); - } - return !(paths.some((testPath) => isUrlType(testPath, serverUrl, inputURL))); -} - -function isPluginUrl(serverUrl: URL | string, inputURL: URL | string) { - return isUrlType('plugins', serverUrl, inputURL); -} - -function isManagedResource(serverUrl: URL | string, inputURL: URL | string) { - const paths = [...getManagedResources()]; - return paths.some((testPath) => isUrlType(testPath, serverUrl, inputURL)); -} - -// next two functions are defined to clarify intent -export function equalUrlsWithSubpath(url1: URL, url2: URL, ignoreScheme?: boolean) { - if (ignoreScheme) { - return url1.host === url2.host && getFormattedPathName(url2.pathname).startsWith(getFormattedPathName(url1.pathname)); - } - return url1.origin === url2.origin && getFormattedPathName(url2.pathname).startsWith(getFormattedPathName(url1.pathname)); -} - -export function equalUrlsIgnoringSubpath(url1: URL, url2: URL, ignoreScheme?: boolean) { - if (ignoreScheme) { - return url1.host.toLowerCase() === url2.host.toLowerCase(); - } - return url1.origin.toLowerCase() === url2.origin.toLowerCase(); -} - -function isTrustedURL(url: URL | string, rootURL: URL | string) { +export const isTrustedURL = (url: URL, rootURL: URL) => { const parsedURL = parseURL(url); const rootParsedURL = parseURL(rootURL); if (!parsedURL || !rootParsedURL) { @@ -144,19 +47,41 @@ function isTrustedURL(url: URL | string, rootURL: URL | string) { } return (getFormattedPathName(rootParsedURL.pathname) !== '/' && equalUrlsWithSubpath(rootParsedURL, parsedURL)) || (getFormattedPathName(rootParsedURL.pathname) === '/' && equalUrlsIgnoringSubpath(rootParsedURL, parsedURL)); -} +}; -function isCustomLoginURL(url: URL | string, serverURL: URL | string): boolean { - const parsedServerURL = parseURL(serverURL); - const parsedURL = parseURL(url); - if (!parsedURL || !parsedServerURL) { +export const isUrlType = (urlType: string, serverURL: URL, inputURL: URL) => { + if (!isInternalURL(inputURL, serverURL)) { return false; } - if (!isTrustedURL(parsedURL, parsedServerURL)) { + return (getFormattedPathName(inputURL.pathname).startsWith(`${getFormattedPathName(serverURL.pathname)}${urlType}/`) || + getFormattedPathName(inputURL.pathname).startsWith(`/${urlType}/`)); +}; + +export const isHelpUrl = (serverURL: URL, inputURL: URL) => isUrlType('help', serverURL, inputURL); +export const isImageProxyUrl = (serverURL: URL, inputURL: URL) => isUrlType('api/v4/image', serverURL, inputURL); +export const isPublicFilesUrl = (serverURL: URL, inputURL: URL) => isUrlType('files', serverURL, inputURL); +export const isAdminUrl = (serverURL: URL, inputURL: URL) => isUrlType('admin_console', serverURL, inputURL); +export const isPluginUrl = (serverURL: URL, inputURL: URL) => isUrlType('plugins', serverURL, inputURL); +export const isChannelExportUrl = (serverURL: URL, inputURL: URL) => isUrlType('plugins/com.mattermost.plugin-channel-export/api/v1/export', serverURL, inputURL); +export const isManagedResource = (serverURL: URL, inputURL: URL) => [...buildConfig.managedResources].some((testPath) => isUrlType(testPath, serverURL, inputURL)); +export const isTeamUrl = (serverURL: URL, inputURL: URL, withApi?: boolean) => { + if (!isInternalURL(inputURL, serverURL)) { return false; } - const subpath = parsedServerURL.pathname; - const urlPath = parsedURL.pathname; + + const paths = [...buildConfig.managedResources, ...nonTeamUrlPaths]; + + if (withApi) { + paths.push('api'); + } + return !(paths.some((testPath) => isUrlType(testPath, serverURL, inputURL))); +}; +export const isCustomLoginURL = (inputURL: URL, serverURL: URL) => { + if (!isTrustedURL(inputURL, serverURL)) { + return false; + } + const subpath = serverURL.pathname; + const urlPath = inputURL.pathname; const replacement = subpath.endsWith('/') ? '/' : ''; const replacedPath = urlPath.replace(subpath, replacement); for (const regexPath of customLoginRegexPaths) { @@ -166,42 +91,10 @@ function isCustomLoginURL(url: URL | string, serverURL: URL | string): boolean { } return false; -} +}; -function isChannelExportUrl(serverUrl: URL | string, inputUrl: URL | string): boolean { - return isUrlType('plugins/com.mattermost.plugin-channel-export/api/v1/export', serverUrl, inputUrl); -} - -function cleanPathName(basePathName: string, pathName: string) { - if (basePathName === '/') { - return pathName; - } - - if (pathName.startsWith(basePathName)) { - return pathName.replace(basePathName, ''); - } - - return pathName; -} - -// RegExp string escaping function, as recommended by -// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#escaping -function escapeRegExp(s: string) { - return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); // $& means the whole matched string -} - -function isCallsPopOutURL(serverURL: URL | string, inputURL: URL | string, callID: string) { - if (!serverURL || !inputURL || !callID) { - return false; - } - - const parsedURL = parseURL(inputURL); - const server = getServerInfo(serverURL); - if (!server || !parsedURL) { - return false; - } - - const matches = parsedURL.pathname.match(new RegExp(`^${escapeRegExp(server.subpath)}([A-Za-z0-9-_]+)/`, 'i')); +export const isCallsPopOutURL = (serverURL: URL, inputURL: URL, callID: string) => { + const matches = inputURL.pathname.match(new RegExp(`^${escapeRegExp(getFormattedPathName(serverURL.pathname))}([A-Za-z0-9-_]+)/`, 'i')); if (matches?.length !== 2) { return false; } @@ -210,25 +103,28 @@ function isCallsPopOutURL(serverURL: URL | string, inputURL: URL | string, callI const subPath = `${teamName}/${CALLS_PLUGIN_ID}/expanded/${callID}`; return isUrlType(subPath, serverURL, inputURL); -} - -export default { - isValidURL, - isValidURI, - isInternalURL, - parseURL, - getServerInfo, - isAdminUrl, - isTeamUrl, - isPluginUrl, - isManagedResource, - getHost, - isTrustedURL, - isCustomLoginURL, - isChannelExportUrl, - isUrlType, - cleanPathName, - startsWithProtocol, - isCallsPopOutURL, - escapeRegExp, +}; + +/** + * Helper functions + */ + +// next two functions are defined to clarify intent +const equalUrlsWithSubpath = (url1: URL, url2: URL, ignoreScheme?: boolean) => { + if (ignoreScheme) { + return url1.host === url2.host && getFormattedPathName(url2.pathname).startsWith(getFormattedPathName(url1.pathname)); + } + return url1.origin === url2.origin && getFormattedPathName(url2.pathname).startsWith(getFormattedPathName(url1.pathname)); +}; +const equalUrlsIgnoringSubpath = (url1: URL, url2: URL, ignoreScheme?: boolean) => { + if (ignoreScheme) { + return url1.host.toLowerCase() === url2.host.toLowerCase(); + } + return url1.origin.toLowerCase() === url2.origin.toLowerCase(); +}; + +// RegExp string escaping function, as recommended by +// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#escaping +const escapeRegExp = (s: string) => { + return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); // $& means the whole matched string }; diff --git a/src/main/app/app.test.js b/src/main/app/app.test.js index cfa89ed9..80a11694 100644 --- a/src/main/app/app.test.js +++ b/src/main/app/app.test.js @@ -159,7 +159,7 @@ describe('main/app/app', () => { await handleAppCertificateError(event, webContents, testURL, 'error-1', certificate, callback); expect(callback).toHaveBeenCalledWith(true); expect(certificateErrorCallbacks.has('http://server-1.com:error-1')).toBe(false); - expect(CertificateStore.add).toHaveBeenCalledWith('http://server-1.com', certificate); + expect(CertificateStore.add).toHaveBeenCalledWith(new URL('http://server-1.com'), certificate); expect(CertificateStore.save).toHaveBeenCalled(); }); @@ -175,7 +175,7 @@ describe('main/app/app', () => { await handleAppCertificateError(event, webContents, testURL, 'error-1', certificate, callback); expect(callback).toHaveBeenCalledWith(false); expect(certificateErrorCallbacks.has('http://server-1.com:error-1')).toBe(false); - expect(CertificateStore.add).toHaveBeenCalledWith('http://server-1.com', certificate, true); + expect(CertificateStore.add).toHaveBeenCalledWith(new URL('http://server-1.com'), certificate, true); expect(CertificateStore.save).toHaveBeenCalled(); }); }); diff --git a/src/main/app/app.ts b/src/main/app/app.ts index 88b4ea44..e9589c8f 100644 --- a/src/main/app/app.ts +++ b/src/main/app/app.ts @@ -4,7 +4,7 @@ import {app, BrowserWindow, Event, dialog, WebContents, Certificate, Details} from 'electron'; import {Logger} from 'common/log'; -import urlUtils from 'common/utils/url'; +import {parseURL} from 'common/utils/url'; import updateManager from 'main/autoUpdater'; import CertificateStore from 'main/certificateStore'; @@ -80,27 +80,26 @@ export function handleAppBeforeQuit() { export async function handleAppCertificateError(event: Event, webContents: WebContents, url: string, error: string, certificate: Certificate, callback: (isTrusted: boolean) => void) { log.verbose('handleAppCertificateError', {url, error, certificate}); - const parsedURL = urlUtils.parseURL(url); + const parsedURL = parseURL(url); if (!parsedURL) { return; } - const origin = parsedURL.origin; - if (CertificateStore.isExplicitlyUntrusted(origin)) { + if (CertificateStore.isExplicitlyUntrusted(parsedURL)) { event.preventDefault(); - log.warn(`Ignoring previously untrusted certificate for ${origin}`); + log.warn(`Ignoring previously untrusted certificate for ${parsedURL.origin}`); callback(false); - } else if (CertificateStore.isTrusted(origin, certificate)) { + } else if (CertificateStore.isTrusted(parsedURL, certificate)) { event.preventDefault(); callback(true); } else { // update the callback - const errorID = `${origin}:${error}`; + const errorID = `${parsedURL.origin}:${error}`; const view = ViewManager.getViewByWebContentsId(webContents.id); if (view?.tab.server) { - const serverURL = urlUtils.parseURL(view.tab.server.url); - if (serverURL && serverURL.origin !== origin) { - log.warn(`Ignoring certificate for unmatched origin ${origin}, will not trust`); + const serverURL = parseURL(view.tab.server.url); + if (serverURL && serverURL.origin !== parsedURL.origin) { + log.warn(`Ignoring certificate for unmatched origin ${parsedURL.origin}, will not trust`); callback(false); return; } @@ -112,8 +111,8 @@ export async function handleAppCertificateError(event: Event, webContents: WebCo certificateErrorCallbacks.set(errorID, callback); return; } - const extraDetail = CertificateStore.isExisting(origin) ? localizeMessage('main.app.app.handleAppCertificateError.dialog.extraDetail', 'Certificate is different from previous one.\n\n') : ''; - const detail = localizeMessage('main.app.app.handleAppCertificateError.certError.dialog.detail', '{extraDetail}origin: {origin}\nError: {error}', {extraDetail, origin, error}); + const extraDetail = CertificateStore.isExisting(parsedURL) ? localizeMessage('main.app.app.handleAppCertificateError.dialog.extraDetail', 'Certificate is different from previous one.\n\n') : ''; + const detail = localizeMessage('main.app.app.handleAppCertificateError.certError.dialog.detail', '{extraDetail}origin: {origin}\nError: {error}', {extraDetail, origin: parsedURL.origin, error}); certificateErrorCallbacks.set(errorID, callback); @@ -154,7 +153,7 @@ export async function handleAppCertificateError(event: Event, webContents: WebCo } if (result.response === 0) { - CertificateStore.add(origin, certificate); + CertificateStore.add(parsedURL, certificate); CertificateStore.save(); certificateErrorCallbacks.get(errorID)(true); @@ -165,7 +164,7 @@ export async function handleAppCertificateError(event: Event, webContents: WebCo } } else { if (result.checkboxChecked) { - CertificateStore.add(origin, certificate, true); + CertificateStore.add(parsedURL, certificate, true); CertificateStore.save(); } certificateErrorCallbacks.get(errorID)(false); diff --git a/src/main/app/initialize.test.js b/src/main/app/initialize.test.js index eda4c398..f82e22ab 100644 --- a/src/main/app/initialize.test.js +++ b/src/main/app/initialize.test.js @@ -6,7 +6,7 @@ import path from 'path'; import {app, session} from 'electron'; import Config from 'common/config'; -import urlUtils from 'common/utils/url'; +import {parseURL, isTrustedURL} from 'common/utils/url'; import parseArgs from 'main/ParseArgs'; import ViewManager from 'main/views/viewManager'; @@ -105,6 +105,7 @@ jest.mock('common/config', () => ({ })); jest.mock('common/utils/url', () => ({ + parseURL: jest.fn(), isTrustedURL: jest.fn(), })); @@ -283,6 +284,9 @@ describe('main/app/initialize', () => { }, }, }); + parseURL.mockImplementation((url) => new URL(url)); + isTrustedURL.mockImplementation((url) => url.toString() === 'http://server-1.com/'); + let callback = jest.fn(); session.defaultSession.setPermissionRequestHandler.mockImplementation((cb) => { cb({id: 1, getURL: () => 'http://server-1.com'}, 'bad-permission', callback); @@ -298,8 +302,6 @@ describe('main/app/initialize', () => { await initialize(); expect(callback).toHaveBeenCalledWith(true); - urlUtils.isTrustedURL.mockImplementation((url) => url === 'http://server-1.com'); - callback = jest.fn(); session.defaultSession.setPermissionRequestHandler.mockImplementation((cb) => { cb({id: 2, getURL: () => 'http://server-1.com'}, 'openExternal', callback); diff --git a/src/main/app/initialize.ts b/src/main/app/initialize.ts index 7d6d5a94..825c9e9f 100644 --- a/src/main/app/initialize.ts +++ b/src/main/app/initialize.ts @@ -43,8 +43,8 @@ import { DOUBLE_CLICK_ON_WINDOW, } from 'common/communication'; import Config from 'common/config'; +import {isTrustedURL, parseURL} from 'common/utils/url'; import {Logger} from 'common/log'; -import urlUtils from 'common/utils/url'; import AllowProtocolDialog from 'main/allowProtocolDialog'; import AppVersionManager from 'main/AppVersionManager'; @@ -460,8 +460,14 @@ async function initializeAfterAppReady() { return; } + const parsedURL = parseURL(requestingURL); + if (!parsedURL) { + callback(false); + return; + } + // is the requesting url trusted? - callback(urlUtils.isTrustedURL(requestingURL, serverURL)); + callback(isTrustedURL(parsedURL, serverURL)); }); if (wasUpdated(AppVersionManager.lastAppVersion)) { diff --git a/src/main/app/utils.ts b/src/main/app/utils.ts index 34569188..014d17e2 100644 --- a/src/main/app/utils.ts +++ b/src/main/app/utils.ts @@ -16,8 +16,8 @@ import {Logger} from 'common/log'; import JsonFileManager from 'common/JsonFileManager'; import ServerManager from 'common/servers/serverManager'; import {MattermostServer} from 'common/servers/MattermostServer'; -import urlUtils from 'common/utils/url'; import {APP_MENU_WILL_CLOSE} from 'common/communication'; +import {isValidURI} from 'common/utils/url'; import updateManager from 'main/autoUpdater'; import {migrationInfoPath, updatePaths} from 'main/constants'; @@ -72,7 +72,7 @@ export function getDeeplinkingURL(args: string[]) { if (Array.isArray(args) && args.length) { // deeplink urls should always be the last argument, but may not be the first (i.e. Windows with the app already running) const url = args[args.length - 1]; - if (url && mainProtocol && url.startsWith(mainProtocol) && urlUtils.isValidURI(url)) { + if (url && mainProtocol && url.startsWith(mainProtocol) && isValidURI(url)) { return url; } } diff --git a/src/main/authManager.test.js b/src/main/authManager.test.js index b9add966..4f508450 100644 --- a/src/main/authManager.test.js +++ b/src/main/authManager.test.js @@ -58,7 +58,7 @@ jest.mock('common/config', () => ({ jest.mock('common/utils/url', () => { const actualUrl = jest.requireActual('common/utils/url'); return { - ...actualUrl.default, + ...actualUrl, isTrustedURL: (url) => { return url.toString() === 'http://trustedurl.com/'; }, diff --git a/src/main/authManager.ts b/src/main/authManager.ts index 0b5cceea..126da3f7 100644 --- a/src/main/authManager.ts +++ b/src/main/authManager.ts @@ -7,7 +7,7 @@ import {LoginModalData} from 'types/auth'; import {Logger} from 'common/log'; import {BASIC_AUTH_PERMISSION} from 'common/permissions'; -import urlUtils from 'common/utils/url'; +import {isCustomLoginURL, isTrustedURL, parseURL} from 'common/utils/url'; import modalManager from 'main/views/modalManager'; import TrustedOriginsStore from 'main/trustedOrigins'; @@ -36,7 +36,7 @@ export class AuthManager { log.verbose('handleAppLogin', {request, authInfo}); event.preventDefault(); - const parsedURL = urlUtils.parseURL(request.url); + const parsedURL = parseURL(request.url); if (!parsedURL) { return; } @@ -46,7 +46,7 @@ export class AuthManager { } this.loginCallbackMap.set(request.url, callback); // if callback is undefined set it to null instead so we know we have set it up with no value - if (urlUtils.isTrustedURL(request.url, serverURL) || urlUtils.isCustomLoginURL(parsedURL, serverURL) || TrustedOriginsStore.checkPermission(request.url, BASIC_AUTH_PERMISSION)) { + if (isTrustedURL(parsedURL, serverURL) || isCustomLoginURL(parsedURL, serverURL) || TrustedOriginsStore.checkPermission(parsedURL, BASIC_AUTH_PERMISSION)) { this.popLoginModal(request, authInfo); } else { this.popPermissionModal(request, authInfo, BASIC_AUTH_PERMISSION); @@ -109,7 +109,11 @@ export class AuthManager { } handlePermissionGranted(url: string, permission: PermissionType) { - TrustedOriginsStore.addPermission(url, permission); + const parsedURL = parseURL(url); + if (!parsedURL) { + return; + } + TrustedOriginsStore.addPermission(parsedURL, permission); TrustedOriginsStore.save(); } } diff --git a/src/main/certificateStore.test.js b/src/main/certificateStore.test.js index 3cb10fdd..265b4eeb 100644 --- a/src/main/certificateStore.test.js +++ b/src/main/certificateStore.test.js @@ -65,26 +65,26 @@ describe('main/certificateStore', () => { it('should return true for stored matching certificate', () => { certificateStore = new CertificateStore('someFilename'); - expect(certificateStore.isTrusted('https://server-1.com', { + expect(certificateStore.isTrusted(new URL('https://server-1.com'), { data: 'someRandomData', issuerName: 'someIssuer', })).toBe(true); }); it('should return false for missing url', () => { - expect(certificateStore.isTrusted('https://server-3.com', { + expect(certificateStore.isTrusted(new URL('https://server-3.com'), { data: 'someRandomData', issuerName: 'someIssuer', })).toBe(false); }); it('should return false for unmatched cert', () => { - expect(certificateStore.isTrusted('https://server-1.com', { + expect(certificateStore.isTrusted(new URL('https://server-1.com'), { data: 'someOtherRandomData', issuerName: 'someIssuer', })).toBe(false); - expect(certificateStore.isTrusted('https://server-1.com', { + expect(certificateStore.isTrusted(new URL('https://server-1.com'), { data: 'someRandomData', issuerName: 'someOtherIssuer', })).toBe(false); @@ -99,8 +99,8 @@ describe('main/certificateStore', () => { }; certificateStore = new CertificateStore('someFilename'); - certificateStore.add(certOrigin, certData); - expect(certificateStore.isTrusted(wssCertOrigin, certData)).toBe(true); + certificateStore.add(new URL(certOrigin), certData); + expect(certificateStore.isTrusted(new URL(wssCertOrigin), certData)).toBe(true); }); }); @@ -113,14 +113,14 @@ describe('main/certificateStore', () => { }); it('should return true for explicitly untrusted cert', () => { - expect(certificateStore.isExplicitlyUntrusted('https://server-2.com', { + expect(certificateStore.isExplicitlyUntrusted(new URL('https://server-2.com'), { data: 'someRandomData', issuerName: 'someIssuer', })).toBe(true); }); it('should return false for trusted cert', () => { - expect(certificateStore.isExplicitlyUntrusted('https://server-1.com', { + expect(certificateStore.isExplicitlyUntrusted(new URL('https://server-1.com'), { data: 'someRandomData', issuerName: 'someIssuer', })).toBe(false); diff --git a/src/main/certificateStore.ts b/src/main/certificateStore.ts index 118d7e0b..ba0b75df 100644 --- a/src/main/certificateStore.ts +++ b/src/main/certificateStore.ts @@ -11,7 +11,6 @@ import {ComparableCertificate} from 'types/certificate'; import {UPDATE_PATHS} from 'common/communication'; import {Logger} from 'common/log'; -import urlUtils from 'common/utils/url'; import * as Validator from 'common/Validator'; import {certificateStorePath} from './constants'; @@ -57,35 +56,32 @@ export class CertificateStore { fs.writeFileSync(this.storeFile, JSON.stringify(this.data, null, ' ')); }; - add = (targetURL: string, certificate: Certificate, dontTrust = false) => { - const host = urlUtils.getHost(targetURL); + add = (targetURL: URL, certificate: Certificate, dontTrust = false) => { const comparableCert = comparableCertificate(certificate, dontTrust); - this.data[host] = comparableCert; + this.data[targetURL.origin] = comparableCert; // Trust certificate for websocket connections on the same origin. - if (host.startsWith('https://')) { - const wssHost = host.replace('https', 'wss'); + if (targetURL.origin.startsWith('https://')) { + const wssHost = targetURL.origin.replace('https', 'wss'); this.data[wssHost] = comparableCert; } }; - isExisting = (targetURL: string) => { - return Object.prototype.hasOwnProperty.call(this.data, urlUtils.getHost(targetURL)); + isExisting = (targetURL: URL) => { + return Object.prototype.hasOwnProperty.call(this.data, targetURL.origin); }; - isTrusted = (targetURL: string, certificate: Certificate) => { - const host = urlUtils.getHost(targetURL); + isTrusted = (targetURL: URL, certificate: Certificate) => { if (!this.isExisting(targetURL)) { return false; } - return areEqual(this.data[host], comparableCertificate(certificate)); + return areEqual(this.data[targetURL.origin], comparableCertificate(certificate)); }; - isExplicitlyUntrusted = (targetURL: string) => { + isExplicitlyUntrusted = (targetURL: URL) => { // Whether or not the certificate was explicitly marked as untrusted by // clicking "Don't ask again" checkbox before cancelling the connection. - const host = urlUtils.getHost(targetURL); - const dontTrust = this.data[host]?.dontTrust; + const dontTrust = this.data[targetURL.origin]?.dontTrust; return dontTrust === undefined ? false : dontTrust; } } diff --git a/src/main/contextMenu.ts b/src/main/contextMenu.ts index 98fa6168..ae4425c3 100644 --- a/src/main/contextMenu.ts +++ b/src/main/contextMenu.ts @@ -5,14 +5,14 @@ import {BrowserView, BrowserWindow, ContextMenuParams, Event} from 'electron'; import electronContextMenu, {Options} from 'electron-context-menu'; -import urlUtils from 'common/utils/url'; +import {parseURL} from 'common/utils/url'; const defaultMenuOptions = { shouldShowMenu: (e: Event, p: ContextMenuParams) => { const isInternalLink = p.linkURL.endsWith('#') && p.linkURL.slice(0, -1) === p.pageURL; let isInternalSrc; try { - const srcurl = urlUtils.parseURL(p.srcURL); + const srcurl = parseURL(p.srcURL); isInternalSrc = srcurl?.protocol === 'file:'; } catch (err) { isInternalSrc = false; diff --git a/src/main/trustedOrigins.test.js b/src/main/trustedOrigins.test.js index 6b43e14f..a9d27cfe 100644 --- a/src/main/trustedOrigins.test.js +++ b/src/main/trustedOrigins.test.js @@ -80,16 +80,16 @@ describe('Trusted Origins', () => { expect(tos.data.size).toBe(2); }); it('should say ok if the permission is set', () => { - expect(tos.checkPermission('https://mattermost.com', BASIC_AUTH_PERMISSION)).toBe(true); + expect(tos.checkPermission(new URL('https://mattermost.com'), BASIC_AUTH_PERMISSION)).toBe(true); }); it('should say ko if the permission is set to false', () => { - expect(tos.checkPermission('https://notmattermost.com', BASIC_AUTH_PERMISSION)).toBe(false); + expect(tos.checkPermission(new URL('https://notmattermost.com'), BASIC_AUTH_PERMISSION)).toBe(false); }); it('should say ko if the uri is not set', () => { - expect(tos.checkPermission('https://undefined.com', BASIC_AUTH_PERMISSION)).toBe(undefined); + expect(tos.checkPermission(new URL('https://undefined.com'), BASIC_AUTH_PERMISSION)).toBe(undefined); }); it('should say null if the permission is unknown', () => { - expect(tos.checkPermission('https://mattermost.com')).toBe(null); + expect(tos.checkPermission(new URL('https://mattermost.com'))).toBe(null); }); }); @@ -105,9 +105,9 @@ describe('Trusted Origins', () => { const tos = mockTOS('permission_test', JSON.stringify(value)); tos.load(); it('deleting revokes access', () => { - expect(tos.checkPermission('https://mattermost.com', BASIC_AUTH_PERMISSION)).toBe(true); - tos.delete('https://mattermost.com'); - expect(tos.checkPermission('https://mattermost.com', BASIC_AUTH_PERMISSION)).toBe(undefined); + expect(tos.checkPermission(new URL('https://mattermost.com'), BASIC_AUTH_PERMISSION)).toBe(true); + tos.delete(new URL('https://mattermost.com')); + expect(tos.checkPermission(new URL('https://mattermost.com'), BASIC_AUTH_PERMISSION)).toBe(undefined); }); }); }); diff --git a/src/main/trustedOrigins.ts b/src/main/trustedOrigins.ts index e5e2e702..e6913f03 100644 --- a/src/main/trustedOrigins.ts +++ b/src/main/trustedOrigins.ts @@ -11,7 +11,6 @@ import {TrustedOrigin, PermissionType} from 'types/trustedOrigin'; import {UPDATE_PATHS} from 'common/communication'; import {Logger} from 'common/log'; -import urlUtils from 'common/utils/url'; import * as Validator from 'common/Validator'; import {trustedOriginsStoreFile} from './constants'; @@ -64,7 +63,7 @@ export class TrustedOriginsStore { // if permissions or targetUrl are invalid, this function will throw an error // this function stablishes all the permissions at once, overwriting whatever was before // to enable just one permission use addPermission instead. - set = (targetURL: string, permissions: Record) => { + set = (targetURL: URL, permissions: Record) => { if (!this.data) { return; } @@ -72,45 +71,29 @@ export class TrustedOriginsStore { if (!validPermissions) { throw new Error(`Invalid permissions set for trusting ${targetURL}`); } - this.data.set(urlUtils.getHost(targetURL), validPermissions); + this.data.set(targetURL.origin, validPermissions); }; // enables usage of `targetURL` for `permission` - addPermission = (targetURL: string, permission: PermissionType) => { - const origin = urlUtils.getHost(targetURL); - this.set(origin, {[permission]: true}); + addPermission = (targetURL: URL, permission: PermissionType) => { + this.set(targetURL, {[permission]: true}); } - delete = (targetURL: string) => { - let host; - try { - host = urlUtils.getHost(targetURL); - this.data?.delete(host); - } catch { - return false; - } - return true; + delete = (targetURL: URL) => { + return this.data?.delete(targetURL.origin); } - isExisting = (targetURL: string) => { - return this.data?.has(urlUtils.getHost(targetURL)) || false; + isExisting = (targetURL: URL) => { + return this.data?.has(targetURL.origin) || false; }; - // if user hasn't set his preferences, it will return null (falsy) - checkPermission = (targetURL: string, permission: PermissionType) => { + checkPermission = (targetURL: URL, permission: PermissionType) => { if (!permission) { log.error(`Missing permission request on ${targetURL}`); return null; } - let origin; - try { - origin = urlUtils.getHost(targetURL); - } catch (e) { - log.error(`invalid host to retrieve permissions: ${targetURL}: ${e}`); - return null; - } - const urlPermissions = this.data?.get(origin); + const urlPermissions = this.data?.get(targetURL.origin); return urlPermissions ? urlPermissions[permission] : undefined; } } diff --git a/src/main/utils.test.js b/src/main/utils.test.js index 90de8137..87575064 100644 --- a/src/main/utils.test.js +++ b/src/main/utils.test.js @@ -107,11 +107,11 @@ describe('main/utils', () => { describe('shouldHaveBackBar', () => { it('should have back bar for custom logins', () => { - expect(Utils.shouldHaveBackBar('https://server-1.com', 'https://server-1.com/login/sso/saml')).toBe(true); + expect(Utils.shouldHaveBackBar(new URL('https://server-1.com'), new URL('https://server-1.com/login/sso/saml'))).toBe(true); }); it('should not have back bar for regular login', () => { - expect(Utils.shouldHaveBackBar('https://server-1.com', 'https://server-1.com/login')).toBe(false); + expect(Utils.shouldHaveBackBar(new URL('https://server-1.com'), new URL('https://server-1.com/login'))).toBe(false); }); }); diff --git a/src/main/utils.ts b/src/main/utils.ts index 7f5f4f07..4f5389e9 100644 --- a/src/main/utils.ts +++ b/src/main/utils.ts @@ -15,8 +15,8 @@ import {app, BrowserWindow} from 'electron'; import {Args} from 'types/args'; import {BACK_BAR_HEIGHT, customLoginRegexPaths, PRODUCTION, TAB_BAR_HEIGHT} from 'common/utils/constants'; -import UrlUtils from 'common/utils/url'; import Utils from 'common/utils/util'; +import {isAdminUrl, isPluginUrl, isTeamUrl, isUrlType, parseURL} from 'common/utils/url'; export function isInsideRectangle(container: Electron.Rectangle, rect: Electron.Rectangle) { return container.x <= rect.x && container.y <= rect.y && container.width >= rect.width && container.height >= rect.height; @@ -48,11 +48,11 @@ export function getAdjustedWindowBoundaries(width: number, height: number, hasBa }; } -export function shouldHaveBackBar(serverUrl: URL | string, inputURL: URL | string) { - if (UrlUtils.isUrlType('login', serverUrl, inputURL)) { - const serverURL = UrlUtils.parseURL(serverUrl); +export function shouldHaveBackBar(serverUrl: URL, inputURL: URL) { + if (isUrlType('login', serverUrl, inputURL)) { + const serverURL = parseURL(serverUrl); const subpath = serverURL ? serverURL.pathname : ''; - const parsedURL = UrlUtils.parseURL(inputURL); + const parsedURL = parseURL(inputURL); if (!parsedURL) { return false; } @@ -67,7 +67,7 @@ export function shouldHaveBackBar(serverUrl: URL | string, inputURL: URL | strin return false; } - return !UrlUtils.isTeamUrl(serverUrl, inputURL) && !UrlUtils.isAdminUrl(serverUrl, inputURL) && !UrlUtils.isPluginUrl(serverUrl, inputURL); + return !isTeamUrl(serverUrl, inputURL) && !isAdminUrl(serverUrl, inputURL) && !isPluginUrl(serverUrl, inputURL); } export function getLocalURLString(urlPath: string, query?: Map, isMain?: boolean) { diff --git a/src/main/views/MattermostView.ts b/src/main/views/MattermostView.ts index 114035c7..0b336cc1 100644 --- a/src/main/views/MattermostView.ts +++ b/src/main/views/MattermostView.ts @@ -7,7 +7,6 @@ import {BrowserViewConstructorOptions, Event, Input} from 'electron/main'; import {EventEmitter} from 'events'; import {RELOAD_INTERVAL, MAX_SERVER_RETRIES, SECOND, MAX_LOADING_SCREEN_SECONDS} from 'common/utils/constants'; -import urlUtils from 'common/utils/url'; import AppState from 'common/appState'; import { LOAD_RETRY, @@ -23,6 +22,7 @@ import { } from 'common/communication'; import ServerManager from 'common/servers/serverManager'; import {Logger} from 'common/log'; +import {isInternalURL, parseURL} from 'common/utils/url'; import {TabView} from 'common/tabs/TabView'; import MainWindow from 'main/windows/mainWindow'; @@ -114,7 +114,7 @@ export class MattermostView extends EventEmitter { return this.loggedIn; } get currentURL() { - return this.view.webContents.getURL(); + return parseURL(this.view.webContents.getURL()); } get webContentsId() { return this.view.webContents.id; @@ -129,8 +129,8 @@ export class MattermostView extends EventEmitter { // If we're logging in from a different tab, force a reload if (loggedIn && - this.currentURL !== this.tab.url.toString() && - !this.currentURL.startsWith(this.tab.url.toString()) + this.currentURL?.toString() !== this.tab.url.toString() && + !this.currentURL?.toString().startsWith(this.tab.url.toString()) ) { this.reload(); } @@ -149,7 +149,7 @@ export class MattermostView extends EventEmitter { } updateHistoryButton = () => { - if (urlUtils.parseURL(this.currentURL)?.toString() === this.tab.url.toString()) { + if (this.currentURL?.toString() === this.tab.url.toString()) { this.view.webContents.clearHistory(); this.atRoot = true; } else { @@ -165,7 +165,7 @@ export class MattermostView extends EventEmitter { let loadURL: string; if (someURL) { - const parsedURL = urlUtils.parseURL(someURL); + const parsedURL = parseURL(someURL); if (parsedURL) { loadURL = parsedURL.toString(); } else { @@ -198,6 +198,9 @@ export class MattermostView extends EventEmitter { if (!mainWindow) { return; } + if (!this.currentURL) { + return; + } if (this.isVisible) { return; } @@ -435,7 +438,7 @@ export class MattermostView extends EventEmitter { this.removeLoading = setTimeout(this.setInitialized, MAX_LOADING_SCREEN_SECONDS, true); this.emit(LOAD_SUCCESS, this.id, loadURL); const mainWindow = MainWindow.get(); - if (mainWindow) { + if (mainWindow && this.currentURL) { this.setBounds(getWindowBoundaries(mainWindow, shouldHaveBackBar(this.tab.url || '', this.currentURL))); } }; @@ -472,8 +475,12 @@ export class MattermostView extends EventEmitter { if (!mainWindow) { return; } + const parsedURL = parseURL(url); + if (!parsedURL) { + return; + } - if (shouldHaveBackBar(this.tab.url || '', url)) { + if (shouldHaveBackBar(this.tab.url || '', parsedURL)) { this.setBounds(getWindowBoundaries(mainWindow, true)); MainWindow.sendToRenderer(TOGGLE_BACK_BUTTON, true); this.log.debug('show back button'); @@ -486,10 +493,11 @@ export class MattermostView extends EventEmitter { private handleUpdateTarget = (e: Event, url: string) => { this.log.silly('handleUpdateTarget', url); - if (url && !urlUtils.isInternalURL(urlUtils.parseURL(url), this.tab.server.url)) { - this.emit(UPDATE_TARGET_URL, url); - } else { + const parsedURL = parseURL(url); + if (parsedURL && isInternalURL(parsedURL, this.tab.server.url)) { this.emit(UPDATE_TARGET_URL); + } else { + this.emit(UPDATE_TARGET_URL, url); } } diff --git a/src/main/views/viewManager.ts b/src/main/views/viewManager.ts index ecc1697b..d8ed9ad7 100644 --- a/src/main/views/viewManager.ts +++ b/src/main/views/viewManager.ts @@ -29,11 +29,11 @@ import { } from 'common/communication'; import Config from 'common/config'; import {Logger} from 'common/log'; -import urlUtils from 'common/utils/url'; import Utils from 'common/utils/util'; import {MattermostServer} from 'common/servers/MattermostServer'; import ServerManager from 'common/servers/serverManager'; import {TabView, TAB_MESSAGING} from 'common/tabs/TabView'; +import {parseURL} from 'common/utils/url'; import {localizeMessage} from 'main/i18nManager'; import MainWindow from 'main/windows/mainWindow'; @@ -174,7 +174,7 @@ export class ViewManager { handleDeepLink = (url: string | URL) => { if (url) { - const parsedURL = urlUtils.parseURL(url)!; + const parsedURL = parseURL(url)!; const tabView = ServerManager.lookupTabByURL(parsedURL, true); if (tabView) { const urlWithSchema = `${tabView.url.origin}${parsedURL.pathname}${parsedURL.search}`; @@ -471,11 +471,17 @@ export class ViewManager { log.debug('handleBrowserHistoryPush', {viewId, pathName}); const currentView = this.getView(viewId); - const cleanedPathName = urlUtils.cleanPathName(currentView?.tab.server.url.pathname || '', pathName); - const redirectedviewId = ServerManager.lookupTabByURL(`${currentView?.tab.server.url.toString().replace(/\/$/, '')}${cleanedPathName}`)?.id || viewId; + if (!currentView) { + return; + } + let cleanedPathName = pathName; + if (currentView.tab.server.url.pathname !== '/' && pathName.startsWith(currentView.tab.server.url.pathname)) { + cleanedPathName = pathName.replace(currentView.tab.server.url.pathname, ''); + } + const redirectedviewId = ServerManager.lookupTabByURL(`${currentView.tab.server.url.toString().replace(/\/$/, '')}${cleanedPathName}`)?.id || viewId; if (this.isViewClosed(redirectedviewId)) { // If it's a closed view, just open it and stop - this.openClosedTab(redirectedviewId, `${currentView?.tab.server.url}${cleanedPathName}`); + this.openClosedTab(redirectedviewId, `${currentView.tab.server.url}${cleanedPathName}`); return; } let redirectedView = this.getView(redirectedviewId) || currentView; @@ -540,7 +546,7 @@ export class ViewManager { log.debug('handleSetCurrentViewBounds', newBounds); const currentView = this.getCurrentView(); - if (currentView) { + if (currentView && currentView.currentURL) { const adjustedBounds = getAdjustedWindowBoundaries(newBounds.width, newBounds.height, shouldHaveBackBar(currentView.tab.url, currentView.currentURL)); currentView.setBounds(adjustedBounds); } diff --git a/src/main/views/webContentEvents.test.js b/src/main/views/webContentEvents.test.js index 5935e79a..c59c9268 100644 --- a/src/main/views/webContentEvents.test.js +++ b/src/main/views/webContentEvents.test.js @@ -5,8 +5,6 @@ import {shell, BrowserWindow} from 'electron'; -import urlUtils from 'common/utils/url'; - import ContextMenu from 'main/contextMenu'; import ViewManager from 'main/views/viewManager'; @@ -40,27 +38,6 @@ jest.mock('common/config', () => ({ spellcheck: true, })); -jest.mock('common/utils/url', () => ({ - parseURL: (url) => { - try { - return new URL(url); - } catch (e) { - return null; - } - }, - getView: jest.fn(), - isTeamUrl: jest.fn(), - isAdminUrl: jest.fn(), - isTrustedPopupWindow: jest.fn(), - isTrustedURL: jest.fn(), - isCustomLoginURL: jest.fn(), - isInternalURL: jest.fn(), - isValidURI: jest.fn(), - isPluginUrl: jest.fn(), - isManagedResource: jest.fn(), - isChannelExportUrl: jest.fn(), -})); - jest.mock('main/app/utils', () => ({ flushCookiesStore: jest.fn(), })); @@ -97,13 +74,11 @@ describe('main/views/webContentsEvents', () => { }); it('should allow navigation when url isTeamURL', () => { - urlUtils.isTeamUrl.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(serverURL)); willNavigate(event, 'http://server-1.com/subpath'); expect(event.preventDefault).not.toBeCalled(); }); it('should allow navigation when url isAdminURL', () => { - urlUtils.isAdminUrl.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}admin_console`)); willNavigate(event, 'http://server-1.com/admin_console/subpath'); expect(event.preventDefault).not.toBeCalled(); }); @@ -116,11 +91,15 @@ describe('main/views/webContentsEvents', () => { }); it('should allow navigation when isCustomLoginURL', () => { - urlUtils.isCustomLoginURL.mockImplementation((parsedURL) => parsedURL.toString().startsWith('http://loginurl.com/login')); - willNavigate(event, 'http://loginurl.com/login/oauth'); + willNavigate(event, 'http://server-1.com/oauth/authorize'); expect(event.preventDefault).not.toBeCalled(); }); + it('should not allow navigation when isCustomLoginURL is external', () => { + willNavigate(event, 'http://loginurl.com/oauth/authorize'); + expect(event.preventDefault).toBeCalled(); + }); + it('should allow navigation when protocol is mailto', () => { willNavigate(event, 'mailto:test@mattermost.com'); expect(event.preventDefault).not.toBeCalled(); @@ -133,7 +112,6 @@ describe('main/views/webContentsEvents', () => { }); it('should allow navigation when it isChannelExportUrl', () => { - urlUtils.isChannelExportUrl.mockImplementation((serverURL, parsedURL) => parsedURL.toString().includes('/plugins/com.mattermost.plugin-channel-export/api/v1/export')); willNavigate(event, 'http://server-1.com/plugins/com.mattermost.plugin-channel-export/api/v1/export'); expect(event.preventDefault).not.toBeCalled(); }); @@ -150,9 +128,6 @@ describe('main/views/webContentsEvents', () => { beforeEach(() => { webContentsEventManager.getServerURLFromWebContentsId = jest.fn().mockImplementation(() => new URL('http://server-1.com')); - urlUtils.isTrustedURL.mockReturnValue(true); - urlUtils.isInternalURL.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(serverURL)); - urlUtils.isCustomLoginURL.mockImplementation((parsedURL) => parsedURL.toString().startsWith('http://loginurl.com/login')); }); afterEach(() => { @@ -162,7 +137,7 @@ describe('main/views/webContentsEvents', () => { it('should add custom login entry on custom login URL', () => { webContentsEventManager.customLogins[1] = {inProgress: false}; - didStartNavigation(event, 'http://loginurl.com/login/oauth'); + didStartNavigation(event, 'http://server-1.com/oauth/authorize'); expect(webContentsEventManager.customLogins[1]).toStrictEqual({inProgress: true}); }); @@ -178,12 +153,7 @@ describe('main/views/webContentsEvents', () => { const newWindow = webContentsEventManager.generateNewWindowListener(1, true); beforeEach(() => { - urlUtils.isValidURI.mockReturnValue(true); webContentsEventManager.getServerURLFromWebContentsId = jest.fn().mockImplementation(() => new URL('http://server-1.com')); - urlUtils.isTeamUrl.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}myteam`)); - urlUtils.isAdminUrl.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}admin_console`)); - urlUtils.isPluginUrl.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}myplugin`)); - urlUtils.isManagedResource.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}trusted`)); BrowserWindow.mockImplementation(() => ({ once: jest.fn(), @@ -212,7 +182,6 @@ describe('main/views/webContentsEvents', () => { }); it('should open invalid URIs in browser', () => { - urlUtils.isValidURI.mockReturnValue(false); expect(newWindow({url: 'https://google.com/?^'})).toStrictEqual({action: 'deny'}); expect(shell.openExternal).toBeCalledWith('https://google.com/?^'); }); @@ -258,7 +227,7 @@ describe('main/views/webContentsEvents', () => { }); it('should open popup window for plugins', () => { - expect(newWindow({url: 'http://server-1.com/myplugin/login'})).toStrictEqual({action: 'deny'}); + expect(newWindow({url: 'http://server-1.com/plugins/myplugin/login'})).toStrictEqual({action: 'deny'}); expect(webContentsEventManager.popupWindow).toBeTruthy(); }); @@ -268,7 +237,6 @@ describe('main/views/webContentsEvents', () => { }); it('should open external URIs in browser', () => { - urlUtils.isValidURI.mockReturnValue(false); expect(newWindow({url: 'https://google.com'})).toStrictEqual({action: 'deny'}); expect(shell.openExternal).toBeCalledWith('https://google.com'); }); diff --git a/src/main/views/webContentEvents.ts b/src/main/views/webContentEvents.ts index 189d6abf..97f0861e 100644 --- a/src/main/views/webContentEvents.ts +++ b/src/main/views/webContentEvents.ts @@ -5,11 +5,26 @@ import {BrowserWindow, session, shell, WebContents} from 'electron'; import Config from 'common/config'; import {Logger} from 'common/log'; -import urlUtils from 'common/utils/url'; +import ServerManager from 'common/servers/serverManager'; +import { + isAdminUrl, + isCallsPopOutURL, + isChannelExportUrl, + isCustomLoginURL, + isHelpUrl, + isImageProxyUrl, + isInternalURL, + isManagedResource, + isPluginUrl, + isPublicFilesUrl, + isTeamUrl, + isTrustedURL, + isValidURI, + parseURL, +} from 'common/utils/url'; import {flushCookiesStore} from 'main/app/utils'; import ContextMenu from 'main/contextMenu'; -import ServerManager from 'common/servers/serverManager'; import MainWindow from 'main/windows/mainWindow'; import ViewManager from 'main/views/viewManager'; @@ -73,18 +88,18 @@ export class WebContentsEventManager { return (event: Event, url: string) => { this.log(webContentsId).debug('will-navigate', url); - const parsedURL = urlUtils.parseURL(url)!; + const parsedURL = parseURL(url)!; const serverURL = this.getServerURLFromWebContentsId(webContentsId); - if (serverURL && (urlUtils.isTeamUrl(serverURL, parsedURL) || urlUtils.isAdminUrl(serverURL, parsedURL) || this.isTrustedPopupWindow(webContentsId))) { + if (serverURL && (isTeamUrl(serverURL, parsedURL) || isAdminUrl(serverURL, parsedURL) || this.isTrustedPopupWindow(webContentsId))) { return; } - if (serverURL && urlUtils.isChannelExportUrl(serverURL, parsedURL)) { + if (serverURL && isChannelExportUrl(serverURL, parsedURL)) { return; } - if (serverURL && urlUtils.isCustomLoginURL(parsedURL, serverURL)) { + if (serverURL && isCustomLoginURL(parsedURL, serverURL)) { return; } if (parsedURL.protocol === 'mailto:') { @@ -96,7 +111,7 @@ export class WebContentsEventManager { } const callID = CallsWidgetWindow.callID; - if (serverURL && callID && urlUtils.isCallsPopOutURL(serverURL, parsedURL, callID)) { + if (serverURL && callID && isCallsPopOutURL(serverURL, parsedURL, callID)) { return; } @@ -109,16 +124,16 @@ export class WebContentsEventManager { return (event: Event, url: string) => { this.log(webContentsId).debug('did-start-navigation', url); - const parsedURL = urlUtils.parseURL(url)!; + const parsedURL = parseURL(url)!; const serverURL = this.getServerURLFromWebContentsId(webContentsId); - if (!serverURL || !urlUtils.isTrustedURL(parsedURL, serverURL)) { + if (!serverURL || !isTrustedURL(parsedURL, serverURL)) { return; } - if (serverURL && urlUtils.isCustomLoginURL(parsedURL, serverURL)) { + if (serverURL && isCustomLoginURL(parsedURL, serverURL)) { this.customLogins[webContentsId].inProgress = true; - } else if (serverURL && this.customLogins[webContentsId].inProgress && urlUtils.isInternalURL(serverURL || new URL(''), parsedURL)) { + } else if (serverURL && this.customLogins[webContentsId].inProgress && isInternalURL(serverURL || new URL(''), parsedURL)) { this.customLogins[webContentsId].inProgress = false; } }; @@ -133,7 +148,7 @@ export class WebContentsEventManager { return (details: Electron.HandlerDetails): {action: 'deny' | 'allow'} => { this.log(webContentsId).debug('new-window', details.url); - const parsedURL = urlUtils.parseURL(details.url); + const parsedURL = parseURL(details.url); if (!parsedURL) { this.log(webContentsId).warn(`Ignoring non-url ${details.url}`); return {action: 'deny'}; @@ -152,7 +167,7 @@ export class WebContentsEventManager { // Check for valid URL // Let the browser handle invalid URIs - if (!urlUtils.isValidURI(details.url)) { + if (!isValidURI(details.url)) { shell.openExternal(details.url); return {action: 'deny'}; } @@ -164,31 +179,30 @@ export class WebContentsEventManager { } // Public download links case - // TODO: We might be handling different types differently in the future, for now // we are going to mimic the browser and just pop a new browser window for public links - if (parsedURL.pathname.match(/^(\/api\/v[3-4]\/public)*\/files\//)) { + if (isPublicFilesUrl(serverURL, parsedURL)) { shell.openExternal(details.url); return {action: 'deny'}; } // Image proxy case - if (parsedURL.pathname.match(/^\/api\/v[3-4]\/image/)) { + if (isImageProxyUrl(serverURL, parsedURL)) { shell.openExternal(details.url); return {action: 'deny'}; } - if (parsedURL.pathname.match(/^\/help\//)) { + if (isHelpUrl(serverURL, parsedURL)) { // Help links case // continue to open special case internal urls in default browser shell.openExternal(details.url); return {action: 'deny'}; } - if (urlUtils.isTeamUrl(serverURL, parsedURL, true)) { + if (isTeamUrl(serverURL, parsedURL, true)) { ViewManager.handleDeepLink(parsedURL); return {action: 'deny'}; } - if (urlUtils.isAdminUrl(serverURL, parsedURL)) { + if (isAdminUrl(serverURL, parsedURL)) { this.log(webContentsId).info(`${details.url} is an admin console page, preventing to open a new window`); return {action: 'deny'}; } @@ -198,7 +212,7 @@ export class WebContentsEventManager { } // TODO: move popups to its own and have more than one. - if (urlUtils.isPluginUrl(serverURL, parsedURL) || urlUtils.isManagedResource(serverURL, parsedURL)) { + if (isPluginUrl(serverURL, parsedURL) || isManagedResource(serverURL, parsedURL)) { let popup: BrowserWindow; if (this.popupWindow) { this.popupWindow.win.once('ready-to-show', () => { @@ -224,13 +238,13 @@ export class WebContentsEventManager { popup = this.popupWindow.win; popup.webContents.on('will-redirect', (event, url) => { - const parsedURL = urlUtils.parseURL(url); + const parsedURL = parseURL(url); if (!parsedURL) { event.preventDefault(); return; } - if (urlUtils.isInternalURL(serverURL, parsedURL) && !urlUtils.isPluginUrl(serverURL, parsedURL) && !urlUtils.isManagedResource(serverURL, parsedURL)) { + if (isInternalURL(serverURL, parsedURL) && !isPluginUrl(serverURL, parsedURL) && !isManagedResource(serverURL, parsedURL)) { event.preventDefault(); } }); @@ -247,7 +261,7 @@ export class WebContentsEventManager { popup.once('ready-to-show', () => popup.show()); - if (urlUtils.isManagedResource(serverURL, parsedURL)) { + if (isManagedResource(serverURL, parsedURL)) { popup.loadURL(details.url); } else { // currently changing the userAgent for popup windows to allow plugins to go through google's oAuth @@ -261,7 +275,7 @@ export class WebContentsEventManager { } const otherServerURL = ServerManager.lookupTabByURL(parsedURL); - if (otherServerURL && urlUtils.isTeamUrl(otherServerURL.server.url, parsedURL, true)) { + if (otherServerURL && isTeamUrl(otherServerURL.server.url, parsedURL, true)) { ViewManager.handleDeepLink(parsedURL); return {action: 'deny'}; } diff --git a/src/main/windows/callsWidgetWindow.ts b/src/main/windows/callsWidgetWindow.ts index c3e76b13..194170d4 100644 --- a/src/main/windows/callsWidgetWindow.ts +++ b/src/main/windows/callsWidgetWindow.ts @@ -21,7 +21,7 @@ import {getLocalPreload, openScreensharePermissionsSettingsMacOS, resetScreensha import {Logger} from 'common/log'; import {CALLS_PLUGIN_ID, MINIMUM_CALLS_WIDGET_HEIGHT, MINIMUM_CALLS_WIDGET_WIDTH} from 'common/utils/constants'; import Utils from 'common/utils/util'; -import urlUtils, {getFormattedPathName} from 'common/utils/url'; +import {getFormattedPathName, isCallsPopOutURL, parseURL} from 'common/utils/url'; import { BROWSER_HISTORY_PUSH, CALLS_ERROR, @@ -90,7 +90,7 @@ export class CallsWidgetWindow { */ getURL = () => { - return this.win && urlUtils.parseURL(this.win?.webContents.getURL()); + return this.win && parseURL(this.win?.webContents.getURL()); } isCallsWidget = (webContentsId: number) => { @@ -101,7 +101,7 @@ export class CallsWidgetWindow { if (!this.mainView) { return undefined; } - const u = urlUtils.parseURL(this.mainView.tab.server.url.toString()) as URL; + const u = parseURL(this.mainView.tab.server.url.toString()) as URL; u.pathname = getFormattedPathName(u.pathname); u.pathname += `plugins/${CALLS_PLUGIN_ID}/standalone/widget.html`; @@ -267,7 +267,11 @@ export class CallsWidgetWindow { return {action: 'deny' as const}; } - if (urlUtils.isCallsPopOutURL(this.mainView?.tab.server.url, url, this.options?.callID)) { + const parsedURL = parseURL(url); + if (!parsedURL) { + return {action: 'deny' as const}; + } + if (isCallsPopOutURL(this.mainView?.tab.server.url, parsedURL, this.options?.callID)) { return { action: 'allow' as const, overrideBrowserWindowOptions: { diff --git a/src/renderer/components/ConfigureServer.tsx b/src/renderer/components/ConfigureServer.tsx index 8270e4ba..8738b542 100644 --- a/src/renderer/components/ConfigureServer.tsx +++ b/src/renderer/components/ConfigureServer.tsx @@ -7,6 +7,9 @@ import classNames from 'classnames'; import {MattermostTeam} from 'types/config'; +import {isValidURL, parseURL} from 'common/utils/url'; +import {MODAL_TRANSITION_TIMEOUT} from 'common/utils/constants'; + import womanLaptop from 'renderer/assets/svg/womanLaptop.svg'; import Header from 'renderer/components/Header'; @@ -14,9 +17,6 @@ import Input, {STATUS, SIZE} from 'renderer/components/Input'; import LoadingBackground from 'renderer/components/LoadingScreen/LoadingBackground'; import SaveButton from 'renderer/components/SaveButton/SaveButton'; -import {MODAL_TRANSITION_TIMEOUT} from 'common/utils/constants'; -import urlUtils from 'common/utils/url'; - import 'renderer/css/components/Button.scss'; import 'renderer/css/components/ConfigureServer.scss'; import 'renderer/css/components/LoadingScreen.css'; @@ -72,7 +72,7 @@ function ConfigureServer({ }, []); const checkProtocolInURL = (checkURL: string): Promise => { - if (urlUtils.startsWithProtocol(checkURL)) { + if (isValidURL(checkURL)) { return Promise.resolve(checkURL); } return window.desktop.modals.pingDomain(checkURL). @@ -115,21 +115,21 @@ function ConfigureServer({ }); } - if (!urlUtils.startsWithProtocol(fullURL)) { - return formatMessage({ - id: 'renderer.components.newTeamModal.error.urlNeedsHttp', - defaultMessage: 'URL should start with http:// or https://.', - }); - } - - if (!urlUtils.isValidURL(fullURL)) { + if (!parseURL(fullURL)) { return formatMessage({ id: 'renderer.components.newTeamModal.error.urlIncorrectFormatting', defaultMessage: 'URL is not formatted correctly.', }); } - if (currentTeams.find(({url: existingURL}) => existingURL === fullURL)) { + if (!isValidURL(fullURL)) { + return formatMessage({ + id: 'renderer.components.newTeamModal.error.urlNeedsHttp', + defaultMessage: 'URL should start with http:// or https://.', + }); + } + + if (currentTeams.find(({url: existingURL}) => parseURL(existingURL)?.toString === parseURL(fullURL)?.toString())) { return formatMessage({ id: 'renderer.components.newTeamModal.error.serverUrlExists', defaultMessage: 'A server with the same URL already exists.', diff --git a/src/renderer/components/NewTeamModal.tsx b/src/renderer/components/NewTeamModal.tsx index de10d3f4..e96937c3 100644 --- a/src/renderer/components/NewTeamModal.tsx +++ b/src/renderer/components/NewTeamModal.tsx @@ -8,7 +8,7 @@ import {FormattedMessage, injectIntl, IntlShape} from 'react-intl'; import {MattermostTeam} from 'types/config'; -import urlUtils from 'common/utils/url'; +import {isValidURL} from 'common/utils/url'; type Props = { onClose?: () => void; @@ -124,7 +124,7 @@ class NewTeamModal extends React.PureComponent { /> ); } - if (!urlUtils.isValidURL(this.state.teamUrl.trim())) { + if (!isValidURL(this.state.teamUrl.trim())) { return ( void; @@ -86,7 +86,7 @@ class LoginModal extends React.PureComponent { /> ); } - const tmpURL = urlUtils.parseURL(this.state.request.url); + const tmpURL = parseURL(this.state.request.url); return ( ; @@ -51,14 +51,14 @@ class PermissionModal extends React.PureComponent { } const {url, permission} = this.state; - const originDisplay = url ? urlUtil.getHost(url) : this.props.intl.formatMessage({id: 'renderer.modals.permission.permissionModal.unknownOrigin', defaultMessage: 'unknown origin'}); - const originLink = url ? originDisplay : ''; + const originDisplay = url ? parseURL(url)?.origin : this.props.intl.formatMessage({id: 'renderer.modals.permission.permissionModal.unknownOrigin', defaultMessage: 'unknown origin'}); + const originLink = originDisplay ?? ''; const click = (e: React.MouseEvent) => { e.preventDefault(); let parseUrl; try { - parseUrl = urlUtil.parseURL(originLink); + parseUrl = parseURL(originLink); this.props.openExternalLink(parseUrl!.protocol, originLink); } catch (err) { console.error(`invalid url ${originLink} supplied to externallink: ${err}`);