From 99125dbb77f0a066721c77c091109b1d90a86f8f Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Fri, 7 May 2021 14:37:23 -0400 Subject: [PATCH 01/17] feat: Cleanup spawned session after process termination --- ui/desktop/electron-app/src/spawn-promise.js | 20 ++++++++- ui/desktop/electron-app/src/spawn-session.js | 45 ++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 ui/desktop/electron-app/src/spawn-session.js diff --git a/ui/desktop/electron-app/src/spawn-promise.js b/ui/desktop/electron-app/src/spawn-promise.js index dc8069791..d96b08953 100644 --- a/ui/desktop/electron-app/src/spawn-promise.js +++ b/ui/desktop/electron-app/src/spawn-promise.js @@ -1,5 +1,6 @@ const path = require('path'); const { spawn, spawnSync } = require('child_process'); +const spawnSession = require('./spawn-session'); const boundaryPath = path.resolve(__dirname, '..', 'cli', 'boundary'); @@ -45,14 +46,26 @@ module.exports = { childProcess.stdout.on('data', (data) => { outputStream += data.toString(); const jsonData = jsonify(outputStream); - if (jsonData) resolve(jsonData); + if (jsonData) { + if (jsonData.termination_reason) { + spawnSession.cancelProcess(childProcess.pid); + } else { + // Track only successfully launched child processes + spawnSession.add({ childProcess, data: jsonData }); + } + outputStream = ''; + resolve(jsonData); + } }); + // @todo Cancel spawned session childprocess on error? Does it get automatically cancelled? + // Test on connection limit errors especially childProcess.stderr.on('data', (data) => { errorStream += data.toString(); const jsonData = jsonify(errorStream); if (jsonData) { const error = jsonData.api_error || jsonData.error; + errorStream = ''; reject(new Error(error.message)); } }); @@ -60,7 +73,10 @@ module.exports = { }, /** - * + * Spawn child process and return output immediately. + * This function is intended for non-connection related tasks. + * @param {string} command + * @return {string} */ spawnSync(command) { const childProcess = spawnSync(boundaryPath, command); diff --git a/ui/desktop/electron-app/src/spawn-session.js b/ui/desktop/electron-app/src/spawn-session.js new file mode 100644 index 000000000..72292deeb --- /dev/null +++ b/ui/desktop/electron-app/src/spawn-session.js @@ -0,0 +1,45 @@ +const process = require('process'); + +const spawnedSessions = []; + +const purge = (process_id, index) => { + process.kill(process_id); + // Remove tracking after cancellation + delete spawnedSessions[index]; +}; + +module.exports = { + /** + * Track session spawned process and it's details. + * This function is intended to tracked launched local proxy. + * @param {object} params + */ + add: ({ childProcess, data }) => { + debugger; + spawnedSessions.push({ + data, + process_id: childProcess.pid, + }); + }, + /** + * Cancel spawned child process using session, if available. + * This function is intended to cancel launched local proxy. + * @param {string} session_id + */ + cancelSession: (session_id) => { + spawnedSessions.find((value, index) => { + const { process_id, data } = value; + if (data.session_id === session_id) purge(process_id, index); + }); + }, + /** + * Cancel spawned child process, if available. + * This function is intended to cancel launched local proxy. + * @param {string} process_id + */ + cancelProcess: (process_id) => { + spawnedSessions.find((value, index) => { + if (value.process_id === process_id) purge(process_id, index); + }); + }, +}; From 2f12cc91f2b41e8320e4f34fe29c3c4421dd16e8 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Fri, 7 May 2021 15:31:19 -0400 Subject: [PATCH 02/17] feat: Cancelling session should close associated spawned proc --- .../app/routes/scopes/scope/projects/sessions.js | 6 ++++++ .../scopes/scope/projects/targets/target/sessions.js | 10 ++++++++++ ui/desktop/electron-app/src/boundary-cli.js | 1 - ui/desktop/electron-app/src/handlers.js | 10 +++++++++- ui/desktop/electron-app/src/spawn-session.js | 3 +-- 5 files changed, 26 insertions(+), 4 deletions(-) diff --git a/ui/desktop/app/routes/scopes/scope/projects/sessions.js b/ui/desktop/app/routes/scopes/scope/projects/sessions.js index 272cf0672..2715a7aa3 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/sessions.js +++ b/ui/desktop/app/routes/scopes/scope/projects/sessions.js @@ -11,6 +11,7 @@ export default class ScopesScopeProjectsSessionsRoute extends Route { // =services + @service ipc; @service session; // =attributes @@ -83,5 +84,10 @@ export default class ScopesScopeProjectsSessionsRoute extends Route { @notifySuccess('notifications.canceled-success') async cancelSession(session) { await session.cancelSession(); + try { + this.ipc.invoke('cancel', { session_id: session.id }); + } catch (e) { + //Ignore errors + } } } diff --git a/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js b/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js index 4f0281e2f..2a849f3b9 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js +++ b/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js @@ -1,8 +1,13 @@ import Route from '@ember/routing/route'; import { action } from '@ember/object'; +import { inject as service } from '@ember/service'; import { notifySuccess, notifyError } from 'core/decorators/notify'; export default class ScopesScopeProjectsTargetsTargetSessionsRoute extends Route { + // =services + + @service ipc; + // =actions /** @@ -14,5 +19,10 @@ export default class ScopesScopeProjectsTargetsTargetSessionsRoute extends Route @notifySuccess('notifications.canceled-success') async cancelSession(session) { await session.cancelSession(); + try { + this.ipc.invoke('cancel', { session_id: session.id }); + } catch (e) { + //Ignore errors + } } } diff --git a/ui/desktop/electron-app/src/boundary-cli.js b/ui/desktop/electron-app/src/boundary-cli.js index 453a959df..eeb5050f8 100644 --- a/ui/desktop/electron-app/src/boundary-cli.js +++ b/ui/desktop/electron-app/src/boundary-cli.js @@ -53,6 +53,5 @@ module.exports = { ? `CLI Version: ${versionNumber}\nCLI Commit: ${gitRevision}` : `CLI Commit: ${gitRevision}`; return { gitRevision, versionNumber, formatted }; - return rawOutput; }, }; diff --git a/ui/desktop/electron-app/src/handlers.js b/ui/desktop/electron-app/src/handlers.js index 4e102212a..04343c5ae 100644 --- a/ui/desktop/electron-app/src/handlers.js +++ b/ui/desktop/electron-app/src/handlers.js @@ -2,7 +2,8 @@ const { shell } = require('electron'); const isDev = require('electron-is-dev'); const handle = require('./ipc-handler'); const boundaryCli = require('./boundary-cli'); -const origin = require('./origin.js'); +const origin = require('./origin'); +const spawnSession = require('./spawn-session'); /** * Returns the current runtime origin, which is used by the main thread to @@ -58,3 +59,10 @@ handle('cliExists', () => boundaryCli.exists()); handle('connect', ({ target_id, token, host_id }) => boundaryCli.connect(target_id, token, host_id) ); + +/** + * Cancel an established boundary session spawned process. + */ +handle('cancel', ({ session_id }) => { + spawnSession.cancelSession(session_id); +}); diff --git a/ui/desktop/electron-app/src/spawn-session.js b/ui/desktop/electron-app/src/spawn-session.js index 72292deeb..f632e4cea 100644 --- a/ui/desktop/electron-app/src/spawn-session.js +++ b/ui/desktop/electron-app/src/spawn-session.js @@ -5,7 +5,7 @@ const spawnedSessions = []; const purge = (process_id, index) => { process.kill(process_id); // Remove tracking after cancellation - delete spawnedSessions[index]; + spawnedSessions.splice(index, 1); }; module.exports = { @@ -15,7 +15,6 @@ module.exports = { * @param {object} params */ add: ({ childProcess, data }) => { - debugger; spawnedSessions.push({ data, process_id: childProcess.pid, From dfbb54a1eec9edc22205e8e6018eb5d7ba824f45 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Tue, 11 May 2021 16:38:26 -0400 Subject: [PATCH 03/17] refactor: Add SessionManager and Session classes to electron --- ui/desktop/electron-app/src/boundary-cli.js | 36 +------ ui/desktop/electron-app/src/handlers.js | 11 ++- ui/desktop/electron-app/src/session/index.js | 99 +++++++++++++++++++ .../electron-app/src/session/manager.js | 39 ++++++++ ui/desktop/electron-app/src/spawn-promise.js | 21 ++-- 5 files changed, 157 insertions(+), 49 deletions(-) create mode 100644 ui/desktop/electron-app/src/session/index.js create mode 100644 ui/desktop/electron-app/src/session/manager.js diff --git a/ui/desktop/electron-app/src/boundary-cli.js b/ui/desktop/electron-app/src/boundary-cli.js index eeb5050f8..9eeaa4f8d 100644 --- a/ui/desktop/electron-app/src/boundary-cli.js +++ b/ui/desktop/electron-app/src/boundary-cli.js @@ -1,46 +1,12 @@ const path = require('path'); -const { spawnAsyncJSONPromise, spawnSync } = require('./spawn-promise'); -const origin = require('./origin.js'); +const { spawnSync } = require('./spawn-promise'); const cliPath = async () => path.resolve(__dirname, '..', 'cli', 'boundary'); -/** - * Super paranoid shell quote/escape and validation. Input must be base62. - * @param {string} str - */ -const escapeAndValidateBase62 = (str) => { - const candidate = str.toString(); - if (candidate.match(/^[A-Za-z0-9_]*$/)) return candidate; - throw new Error(` - Could not invoke command: - input contained unsafe characters. - `); -}; - module.exports = { // Check boundary cli existence exists: () => Boolean(cliPath()), - // Initiate connection and return output - connect: (target_id, token, host_id) => { - const addr = origin.origin; - const sanitized = { - target_id: escapeAndValidateBase62(target_id), - token: escapeAndValidateBase62(token), - }; - if (host_id) sanitized.host_id = escapeAndValidateBase62(host_id); - const command = [ - 'connect', - `-target-id=${sanitized.target_id}`, - `-token=${sanitized.token}`, - `-addr=${addr}`, - '-format=json', - ]; - - if (host_id) command.push(`-host-id=${sanitized.host_id}`); - return spawnAsyncJSONPromise(command); - }, - // Returns JSON-formatted version information from the CLI version: () => { const command = ['-v']; diff --git a/ui/desktop/electron-app/src/handlers.js b/ui/desktop/electron-app/src/handlers.js index 04343c5ae..7127c1f12 100644 --- a/ui/desktop/electron-app/src/handlers.js +++ b/ui/desktop/electron-app/src/handlers.js @@ -3,7 +3,7 @@ const isDev = require('electron-is-dev'); const handle = require('./ipc-handler'); const boundaryCli = require('./boundary-cli'); const origin = require('./origin'); -const spawnSession = require('./spawn-session'); +const sessionManager = require('./session/manager'); /** * Returns the current runtime origin, which is used by the main thread to @@ -57,12 +57,13 @@ handle('cliExists', () => boundaryCli.exists()); * Establishes a boundary session and returns session details. */ handle('connect', ({ target_id, token, host_id }) => - boundaryCli.connect(target_id, token, host_id) + sessionManager.start(origin.origin, target_id, token, host_id) ); /** * Cancel an established boundary session spawned process. */ -handle('cancel', ({ session_id }) => { - spawnSession.cancelSession(session_id); -}); +handle('cancel', ({ session_id }) => + // spawnSession.cancelSession(session_id); + sessionManager.stop(session_id) +); diff --git a/ui/desktop/electron-app/src/session/index.js b/ui/desktop/electron-app/src/session/index.js new file mode 100644 index 000000000..3d5563f1d --- /dev/null +++ b/ui/desktop/electron-app/src/session/index.js @@ -0,0 +1,99 @@ +const { spawnAsyncJSONPromise } = require('../spawn-promise'); + +// FIXME create structure 'util' folder for spawn-promise + +/** + * Super paranoid shell quote/escape and validation. Input must be base62. + * @param {string} str + */ +const escapeAndValidateBase62 = (str) => { + const candidate = str.toString(); + if (candidate.match(/^[A-Za-z0-9_]*$/)) return candidate; + throw new Error(` + Could not invoke command: + input contained unsafe characters. + `); +}; + +class Session { + #id; + #pid; + #addr; + #token; + #hostId; + #targetId; + #proxyDetails; + + /** + * Initialize a session to a controller address + * using target details. + * @param {string} addr + * @param {string} targetId + * @param {string} token + * @param {string} hostId + */ + constructor(addr, targetId, token, hostId) { + this.#addr = addr; + this.#targetId = targetId; + this.#token = token; + this.#hostId = hostId; + } + + /** + * @return {boolean} + */ + get isActive() { + return Boolean(this.#pid); + } + + /** + * Returns session proxy details. + * Only available after successful start of a session. + * @return {object} + */ + get proxyDetails() { + return this.#proxyDetails; + } + + // Spawn process + // Track spawned process this.pid = pid; + /** + * Using cli, initialize a session to a target. + * Tracks local proxy details if successful. + */ + start() { + const command = this.cliCommand(); + return spawnAsyncJSONPromise(command).then((spawnedSession) => { + this.#id = this.#proxyDetails.session_id; + this.#pid = spawnedSession.childProcess.pid; + this.#proxyDetails = spawnedSession.response; + }); + } + + /** + * Generate cli command for session. + * @returns {[]} + */ + cliCommand() { + const sanitized = { + target_id: escapeAndValidateBase62(this.#targetId), + token: escapeAndValidateBase62(this.#token), + }; + + const command = [ + 'connect', + `-target-id=${sanitized.target_id}`, + `-token=${sanitized.token}`, + `-addr=${this.#addr}`, + '-format=json', + ]; + + if (this.#hostId) { + sanitized.host_id = escapeAndValidateBase62(this.#hostId); + command.push(`-host-id=${sanitized.host_id}`); + } + return command; + } +} + +module.exports = Session; diff --git a/ui/desktop/electron-app/src/session/manager.js b/ui/desktop/electron-app/src/session/manager.js new file mode 100644 index 000000000..60be77204 --- /dev/null +++ b/ui/desktop/electron-app/src/session/manager.js @@ -0,0 +1,39 @@ +const Session = require('./index.js'); + +class SessionManager { + #sessions; + + constructor() { + this.#sessions = []; + } + + /** + * Start a session and track it. + * Returns session proxy details if successfully started. + * Only successfully started sessions will be tracked. + * @param {string} addr + * @param {string} target_id + * @param {string} token + * @param {string} host_id + */ + start(addr, target_id, token, host_id) { + const session = new Session(addr, target_id, token, host_id); + return session.start().then(() => { + this.#sessions.push(session); + return session.proxyDetails; + }); + } + + stopById() { + } + + stopAll() { + } + + get sessions() { + return this.#sessions; + } +} + +const sessionManager = new SessionManager(); +module.exports = sessionManager; diff --git a/ui/desktop/electron-app/src/spawn-promise.js b/ui/desktop/electron-app/src/spawn-promise.js index d96b08953..10467ffc6 100644 --- a/ui/desktop/electron-app/src/spawn-promise.js +++ b/ui/desktop/electron-app/src/spawn-promise.js @@ -47,25 +47,28 @@ module.exports = { outputStream += data.toString(); const jsonData = jsonify(outputStream); if (jsonData) { - if (jsonData.termination_reason) { - spawnSession.cancelProcess(childProcess.pid); - } else { - // Track only successfully launched child processes - spawnSession.add({ childProcess, data: jsonData }); - } - outputStream = ''; - resolve(jsonData); + // if (jsonData.termination_reason) { + // spawnSession.cancelProcess(childProcess.pid); + // } else { + // // Track only successfully launched child processes + // spawnSession.add({ childProcess, data: jsonData }); + // } + // FIXME + // outputStream = ''; + resolve({ childProcess, response: jsonData }); } }); // @todo Cancel spawned session childprocess on error? Does it get automatically cancelled? + // @todo on error - should childprocess be closed? // Test on connection limit errors especially childProcess.stderr.on('data', (data) => { errorStream += data.toString(); const jsonData = jsonify(errorStream); if (jsonData) { const error = jsonData.api_error || jsonData.error; - errorStream = ''; + // FIXME + // errorStream = ''; reject(new Error(error.message)); } }); From 2be18bcf6f9a5f335d88ee684d1680c093f62b22 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Tue, 11 May 2021 18:09:38 -0400 Subject: [PATCH 04/17] chore: Cleanup --- .../routes/scopes/scope/projects/sessions.js | 6 +--- .../scope/projects/targets/target/sessions.js | 6 +--- ui/desktop/electron-app/src/handlers.js | 5 +-- ui/desktop/electron-app/src/session/index.js | 31 +++++++++++++++---- .../electron-app/src/session/manager.js | 16 ++++++++-- .../acceptance/projects/sessions-test.js | 17 ++++++++++ .../projects/targets/sessions-test.js | 12 +++++++ 7 files changed, 71 insertions(+), 22 deletions(-) diff --git a/ui/desktop/app/routes/scopes/scope/projects/sessions.js b/ui/desktop/app/routes/scopes/scope/projects/sessions.js index 2715a7aa3..908636f1a 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/sessions.js +++ b/ui/desktop/app/routes/scopes/scope/projects/sessions.js @@ -84,10 +84,6 @@ export default class ScopesScopeProjectsSessionsRoute extends Route { @notifySuccess('notifications.canceled-success') async cancelSession(session) { await session.cancelSession(); - try { - this.ipc.invoke('cancel', { session_id: session.id }); - } catch (e) { - //Ignore errors - } + await this.ipc.invoke('cancel', { session_id: session.id }); } } diff --git a/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js b/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js index 2a849f3b9..2263aa448 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js +++ b/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js @@ -19,10 +19,6 @@ export default class ScopesScopeProjectsTargetsTargetSessionsRoute extends Route @notifySuccess('notifications.canceled-success') async cancelSession(session) { await session.cancelSession(); - try { - this.ipc.invoke('cancel', { session_id: session.id }); - } catch (e) { - //Ignore errors - } + await this.ipc.invoke('cancel', { session_id: session.id }); } } diff --git a/ui/desktop/electron-app/src/handlers.js b/ui/desktop/electron-app/src/handlers.js index 7127c1f12..3aee556f9 100644 --- a/ui/desktop/electron-app/src/handlers.js +++ b/ui/desktop/electron-app/src/handlers.js @@ -63,7 +63,4 @@ handle('connect', ({ target_id, token, host_id }) => /** * Cancel an established boundary session spawned process. */ -handle('cancel', ({ session_id }) => - // spawnSession.cancelSession(session_id); - sessionManager.stop(session_id) -); +handle('cancel', ({ session_id }) => sessionManager.stopById(session_id)); diff --git a/ui/desktop/electron-app/src/session/index.js b/ui/desktop/electron-app/src/session/index.js index 3d5563f1d..cfc760ba7 100644 --- a/ui/desktop/electron-app/src/session/index.js +++ b/ui/desktop/electron-app/src/session/index.js @@ -1,4 +1,4 @@ -const { spawnAsyncJSONPromise } = require('../spawn-promise'); +const { spawnAsyncJSONPromise } = require('../spawn-promise.js'); // FIXME create structure 'util' folder for spawn-promise @@ -21,6 +21,7 @@ class Session { #addr; #token; #hostId; + #process; #targetId; #proxyDetails; @@ -39,11 +40,19 @@ class Session { this.#hostId = hostId; } + /** + * Session id + * @return {string} + */ + get id() { + return this.#id; + } + /** * @return {boolean} */ get isActive() { - return Boolean(this.#pid); + return this.#process.connected; } /** @@ -55,8 +64,6 @@ class Session { return this.#proxyDetails; } - // Spawn process - // Track spawned process this.pid = pid; /** * Using cli, initialize a session to a target. * Tracks local proxy details if successful. @@ -64,9 +71,21 @@ class Session { start() { const command = this.cliCommand(); return spawnAsyncJSONPromise(command).then((spawnedSession) => { - this.#id = this.#proxyDetails.session_id; - this.#pid = spawnedSession.childProcess.pid; + this.#process = spawnedSession.childProcess; this.#proxyDetails = spawnedSession.response; + this.#process = spawnedSession.childProcess; + this.#id = this.#proxyDetails.session_id; + }); + } + + /** + * Stop proxy used by session. + */ + stop() { + return new Promise((resolve, reject) => { + this.#process.on('close', () => resolve()); + this.#process.on('error', (e) => reject(e)); + this.#process.kill(); }); } diff --git a/ui/desktop/electron-app/src/session/manager.js b/ui/desktop/electron-app/src/session/manager.js index 60be77204..840ee76f3 100644 --- a/ui/desktop/electron-app/src/session/manager.js +++ b/ui/desktop/electron-app/src/session/manager.js @@ -16,7 +16,7 @@ class SessionManager { * @param {string} token * @param {string} host_id */ - start(addr, target_id, token, host_id) { + async start(addr, target_id, token, host_id) { const session = new Session(addr, target_id, token, host_id); return session.start().then(() => { this.#sessions.push(session); @@ -24,7 +24,19 @@ class SessionManager { }); } - stopById() { + /** + * Stop a session and remove it from tracked sessions. + * @param {string} session_id + */ + async stopById(session_id) { + const index = this.#sessions.findIndex( + (session) => session.id === session_id + ); + if (index === -1) return; + const session = this.#sessions[index]; + return session.stop().then(() => { + this.#sessions.splice(index, 1); + }); } stopAll() { diff --git a/ui/desktop/tests/acceptance/projects/sessions-test.js b/ui/desktop/tests/acceptance/projects/sessions-test.js index eb3e0bab1..012db14cb 100644 --- a/ui/desktop/tests/acceptance/projects/sessions-test.js +++ b/ui/desktop/tests/acceptance/projects/sessions-test.js @@ -19,6 +19,7 @@ import { authenticateSession, invalidateSession, } from 'ember-simple-auth/test-support'; +import sinon from 'sinon'; module('Acceptance | projects | sessions', function (hooks) { setupApplicationTest(hooks); @@ -43,6 +44,7 @@ module('Acceptance | projects | sessions', function (hooks) { const stubs = { global: null, org: null, + ipcService: null, }; const urls = { @@ -144,6 +146,9 @@ module('Acceptance | projects | sessions', function (hooks) { window.addEventListener('message', messageHandler); setDefaultOrigin(this); + + const ipcService = this.owner.lookup('service:ipc'); + stubs.ipcService = sinon.stub(ipcService, 'invoke'); }); hooks.afterEach(function () { @@ -240,6 +245,7 @@ module('Acceptance | projects | sessions', function (hooks) { test('cancelling a session', async function (assert) { assert.expect(1); + stubs.ipcService.withArgs('cancel'); later(async () => { run.cancelTimers(); await click('tbody tr:first-child td:last-child button'); @@ -258,4 +264,15 @@ module('Acceptance | projects | sessions', function (hooks) { }, 750); await visit(urls.sessions); }); + + test('cancelling a session with ipc error shows notification', async function (assert) { + assert.expect(1); + stubs.ipcService.withArgs('cancel').throws(); + later(async () => { + run.cancelTimers(); + await click('tbody tr:first-child td:last-child button'); + assert.ok(find('[role="alert"].is-error')); + }, 750); + await visit(urls.sessions); + }); }); diff --git a/ui/desktop/tests/acceptance/projects/targets/sessions-test.js b/ui/desktop/tests/acceptance/projects/targets/sessions-test.js index a28682950..9fde4b210 100644 --- a/ui/desktop/tests/acceptance/projects/targets/sessions-test.js +++ b/ui/desktop/tests/acceptance/projects/targets/sessions-test.js @@ -248,6 +248,7 @@ module('Acceptance | projects | targets | sessions', function (hooks) { test('cancelling a session', async function (assert) { assert.expect(2); const sessionsCount = this.server.schema.sessions.all().models.length; + stubs.ipcService.withArgs('cancel'); later(async () => { run.cancelTimers(); await click('tbody tr:first-child td:last-child button'); @@ -268,6 +269,17 @@ module('Acceptance | projects | targets | sessions', function (hooks) { await visit(urls.sessions); }); + test('cancelling a session with ipc error shows notification', async function (assert) { + assert.expect(1); + stubs.ipcService.withArgs('cancel').throws(); + later(async () => { + run.cancelTimers(); + await click('tbody tr:first-child td:last-child button'); + assert.ok(find('[role="alert"].is-error')); + }, 750); + await visit(urls.sessions); + }); + test('connecting to a target', async function (assert) { assert.expect(3); stubs.ipcService.withArgs('cliExists').returns(true); From 828faf2be4733dd2a046b9d2c5dcf720f2c489e9 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Wed, 12 May 2021 09:53:10 -0400 Subject: [PATCH 05/17] refactor: Create helpers folder in desktop/electron --- ui/desktop/electron-app/src/boundary-cli.js | 2 +- .../src/{ => helpers}/spawn-promise.js | 0 ui/desktop/electron-app/src/session/index.js | 4 +- ui/desktop/electron-app/src/spawn-session.js | 44 ------------------- 4 files changed, 2 insertions(+), 48 deletions(-) rename ui/desktop/electron-app/src/{ => helpers}/spawn-promise.js (100%) delete mode 100644 ui/desktop/electron-app/src/spawn-session.js diff --git a/ui/desktop/electron-app/src/boundary-cli.js b/ui/desktop/electron-app/src/boundary-cli.js index 9eeaa4f8d..216886971 100644 --- a/ui/desktop/electron-app/src/boundary-cli.js +++ b/ui/desktop/electron-app/src/boundary-cli.js @@ -1,5 +1,5 @@ const path = require('path'); -const { spawnSync } = require('./spawn-promise'); +const { spawnSync } = require('./helpers/spawn-promise.js'); const cliPath = async () => path.resolve(__dirname, '..', 'cli', 'boundary'); diff --git a/ui/desktop/electron-app/src/spawn-promise.js b/ui/desktop/electron-app/src/helpers/spawn-promise.js similarity index 100% rename from ui/desktop/electron-app/src/spawn-promise.js rename to ui/desktop/electron-app/src/helpers/spawn-promise.js diff --git a/ui/desktop/electron-app/src/session/index.js b/ui/desktop/electron-app/src/session/index.js index cfc760ba7..d7a4af930 100644 --- a/ui/desktop/electron-app/src/session/index.js +++ b/ui/desktop/electron-app/src/session/index.js @@ -1,6 +1,4 @@ -const { spawnAsyncJSONPromise } = require('../spawn-promise.js'); - -// FIXME create structure 'util' folder for spawn-promise +const { spawnAsyncJSONPromise } = require('../helpers/spawn-promise.js'); /** * Super paranoid shell quote/escape and validation. Input must be base62. diff --git a/ui/desktop/electron-app/src/spawn-session.js b/ui/desktop/electron-app/src/spawn-session.js deleted file mode 100644 index f632e4cea..000000000 --- a/ui/desktop/electron-app/src/spawn-session.js +++ /dev/null @@ -1,44 +0,0 @@ -const process = require('process'); - -const spawnedSessions = []; - -const purge = (process_id, index) => { - process.kill(process_id); - // Remove tracking after cancellation - spawnedSessions.splice(index, 1); -}; - -module.exports = { - /** - * Track session spawned process and it's details. - * This function is intended to tracked launched local proxy. - * @param {object} params - */ - add: ({ childProcess, data }) => { - spawnedSessions.push({ - data, - process_id: childProcess.pid, - }); - }, - /** - * Cancel spawned child process using session, if available. - * This function is intended to cancel launched local proxy. - * @param {string} session_id - */ - cancelSession: (session_id) => { - spawnedSessions.find((value, index) => { - const { process_id, data } = value; - if (data.session_id === session_id) purge(process_id, index); - }); - }, - /** - * Cancel spawned child process, if available. - * This function is intended to cancel launched local proxy. - * @param {string} process_id - */ - cancelProcess: (process_id) => { - spawnedSessions.find((value, index) => { - if (value.process_id === process_id) purge(process_id, index); - }); - }, -}; From 74501a440ff27988c13ecad8c1d38b77542ef823 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Wed, 12 May 2021 10:27:38 -0400 Subject: [PATCH 06/17] refactor: Create structure in desktop/electron-app --- ui/desktop/electron-app/src/boundary-cli.js | 23 ---------- .../{ => config}/content-security-policy.js | 2 +- .../electron-app/src/{ => config}/menu.js | 2 +- .../src/{ => helpers}/app-updater.js | 0 .../electron-app/src/helpers/spawn-promise.js | 42 ++++++------------- ui/desktop/electron-app/src/index.js | 10 ++--- .../electron-app/src/{ => ipc}/handlers.js | 12 +++--- .../electron-app/src/{ => ipc}/ipc-handler.js | 0 .../src/{origin.js => origin/index.js} | 0 ui/desktop/electron-app/src/session/index.js | 17 ++++++-- ui/desktop/electron-app/src/utils/jsonify.js | 11 +++++ 11 files changed, 50 insertions(+), 69 deletions(-) delete mode 100644 ui/desktop/electron-app/src/boundary-cli.js rename ui/desktop/electron-app/src/{ => config}/content-security-policy.js (96%) rename ui/desktop/electron-app/src/{ => config}/menu.js (97%) rename ui/desktop/electron-app/src/{ => helpers}/app-updater.js (100%) rename ui/desktop/electron-app/src/{ => ipc}/handlers.js (87%) rename ui/desktop/electron-app/src/{ => ipc}/ipc-handler.js (100%) rename ui/desktop/electron-app/src/{origin.js => origin/index.js} (100%) create mode 100644 ui/desktop/electron-app/src/utils/jsonify.js diff --git a/ui/desktop/electron-app/src/boundary-cli.js b/ui/desktop/electron-app/src/boundary-cli.js deleted file mode 100644 index 216886971..000000000 --- a/ui/desktop/electron-app/src/boundary-cli.js +++ /dev/null @@ -1,23 +0,0 @@ -const path = require('path'); -const { spawnSync } = require('./helpers/spawn-promise.js'); - -const cliPath = async () => path.resolve(__dirname, '..', 'cli', 'boundary'); - -module.exports = { - // Check boundary cli existence - exists: () => Boolean(cliPath()), - - // Returns JSON-formatted version information from the CLI - version: () => { - const command = ['-v']; - const rawOutput = spawnSync(command); - let gitRevision = /Git Revision:\s*(?.*)\n/.exec(rawOutput); - let versionNumber = /Version Number:\s*(?.*)\n/.exec(rawOutput); - if (gitRevision) gitRevision = gitRevision.groups.rev; - if (versionNumber) versionNumber = versionNumber.groups.ver; - const formatted = versionNumber - ? `CLI Version: ${versionNumber}\nCLI Commit: ${gitRevision}` - : `CLI Commit: ${gitRevision}`; - return { gitRevision, versionNumber, formatted }; - }, -}; diff --git a/ui/desktop/electron-app/src/content-security-policy.js b/ui/desktop/electron-app/src/config/content-security-policy.js similarity index 96% rename from ui/desktop/electron-app/src/content-security-policy.js rename to ui/desktop/electron-app/src/config/content-security-policy.js index 6f5156bb6..55f9a67de 100644 --- a/ui/desktop/electron-app/src/content-security-policy.js +++ b/ui/desktop/electron-app/src/config/content-security-policy.js @@ -1,4 +1,4 @@ -const origin = require('./origin.js'); +const origin = require('../origin.js'); const isDev = require('electron-is-dev'); const csp = { diff --git a/ui/desktop/electron-app/src/menu.js b/ui/desktop/electron-app/src/config/menu.js similarity index 97% rename from ui/desktop/electron-app/src/menu.js rename to ui/desktop/electron-app/src/config/menu.js index 2ba8eb6e7..5f669e4bb 100644 --- a/ui/desktop/electron-app/src/menu.js +++ b/ui/desktop/electron-app/src/config/menu.js @@ -1,5 +1,5 @@ const { shell } = require('electron'); -const appUpdater = require('./app-updater.js'); +const appUpdater = require('../helpers/app-updater.js'); const generateMenuTemplate = () => { return [ diff --git a/ui/desktop/electron-app/src/app-updater.js b/ui/desktop/electron-app/src/helpers/app-updater.js similarity index 100% rename from ui/desktop/electron-app/src/app-updater.js rename to ui/desktop/electron-app/src/helpers/app-updater.js diff --git a/ui/desktop/electron-app/src/helpers/spawn-promise.js b/ui/desktop/electron-app/src/helpers/spawn-promise.js index 10467ffc6..4df9db988 100644 --- a/ui/desktop/electron-app/src/helpers/spawn-promise.js +++ b/ui/desktop/electron-app/src/helpers/spawn-promise.js @@ -1,19 +1,9 @@ const path = require('path'); const { spawn, spawnSync } = require('child_process'); -const spawnSession = require('./spawn-session'); +const jsonify = require('../utils/jsonify.js'); const boundaryPath = path.resolve(__dirname, '..', 'cli', 'boundary'); -// Convert to json -const jsonify = (data) => { - if (typeof data !== 'string') data = JSON.stringify(data); - try { - return JSON.parse(data); - } catch (e) { - // Ignore parse errors - } -}; - // You can throw exceptions, or allow them to occur, and this is supported. // Exceptions thrown in this way will be returned to the UI as an // IPC rejection. However in most cases, you probably want to return a @@ -43,35 +33,29 @@ module.exports = { let outputStream = ''; let errorStream = ''; - childProcess.stdout.on('data', (data) => { + const processData = (data) => { outputStream += data.toString(); const jsonData = jsonify(outputStream); if (jsonData) { - // if (jsonData.termination_reason) { - // spawnSession.cancelProcess(childProcess.pid); - // } else { - // // Track only successfully launched child processes - // spawnSession.add({ childProcess, data: jsonData }); - // } - // FIXME - // outputStream = ''; + childProcess.removeListener('data', processData); resolve({ childProcess, response: jsonData }); } - }); + }; - // @todo Cancel spawned session childprocess on error? Does it get automatically cancelled? - // @todo on error - should childprocess be closed? - // Test on connection limit errors especially - childProcess.stderr.on('data', (data) => { - errorStream += data.toString(); + childProcess.stdout.on('data', processData); + + // Errors are cli returned json objects + const processError = (error) => { + errorStream += error.toString(); const jsonData = jsonify(errorStream); if (jsonData) { const error = jsonData.api_error || jsonData.error; - // FIXME - // errorStream = ''; + childProcess.removeListener('error', processError); reject(new Error(error.message)); } - }); + }; + + childProcess.stderr.on('data', processError); }); }, diff --git a/ui/desktop/electron-app/src/index.js b/ui/desktop/electron-app/src/index.js index 0fbcb3524..3ff94bf64 100644 --- a/ui/desktop/electron-app/src/index.js +++ b/ui/desktop/electron-app/src/index.js @@ -13,13 +13,13 @@ const { Menu, MenuItem, } = require('electron'); -require('./handlers.js'); +require('./ipc/handlers.js'); -const origin = require('./origin.js'); -const { generateCSPHeader } = require('./content-security-policy.js'); +const origin = require('./origin/index.js'); +const { generateCSPHeader } = require('./config/content-security-policy.js'); -const menu = require('./menu.js'); -const appUpdater = require('./app-updater.js'); +const menu = require('./config/menu.js'); +const appUpdater = require('./helpers/app-updater.js'); const isDev = require('electron-is-dev'); // Register the custom file protocol diff --git a/ui/desktop/electron-app/src/handlers.js b/ui/desktop/electron-app/src/ipc/handlers.js similarity index 87% rename from ui/desktop/electron-app/src/handlers.js rename to ui/desktop/electron-app/src/ipc/handlers.js index 1a03eea6a..d3a8d4efd 100644 --- a/ui/desktop/electron-app/src/handlers.js +++ b/ui/desktop/electron-app/src/ipc/handlers.js @@ -1,9 +1,9 @@ const { shell } = require('electron'); const isDev = require('electron-is-dev'); -const handle = require('./ipc-handler'); -const boundaryCli = require('./boundary-cli'); -const origin = require('./origin'); -const sessionManager = require('./session/manager'); +const handle = require('./ipc-handler.js'); +const boundaryCli = require('../cli/index.js'); +const origin = require('../origin/index.js'); +const sessionManager = require('../session/manager.js'); /** * Returns the current runtime origin, which is used by the main thread to @@ -23,9 +23,7 @@ handle('setOrigin', async (requestOrigin) => { /** * Resets the origin. */ -handle('resetOrigin', async (requestOrigin) => { - origin.resetOrigin(); -}); +handle('resetOrigin', async () => origin.resetOrigin()); /** * Opens the specified URL in an external browser. Only secure HTTPs URLs are diff --git a/ui/desktop/electron-app/src/ipc-handler.js b/ui/desktop/electron-app/src/ipc/ipc-handler.js similarity index 100% rename from ui/desktop/electron-app/src/ipc-handler.js rename to ui/desktop/electron-app/src/ipc/ipc-handler.js diff --git a/ui/desktop/electron-app/src/origin.js b/ui/desktop/electron-app/src/origin/index.js similarity index 100% rename from ui/desktop/electron-app/src/origin.js rename to ui/desktop/electron-app/src/origin/index.js diff --git a/ui/desktop/electron-app/src/session/index.js b/ui/desktop/electron-app/src/session/index.js index d7a4af930..57a41dfa1 100644 --- a/ui/desktop/electron-app/src/session/index.js +++ b/ui/desktop/electron-app/src/session/index.js @@ -1,5 +1,5 @@ const { spawnAsyncJSONPromise } = require('../helpers/spawn-promise.js'); - +const jsonify = require('../utils/jsonify.js'); /** * Super paranoid shell quote/escape and validation. Input must be base62. * @param {string} str @@ -15,7 +15,6 @@ const escapeAndValidateBase62 = (str) => { class Session { #id; - #pid; #addr; #token; #hostId; @@ -73,11 +72,12 @@ class Session { this.#proxyDetails = spawnedSession.response; this.#process = spawnedSession.childProcess; this.#id = this.#proxyDetails.session_id; + this.monitor(); }); } /** - * Stop proxy used by session. + * Stop proxy process used by session. */ stop() { return new Promise((resolve, reject) => { @@ -87,6 +87,17 @@ class Session { }); } + monitor() { + let outputStream = ''; + this.#process.on('data', (data) => { + outputStream += data; + const jsonData = jsonify(outputStream); + if (jsonData && jsonData.termination_reason) { + this.stop(); + } + }); + } + /** * Generate cli command for session. * @returns {[]} diff --git a/ui/desktop/electron-app/src/utils/jsonify.js b/ui/desktop/electron-app/src/utils/jsonify.js new file mode 100644 index 000000000..e75bab6fb --- /dev/null +++ b/ui/desktop/electron-app/src/utils/jsonify.js @@ -0,0 +1,11 @@ +// Convert to json +const jsonify = (data) => { + if (typeof data !== 'string') data = JSON.stringify(data); + try { + return JSON.parse(data); + } catch (e) { + // Ignore parse errors + } +}; + +module.exports = jsonify; From ee748de5c9a70ac41ca7d3b47cf28b4b43da26b3 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Wed, 12 May 2021 11:10:11 -0400 Subject: [PATCH 07/17] chore: Run cancellation tests in electron --- .../electron-app/config/forge.config.js | 2 +- .../src/config/content-security-policy.js | 2 +- .../electron-app/src/helpers/spawn-promise.js | 2 +- ui/desktop/electron-app/src/session/index.js | 13 --------- yarn.lock | 27 ++++--------------- 5 files changed, 8 insertions(+), 38 deletions(-) diff --git a/ui/desktop/electron-app/config/forge.config.js b/ui/desktop/electron-app/config/forge.config.js index 293ae0952..c426f9aff 100644 --- a/ui/desktop/electron-app/config/forge.config.js +++ b/ui/desktop/electron-app/config/forge.config.js @@ -1,5 +1,5 @@ const process = require('process'); -const { version } = require('../src/boundary-cli.js'); +const { version } = require('../src/cli/index.js'); const formattedCLIVersion = version().formatted; diff --git a/ui/desktop/electron-app/src/config/content-security-policy.js b/ui/desktop/electron-app/src/config/content-security-policy.js index 55f9a67de..8895178ef 100644 --- a/ui/desktop/electron-app/src/config/content-security-policy.js +++ b/ui/desktop/electron-app/src/config/content-security-policy.js @@ -1,4 +1,4 @@ -const origin = require('../origin.js'); +const origin = require('../origin/index.js'); const isDev = require('electron-is-dev'); const csp = { diff --git a/ui/desktop/electron-app/src/helpers/spawn-promise.js b/ui/desktop/electron-app/src/helpers/spawn-promise.js index 4df9db988..86ac21f68 100644 --- a/ui/desktop/electron-app/src/helpers/spawn-promise.js +++ b/ui/desktop/electron-app/src/helpers/spawn-promise.js @@ -2,7 +2,7 @@ const path = require('path'); const { spawn, spawnSync } = require('child_process'); const jsonify = require('../utils/jsonify.js'); -const boundaryPath = path.resolve(__dirname, '..', 'cli', 'boundary'); +const boundaryPath = path.resolve(__dirname, '..', '..', 'cli', 'boundary'); // You can throw exceptions, or allow them to occur, and this is supported. // Exceptions thrown in this way will be returned to the UI as an diff --git a/ui/desktop/electron-app/src/session/index.js b/ui/desktop/electron-app/src/session/index.js index 57a41dfa1..be20bff12 100644 --- a/ui/desktop/electron-app/src/session/index.js +++ b/ui/desktop/electron-app/src/session/index.js @@ -1,5 +1,4 @@ const { spawnAsyncJSONPromise } = require('../helpers/spawn-promise.js'); -const jsonify = require('../utils/jsonify.js'); /** * Super paranoid shell quote/escape and validation. Input must be base62. * @param {string} str @@ -72,7 +71,6 @@ class Session { this.#proxyDetails = spawnedSession.response; this.#process = spawnedSession.childProcess; this.#id = this.#proxyDetails.session_id; - this.monitor(); }); } @@ -87,17 +85,6 @@ class Session { }); } - monitor() { - let outputStream = ''; - this.#process.on('data', (data) => { - outputStream += data; - const jsonData = jsonify(outputStream); - if (jsonData && jsonData.termination_reason) { - this.stop(); - } - }); - } - /** * Generate cli command for session. * @returns {[]} diff --git a/yarn.lock b/yarn.lock index 8e43f086e..497898c23 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7578,7 +7578,7 @@ debug@~4.1.0: dependencies: ms "^2.1.1" -debuglog@*, debuglog@^1.0.1: +debuglog@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/debuglog/-/debuglog-1.0.1.tgz#aa24ffb9ac3df9a2351837cfb2d279360cd78492" integrity sha1-qiT/uaw9+aI1GDfPstJ5NgzXhJI= @@ -12380,7 +12380,7 @@ import-lazy@^4.0.0: resolved "https://registry.yarnpkg.com/import-lazy/-/import-lazy-4.0.0.tgz#e8eb627483a0a43da3c03f3e35548be5cb0cc153" integrity sha512-rKtvo6a868b5Hu3heneU+L4yEQ4jYKLtjpnPeUdK7h0yzXGmyBTypknlkCvHFBqfX9YlorEiMM6Dnq/5atfHkw== -imurmurhash@*, imurmurhash@^0.1.4: +imurmurhash@^0.1.4: version "0.1.4" resolved "https://registry.yarnpkg.com/imurmurhash/-/imurmurhash-0.1.4.tgz#9218b9b2b928a238b13dc4fb6b6d576f231453ea" integrity sha1-khi5srkoojixPcT7a21XbyMUU+o= @@ -14078,11 +14078,6 @@ lodash._baseflatten@^3.0.0: lodash.isarguments "^3.0.0" lodash.isarray "^3.0.0" -lodash._baseindexof@*: - version "3.1.0" - resolved "https://registry.yarnpkg.com/lodash._baseindexof/-/lodash._baseindexof-3.1.0.tgz#fe52b53a1c6761e42618d654e4a25789ed61822c" - integrity sha1-/lK1OhxnYeQmGNZU5KJXie1hgiw= - lodash._baseuniq@~4.6.0: version "4.6.0" resolved "https://registry.yarnpkg.com/lodash._baseuniq/-/lodash._baseuniq-4.6.0.tgz#0ebb44e456814af7905c6212fa2c9b2d51b841e8" @@ -14091,16 +14086,11 @@ lodash._baseuniq@~4.6.0: lodash._createset "~4.0.0" lodash._root "~3.0.0" -lodash._bindcallback@*, lodash._bindcallback@^3.0.0: +lodash._bindcallback@^3.0.0: version "3.0.1" resolved "https://registry.yarnpkg.com/lodash._bindcallback/-/lodash._bindcallback-3.0.1.tgz#e531c27644cf8b57a99e17ed95b35c748789392e" integrity sha1-5THCdkTPi1epnhftlbNcdIeJOS4= -lodash._cacheindexof@*: - version "3.0.2" - resolved "https://registry.yarnpkg.com/lodash._cacheindexof/-/lodash._cacheindexof-3.0.2.tgz#3dc69ac82498d2ee5e3ce56091bafd2adc7bde92" - integrity sha1-PcaayCSY0u5ePOVgkbr9Ktx73pI= - lodash._createassigner@^3.0.0: version "3.1.1" resolved "https://registry.yarnpkg.com/lodash._createassigner/-/lodash._createassigner-3.1.1.tgz#838a5bae2fdaca63ac22dee8e19fa4e6d6970b11" @@ -14110,19 +14100,12 @@ lodash._createassigner@^3.0.0: lodash._isiterateecall "^3.0.0" lodash.restparam "^3.0.0" -lodash._createcache@*: - version "3.1.2" - resolved "https://registry.yarnpkg.com/lodash._createcache/-/lodash._createcache-3.1.2.tgz#56d6a064017625e79ebca6b8018e17440bdcf093" - integrity sha1-VtagZAF2JeeevKa4AY4XRAvc8JM= - dependencies: - lodash._getnative "^3.0.0" - lodash._createset@~4.0.0: version "4.0.3" resolved "https://registry.yarnpkg.com/lodash._createset/-/lodash._createset-4.0.3.tgz#0f4659fbb09d75194fa9e2b88a6644d363c9fe26" integrity sha1-D0ZZ+7CddRlPqeK4imZE02PJ/iY= -lodash._getnative@*, lodash._getnative@^3.0.0: +lodash._getnative@^3.0.0: version "3.9.1" resolved "https://registry.yarnpkg.com/lodash._getnative/-/lodash._getnative-3.9.1.tgz#570bc7dede46d61cdcde687d65d3eecbaa3aaff5" integrity sha1-VwvH3t5G1hzc3mh9ZdPuy6o6r/U= @@ -14340,7 +14323,7 @@ lodash.pick@^4.4.0: resolved "https://registry.yarnpkg.com/lodash.pick/-/lodash.pick-4.4.0.tgz#52f05610fff9ded422611441ed1fc123a03001b3" integrity sha1-UvBWEP/53tQiYRRB7R/BI6AwAbM= -lodash.restparam@*, lodash.restparam@^3.0.0: +lodash.restparam@^3.0.0: version "3.6.1" resolved "https://registry.yarnpkg.com/lodash.restparam/-/lodash.restparam-3.6.1.tgz#936a4e309ef330a7645ed4145986c85ae5b20805" integrity sha1-k2pOMJ7zMKdkXtQUWYbIWuWyCAU= From 3ef7436f3f24d8fd819c6cc4fd1d3402347a0536 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Wed, 12 May 2021 11:40:50 -0400 Subject: [PATCH 08/17] chore: Cleanup SessionManager interface --- ui/desktop/electron-app/src/session/index.js | 23 ++++++++----------- .../electron-app/src/session/manager.js | 23 ++++--------------- 2 files changed, 15 insertions(+), 31 deletions(-) diff --git a/ui/desktop/electron-app/src/session/index.js b/ui/desktop/electron-app/src/session/index.js index be20bff12..34f4440b7 100644 --- a/ui/desktop/electron-app/src/session/index.js +++ b/ui/desktop/electron-app/src/session/index.js @@ -48,16 +48,7 @@ class Session { * @return {boolean} */ get isActive() { - return this.#process.connected; - } - - /** - * Returns session proxy details. - * Only available after successful start of a session. - * @return {object} - */ - get proxyDetails() { - return this.#proxyDetails; + return !this.#process.killed; } /** @@ -71,6 +62,7 @@ class Session { this.#proxyDetails = spawnedSession.response; this.#process = spawnedSession.childProcess; this.#id = this.#proxyDetails.session_id; + return this.#proxyDetails; }); } @@ -79,9 +71,14 @@ class Session { */ stop() { return new Promise((resolve, reject) => { - this.#process.on('close', () => resolve()); - this.#process.on('error', (e) => reject(e)); - this.#process.kill(); + if (this.isActive) { + this.#process.on('close', () => resolve()); + this.#process.on('error', (e) => reject(e)); + this.#process.kill(); + } else { + // Do nothing when process isn't active + resolve(); + } }); } diff --git a/ui/desktop/electron-app/src/session/manager.js b/ui/desktop/electron-app/src/session/manager.js index 840ee76f3..cb25bef93 100644 --- a/ui/desktop/electron-app/src/session/manager.js +++ b/ui/desktop/electron-app/src/session/manager.js @@ -10,7 +10,6 @@ class SessionManager { /** * Start a session and track it. * Returns session proxy details if successfully started. - * Only successfully started sessions will be tracked. * @param {string} addr * @param {string} target_id * @param {string} token @@ -18,33 +17,21 @@ class SessionManager { */ async start(addr, target_id, token, host_id) { const session = new Session(addr, target_id, token, host_id); - return session.start().then(() => { - this.#sessions.push(session); - return session.proxyDetails; - }); + this.#sessions.push(session); + return session.start(); } /** - * Stop a session and remove it from tracked sessions. + * Stop a session using identifier. * @param {string} session_id */ async stopById(session_id) { - const index = this.#sessions.findIndex( - (session) => session.id === session_id - ); - if (index === -1) return; - const session = this.#sessions[index]; - return session.stop().then(() => { - this.#sessions.splice(index, 1); - }); + const session = this.#sessions.find((session) => session.id === session_id); + return session.stop(); } stopAll() { } - - get sessions() { - return this.#sessions; - } } const sessionManager = new SessionManager(); From 4e13ea56c3a4b89c20b451ae8a78677f81eab0e5 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Wed, 12 May 2021 12:35:17 -0400 Subject: [PATCH 09/17] Add bug fix to changelog --- ui/desktop/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ui/desktop/CHANGELOG.md b/ui/desktop/CHANGELOG.md index e72d90c5e..614ad5f8a 100644 --- a/ui/desktop/CHANGELOG.md +++ b/ui/desktop/CHANGELOG.md @@ -2,6 +2,12 @@ Canonical reference for changes, improvements, and bugfixes for Boundary Desktop. +## Next + +### Bug Fixes + +- Close spawned boundary process on session cancel. ([PR](https://github.com/hashicorp/boundary-ui/pull/549)) + ## v1.0.0 [CLI 0.2.0] (2021.04.14) ### New From 56936f78b89c94386206d55aa986079bfa115105 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Wed, 12 May 2021 15:31:28 -0400 Subject: [PATCH 10/17] chore: Update cancel to stop for ipc sessions call Additional cleanup: remove unused async calls, separate session and associated manager models, & attempt encoding origin url as part of sanitization for cli command --- .../routes/scopes/scope/projects/sessions.js | 2 +- .../scope/projects/targets/target/sessions.js | 2 +- .../electron-app/src/helpers/spawn-promise.js | 6 +-- ui/desktop/electron-app/src/ipc/handlers.js | 2 +- .../src/models/session-manager.js | 33 ++++++++++++++++ .../{session/index.js => models/session.js} | 22 +++-------- .../electron-app/src/session/manager.js | 39 ++----------------- ui/desktop/electron-app/src/utils/base62.js | 14 +++++++ .../acceptance/projects/sessions-test.js | 4 +- .../projects/targets/sessions-test.js | 2 +- 10 files changed, 66 insertions(+), 60 deletions(-) create mode 100644 ui/desktop/electron-app/src/models/session-manager.js rename ui/desktop/electron-app/src/{session/index.js => models/session.js} (78%) create mode 100644 ui/desktop/electron-app/src/utils/base62.js diff --git a/ui/desktop/app/routes/scopes/scope/projects/sessions.js b/ui/desktop/app/routes/scopes/scope/projects/sessions.js index 908636f1a..cd11b0569 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/sessions.js +++ b/ui/desktop/app/routes/scopes/scope/projects/sessions.js @@ -84,6 +84,6 @@ export default class ScopesScopeProjectsSessionsRoute extends Route { @notifySuccess('notifications.canceled-success') async cancelSession(session) { await session.cancelSession(); - await this.ipc.invoke('cancel', { session_id: session.id }); + await this.ipc.invoke('stop', { session_id: session.id }); } } diff --git a/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js b/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js index 2263aa448..f84af4eac 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js +++ b/ui/desktop/app/routes/scopes/scope/projects/targets/target/sessions.js @@ -19,6 +19,6 @@ export default class ScopesScopeProjectsTargetsTargetSessionsRoute extends Route @notifySuccess('notifications.canceled-success') async cancelSession(session) { await session.cancelSession(); - await this.ipc.invoke('cancel', { session_id: session.id }); + await this.ipc.invoke('stop', { session_id: session.id }); } } diff --git a/ui/desktop/electron-app/src/helpers/spawn-promise.js b/ui/desktop/electron-app/src/helpers/spawn-promise.js index 86ac21f68..b8ff5d980 100644 --- a/ui/desktop/electron-app/src/helpers/spawn-promise.js +++ b/ui/desktop/electron-app/src/helpers/spawn-promise.js @@ -37,7 +37,7 @@ module.exports = { outputStream += data.toString(); const jsonData = jsonify(outputStream); if (jsonData) { - childProcess.removeListener('data', processData); + childProcess.removeAllListeners(); resolve({ childProcess, response: jsonData }); } }; @@ -50,12 +50,12 @@ module.exports = { const jsonData = jsonify(errorStream); if (jsonData) { const error = jsonData.api_error || jsonData.error; - childProcess.removeListener('error', processError); + childProcess.removeAllListeners(); reject(new Error(error.message)); } }; - childProcess.stderr.on('data', processError); + childProcess.stderr.on('error', processError); }); }, diff --git a/ui/desktop/electron-app/src/ipc/handlers.js b/ui/desktop/electron-app/src/ipc/handlers.js index d3a8d4efd..e727a4b48 100644 --- a/ui/desktop/electron-app/src/ipc/handlers.js +++ b/ui/desktop/electron-app/src/ipc/handlers.js @@ -61,4 +61,4 @@ handle('connect', ({ target_id, token, host_id }) => /** * Cancel an established boundary session spawned process. */ -handle('cancel', ({ session_id }) => sessionManager.stopById(session_id)); +handle('stop', ({ session_id }) => sessionManager.stopById(session_id)); diff --git a/ui/desktop/electron-app/src/models/session-manager.js b/ui/desktop/electron-app/src/models/session-manager.js new file mode 100644 index 000000000..dfcfd923a --- /dev/null +++ b/ui/desktop/electron-app/src/models/session-manager.js @@ -0,0 +1,33 @@ +const Session = require('./session.js'); + +class SessionManager { + #sessions = []; + + /** + * Start a session and track it. + * Returns session proxy details if successfully started. + * @param {string} addr + * @param {string} target_id + * @param {string} token + * @param {string} host_id + */ + start(addr, target_id, token, host_id) { + const session = new Session(addr, target_id, token, host_id); + this.#sessions.push(session); + return session.start(); + } + + /** + * Stop a session using identifier. + * @param {string} session_id + */ + stopById(session_id) { + const session = this.#sessions.find((session) => session.id === session_id); + return session.stop(); + } + + stopAll() { + } +} + +module.exports = SessionManager; diff --git a/ui/desktop/electron-app/src/session/index.js b/ui/desktop/electron-app/src/models/session.js similarity index 78% rename from ui/desktop/electron-app/src/session/index.js rename to ui/desktop/electron-app/src/models/session.js index 34f4440b7..105b7d723 100644 --- a/ui/desktop/electron-app/src/session/index.js +++ b/ui/desktop/electron-app/src/models/session.js @@ -1,16 +1,5 @@ const { spawnAsyncJSONPromise } = require('../helpers/spawn-promise.js'); -/** - * Super paranoid shell quote/escape and validation. Input must be base62. - * @param {string} str - */ -const escapeAndValidateBase62 = (str) => { - const candidate = str.toString(); - if (candidate.match(/^[A-Za-z0-9_]*$/)) return candidate; - throw new Error(` - Could not invoke command: - input contained unsafe characters. - `); -}; +const base62 = require('../utils/base62.js'); class Session { #id; @@ -88,20 +77,21 @@ class Session { */ cliCommand() { const sanitized = { - target_id: escapeAndValidateBase62(this.#targetId), - token: escapeAndValidateBase62(this.#token), + target_id: base62.escapeAndValidate(this.#targetId), + token: base62.escapeAndValidate(this.#token), + addr: encodeURI(this.#addr), }; const command = [ 'connect', `-target-id=${sanitized.target_id}`, `-token=${sanitized.token}`, - `-addr=${this.#addr}`, + `-addr=${sanitized.addr}`, '-format=json', ]; if (this.#hostId) { - sanitized.host_id = escapeAndValidateBase62(this.#hostId); + sanitized.host_id = base62.escapeAndValidate(this.#hostId); command.push(`-host-id=${sanitized.host_id}`); } return command; diff --git a/ui/desktop/electron-app/src/session/manager.js b/ui/desktop/electron-app/src/session/manager.js index cb25bef93..dc96e096f 100644 --- a/ui/desktop/electron-app/src/session/manager.js +++ b/ui/desktop/electron-app/src/session/manager.js @@ -1,38 +1,7 @@ -const Session = require('./index.js'); - -class SessionManager { - #sessions; - - constructor() { - this.#sessions = []; - } - - /** - * Start a session and track it. - * Returns session proxy details if successfully started. - * @param {string} addr - * @param {string} target_id - * @param {string} token - * @param {string} host_id - */ - async start(addr, target_id, token, host_id) { - const session = new Session(addr, target_id, token, host_id); - this.#sessions.push(session); - return session.start(); - } - - /** - * Stop a session using identifier. - * @param {string} session_id - */ - async stopById(session_id) { - const session = this.#sessions.find((session) => session.id === session_id); - return session.stop(); - } - - stopAll() { - } -} +const SessionManager = require('../models/session-manager.js'); +/** + * Singleton for SessionManager + */ const sessionManager = new SessionManager(); module.exports = sessionManager; diff --git a/ui/desktop/electron-app/src/utils/base62.js b/ui/desktop/electron-app/src/utils/base62.js new file mode 100644 index 000000000..0c844b790 --- /dev/null +++ b/ui/desktop/electron-app/src/utils/base62.js @@ -0,0 +1,14 @@ +module.exports = { + /** + * Super paranoid shell quote/escape and validation. Input must be base62. + * @param {string} str + */ + escapeAndValidate: (str) => { + const candidate = str.toString(); + if (candidate.match(/^[A-Za-z0-9_]*$/)) return candidate; + throw new Error(` + Could not invoke command: + input contained unsafe characters. + `); + }, +}; diff --git a/ui/desktop/tests/acceptance/projects/sessions-test.js b/ui/desktop/tests/acceptance/projects/sessions-test.js index 012db14cb..2cdbafc16 100644 --- a/ui/desktop/tests/acceptance/projects/sessions-test.js +++ b/ui/desktop/tests/acceptance/projects/sessions-test.js @@ -245,7 +245,7 @@ module('Acceptance | projects | sessions', function (hooks) { test('cancelling a session', async function (assert) { assert.expect(1); - stubs.ipcService.withArgs('cancel'); + stubs.ipcService.withArgs('stop'); later(async () => { run.cancelTimers(); await click('tbody tr:first-child td:last-child button'); @@ -267,7 +267,7 @@ module('Acceptance | projects | sessions', function (hooks) { test('cancelling a session with ipc error shows notification', async function (assert) { assert.expect(1); - stubs.ipcService.withArgs('cancel').throws(); + stubs.ipcService.withArgs('stop').throws(); later(async () => { run.cancelTimers(); await click('tbody tr:first-child td:last-child button'); diff --git a/ui/desktop/tests/acceptance/projects/targets/sessions-test.js b/ui/desktop/tests/acceptance/projects/targets/sessions-test.js index 9fde4b210..e6031ab88 100644 --- a/ui/desktop/tests/acceptance/projects/targets/sessions-test.js +++ b/ui/desktop/tests/acceptance/projects/targets/sessions-test.js @@ -271,7 +271,7 @@ module('Acceptance | projects | targets | sessions', function (hooks) { test('cancelling a session with ipc error shows notification', async function (assert) { assert.expect(1); - stubs.ipcService.withArgs('cancel').throws(); + stubs.ipcService.withArgs('stop').throws(); later(async () => { run.cancelTimers(); await click('tbody tr:first-child td:last-child button'); From 13146975e2c7e5a2e10b59ef4baf5951fb0b58f8 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Wed, 12 May 2021 16:57:27 -0400 Subject: [PATCH 11/17] fix: Add missing cli folder in electron-app --- ui/desktop/electron-app/.gitignore | 2 +- ui/desktop/electron-app/src/cli/index.js | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 ui/desktop/electron-app/src/cli/index.js diff --git a/ui/desktop/electron-app/.gitignore b/ui/desktop/electron-app/.gitignore index 0928c4d31..75a95bf75 100644 --- a/ui/desktop/electron-app/.gitignore +++ b/ui/desktop/electron-app/.gitignore @@ -93,7 +93,7 @@ ember-dist/ ember-test/ # CLI -cli/ +/cli/ # Autoupdate nextVersion/ diff --git a/ui/desktop/electron-app/src/cli/index.js b/ui/desktop/electron-app/src/cli/index.js new file mode 100644 index 000000000..cfb5153a5 --- /dev/null +++ b/ui/desktop/electron-app/src/cli/index.js @@ -0,0 +1,23 @@ +const path = require('path'); +const { spawnSync } = require('../helpers/spawn-promise.js'); + +const cliPath = async () => path.resolve(__dirname, '..', 'cli', 'boundary'); + +module.exports = { + // Check boundary cli existence + exists: () => Boolean(cliPath()), + + // Returns JSON-formatted version information from the CLI + version: () => { + const command = ['-v']; + const rawOutput = spawnSync(command); + let gitRevision = /Git Revision:\s*(?.*)\n/.exec(rawOutput); + let versionNumber = /Version Number:\s*(?.*)\n/.exec(rawOutput); + if (gitRevision) gitRevision = gitRevision.groups.rev; + if (versionNumber) versionNumber = versionNumber.groups.ver; + const formatted = versionNumber + ? `CLI Version: ${versionNumber}\nCLI Commit: ${gitRevision}` + : `CLI Commit: ${gitRevision}`; + return { gitRevision, versionNumber, formatted }; + }, +}; From ae8e20b8e1e8a6f9fae118dfae6e090a40e88896 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Thu, 13 May 2021 10:23:22 -0400 Subject: [PATCH 12/17] refactor: Create services folder in desktop/electron-app --- ui/desktop/electron-app/src/ipc/handlers.js | 2 +- .../src/{session/manager.js => services/session-manager.js} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename ui/desktop/electron-app/src/{session/manager.js => services/session-manager.js} (100%) diff --git a/ui/desktop/electron-app/src/ipc/handlers.js b/ui/desktop/electron-app/src/ipc/handlers.js index e727a4b48..befb10dda 100644 --- a/ui/desktop/electron-app/src/ipc/handlers.js +++ b/ui/desktop/electron-app/src/ipc/handlers.js @@ -3,7 +3,7 @@ const isDev = require('electron-is-dev'); const handle = require('./ipc-handler.js'); const boundaryCli = require('../cli/index.js'); const origin = require('../origin/index.js'); -const sessionManager = require('../session/manager.js'); +const sessionManager = require('../services/session-manager.js'); /** * Returns the current runtime origin, which is used by the main thread to diff --git a/ui/desktop/electron-app/src/session/manager.js b/ui/desktop/electron-app/src/services/session-manager.js similarity index 100% rename from ui/desktop/electron-app/src/session/manager.js rename to ui/desktop/electron-app/src/services/session-manager.js From 9f32b8cdc3654387ab7f09decf219d701c9380ee Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Thu, 13 May 2021 10:36:28 -0400 Subject: [PATCH 13/17] refactor: Origin runtime settings into origin service desktop --- .../src/config/content-security-policy.js | 2 +- .../src/helpers/net-request-promise.js | 24 ++++++++++++++ ui/desktop/electron-app/src/index.js | 2 +- ui/desktop/electron-app/src/ipc/handlers.js | 2 +- .../index.js => models/runtime-settings.js} | 33 ++----------------- .../electron-app/src/services/origin.js | 8 +++++ 6 files changed, 37 insertions(+), 34 deletions(-) create mode 100644 ui/desktop/electron-app/src/helpers/net-request-promise.js rename ui/desktop/electron-app/src/{origin/index.js => models/runtime-settings.js} (59%) create mode 100644 ui/desktop/electron-app/src/services/origin.js diff --git a/ui/desktop/electron-app/src/config/content-security-policy.js b/ui/desktop/electron-app/src/config/content-security-policy.js index 8895178ef..1de13924e 100644 --- a/ui/desktop/electron-app/src/config/content-security-policy.js +++ b/ui/desktop/electron-app/src/config/content-security-policy.js @@ -1,4 +1,4 @@ -const origin = require('../origin/index.js'); +const origin = require('../services/origin.js'); const isDev = require('electron-is-dev'); const csp = { diff --git a/ui/desktop/electron-app/src/helpers/net-request-promise.js b/ui/desktop/electron-app/src/helpers/net-request-promise.js new file mode 100644 index 000000000..eb48426c7 --- /dev/null +++ b/ui/desktop/electron-app/src/helpers/net-request-promise.js @@ -0,0 +1,24 @@ +const { net } = require('electron'); + +const requestTimeoutSeconds = 10; +// Simple promise wrapper around Electron's net.request feature. +// https://www.electronjs.org/docs/api/client-request +const netRequestPromise = (url) => + new Promise((resolve, reject) => { + try { + const request = net.request(url); + // We don't worry about canceling the request timeout timer once a + // request ends because: request.abort() has no effect after a request + // ends and the promise will already have been completed, so a rejection + // has no effect either. + const requestTimeout = setTimeout(() => { + request.abort(); + reject(new Error('Request timeout')); + }, requestTimeoutSeconds * 1000); + request.on('error', reject); + request.on('response', resolve); + request.end(); + } catch (e) { + reject(e); + } + }); diff --git a/ui/desktop/electron-app/src/index.js b/ui/desktop/electron-app/src/index.js index 3ff94bf64..8e61bea48 100644 --- a/ui/desktop/electron-app/src/index.js +++ b/ui/desktop/electron-app/src/index.js @@ -15,7 +15,7 @@ const { } = require('electron'); require('./ipc/handlers.js'); -const origin = require('./origin/index.js'); +const origin = require('./services/origin.js'); const { generateCSPHeader } = require('./config/content-security-policy.js'); const menu = require('./config/menu.js'); diff --git a/ui/desktop/electron-app/src/ipc/handlers.js b/ui/desktop/electron-app/src/ipc/handlers.js index befb10dda..f32a00e28 100644 --- a/ui/desktop/electron-app/src/ipc/handlers.js +++ b/ui/desktop/electron-app/src/ipc/handlers.js @@ -2,7 +2,7 @@ const { shell } = require('electron'); const isDev = require('electron-is-dev'); const handle = require('./ipc-handler.js'); const boundaryCli = require('../cli/index.js'); -const origin = require('../origin/index.js'); +const origin = require('../services/origin.js'); const sessionManager = require('../services/session-manager.js'); /** diff --git a/ui/desktop/electron-app/src/origin/index.js b/ui/desktop/electron-app/src/models/runtime-settings.js similarity index 59% rename from ui/desktop/electron-app/src/origin/index.js rename to ui/desktop/electron-app/src/models/runtime-settings.js index 5cc150c32..fdd91608c 100644 --- a/ui/desktop/electron-app/src/origin/index.js +++ b/ui/desktop/electron-app/src/models/runtime-settings.js @@ -1,34 +1,7 @@ -const { net } = require('electron'); - -const requestTimeoutSeconds = 10; -// Simple promise wrapper around Electron's net.request feature. -// https://www.electronjs.org/docs/api/client-request -const netRequestPromise = (url) => - new Promise((resolve, reject) => { - try { - const request = net.request(url); - // We don't worry about canceling the request timeout timer once a - // request ends because: request.abort() has no effect after a request - // ends and the promise will already have been completed, so a rejection - // has no effect either. - const requestTimeout = setTimeout(() => { - request.abort(); - reject(new Error('Request timeout')); - }, requestTimeoutSeconds * 1000); - request.on('error', reject); - request.on('response', resolve); - request.end(); - } catch (e) { - reject(e); - } - }); - -// Provides a singleton class instance to enable a consistent view of -// runtime settings across the application. +const netRequestPromise = require('../helpers/net-request-promise.js'); // Runtime settings are any configuration, especially user-specified, // set up at runtime. For example, the Boundary origin is a runtime setting. - class RuntimeSettings { // Internal private origin is exposed via getter/setter below. #origin = undefined; @@ -84,6 +57,4 @@ class RuntimeSettings { } } -const runtimeSettings = new RuntimeSettings(); - -module.exports = runtimeSettings; +module.exports = RuntimeSettings; diff --git a/ui/desktop/electron-app/src/services/origin.js b/ui/desktop/electron-app/src/services/origin.js new file mode 100644 index 000000000..8d33f04fc --- /dev/null +++ b/ui/desktop/electron-app/src/services/origin.js @@ -0,0 +1,8 @@ +const RuntimeSesttings = require('../models/runtime-settings.js'); + +// Provides a singleton class instance to enable a consistent view of +// runtime settings across the application. + +const runtimeSettings = new RuntimeSettings(); + +module.exports = runtimeSettings; From cc01bddea32c27788ad00eb858cd8421e346b2f3 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Thu, 13 May 2021 12:50:01 -0400 Subject: [PATCH 14/17] refactor: Create net-request-promise helper for runtimesettin --- ui/desktop/electron-app/src/cli/index.js | 2 +- .../electron-app/src/helpers/net-request-promise.js | 2 ++ ui/desktop/electron-app/src/models/session.js | 11 +++++------ ui/desktop/electron-app/src/services/origin.js | 2 +- .../src/utils/{base62.js => sanitizer.js} | 10 +++++++++- 5 files changed, 18 insertions(+), 9 deletions(-) rename ui/desktop/electron-app/src/utils/{base62.js => sanitizer.js} (58%) diff --git a/ui/desktop/electron-app/src/cli/index.js b/ui/desktop/electron-app/src/cli/index.js index cfb5153a5..0aaad4e31 100644 --- a/ui/desktop/electron-app/src/cli/index.js +++ b/ui/desktop/electron-app/src/cli/index.js @@ -1,7 +1,7 @@ const path = require('path'); const { spawnSync } = require('../helpers/spawn-promise.js'); -const cliPath = async () => path.resolve(__dirname, '..', 'cli', 'boundary'); +const cliPath = async () => path.resolve(__dirname, '..', '..', 'cli', 'boundary'); module.exports = { // Check boundary cli existence diff --git a/ui/desktop/electron-app/src/helpers/net-request-promise.js b/ui/desktop/electron-app/src/helpers/net-request-promise.js index eb48426c7..c86030c79 100644 --- a/ui/desktop/electron-app/src/helpers/net-request-promise.js +++ b/ui/desktop/electron-app/src/helpers/net-request-promise.js @@ -22,3 +22,5 @@ const netRequestPromise = (url) => reject(e); } }); + +module.exports = netRequestPromise; diff --git a/ui/desktop/electron-app/src/models/session.js b/ui/desktop/electron-app/src/models/session.js index 105b7d723..20d5b37a4 100644 --- a/ui/desktop/electron-app/src/models/session.js +++ b/ui/desktop/electron-app/src/models/session.js @@ -1,5 +1,5 @@ const { spawnAsyncJSONPromise } = require('../helpers/spawn-promise.js'); -const base62 = require('../utils/base62.js'); +const sanitizer = require('../utils/sanitizer.js'); class Session { #id; @@ -77,21 +77,20 @@ class Session { */ cliCommand() { const sanitized = { - target_id: base62.escapeAndValidate(this.#targetId), - token: base62.escapeAndValidate(this.#token), - addr: encodeURI(this.#addr), + target_id: sanitizer.base62EscapeAndValidate(this.#targetId), + token: sanitizer.base62EscapeAndValidate(this.#token), }; const command = [ 'connect', `-target-id=${sanitized.target_id}`, `-token=${sanitized.token}`, - `-addr=${sanitized.addr}`, + `-addr=${this.#addr}`, '-format=json', ]; if (this.#hostId) { - sanitized.host_id = base62.escapeAndValidate(this.#hostId); + sanitized.host_id = sanitizer.base62EscapeAndValidate(this.#hostId); command.push(`-host-id=${sanitized.host_id}`); } return command; diff --git a/ui/desktop/electron-app/src/services/origin.js b/ui/desktop/electron-app/src/services/origin.js index 8d33f04fc..8c5634079 100644 --- a/ui/desktop/electron-app/src/services/origin.js +++ b/ui/desktop/electron-app/src/services/origin.js @@ -1,4 +1,4 @@ -const RuntimeSesttings = require('../models/runtime-settings.js'); +const RuntimeSettings = require('../models/runtime-settings.js'); // Provides a singleton class instance to enable a consistent view of // runtime settings across the application. diff --git a/ui/desktop/electron-app/src/utils/base62.js b/ui/desktop/electron-app/src/utils/sanitizer.js similarity index 58% rename from ui/desktop/electron-app/src/utils/base62.js rename to ui/desktop/electron-app/src/utils/sanitizer.js index 0c844b790..bc48fe0cf 100644 --- a/ui/desktop/electron-app/src/utils/base62.js +++ b/ui/desktop/electron-app/src/utils/sanitizer.js @@ -1,9 +1,11 @@ +const { quote } = require('shell-quote'); + module.exports = { /** * Super paranoid shell quote/escape and validation. Input must be base62. * @param {string} str */ - escapeAndValidate: (str) => { + base62EscapeAndValidate: (str) => { const candidate = str.toString(); if (candidate.match(/^[A-Za-z0-9_]*$/)) return candidate; throw new Error(` @@ -11,4 +13,10 @@ module.exports = { input contained unsafe characters. `); }, + /** + * Return a quoted string suitable for using in shell commands. + * @param {string} str + * @returns {string} + */ + shellEscape: (str) => quote([str]), }; From 1397df807f83d144ee563822f74fbde525b94073 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Fri, 14 May 2021 10:40:50 -0400 Subject: [PATCH 15/17] fix: Add url validation to origin addr in desktop --- ui/desktop/electron-app/src/models/session.js | 3 ++- ui/desktop/electron-app/src/utils/sanitizer.js | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/ui/desktop/electron-app/src/models/session.js b/ui/desktop/electron-app/src/models/session.js index 20d5b37a4..6ec9d3ce8 100644 --- a/ui/desktop/electron-app/src/models/session.js +++ b/ui/desktop/electron-app/src/models/session.js @@ -79,13 +79,14 @@ class Session { const sanitized = { target_id: sanitizer.base62EscapeAndValidate(this.#targetId), token: sanitizer.base62EscapeAndValidate(this.#token), + addr: sanitizer.urlValidate(this.#addr), }; const command = [ 'connect', `-target-id=${sanitized.target_id}`, `-token=${sanitized.token}`, - `-addr=${this.#addr}`, + `-addr=${sanitized.addr}`, '-format=json', ]; diff --git a/ui/desktop/electron-app/src/utils/sanitizer.js b/ui/desktop/electron-app/src/utils/sanitizer.js index bc48fe0cf..582f2532c 100644 --- a/ui/desktop/electron-app/src/utils/sanitizer.js +++ b/ui/desktop/electron-app/src/utils/sanitizer.js @@ -1,4 +1,4 @@ -const { quote } = require('shell-quote'); +const { URL } = require('url'); module.exports = { /** @@ -18,5 +18,16 @@ module.exports = { * @param {string} str * @returns {string} */ - shellEscape: (str) => quote([str]), + urlValidate: (str) => { + try { + const url = new URL(str); + return url.href; + } catch (e) { + // Parse error + throw new Error(` + Could not invoke command: + input contained unsafe url. + `); + } + }, }; From 82a0c7c17cb912bc19ef63fc11ce80eaa01def86 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Fri, 14 May 2021 14:29:26 -0400 Subject: [PATCH 16/17] refactor: Rename origin service to runtime-settings --- .../src/config/content-security-policy.js | 6 +++--- ui/desktop/electron-app/src/index.js | 4 ++-- ui/desktop/electron-app/src/ipc/handlers.js | 12 ++++++------ .../src/services/{origin.js => runtime-settings.js} | 0 4 files changed, 11 insertions(+), 11 deletions(-) rename ui/desktop/electron-app/src/services/{origin.js => runtime-settings.js} (100%) diff --git a/ui/desktop/electron-app/src/config/content-security-policy.js b/ui/desktop/electron-app/src/config/content-security-policy.js index 1de13924e..19ccafc93 100644 --- a/ui/desktop/electron-app/src/config/content-security-policy.js +++ b/ui/desktop/electron-app/src/config/content-security-policy.js @@ -1,4 +1,4 @@ -const origin = require('../services/origin.js'); +const runtimeSettings = require('../services/runtime-settings.js'); const isDev = require('electron-is-dev'); const csp = { @@ -28,9 +28,9 @@ const generateCSPHeader = () => { const policy = Object.assign({}, csp); // If an origin is specified, add it to the connect-src directive - if (origin.origin) { + if (runtimeSettings.origin) { policy['connect-src'] = policy['connect-src'].slice(); - policy['connect-src'].push(origin.origin); + policy['connect-src'].push(runtimeSettings.origin); } return Object.keys(policy) diff --git a/ui/desktop/electron-app/src/index.js b/ui/desktop/electron-app/src/index.js index 8e61bea48..93c9c3024 100644 --- a/ui/desktop/electron-app/src/index.js +++ b/ui/desktop/electron-app/src/index.js @@ -15,7 +15,7 @@ const { } = require('electron'); require('./ipc/handlers.js'); -const origin = require('./services/origin.js'); +const runtimeSettings = require('./services/runtime-settings.js'); const { generateCSPHeader } = require('./config/content-security-policy.js'); const menu = require('./config/menu.js'); @@ -131,7 +131,7 @@ app.on('ready', async () => { // If the user-specified origin changes, reload the page so that // the CSP can be refreshed with the this source allowed - origin.onOriginChange(() => mainWindow.loadURL(emberAppURL)); + runtimeSettings.onOriginChange(() => mainWindow.loadURL(emberAppURL)); // If you want to open up dev tools programmatically, call // mainWindow.openDevTools(); diff --git a/ui/desktop/electron-app/src/ipc/handlers.js b/ui/desktop/electron-app/src/ipc/handlers.js index f32a00e28..e3bdcbabd 100644 --- a/ui/desktop/electron-app/src/ipc/handlers.js +++ b/ui/desktop/electron-app/src/ipc/handlers.js @@ -2,28 +2,28 @@ const { shell } = require('electron'); const isDev = require('electron-is-dev'); const handle = require('./ipc-handler.js'); const boundaryCli = require('../cli/index.js'); -const origin = require('../services/origin.js'); const sessionManager = require('../services/session-manager.js'); +const runtimeSettings = require('../services/runtime-settings.js'); /** * Returns the current runtime origin, which is used by the main thread to * rewrite the CSP to allow requests. */ -handle('getOrigin', () => origin.origin); +handle('getOrigin', () => runtimeSettings.origin); /** * Sets the origin to be used in the content security policy and triggers * a main window reload. */ handle('setOrigin', async (requestOrigin) => { - await origin.validateOrigin(requestOrigin); - origin.origin = requestOrigin; + await runtimeSettings.validateOrigin(requestOrigin); + runtimeSettings.origin = requestOrigin; }); /** * Resets the origin. */ -handle('resetOrigin', async () => origin.resetOrigin()); +handle('resetOrigin', async () => runtimeSettings.resetOrigin()); /** * Opens the specified URL in an external browser. Only secure HTTPs URLs are @@ -55,7 +55,7 @@ handle('cliExists', () => boundaryCli.exists()); * Establishes a boundary session and returns session details. */ handle('connect', ({ target_id, token, host_id }) => - sessionManager.start(origin.origin, target_id, token, host_id) + sessionManager.start(runtimeSettings.origin, target_id, token, host_id) ); /** diff --git a/ui/desktop/electron-app/src/services/origin.js b/ui/desktop/electron-app/src/services/runtime-settings.js similarity index 100% rename from ui/desktop/electron-app/src/services/origin.js rename to ui/desktop/electron-app/src/services/runtime-settings.js From 1a0650ff7cee6bc9db6c6a8a381a0de946d43130 Mon Sep 17 00:00:00 2001 From: Susmitha Girumala Date: Fri, 14 May 2021 15:00:24 -0400 Subject: [PATCH 17/17] fix: Add url validation to origin validation in desktop --- ui/desktop/electron-app/src/ipc/handlers.js | 6 ++++-- ui/desktop/electron-app/src/utils/sanitizer.js | 7 ++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/ui/desktop/electron-app/src/ipc/handlers.js b/ui/desktop/electron-app/src/ipc/handlers.js index e3bdcbabd..529fc34a5 100644 --- a/ui/desktop/electron-app/src/ipc/handlers.js +++ b/ui/desktop/electron-app/src/ipc/handlers.js @@ -4,6 +4,7 @@ const handle = require('./ipc-handler.js'); const boundaryCli = require('../cli/index.js'); const sessionManager = require('../services/session-manager.js'); const runtimeSettings = require('../services/runtime-settings.js'); +const sanitizer = require('../utils/sanitizer.js'); /** * Returns the current runtime origin, which is used by the main thread to @@ -16,8 +17,9 @@ handle('getOrigin', () => runtimeSettings.origin); * a main window reload. */ handle('setOrigin', async (requestOrigin) => { - await runtimeSettings.validateOrigin(requestOrigin); - runtimeSettings.origin = requestOrigin; + const origin = sanitizer.urlValidate(requestOrigin); + await runtimeSettings.validateOrigin(origin); + runtimeSettings.origin = origin; }); /** diff --git a/ui/desktop/electron-app/src/utils/sanitizer.js b/ui/desktop/electron-app/src/utils/sanitizer.js index 582f2532c..9e635b0d5 100644 --- a/ui/desktop/electron-app/src/utils/sanitizer.js +++ b/ui/desktop/electron-app/src/utils/sanitizer.js @@ -21,13 +21,10 @@ module.exports = { urlValidate: (str) => { try { const url = new URL(str); - return url.href; + return `${url.protocol}://${url.host}`; } catch (e) { // Parse error - throw new Error(` - Could not invoke command: - input contained unsafe url. - `); + throw new Error(`URL ${str} could not be validated.`); } }, };