Skip to content

Commit

Permalink
Merge pull request #771 from salesforcecli/mdonnalley/improvements
Browse files Browse the repository at this point in the history 
feat: support allowed github urls
  • Loading branch information
@iowillhoit
iowillhoit authored Mar 28, 2024
2 parents b66760c + 48759d6 commit c11247b
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 62 deletions.
2 changes: 1 addition & 1 deletion messages/verify.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
17 changes: 15 additions & 2 deletions src/hooks/verifyInstallSignature.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
*/

import { Hook } from '@oclif/core';
import { Logger } from '@salesforce/core';
import { Logger, Messages } from '@salesforce/core';
import { ux } from '@oclif/core';
import {
ConfigContext,
doInstallationCodeSigningVerification,
doPrompt,
InstallationVerification,
VerificationConfig,
isAllowListed,
} from '../shared/installationVerification.js';

import { NpmName } from '../shared/NpmName.js';
Expand Down Expand Up @@ -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,
Expand All @@ -64,6 +65,18 @@ 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();
}
Expand Down
59 changes: 39 additions & 20 deletions src/shared/installationVerification.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<boolean> {
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
*/
Expand Down Expand Up @@ -236,23 +262,11 @@ export class InstallationVerification implements Verifier {
}

public async isAllowListed(): Promise<boolean> {
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(),
});
}

/**
Expand Down Expand Up @@ -437,9 +451,14 @@ export class VerificationConfig {
}
}

export async function doPrompt(): Promise<void> {
export async function doPrompt(plugin?: string): Promise<void> {
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.
Expand Down Expand Up @@ -473,7 +492,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.'), '');
}
Expand Down
77 changes: 40 additions & 37 deletions test/hooks/verifyInstallSignatureHook.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -34,52 +39,50 @@ 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(() => {
sandbox.restore();
});

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;
});
Expand Down
4 changes: 2 additions & 2 deletions test/nuts/plugin-install.nut.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand Down

0 comments on commit c11247b

Please sign in to comment.