From 1f85c360dd2040b7fad9d84b524797ed66be0ebc Mon Sep 17 00:00:00 2001 From: mcasimir Date: Fri, 4 Mar 2022 11:38:45 +0100 Subject: [PATCH 01/10] always use loose validation --- .../compass-connect/src/modules/telemetry.js | 2 +- .../src/modules/telemetry.ts | 4 +- .../src/stores/connections-store.ts | 4 +- .../tests/connection.test.ts | 4 +- .../src/stores/store.js | 3 +- packages/compass-metrics/src/modules/rules.js | 2 +- .../src/components/connect-form.tsx | 11 ++---- .../connection-form/src/utils/validation.ts | 39 ++++++++++++------- packages/connection-model/lib/connect.js | 2 +- packages/connection-model/lib/model.js | 6 +-- .../data-service/src/connection-secrets.ts | 8 +++- packages/data-service/src/connection-title.ts | 4 +- packages/data-service/src/data-service.ts | 4 +- .../src/legacy/legacy-connection-model.ts | 21 +++++++--- scripts/import-test-connections.js | 7 +++- 15 files changed, 75 insertions(+), 46 deletions(-) diff --git a/packages/compass-connect/src/modules/telemetry.js b/packages/compass-connect/src/modules/telemetry.js index d6171ad801f..cd32ca3fcb6 100644 --- a/packages/compass-connect/src/modules/telemetry.js +++ b/packages/compass-connect/src/modules/telemetry.js @@ -49,7 +49,7 @@ async function getHostInformation(host) { async function getConnectionData(connectionInfo) { const {connectionOptions: {connectionString, sshTunnel}} = connectionInfo; - const connectionStringData = new ConnectionString(connectionString); + const connectionStringData = new ConnectionString(connectionString, {looseValidation: true}); const hostName = connectionStringData.hosts[0]; const searchParams = connectionStringData.searchParams; diff --git a/packages/compass-connections/src/modules/telemetry.ts b/packages/compass-connections/src/modules/telemetry.ts index b4f7f333345..570b2c776b0 100644 --- a/packages/compass-connections/src/modules/telemetry.ts +++ b/packages/compass-connections/src/modules/telemetry.ts @@ -59,7 +59,9 @@ async function getConnectionData({ }: Pick): Promise< Record > { - const connectionStringData = new ConnectionString(connectionString); + const connectionStringData = new ConnectionString(connectionString, { + looseValidation: true, + }); const hostName = connectionStringData.hosts[0]; const searchParams = connectionStringData.typedSearchParams(); diff --git a/packages/compass-connections/src/stores/connections-store.ts b/packages/compass-connections/src/stores/connections-store.ts index 43ac76cc6f8..6208d46f88a 100644 --- a/packages/compass-connections/src/stores/connections-store.ts +++ b/packages/compass-connections/src/stores/connections-store.ts @@ -43,7 +43,9 @@ function setAppNameParamIfMissing( let connectionStringUrl; try { - connectionStringUrl = new ConnectionString(connectionString); + connectionStringUrl = new ConnectionString(connectionString, { + looseValidation: true, + }); } catch (e) { // } diff --git a/packages/compass-e2e-tests/tests/connection.test.ts b/packages/compass-e2e-tests/tests/connection.test.ts index 42f6c167771..0a5bd7dbad6 100644 --- a/packages/compass-e2e-tests/tests/connection.test.ts +++ b/packages/compass-e2e-tests/tests/connection.test.ts @@ -104,8 +104,8 @@ describe('SRV connectivity', function () { expect(resolutionLogs).to.have.lengthOf(1); const { from, to, resolutionDetails } = resolutionLogs[0]; - const fromCS = new ConnectionString(from); - const toCS = new ConnectionString(to); + const fromCS = new ConnectionString(from, { looseValidation: true }); + const toCS = new ConnectionString(to, { looseValidation: true }); fromCS.searchParams.delete('appname'); toCS.searchParams.delete('appname'); toCS.hosts.sort(); diff --git a/packages/compass-export-to-language/src/stores/store.js b/packages/compass-export-to-language/src/stores/store.js index 1d3ac75fc1b..e07b314ea60 100644 --- a/packages/compass-export-to-language/src/stores/store.js +++ b/packages/compass-export-to-language/src/stores/store.js @@ -34,7 +34,8 @@ function getCurrentlyConnectedUri(dataService) { try { connectionStringUrl = new ConnectionString( - dataService.getConnectionOptions().connectionString + dataService.getConnectionOptions().connectionString, + {looseValidation: true} ); } catch (e) { return ''; diff --git a/packages/compass-metrics/src/modules/rules.js b/packages/compass-metrics/src/modules/rules.js index 972b015334c..0005fae4ef0 100644 --- a/packages/compass-metrics/src/modules/rules.js +++ b/packages/compass-metrics/src/modules/rules.js @@ -6,7 +6,7 @@ const LOCALHOST = /(^localhost)|(^127\.0\.0\.1)/gi; async function getCloudInfoFromDataService(dataService) { try { - const url = new ConnectionString(dataService.getConnectionOptions().connectionString); + const url = new ConnectionString(dataService.getConnectionOptions().connectionString, {looseValidation: true}); const firstServerHostname = (url.hosts[0] || '').split(':')[0]; return await getCloudInfo(firstServerHostname); } catch (e) { diff --git a/packages/connection-form/src/components/connect-form.tsx b/packages/connection-form/src/components/connect-form.tsx index 98637df1984..78cc7b8acb3 100644 --- a/packages/connection-form/src/components/connect-form.tsx +++ b/packages/connection-form/src/components/connect-form.tsx @@ -150,16 +150,11 @@ function ConnectForm({ const callOnSaveConnectionClickedAndStoreErrors = useCallback( async (connectionInfo: ConnectionInfo): Promise => { try { - const formErrors = validateConnectionOptionsErrors( - connectionInfo.connectionOptions, - { looseValidation: false } - ); - if (formErrors.length) { - setErrors(formErrors); - return; - } await onSaveConnectionClicked?.(connectionInfo); } catch (err) { + // save errors are already handled as toast notifications, + // keeping so we don't rely too much on far-away code and leave errors + // uncaught in case that code would change setErrors([ { message: `Unable to save connection: ${(err as Error).message}`, diff --git a/packages/connection-form/src/utils/validation.ts b/packages/connection-form/src/utils/validation.ts index f6ae12de892..35c8a15b6c5 100644 --- a/packages/connection-form/src/utils/validation.ts +++ b/packages/connection-form/src/utils/validation.ts @@ -258,21 +258,18 @@ export function validateConnectionOptionsWarnings( connectionOptions: ConnectionOptions ): ConnectionFormWarning[] { let connectionString: ConnectionString; - let parserWarning: ConnectionFormWarning[] = []; try { - connectionString = new ConnectionString(connectionOptions.connectionString); - } catch (err: any) { - parserWarning = [{ message: err.message }]; connectionString = new ConnectionString( connectionOptions.connectionString, { looseValidation: true, } ); + } catch (err: any) { + return []; } return [ - ...parserWarning, ...validateReadPreferenceWarnings(connectionString), ...validateDeprecatedOptionsWarnings(connectionString), ...validateCertificateValidationWarnings(connectionString), @@ -288,15 +285,28 @@ function validateCertificateValidationWarnings( connectionString: ConnectionString ): ConnectionFormWarning[] { const warnings: ConnectionFormWarning[] = []; - if ( - isSecure(connectionString) && - (connectionString.searchParams.has('tlsInsecure') || - connectionString.searchParams.has('tlsAllowInvalidHostnames') || - connectionString.searchParams.has('tlsAllowInvalidCertificates')) - ) { + if (!isSecure(connectionString)) { + return []; + } + + const tlsInsecure = + connectionString.searchParams.get('tlsInsecure') === 'true'; + const tlsAllowInvalidHostnames = + connectionString.searchParams.get('tlsAllowInvalidHostnames') === 'true'; + const tlsAllowInvalidCertificates = + connectionString.searchParams.get('tlsAllowInvalidCertificates') === 'true'; + + const settings = [ + tlsInsecure ? 'tlsInsecure' : undefined, + tlsAllowInvalidHostnames ? 'tlsAllowInvalidHostnames' : undefined, + tlsAllowInvalidCertificates ? 'tlsAllowInvalidCertificates' : undefined, + ].filter(Boolean); + + if (tlsInsecure || tlsAllowInvalidHostnames || tlsAllowInvalidCertificates) { warnings.push({ - message: - 'Disabling certificate validation is not recommended as it may create a security vulnerability', + message: `Certificate validation is disabled on the TLS settings (${settings.join( + ', ' + )}). For a more secure connection enable certificate validation if possible.`, }); } @@ -392,8 +402,7 @@ function validateTLSAndHostWarnings( if (nonLocalhostsCount && !isSecure(connectionString)) { warnings.push({ - message: - 'Connecting without tls is not recommended as it may create a security vulnerability.', + message: 'For a more secure connection enable tls.', }); } return warnings; diff --git a/packages/connection-model/lib/connect.js b/packages/connection-model/lib/connect.js index 2a01622a3a5..3dd8fff0070 100644 --- a/packages/connection-model/lib/connect.js +++ b/packages/connection-model/lib/connect.js @@ -14,7 +14,7 @@ const { const debug = require('debug')('mongodb-connection-model:connect'); function removeGssapiServiceName(url) { - const uri = new ConnectionString(url); + const uri = new ConnectionString(url, {looseValidation: true}); uri.searchParams.delete('gssapiServiceName'); return uri.toString(); } diff --git a/packages/connection-model/lib/model.js b/packages/connection-model/lib/model.js index 8aaa01737bc..e6cf1117eb6 100644 --- a/packages/connection-model/lib/model.js +++ b/packages/connection-model/lib/model.js @@ -446,7 +446,7 @@ function encodeURIComponentRFC3986(str) { } function setAuthSourceToExternal(url) { - const uri = new ConnectionString(url); + const uri = new ConnectionString(url, {looseValidation: true}); uri.searchParams.set('authSource', '$external'); return uri.toString(); } @@ -1118,7 +1118,7 @@ async function createConnectionFromUrl(url) { hosts: parsed.hosts, // If this is using an srv record, we can just take the original // URL before SRV resolution to get the "hostname". - hostname: isSrvRecord ? new ConnectionString(unescapedUrl).hosts[0] : parsed.hosts[0].host, + hostname: isSrvRecord ? new ConnectionString(unescapedUrl, {looseValidation: true}).hosts[0] : parsed.hosts[0].host, auth: parsed.auth, isSrvRecord }, @@ -1188,7 +1188,7 @@ async function createConnectionFromUrl(url) { // Since the 3.x parser does not recognize loadBalanced as an option, we have to // parse it ourselves. - const loadBalanced = new ConnectionString(unescapedUrl).searchParams.get('loadBalanced'); + const loadBalanced = new ConnectionString(unescapedUrl, {looseValidation: true}).searchParams.get('loadBalanced'); switch (loadBalanced) { case 'true': attrs.loadBalanced = true; diff --git a/packages/data-service/src/connection-secrets.ts b/packages/data-service/src/connection-secrets.ts index 816309f6164..e6f5ae658d0 100644 --- a/packages/data-service/src/connection-secrets.ts +++ b/packages/data-service/src/connection-secrets.ts @@ -26,7 +26,9 @@ export function mergeSecrets( const connectionOptions = connectionInfoWithSecrets.connectionOptions; - const uri = new ConnectionString(connectionOptions.connectionString); + const uri = new ConnectionString(connectionOptions.connectionString, { + looseValidation: true, + }); // can remove the proxyPassword addition once we have NODE-3633 const searchParams = uri.typedSearchParams< MongoClientOptions & { proxyPassword?: string } @@ -83,7 +85,9 @@ export function extractSecrets(connectionInfo: Readonly): { const secrets: ConnectionSecrets = {}; const connectionOptions = connectionInfoWithoutSecrets.connectionOptions; - const uri = new ConnectionString(connectionOptions.connectionString); + const uri = new ConnectionString(connectionOptions.connectionString, { + looseValidation: true, + }); // can remove the proxyPassword addition once we have NODE-3633 const searchParams = uri.typedSearchParams< MongoClientOptions & { proxyPassword?: string } diff --git a/packages/data-service/src/connection-title.ts b/packages/data-service/src/connection-title.ts index d80ed15d000..035d0ab1b81 100644 --- a/packages/data-service/src/connection-title.ts +++ b/packages/data-service/src/connection-title.ts @@ -7,7 +7,9 @@ export function getConnectionTitle(info: ConnectionInfo): string { } try { - const url = new ConnectionString(info.connectionOptions.connectionString); + const url = new ConnectionString(info.connectionOptions.connectionString, { + looseValidation: true, + }); if (url.isSRV) { return url.hosts[0]; } diff --git a/packages/data-service/src/data-service.ts b/packages/data-service/src/data-service.ts index 0b1104a0671..a79c7c36f54 100644 --- a/packages/data-service/src/data-service.ts +++ b/packages/data-service/src/data-service.ts @@ -132,7 +132,9 @@ class DataService extends EventEmitter { } getConnectionString(): ConnectionStringUrl { - return new ConnectionStringUrl(this._connectionOptions.connectionString); + return new ConnectionStringUrl(this._connectionOptions.connectionString, { + looseValidation: true, + }); } getReadPreference(): ReadPreferenceLike { diff --git a/packages/data-service/src/legacy/legacy-connection-model.ts b/packages/data-service/src/legacy/legacy-connection-model.ts index 9d272d619e5..a9c26485395 100644 --- a/packages/data-service/src/legacy/legacy-connection-model.ts +++ b/packages/data-service/src/legacy/legacy-connection-model.ts @@ -33,7 +33,8 @@ function deleteCompassAppNameParam( try { connectionStringUrl = new ConnectionString( - connectionInfo.connectionOptions.connectionString + connectionInfo.connectionOptions.connectionString, + { looseValidation: true } ); } catch { return connectionInfo; @@ -261,7 +262,9 @@ function setConnectionStringParam( param: K, value: string ) { - const url = new ConnectionString(connectionOptions.connectionString); + const url = new ConnectionString(connectionOptions.connectionString, { + looseValidation: true, + }); url.typedSearchParams().set(param, value); connectionOptions.connectionString = url.toString(); } @@ -270,7 +273,9 @@ function modelSslPropertiesToConnectionOptions( driverOptions: MongoClientOptions, connectionOptions: ConnectionOptions ): void { - const url = new ConnectionString(connectionOptions.connectionString); + const url = new ConnectionString(connectionOptions.connectionString, { + looseValidation: true, + }); const searchParams = url.typedSearchParams(); if (driverOptions.sslValidate === false) { @@ -431,7 +436,9 @@ function convertSslOptionsToLegacyProperties( options: ConnectionOptions, properties: Partial ): void { - const url = new ConnectionString(options.connectionString); + const url = new ConnectionString(options.connectionString, { + looseValidation: true, + }); const searchParams = url.typedSearchParams(); const tlsCAFile = searchParams.get('tlsCAFile'); const tlsCertificateKeyFile = searchParams.get('tlsCertificateKeyFile'); @@ -461,7 +468,9 @@ function convertSslOptionsToLegacyProperties( } function optionsToSslMethod(options: ConnectionOptions): SslMethod { - const url = new ConnectionString(options.connectionString); + const url = new ConnectionString(options.connectionString, { + looseValidation: true, + }); const searchParams = url.typedSearchParams(); const tls = searchParams.get('tls') || searchParams.get('ssl'); @@ -502,7 +511,7 @@ function optionsToSslMethod(options: ConnectionOptions): SslMethod { // connection won't fail and MONGODB-AWS connections will appear // as unauthenticated. function removeAWSParams(connectionString: string): string { - const url = new ConnectionString(connectionString); + const url = new ConnectionString(connectionString, { looseValidation: true }); const searchParams = url.typedSearchParams(); if (searchParams.get('authMechanism') === 'MONGODB-AWS') { diff --git a/scripts/import-test-connections.js b/scripts/import-test-connections.js index 5ab9d3443b0..9c1b56c49d9 100644 --- a/scripts/import-test-connections.js +++ b/scripts/import-test-connections.js @@ -25,7 +25,9 @@ const buildConnectionString = (scheme, username, password, host, params) => { return ''; } - const url = new ConnectionStringUrl(`${scheme}://${host}/admin`); + const url = new ConnectionStringUrl(`${scheme}://${host}/admin`, { + looseValidation: true, + }); url.username = username; url.password = password; @@ -145,7 +147,8 @@ if (COMPASS_TEST_ANALYTICS_NODE_URL) { if (E2E_TESTS_ATLAS_HOST && E2E_TESTS_ATLAS_X509_PEM_PATH) { const url = new ConnectionStringUrl( - `mongodb+srv://${E2E_TESTS_ATLAS_HOST || ''}/admin` + `mongodb+srv://${E2E_TESTS_ATLAS_HOST || ''}/admin`, + { looseValidation: true } ); url.searchParams.set('authMechanism', 'MONGODB-X509'); From 35c301e7eb30e5dde8e1aab5032f509ba845ba50 Mon Sep 17 00:00:00 2001 From: mcasimir Date: Fri, 4 Mar 2022 12:13:31 +0100 Subject: [PATCH 02/10] rephrase TLS error messages in a more positive way --- .../src/utils/validation.spec.ts | 17 +++++++++++++++-- .../connection-form/src/utils/validation.ts | 13 +++---------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/packages/connection-form/src/utils/validation.spec.ts b/packages/connection-form/src/utils/validation.spec.ts index 5cc1fbc0b0e..555a3c1af05 100644 --- a/packages/connection-form/src/utils/validation.spec.ts +++ b/packages/connection-form/src/utils/validation.spec.ts @@ -345,11 +345,24 @@ describe('validation', function () { }); expect(result[0]).to.deep.equal({ message: - 'Disabling certificate validation is not recommended as it may create a security vulnerability', + 'TLS/SSL certificate validation is disabled. For a more secure connection enable certificate validation if possible.', }); }); }); + it('should not return warnings when certificate validation is enabled', function () { + [ + 'tlsInsecure', + 'tlsAllowInvalidHostnames', + 'tlsAllowInvalidCertificates', + ].forEach((option) => { + const result = validateConnectionOptionsWarnings({ + connectionString: `mongodb+srv://myserver.com?${option}=false`, + }); + expect(result).to.deep.equal([]); + }); + }); + it('should return warnings if unknown readPreference', function () { const result = validateConnectionOptionsWarnings({ connectionString: `mongodb://myserver.com?readPreference=invalidReadPreference`, @@ -438,7 +451,7 @@ describe('validation', function () { expect(result).to.deep.equal([ { message: - 'Connecting without tls is not recommended as it may create a security vulnerability.', + 'Connecting to a remote server without TLS/SSL is not recommended. For a more secure connection enable TLS/SSL if possible.', }, ]); } diff --git a/packages/connection-form/src/utils/validation.ts b/packages/connection-form/src/utils/validation.ts index 35c8a15b6c5..3f0db2d61fa 100644 --- a/packages/connection-form/src/utils/validation.ts +++ b/packages/connection-form/src/utils/validation.ts @@ -296,17 +296,9 @@ function validateCertificateValidationWarnings( const tlsAllowInvalidCertificates = connectionString.searchParams.get('tlsAllowInvalidCertificates') === 'true'; - const settings = [ - tlsInsecure ? 'tlsInsecure' : undefined, - tlsAllowInvalidHostnames ? 'tlsAllowInvalidHostnames' : undefined, - tlsAllowInvalidCertificates ? 'tlsAllowInvalidCertificates' : undefined, - ].filter(Boolean); - if (tlsInsecure || tlsAllowInvalidHostnames || tlsAllowInvalidCertificates) { warnings.push({ - message: `Certificate validation is disabled on the TLS settings (${settings.join( - ', ' - )}). For a more secure connection enable certificate validation if possible.`, + message: `TLS/SSL certificate validation is disabled. For a more secure connection enable certificate validation if possible.`, }); } @@ -402,7 +394,8 @@ function validateTLSAndHostWarnings( if (nonLocalhostsCount && !isSecure(connectionString)) { warnings.push({ - message: 'For a more secure connection enable tls.', + message: + 'Connecting to a remote server without TLS/SSL is not recommended. For a more secure connection enable TLS/SSL if possible.', }); } return warnings; From 4e643cab69912c23cf1845b39e62074a249d5ec2 Mon Sep 17 00:00:00 2001 From: mcasimir Date: Fri, 4 Mar 2022 14:26:59 +0100 Subject: [PATCH 03/10] only update lastUsed if saved previously --- .../src/components/connections.tsx | 3 +- .../src/stores/connections-store.spec.ts | 7 ++- .../src/stores/connections-store.ts | 22 ++++----- .../src/connection-storage.spec.ts | 45 ++++++++++++++++++- .../data-service/src/connection-storage.ts | 22 +++++++++ 5 files changed, 83 insertions(+), 16 deletions(-) diff --git a/packages/compass-connections/src/components/connections.tsx b/packages/compass-connections/src/components/connections.tsx index d3a3e534c3d..7caa66cc6b1 100644 --- a/packages/compass-connections/src/components/connections.tsx +++ b/packages/compass-connections/src/components/connections.tsx @@ -17,7 +17,6 @@ import { createLoggerAndTelemetry } from '@mongodb-js/compass-logging'; import ResizableSidebar from './resizeable-sidebar'; import FormHelp from './form-help/form-help'; import Connecting from './connecting/connecting'; -import type { ConnectionStore } from '../stores/connections-store'; import { useConnections } from '../stores/connections-store'; import { cloneDeep } from 'lodash'; @@ -65,7 +64,7 @@ function Connections({ connectionInfo: ConnectionInfo, dataService: DataService ) => void; - connectionStorage?: ConnectionStore; + connectionStorage?: ConnectionStorage; appName: string; connectFn?: (connectionOptions: ConnectionOptions) => Promise; }): React.ReactElement { diff --git a/packages/compass-connections/src/stores/connections-store.spec.ts b/packages/compass-connections/src/stores/connections-store.spec.ts index cb1aa43b74c..2aa41301d75 100644 --- a/packages/compass-connections/src/stores/connections-store.spec.ts +++ b/packages/compass-connections/src/stores/connections-store.spec.ts @@ -4,8 +4,8 @@ import type { RenderResult } from '@testing-library/react-hooks'; import { renderHook, act } from '@testing-library/react-hooks'; import sinon from 'sinon'; -import type { ConnectionStore } from './connections-store'; import { useConnections } from './connections-store'; +import type { ConnectionStorage } from 'mongodb-data-service'; const noop = (): any => { /* no-op */ @@ -33,20 +33,23 @@ const mockConnections = [ ]; describe('use-connections hook', function () { - let mockConnectionStorage: ConnectionStore; + let mockConnectionStorage: ConnectionStorage; let loadAllSpy: sinon.SinonSpy; let saveSpy: sinon.SinonSpy; let deleteSpy: sinon.SinonSpy; + let loadSpy: sinon.SinonSpy; beforeEach(function () { loadAllSpy = sinon.spy(); saveSpy = sinon.spy(); deleteSpy = sinon.spy(); + loadSpy = sinon.spy(); mockConnectionStorage = { loadAll: loadAllSpy, save: saveSpy, delete: deleteSpy, + load: loadSpy, }; }); diff --git a/packages/compass-connections/src/stores/connections-store.ts b/packages/compass-connections/src/stores/connections-store.ts index 6208d46f88a..532f6aca8eb 100644 --- a/packages/compass-connections/src/stores/connections-store.ts +++ b/packages/compass-connections/src/stores/connections-store.ts @@ -2,6 +2,7 @@ import type { ConnectionInfo, ConnectionOptions, DataService, + ConnectionStorage, } from 'mongodb-data-service'; import { getConnectionTitle } from 'mongodb-data-service'; import { useEffect, useReducer, useRef } from 'react'; @@ -30,11 +31,6 @@ export function createNewConnectionInfo(): ConnectionInfo { }, }; } -export interface ConnectionStore { - loadAll: () => Promise; - save: (connectionInfo: ConnectionInfo) => Promise; - delete: (connectionInfo: ConnectionInfo) => Promise; -} function setAppNameParamIfMissing( connectionString: string, @@ -185,7 +181,7 @@ async function loadConnections( type: 'set-connections'; connections: ConnectionInfo[]; }>, - connectionStorage: ConnectionStore + connectionStorage: ConnectionStorage ) { try { const loadedConnections = await connectionStorage.loadAll(); @@ -209,7 +205,7 @@ export function useConnections({ connectionInfo: ConnectionInfo, dataService: DataService ) => void; - connectionStorage: ConnectionStore; + connectionStorage: ConnectionStorage; connectFn: (connectionOptions: ConnectionOptions) => Promise; appName: string; }): { @@ -265,9 +261,15 @@ export function useConnections({ try { onConnected(connectionInfo, dataService); - // Update lastUsed date as now and save the connection. - connectionInfo.lastUsed = new Date(); - await saveConnectionInfo(connectionInfo); + // if a connection has been saved already we only want to update the lastUsed + // attribute, otherwise we are going to save the entire connection info. + const connectionInfoToBeSaved = + (await connectionStorage.load(connectionInfo.id)) ?? connectionInfo; + + await saveConnectionInfo({ + ...cloneDeep(connectionInfoToBeSaved), + lastUsed: new Date(), + }); } catch (err) { debug( `error occurred connection with id ${connectionInfo.id || ''}: ${ diff --git a/packages/data-service/src/connection-storage.spec.ts b/packages/data-service/src/connection-storage.spec.ts index a1e17f2a26b..e53bbed9bc0 100644 --- a/packages/data-service/src/connection-storage.spec.ts +++ b/packages/data-service/src/connection-storage.spec.ts @@ -62,7 +62,7 @@ function writeFakeConnection( fs.writeFileSync(filePath, JSON.stringify(legacyConnection)); } -describe('ConnectionStorage', function () { +describe.only('ConnectionStorage', function () { let tmpDir: string; beforeEach(function () { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'connection-storage-tests')); @@ -74,7 +74,7 @@ describe('ConnectionStorage', function () { fs.rmdirSync(tmpDir, { recursive: true }); }); - describe('load', function () { + describe('loadAll', function () { it('should load an empty array with no connections', async function () { const connectionStorage = new ConnectionStorage(); const connections = await connectionStorage.loadAll(); @@ -111,6 +111,47 @@ describe('ConnectionStorage', function () { }); }); + describe('load', function () { + it('should return undefined if id is undefined', async function () { + const connectionStorage = new ConnectionStorage(); + expect(await connectionStorage.load(undefined)).to.be.undefined; + expect(await connectionStorage.load('')).to.be.undefined; + }); + + it('should return undefined if a connection does not exist', async function () { + const connectionStorage = new ConnectionStorage(); + const connection = await connectionStorage.load('note-exis-stin-gone'); + expect(connection).to.be.undefined; + }); + + it('should return an existing connection', async function () { + const id = uuid(); + writeFakeConnection(tmpDir, { _id: id }); + const connectionStorage = new ConnectionStorage(); + const connection = await connectionStorage.load(id); + expect(connection).to.deep.equal({ + id, + connectionOptions: { + connectionString: + 'mongodb://localhost:27017/?readPreference=primary&ssl=false', + }, + }); + }); + + it('should convert lastUsed', async function () { + const id = uuid(); + const lastUsed = new Date('2021-10-26T13:51:27.585Z'); + writeFakeConnection(tmpDir, { + _id: id, + lastUsed, + }); + + const connectionStorage = new ConnectionStorage(); + const connection = await connectionStorage.load(id); + expect(connection.lastUsed).to.deep.equal(lastUsed); + }); + }); + describe('save', function () { it('saves a valid connection object', async function () { const id: string = uuid(); diff --git a/packages/data-service/src/connection-storage.ts b/packages/data-service/src/connection-storage.ts index 07865a07379..0022d49a4e3 100644 --- a/packages/data-service/src/connection-storage.ts +++ b/packages/data-service/src/connection-storage.ts @@ -67,6 +67,28 @@ export class ConnectionStorage { const model = await convertConnectionInfoToModel(connectionOptions); model.destroy(); } + + /** + * Updates attributes of the model. + * + * @param {string} id ID of the model to update + * @param {object} attributes Attributes of model to update + */ + async load(id: string): Promise { + if (!id) { + return undefined; + } + + // model.fetch doesn't seem to fail or return any useful info + // to determine if the model exists or not on disk + // this is why here we have to re-load all the connections in + // in order to ensure we can return undefined for a connection id + // that does not exist. + + const connections = await this.loadAll(); + + return connections.find((connection) => id === connection.id); + } } export function promisifyAmpersandMethod( From 56bba3ddebb1dc3dc066842fb03bc037cf230a01 Mon Sep 17 00:00:00 2001 From: mcasimir Date: Fri, 4 Mar 2022 14:33:15 +0100 Subject: [PATCH 04/10] fix tests --- .../src/components/connections.spec.tsx | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/compass-connections/src/components/connections.spec.tsx b/packages/compass-connections/src/components/connections.spec.tsx index acb33a8d2b0..7fafe532179 100644 --- a/packages/compass-connections/src/components/connections.spec.tsx +++ b/packages/compass-connections/src/components/connections.spec.tsx @@ -7,23 +7,28 @@ import { fireEvent, } from '@testing-library/react'; import { expect } from 'chai'; -import type { ConnectionInfo, ConnectionOptions } from 'mongodb-data-service'; +import type { + ConnectionInfo, + ConnectionOptions, + ConnectionStorage, +} from 'mongodb-data-service'; import { v4 as uuid } from 'uuid'; import sinon from 'sinon'; import Connections from './connections'; -import type { ConnectionStore } from '../stores/connections-store'; import { ToastArea } from '@mongodb-js/compass-components'; function getMockConnectionStorage( mockConnections: ConnectionInfo[] -): ConnectionStore { +): ConnectionStorage { return { loadAll: () => { return Promise.resolve(mockConnections); }, save: () => Promise.resolve(), delete: () => Promise.resolve(), + load: (id: string) => + Promise.resolve(mockConnections.find((conn) => conn.id === id)), }; } @@ -113,7 +118,7 @@ describe('Connections Component', function () { describe('when rendered with saved connections in storage', function () { let mockConnectFn: sinon.SinonSpy; - let mockStorage: ConnectionStore; + let mockStorage: ConnectionStorage; let savedConnectionId: string; let savedConnectionWithAppNameId: string; let saveConnectionSpy: sinon.SinonSpy; From cb26e4bf282a7e30943010c3ac7362dd941fdace Mon Sep 17 00:00:00 2001 From: mcasimir Date: Fri, 4 Mar 2022 14:33:35 +0100 Subject: [PATCH 05/10] remove .only --- packages/data-service/src/connection-storage.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/data-service/src/connection-storage.spec.ts b/packages/data-service/src/connection-storage.spec.ts index e53bbed9bc0..98748f5fad9 100644 --- a/packages/data-service/src/connection-storage.spec.ts +++ b/packages/data-service/src/connection-storage.spec.ts @@ -62,7 +62,7 @@ function writeFakeConnection( fs.writeFileSync(filePath, JSON.stringify(legacyConnection)); } -describe.only('ConnectionStorage', function () { +describe('ConnectionStorage', function () { let tmpDir: string; beforeEach(function () { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'connection-storage-tests')); From 44a1057d27765046bf736bb6771674c4d34ea04b Mon Sep 17 00:00:00 2001 From: mcasimir Date: Fri, 4 Mar 2022 16:30:33 +0100 Subject: [PATCH 06/10] don't update connection list if save fails --- .../src/stores/connections-store.spec.ts | 50 ++++++++++++++++--- .../src/stores/connections-store.ts | 25 +++++++++- 2 files changed, 66 insertions(+), 9 deletions(-) diff --git a/packages/compass-connections/src/stores/connections-store.spec.ts b/packages/compass-connections/src/stores/connections-store.spec.ts index 2aa41301d75..9c513d2c648 100644 --- a/packages/compass-connections/src/stores/connections-store.spec.ts +++ b/packages/compass-connections/src/stores/connections-store.spec.ts @@ -101,7 +101,7 @@ describe('use-connections hook', function () { await result.current.saveConnection({ id: 'oranges', connectionOptions: { - connectionString: 'aba', + connectionString: 'mongodb://aba', }, favorite: { name: 'not peaches', @@ -121,7 +121,7 @@ describe('use-connections hook', function () { expect(hookResult.current.state.connections[1]).to.deep.equal({ id: 'oranges', connectionOptions: { - connectionString: 'aba', + connectionString: 'mongodb://aba', }, favorite: { name: 'not peaches', @@ -155,7 +155,7 @@ describe('use-connections hook', function () { await result.current.saveConnection({ id: 'pineapples', connectionOptions: { - connectionString: '', + connectionString: 'mongodb://bacon', }, favorite: { name: 'bacon', @@ -175,7 +175,7 @@ describe('use-connections hook', function () { expect(hookResult.current.state.connections[0]).to.deep.equal({ id: 'pineapples', connectionOptions: { - connectionString: '', + connectionString: 'mongodb://bacon', }, favorite: { name: 'bacon', @@ -184,6 +184,42 @@ describe('use-connections hook', function () { }); }); + describe('saving an invalid connection', function () { + let hookResult: RenderResult>; + beforeEach(async function () { + const { result } = renderHook(() => + useConnections({ + onConnected: noop, + connectionStorage: mockConnectionStorage, + connectFn: noop, + appName: 'Test App Name', + }) + ); + + await act(async () => { + await result.current.saveConnection({ + id: 'pineapples', + connectionOptions: { + connectionString: 'bacon', + }, + favorite: { + name: 'bacon', + }, + }); + }); + + hookResult = result; + }); + + it('calls to save a connection on the store', function () { + expect(saveSpy.callCount).to.equal(0); + }); + + it('does not add the new connection to the current connections list', function () { + expect(hookResult.current.state.connections).to.be.undefined; + }); + }); + describe('saving the current active connection', function () { let hookResult: RenderResult>; beforeEach(async function () { @@ -212,7 +248,7 @@ describe('use-connections hook', function () { await result.current.saveConnection({ id: 'turtle', connectionOptions: { - connectionString: 'nice', + connectionString: 'mongodb://nice', }, favorite: { name: 'snakes! ah!', @@ -228,7 +264,7 @@ describe('use-connections hook', function () { expect(hookResult.current.state.activeConnectionInfo).to.deep.equal({ id: 'turtle', connectionOptions: { - connectionString: 'nice', + connectionString: 'mongodb://nice', }, favorite: { name: 'snakes! ah!', @@ -241,7 +277,7 @@ describe('use-connections hook', function () { expect(hookResult.current.state.connections[0]).to.deep.equal({ id: 'turtle', connectionOptions: { - connectionString: 'nice', + connectionString: 'mongodb://nice', }, favorite: { name: 'snakes! ah!', diff --git a/packages/compass-connections/src/stores/connections-store.ts b/packages/compass-connections/src/stores/connections-store.ts index 532f6aca8eb..66c938413ed 100644 --- a/packages/compass-connections/src/stores/connections-store.ts +++ b/packages/compass-connections/src/stores/connections-store.ts @@ -32,6 +32,14 @@ export function createNewConnectionInfo(): ConnectionInfo { }; } +function ensureWellFormedConnectionString(connectionString: string) { + new ConnectionString( + connectionString, + + { looseValidation: true } + ); +} + function setAppNameParamIfMissing( connectionString: string, appName: string @@ -232,10 +240,17 @@ export function useConnections({ const connectedConnectionInfo = useRef(); const connectedDataService = useRef(); - async function saveConnectionInfo(connectionInfo: ConnectionInfo) { + async function saveConnectionInfo( + connectionInfo: ConnectionInfo + ): Promise { try { + ensureWellFormedConnectionString( + connectionInfo?.connectionOptions?.connectionString + ); await connectionStorage.save(connectionInfo); debug(`saved connection with id ${connectionInfo.id || ''}`); + + return true; } catch (err) { debug( `error saving connection with id ${connectionInfo.id || ''}: ${ @@ -250,6 +265,8 @@ export function useConnections({ (err as Error).message }`, }); + + return false; } } @@ -383,7 +400,11 @@ export function useConnections({ }); }, async saveConnection(connectionInfo: ConnectionInfo) { - await saveConnectionInfo(connectionInfo); + const saved = await saveConnectionInfo(connectionInfo); + + if (!saved) { + return; + } const existingConnectionIndex = connections.findIndex( (connection) => connection.id === connectionInfo.id From c1717ffb137841fdcdd3e2827ecc3f852c56b71a Mon Sep 17 00:00:00 2001 From: mcasimir Date: Fri, 4 Mar 2022 18:15:52 +0100 Subject: [PATCH 07/10] fix requested changes --- packages/compass-connections/src/stores/connections-store.ts | 4 +++- packages/data-service/src/connection-storage.ts | 5 ++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/compass-connections/src/stores/connections-store.ts b/packages/compass-connections/src/stores/connections-store.ts index 66c938413ed..8f4fccacc36 100644 --- a/packages/compass-connections/src/stores/connections-store.ts +++ b/packages/compass-connections/src/stores/connections-store.ts @@ -281,7 +281,9 @@ export function useConnections({ // if a connection has been saved already we only want to update the lastUsed // attribute, otherwise we are going to save the entire connection info. const connectionInfoToBeSaved = - (await connectionStorage.load(connectionInfo.id)) ?? connectionInfo; + (connectionInfo.lastUsed && + (await connectionStorage.load(connectionInfo.id))) ?? + connectionInfo; await saveConnectionInfo({ ...cloneDeep(connectionInfoToBeSaved), diff --git a/packages/data-service/src/connection-storage.ts b/packages/data-service/src/connection-storage.ts index 0022d49a4e3..fa337ab8def 100644 --- a/packages/data-service/src/connection-storage.ts +++ b/packages/data-service/src/connection-storage.ts @@ -69,10 +69,9 @@ export class ConnectionStorage { } /** - * Updates attributes of the model. + * Fetch one ConnectionInfo. * - * @param {string} id ID of the model to update - * @param {object} attributes Attributes of model to update + * @param {string} id Id of the ConnectionInfo to fetch */ async load(id: string): Promise { if (!id) { From d3bc06e5d4278025251fc36927a3bb02bac86995 Mon Sep 17 00:00:00 2001 From: mcasimir Date: Fri, 4 Mar 2022 22:11:10 +0100 Subject: [PATCH 08/10] don't check for lastUsed --- packages/compass-connections/src/stores/connections-store.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/compass-connections/src/stores/connections-store.ts b/packages/compass-connections/src/stores/connections-store.ts index 8f4fccacc36..66c938413ed 100644 --- a/packages/compass-connections/src/stores/connections-store.ts +++ b/packages/compass-connections/src/stores/connections-store.ts @@ -281,9 +281,7 @@ export function useConnections({ // if a connection has been saved already we only want to update the lastUsed // attribute, otherwise we are going to save the entire connection info. const connectionInfoToBeSaved = - (connectionInfo.lastUsed && - (await connectionStorage.load(connectionInfo.id))) ?? - connectionInfo; + (await connectionStorage.load(connectionInfo.id)) ?? connectionInfo; await saveConnectionInfo({ ...cloneDeep(connectionInfoToBeSaved), From c6019a18610e5a2e303bc6b913b9823ef819ab48 Mon Sep 17 00:00:00 2001 From: mcasimir Date: Wed, 9 Mar 2022 16:28:26 +0100 Subject: [PATCH 09/10] strict validation of well formed uri --- .../compass-connections/src/stores/connections-store.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/compass-connections/src/stores/connections-store.ts b/packages/compass-connections/src/stores/connections-store.ts index 66c938413ed..90a0bb51963 100644 --- a/packages/compass-connections/src/stores/connections-store.ts +++ b/packages/compass-connections/src/stores/connections-store.ts @@ -33,11 +33,7 @@ export function createNewConnectionInfo(): ConnectionInfo { } function ensureWellFormedConnectionString(connectionString: string) { - new ConnectionString( - connectionString, - - { looseValidation: true } - ); + new ConnectionString(connectionString); } function setAppNameParamIfMissing( From b766f0feaa03e81f30c7dd086da11c560f5d6421 Mon Sep 17 00:00:00 2001 From: mcasimir Date: Wed, 9 Mar 2022 16:30:56 +0100 Subject: [PATCH 10/10] revert change to tls warnings --- packages/connection-form/src/utils/validation.spec.ts | 4 ++-- packages/connection-form/src/utils/validation.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/connection-form/src/utils/validation.spec.ts b/packages/connection-form/src/utils/validation.spec.ts index 555a3c1af05..1d4430af2c2 100644 --- a/packages/connection-form/src/utils/validation.spec.ts +++ b/packages/connection-form/src/utils/validation.spec.ts @@ -345,7 +345,7 @@ describe('validation', function () { }); expect(result[0]).to.deep.equal({ message: - 'TLS/SSL certificate validation is disabled. For a more secure connection enable certificate validation if possible.', + 'Disabling certificate validation is not recommended as it may create a security vulnerability', }); }); }); @@ -451,7 +451,7 @@ describe('validation', function () { expect(result).to.deep.equal([ { message: - 'Connecting to a remote server without TLS/SSL is not recommended. For a more secure connection enable TLS/SSL if possible.', + 'Connecting without tls is not recommended as it may create a security vulnerability.', }, ]); } diff --git a/packages/connection-form/src/utils/validation.ts b/packages/connection-form/src/utils/validation.ts index 3f0db2d61fa..35a856c5499 100644 --- a/packages/connection-form/src/utils/validation.ts +++ b/packages/connection-form/src/utils/validation.ts @@ -298,7 +298,7 @@ function validateCertificateValidationWarnings( if (tlsInsecure || tlsAllowInvalidHostnames || tlsAllowInvalidCertificates) { warnings.push({ - message: `TLS/SSL certificate validation is disabled. For a more secure connection enable certificate validation if possible.`, + message: `Disabling certificate validation is not recommended as it may create a security vulnerability`, }); } @@ -395,7 +395,7 @@ function validateTLSAndHostWarnings( if (nonLocalhostsCount && !isSecure(connectionString)) { warnings.push({ message: - 'Connecting to a remote server without TLS/SSL is not recommended. For a more secure connection enable TLS/SSL if possible.', + 'Connecting without tls is not recommended as it may create a security vulnerability.', }); } return warnings;