diff --git a/server/src/nest/plugins/paths.ts b/server/src/nest/plugins/paths.ts index 276a3343c..32b4fd630 100644 --- a/server/src/nest/plugins/paths.ts +++ b/server/src/nest/plugins/paths.ts @@ -23,9 +23,31 @@ export function pluginsDataRoot(): string { return readEnv().plugins.dataDir || path.join(DATA_ROOT, 'plugins-data'); } +/** + * Plugin ids reach these helpers straight from a route parameter — `POST + * /api/admin/plugins/:id/uninstall` hands its id to pluginCodeDir, and what comes + * back goes to a recursive remove. Express decodes `%2F` AFTER routing, so + * `..%2F..%2Fuploads` still matches the route and arrives here as + * `../../uploads`; a plain path.join then points outside the plugin tree. + * + * Deliberately narrower than the manifest's ID_RE, which demands 3–40 lowercase + * characters and is the right rule at install time. This one only has to keep an + * id from leaving its parent, so it stays permissive about length and case and + * refuses exactly what steers a path: separators, a leading dot, and the + * drive/stream colon. + */ +const SAFE_PLUGIN_ID = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; + +function safePluginId(id: string): string { + if (typeof id !== 'string' || id.length > 64 || !SAFE_PLUGIN_ID.test(id)) { + throw new Error(`invalid plugin id: ${JSON.stringify(id)}`); + } + return id; +} + /** A plugin's installed code directory (contains trek-plugin.json + server/index.js). */ export function pluginCodeDir(id: string): string { - return path.join(pluginsCodeRoot(), id); + return path.join(pluginsCodeRoot(), safePluginId(id)); } /** @@ -60,7 +82,7 @@ export function ensurePluginModuleType(codeDir: string): void { /** A plugin's writable data directory (its own sqlite file + any blobs). */ export function pluginDataDir(id: string): string { - return path.join(pluginsDataRoot(), id); + return path.join(pluginsDataRoot(), safePluginId(id)); } /** The plugin's own sqlite file — opened by the HOST, reached by the plugin only via RPC. */ diff --git a/server/tests/unit/plugins/protocol-paths.test.ts b/server/tests/unit/plugins/protocol-paths.test.ts index c7d98b0a2..a24ca888f 100644 --- a/server/tests/unit/plugins/protocol-paths.test.ts +++ b/server/tests/unit/plugins/protocol-paths.test.ts @@ -5,7 +5,7 @@ import { describe, it, expect, afterEach } from 'vitest'; import { isKnownPermission, METHOD_PERMISSION, KNOWN_METHODS, HOOK_PERMISSION } from '../../../src/nest/plugins/protocol/envelope'; import path from 'node:path'; -import { pluginsCodeRoot, pluginsDataRoot, pluginCodeDir, pluginDbFile, resolveChildEntry, serverCodeRoot, pluginPermissionArgs, pluginRealCodeDir, ensurePluginModuleType } from '../../../src/nest/plugins/paths'; +import { pluginsCodeRoot, pluginsDataRoot, pluginCodeDir, pluginDataDir, pluginDbFile, resolveChildEntry, serverCodeRoot, pluginPermissionArgs, pluginRealCodeDir, ensurePluginModuleType } from '../../../src/nest/plugins/paths'; afterEach(() => { delete process.env.TREK_PLUGINS_DIR; @@ -101,3 +101,44 @@ describe('paths', () => { } }); }); + +/** + * The id these helpers get comes off a route parameter — POST + * /api/admin/plugins/:id/uninstall walks it into pluginCodeDir and hands the result + * to a recursive remove. Express decodes %2F only AFTER routing, so a traversal + * still matches the route and arrives here decoded. + */ +describe('plugin id containment', () => { + const traversals = [ + '../../uploads', + '../..', + '..', + '.', + 'a/b', + 'a\b', + 'C:\Windows', + '.hidden', + '', + ]; + + it('refuses any id that could point outside its parent', () => { + for (const id of traversals) { + expect(() => pluginCodeDir(id), id).toThrow(/invalid plugin id/); + expect(() => pluginDataDir(id), id).toThrow(/invalid plugin id/); + } + }); + + it('still accepts the ids plugins actually use', () => { + for (const id of ['x', 'a1', '001', 'flight-tracker', 'Bad-Id', 'trip.todos', 'koffi_and_friends']) { + expect(path.basename(pluginCodeDir(id))).toBe(id); + expect(path.basename(pluginDataDir(id))).toBe(id); + } + }); + + it('keeps every accepted id inside the plugin roots', () => { + for (const id of ['x', 'flight-tracker', 'a.b', 'a-b_c']) { + expect(pluginCodeDir(id).startsWith(pluginsCodeRoot() + path.sep)).toBe(true); + expect(pluginDataDir(id).startsWith(pluginsDataRoot() + path.sep)).toBe(true); + } + }); +});