From 4d8d947191ee8a4c8245c8e4d3855856eee06270 Mon Sep 17 00:00:00 2001 From: Mike Donnalley Date: Wed, 27 Mar 2024 09:38:30 -0600 Subject: [PATCH 1/5] fix: show name in prompt --- messages/verify.md | 2 +- src/hooks/verifyInstallSignature.ts | 2 +- src/shared/installationVerification.ts | 11 ++++++++--- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/messages/verify.md b/messages/verify.md index ab90745a..a88f9743 100644 --- a/messages/verify.md +++ b/messages/verify.md @@ -38,7 +38,7 @@ A digital signature is specified for this plugin but it didn't verify against th # InstallConfirmation -This plugin isn't signed by Salesforce. Only install the plugin if you trust its creator. Do you want to continue the installation?, +%s isn't signed by Salesforce. Only install the plugin if you trust its creator. Do you want to continue the installation?, # SuggestAllowList diff --git a/src/hooks/verifyInstallSignature.ts b/src/hooks/verifyInstallSignature.ts index ea5dde03..8dcefbe5 100644 --- a/src/hooks/verifyInstallSignature.ts +++ b/src/hooks/verifyInstallSignature.ts @@ -65,7 +65,7 @@ export const hook: Hook.PluginsPreinstall = async function (options) { this.error(error); } } else { - await doPrompt(); + await doPrompt(options.plugin.url); } }; diff --git a/src/shared/installationVerification.ts b/src/shared/installationVerification.ts index 5a5275a0..07769f22 100644 --- a/src/shared/installationVerification.ts +++ b/src/shared/installationVerification.ts @@ -437,9 +437,14 @@ export class VerificationConfig { } } -export async function doPrompt(): Promise { +export async function doPrompt(plugin?: string): Promise { const messages = Messages.loadMessages('@salesforce/plugin-trust', 'verify'); - if (!(await prompts.confirm({ message: messages.getMessage('InstallConfirmation'), ms: 30_000 }))) { + if ( + !(await prompts.confirm({ + message: messages.getMessage('InstallConfirmation', [plugin ?? 'This plugin']), + ms: 30_000, + })) + ) { throw new SfError('The user canceled the plugin installation.', 'InstallationCanceledError'); } // they approved the plugin. Let them know how to automate this. @@ -473,7 +478,7 @@ export async function doInstallationCodeSigningVerification( if (!verificationConfig.verifier) { throw new Error('VerificationConfig.verifier is not set.'); } - return await doPrompt(); + return await doPrompt(plugin.plugin); } else if (err.name === 'PluginNotFound' || err.name === 'PluginAccessDenied') { throw setErrorName(new SfError(err.message ?? 'The user canceled the plugin installation.'), ''); } From 81cf5932fcbd1c234dbf740e8a3483fdac9446e4 Mon Sep 17 00:00:00 2001 From: Mike Donnalley Date: Wed, 27 Mar 2024 09:59:31 -0600 Subject: [PATCH 2/5] feat: support github urls in allow list file --- src/hooks/verifyInstallSignature.ts | 19 ++++++++-- src/shared/installationVerification.ts | 48 +++++++++++++++++--------- 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/hooks/verifyInstallSignature.ts b/src/hooks/verifyInstallSignature.ts index 8dcefbe5..45bfc223 100644 --- a/src/hooks/verifyInstallSignature.ts +++ b/src/hooks/verifyInstallSignature.ts @@ -6,7 +6,7 @@ */ import { Hook } from '@oclif/core'; -import { Logger } from '@salesforce/core'; +import { Logger, Messages } from '@salesforce/core'; import { ux } from '@oclif/core'; import { ConfigContext, @@ -14,6 +14,7 @@ import { doPrompt, InstallationVerification, VerificationConfig, + isAllowListed, } from '../shared/installationVerification.js'; import { NpmName } from '../shared/NpmName.js'; @@ -43,7 +44,7 @@ export const hook: Hook.PluginsPreinstall = async function (options) { npmName.tag = npmName.tag.slice(1); } - const configContext: ConfigContext = { + const configContext = { cacheDir: options.config.cacheDir, configDir: options.config.configDir, dataDir: options.config.dataDir, @@ -64,8 +65,20 @@ export const hook: Hook.PluginsPreinstall = async function (options) { logger.debug(error.message); this.error(error); } + } else if (options.plugin.url) { + const isAllowed = await isAllowListed({ + logger: await Logger.child('verifyInstallSignature'), + name: options.plugin.url, + configPath: options.config.configDir, + }); + if (isAllowed) { + const messages = Messages.loadMessages('@salesforce/plugin-trust', 'verify'); + ux.log(messages.getMessage('SkipSignatureCheck', [options.plugin.url])); + } else { + await doPrompt(options.plugin.url); + } } else { - await doPrompt(options.plugin.url); + await doPrompt(); } }; diff --git a/src/shared/installationVerification.ts b/src/shared/installationVerification.ts index 07769f22..44a95214 100644 --- a/src/shared/installationVerification.ts +++ b/src/shared/installationVerification.ts @@ -156,6 +156,32 @@ const errorHandlerForVerify = (err: Error): Error => { export const getNpmRegistry = (): URL => new URL(process.env.SF_NPM_REGISTRY ?? process.env.SFDX_NPM_REGISTRY ?? DEFAULT_REGISTRY); +export async function isAllowListed({ + logger, + configPath, + name, +}: { + logger: Logger; + configPath: string; + name?: string; +}): Promise { + const allowListedFilePath = path.join(configPath, ALLOW_LIST_FILENAME); + logger.debug(`isAllowListed | allowlistFilePath: ${allowListedFilePath}`); + let fileContent: string; + try { + fileContent = await fs.promises.readFile(allowListedFilePath, 'utf8'); + const allowlistArray = JSON.parse(fileContent) as string[]; + logger.debug('isAllowListed | Successfully parsed allowlist.'); + return name ? allowlistArray.includes(name) : false; + } catch (err) { + if (err instanceof Error && 'code' in err && err.code === 'ENOENT') { + return false; + } else { + throw err; + } + } +} + /** * class for verifying a digital signature pack of an npm */ @@ -236,23 +262,11 @@ export class InstallationVerification implements Verifier { } public async isAllowListed(): Promise { - const logger = await this.getLogger(); - const allowListedFilePath = path.join(this.getConfigPath() ?? '', ALLOW_LIST_FILENAME); - logger.debug(`isAllowListed | allowlistFilePath: ${allowListedFilePath}`); - let fileContent: string; - try { - fileContent = await fs.promises.readFile(allowListedFilePath, 'utf8'); - const allowlistArray = JSON.parse(fileContent) as string[]; - logger.debug('isAllowListed | Successfully parsed allowlist.'); - const nameToFind = this.pluginNpmName?.toString(); - return nameToFind ? allowlistArray.includes(nameToFind) : false; - } catch (err) { - if (err instanceof Error && 'code' in err && err.code === 'ENOENT') { - return false; - } else { - throw err; - } - } + return isAllowListed({ + logger: await this.getLogger(), + configPath: this.getConfigPath() ?? '', + name: this.pluginNpmName?.toString(), + }); } /** From 913c35ea410a29eac94c7233d917ca389fa9119f Mon Sep 17 00:00:00 2001 From: Mike Donnalley Date: Wed, 27 Mar 2024 10:19:05 -0600 Subject: [PATCH 3/5] test: add test for allowed github urls --- test/hooks/verifyInstallSignatureHook.test.ts | 77 ++++++++++--------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/test/hooks/verifyInstallSignatureHook.test.ts b/test/hooks/verifyInstallSignatureHook.test.ts index 1b80e1e5..e41c7faf 100644 --- a/test/hooks/verifyInstallSignatureHook.test.ts +++ b/test/hooks/verifyInstallSignatureHook.test.ts @@ -4,22 +4,27 @@ * Licensed under the BSD 3-Clause license. * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ -import { expect, config } from 'chai'; +import fs from 'node:fs'; +import { assert, expect, config as chaiConfig } from 'chai'; import sinon from 'sinon'; - import { stubMethod } from '@salesforce/ts-sinon'; import { prompts } from '@salesforce/sf-plugins-core'; +import { Config, ux } from '@oclif/core'; import { InstallationVerification, VerificationConfig } from '../../src/shared/installationVerification.js'; -import { hook } from '../../src/hooks/verifyInstallSignature.js'; -config.truncateThreshold = 0; +chaiConfig.truncateThreshold = 0; describe('plugin install hook', () => { let sandbox: sinon.SinonSandbox; let vConfig: VerificationConfig; let promptSpy: sinon.SinonSpy; let verifySpy: sinon.SinonSpy; + let config: Config; + + before(async () => { + config = await Config.load(import.meta.url); + }); beforeEach(() => { sandbox = sinon.createSandbox(); @@ -34,6 +39,7 @@ describe('plugin install hook', () => { stubMethod(sandbox, vConfig.verifier, 'isAllowListed').callsFake(async () => false); promptSpy = stubMethod(sandbox, prompts, 'confirm').resolves(false); + stubMethod(sandbox, ux, 'log').callsFake(() => {}); }); afterEach(() => { @@ -41,45 +47,42 @@ describe('plugin install hook', () => { }); it('exits by calling this.error', async () => { - let calledError = false; - await hook.call( - // @ts-expect-error not a valid mock for context - { - error: () => (calledError = true), - }, - { - plugin: { name: 'test', type: 'npm' }, - config: {}, - } - ); - expect(calledError).to.equal(true); - }); - - it('should prompt for repo urls', async () => { try { - await hook.call( - // @ts-expect-error not a valid mock for context - {}, - { - plugin: { name: 'test', type: 'repo' }, - config: {}, - } - ); + await config.runHook('plugins:preinstall:verify:signature', { + plugin: { name: 'test', type: 'npm', tag: 'latest' }, + }); + assert.fail('Expected error to be thrown'); } catch (error) { expect(error).to.have.property('name', 'InstallationCanceledError'); - expect(promptSpy.called).to.be.true; } }); - it('should skip signature verification for JIT plugins with matching version', async () => { - await hook.call( - // @ts-expect-error not a valid mock for context - {}, - { - plugin: { name: '@ns/test', type: 'npm', tag: '1.2.3' }, - config: { pjson: { oclif: { jitPlugins: { '@ns/test': '1.2.3' } } } }, - } - ); + it('should prompt for repo urls that are not allowlisted', async () => { + const result = await config.runHook('plugins:preinstall:verify:signature', { + plugin: { url: 'https://github.com/oclif/plugin-version', type: 'repo' }, + }); + + expect(result.failures).to.have.length(1); + expect(result.failures[0].error).to.have.property('name', 'InstallationCanceledError'); + expect(promptSpy.called).to.be.true; + }); + + it('should not prompt for repo urls that are allowlisted', async () => { + stubMethod(sandbox, fs.promises, 'readFile').resolves(JSON.stringify(['https://github.com/oclif/plugin-version'])); + const result = await config.runHook('plugins:preinstall:verify:signature', { + plugin: { url: 'https://github.com/oclif/plugin-version', type: 'repo' }, + }); + + expect(result.failures).to.have.length(0); + expect(result.successes).to.have.length(1); + expect(promptSpy.called).to.be.false; + }); + + it.only('should skip signature verification for JIT plugins with matching version', async () => { + sandbox.stub(config, 'pjson').value({ oclif: { jitPlugins: { '@ns/test': '1.2.3' } } }); + await config.runHook('plugins:preinstall:verify:signature', { + plugin: { name: '@ns/test', type: 'npm', tag: '1.2.3' }, + }); expect(promptSpy.called).to.be.false; expect(verifySpy.called).to.be.false; }); From 3567e702ee53391da2f9224e3bff83c764d18833 Mon Sep 17 00:00:00 2001 From: Mike Donnalley Date: Wed, 27 Mar 2024 10:23:45 -0600 Subject: [PATCH 4/5] test: linting error --- test/nuts/plugin-install.nut.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/nuts/plugin-install.nut.ts b/test/nuts/plugin-install.nut.ts index 728ae4ab..390d5df6 100644 --- a/test/nuts/plugin-install.nut.ts +++ b/test/nuts/plugin-install.nut.ts @@ -73,7 +73,7 @@ describe('plugins:install commands', () => { ); expect(replaceAllWhitespace(result.stdout)).to.contain( - replaceAllWhitespace(messages.getMessage('InstallConfirmation')) + replaceAllWhitespace(messages.getMessage('InstallConfirmation', [UNSIGNED_MODULE_NAME])) ); expect(result.stdout).to.contain('Do you want to continue the installation?'); expect(result.stderr).to.contain('The user canceled the plugin installation'); @@ -89,7 +89,7 @@ describe('plugins:install commands', () => { } ); expect(replaceAllWhitespace(result.stdout)).to.contain( - replaceAllWhitespace(messages.getMessage('InstallConfirmation')) + replaceAllWhitespace(messages.getMessage('InstallConfirmation', [UNSIGNED_MODULE_NAME])) ); expect(result.stdout).to.contain('Do you want to continue the installation?'); expect(result.stdout).to.contain('Finished digital signature check'); From 48759d6abf8f7ac358d2fea385b1323011d566ab Mon Sep 17 00:00:00 2001 From: Mike Donnalley Date: Thu, 28 Mar 2024 12:20:51 -0600 Subject: [PATCH 5/5] fix: update install confirmation message --- messages/verify.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/messages/verify.md b/messages/verify.md index a88f9743..c95c194b 100644 --- a/messages/verify.md +++ b/messages/verify.md @@ -38,7 +38,7 @@ A digital signature is specified for this plugin but it didn't verify against th # InstallConfirmation -%s isn't signed by Salesforce. Only install the plugin if you trust its creator. Do you want to continue the installation?, +%s isn't signed by Salesforce. Only install the plugin if you trust its creator. Do you want to continue the installation? # SuggestAllowList