From bc59289f73d80319280e2bcbc2255207a429ab84 Mon Sep 17 00:00:00 2001 From: Maurice Date: Tue, 25 Aug 2026 10:58:32 +0200 Subject: [PATCH] fix(plugins): keep a plugin id from pointing outside its own directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /api/admin/plugins/:id/uninstall` walks its route parameter into pluginCodeDir() and hands the result to a recursive remove. Express decodes `%2F` after routing, so `..%2F..%2Fuploads` matches the route and reaches the helper as `../../uploads` — path.join then lands in the uploads tree, and `deleteData` gets the data root the same way. Verified against the express version in the lockfile. pluginCodeDir and pluginDataDir now reject an id that could leave its parent. Narrower than the manifest's ID_RE on purpose: that rule wants 3-40 lowercase characters and belongs at install time, while this one only has to make the id unable to steer a path, so it stays permissive about length and case. Every other filesystem sink in the backend already has its guard — storage keys, extraction, the wiki, backup filenames. This one did not. --- server/src/nest/plugins/paths.ts | 26 ++++++++++- .../tests/unit/plugins/protocol-paths.test.ts | 43 ++++++++++++++++++- 2 files changed, 66 insertions(+), 3 deletions(-) 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); + } + }); +});