Plugin Repo: Prevent plugin authors from marking their own plugins as recommended (#9462)

.eslintignore

@@ -968,6 +968,8 @@ packages/plugin-repo-cli/lib/types.js
 packages/plugin-repo-cli/lib/updateReadme.test.js
 packages/plugin-repo-cli/lib/updateReadme.js
 packages/plugin-repo-cli/lib/utils.js
+packages/plugin-repo-cli/lib/validateUntrustedManifest.test.js
+packages/plugin-repo-cli/lib/validateUntrustedManifest.js
 packages/plugins/ToggleSidebars/api/index.js
 packages/plugins/ToggleSidebars/api/types.js
 packages/plugins/ToggleSidebars/src/index.js

.gitignore

@@ -950,6 +950,8 @@ packages/plugin-repo-cli/lib/types.js
 packages/plugin-repo-cli/lib/updateReadme.test.js
 packages/plugin-repo-cli/lib/updateReadme.js
 packages/plugin-repo-cli/lib/utils.js
+packages/plugin-repo-cli/lib/validateUntrustedManifest.test.js
+packages/plugin-repo-cli/lib/validateUntrustedManifest.js
 packages/plugins/ToggleSidebars/api/index.js
 packages/plugins/ToggleSidebars/api/types.js
 packages/plugins/ToggleSidebars/src/index.js

packages/plugin-repo-cli/index.ts

@@ -7,10 +7,7 @@ require('source-map-support').install();
 import * as fs from 'fs-extra';
 import * as path from 'path';
 import * as process from 'process';
-import validatePluginId from '@joplin/lib/services/plugins/utils/validatePluginId';
-import validatePluginVersion from '@joplin/lib/services/plugins/utils/validatePluginVersion';
 import { resolveRelativePathWithinDir, gitPullTry, gitRepoCleanTry, gitRepoClean } from '@joplin/tools/tool-utils.js';
-import checkIfPluginCanBeAdded from './lib/checkIfPluginCanBeAdded';
 import updateReadme from './lib/updateReadme';
 import { NpmPackage } from './lib/types';
 import gitCompareUrl from './lib/gitCompareUrl';
@@ -18,6 +15,7 @@ import commandUpdateRelease from './commands/updateRelease';
 import { isJoplinPluginPackage, readJsonFile } from './lib/utils';
 import { applyManifestOverrides, getObsoleteManifests, getSupersededPackages, readManifestOverrides } from './lib/overrideUtils';
 import { execCommand } from '@joplin/utils';
+import validateUntrustedManifest from './lib/validateUntrustedManifest';
 
 function pluginInfoFromSearchResults(results: any[]): NpmPackage[] {
 	const output: NpmPackage[] = [];
@@ -63,17 +61,12 @@ async function extractPluginFilesFromPackage(existingManifests: any, workDir: st
 	if (!(await fs.pathExists(manifestFilePath))) throw new Error(`Could not find manifest file at ${manifestFilePath}`);
 	if (!(await fs.pathExists(pluginFilePath))) throw new Error(`Could not find plugin file at ${pluginFilePath}`);
 
-	// At this point, we need to check the manifest ID as it's used in various
-	// places including as directory name and object key in manifests.json, so
-	// it needs to be correct. It's mostly for security reasons. The other
-	// manifest properties are checked when the plugin is loaded into the app.
 	const manifest = await readJsonFile(manifestFilePath);
-	validatePluginId(manifest.id);
-	validatePluginVersion(manifest.version);
-
 	manifest._npm_package_name = packageName;
 
-	checkIfPluginCanBeAdded(existingManifests, manifest);
+	// We need to validate the manifest to make sure the plugin author isn't
+	// trying to override an existing plugin, use an invalid ID, etc..
+	validateUntrustedManifest(manifest, existingManifests);
 
 	const pluginDestDir = resolveRelativePathWithinDir(destDir, manifest.id);
 	await fs.mkdirp(pluginDestDir);

packages/plugin-repo-cli/lib/validateUntrustedManifest.test.ts

@@ -0,0 +1,96 @@
+
+import validateUntrustedManifest from './validateUntrustedManifest';
+
+const originalManifests = {
+	'joplin-plugin.this.is.a.test': {
+		id: 'joplin-plugin.this.is.a.test',
+		_npm_package_name: 'joplin-plugin-this-is-a-test',
+		version: '0.0.1',
+	},
+};
+
+// Note: Most of the checks below have additional tests in other files.
+// This test suite is primarily to ensure that the checks are present.
+
+describe('validateUntrustedManifest', () => {
+	test('should only allow valid plugin IDs', () => {
+		const badManifest = {
+			id: 'bad',
+			_npm_package_name: 'joplin-plugin-test',
+			version: '1.2.34',
+		};
+
+		expect(
+			() => validateUntrustedManifest(badManifest, originalManifests),
+		).toThrow('ID cannot be shorter than');
+
+
+		const goodManifest = {
+			...badManifest,
+			id: 'this.plugin.has.a.better.id',
+		};
+
+		expect(
+			() => validateUntrustedManifest(goodManifest, originalManifests),
+		).not.toThrow();
+	});
+
+	test('should only allow valid plugin versions', () => {
+		const badManifest = {
+			id: 'joplin-plugin.this.is.a.test',
+			_npm_package_name: 'joplin-plugin-this-is-a-test',
+			version: 'bad!',
+		};
+
+		expect(
+			() => validateUntrustedManifest(badManifest, originalManifests),
+		).toThrow();
+
+
+		const goodManifest = {
+			...badManifest,
+			version: '0.0.2',
+		};
+
+		expect(
+			() => validateUntrustedManifest(goodManifest, originalManifests),
+		).not.toThrow();
+	});
+
+	test('should not allow plugin authors to mark their own plugins as recommended', () => {
+		const newManifest1 = {
+			id: 'joplin-plugin.this.is.another.test',
+			_recommended: true,
+			_npm_package_name: 'joplin-plugin-another-test',
+			version: '1.2.3',
+		};
+
+		expect(
+			() => validateUntrustedManifest(newManifest1, originalManifests),
+		).toThrow('mark itself as recommended');
+
+		// Should also throw for falsey values
+		const newManifest2 = {
+			id: 'joplin-plugin.yet.another.test',
+			_recommended: '',
+			_npm_package_name: 'another-test',
+			version: '1.2.3',
+		};
+
+		expect(
+			() => validateUntrustedManifest(newManifest2, originalManifests),
+		).toThrow('mark itself as recommended');
+	});
+
+	test('should not allow changing the NPM package for an existing plugin', () => {
+		const newManifest = {
+			id: 'joplin-plugin.this.is.a.test',
+			_npm_package_name: 'joplin-plugin-test',
+			version: '0.0.2',
+		};
+
+		expect(
+			() => validateUntrustedManifest(newManifest, originalManifests),
+		).toThrow();
+	});
+});

packages/plugin-repo-cli/lib/validateUntrustedManifest.ts

@@ -0,0 +1,29 @@
+import validatePluginId from '@joplin/lib/services/plugins/utils/validatePluginId';
+import validatePluginVersion from '@joplin/lib/services/plugins/utils/validatePluginVersion';
+import checkIfPluginCanBeAdded from './checkIfPluginCanBeAdded';
+
+// Assumes that
+// 1. manifest._npm_package_name is correct,
+// 2. other fields were set by the plugin author and are thus untrusted.
+const validateUntrustedManifest = (manifest: any, existingManifests: any) => {
+	// At this point, we need to check the manifest ID as it's used in various
+	// places including as directory name and object key in manifests.json, so
+	// it needs to be correct. It's mostly for security reasons. The other
+	// manifest properties are checked when the plugin is loaded into the app.
+	validatePluginId(manifest.id);
+	validatePluginVersion(manifest.version);
+
+	// This prevents a plugin author from marking their own plugin as _recommended
+	// or _built_in.
+	if (typeof manifest._recommended !== 'undefined') {
+		throw new Error(`Plugin ${manifest.id} cannot mark itself as recommended.`);
+	}
+
+	if (typeof manifest._built_in !== 'undefined') {
+		throw new Error(`Plugin ${manifest.id} cannot mark itself as built-in.`);
+	}
+
+	checkIfPluginCanBeAdded(existingManifests, manifest);
+};
+
+export default validateUntrustedManifest;