diff --git a/backend/package.json b/backend/package.json index 8f4da062..7f3e3247 100644 --- a/backend/package.json +++ b/backend/package.json @@ -85,7 +85,8 @@ "utf-8-validate": "^6.0.4", "validator": "^13.12.0", "websocket": "^1.0.35", - "winston": "^3.15.0" + "winston": "^3.15.0", + "yauzl": "^3.4.0" }, "devDependencies": { "@faker-js/faker": "^8.0.0", diff --git a/backend/src/app.js b/backend/src/app.js index bc9f99c8..43f4d4ec 100644 --- a/backend/src/app.js +++ b/backend/src/app.js @@ -118,8 +118,8 @@ app.use('/v2', routesV2); app.use('/static', express.static(path.join(__dirname, '../static'))); app.use('/builtin-widgets', express.static(path.join(__dirname, '../static/builtin-widgets'))); app.use('/images', express.static(path.join(__dirname, '../static/images'))); -app.use('/static/plugin', express.static(path.join(__dirname, '../static/plugin'))); -app.use('/plugin', express.static(path.join(__dirname, '../static/plugin'))); +app.use('/static/plugin', express.static(path.join(__dirname, '../static/plugin'), { dotfiles: 'ignore' })); +app.use('/plugin', express.static(path.join(__dirname, '../static/plugin'), { dotfiles: 'ignore' })); // Serve uploaded files with date-based directory structure app.use('/d', express.static(path.join(__dirname, '../static/uploads'), { setHeaders: (res, path) => { diff --git a/backend/src/controllers/plugin.controller.js b/backend/src/controllers/plugin.controller.js index 0ebaef68..26ba20d0 100644 --- a/backend/src/controllers/plugin.controller.js +++ b/backend/src/controllers/plugin.controller.js @@ -10,7 +10,7 @@ const httpStatus = require('http-status'); const fs = require('fs'); const fsp = require('fs/promises'); const path = require('path'); -const { spawn } = require('child_process'); +const yauzl = require('yauzl'); const catchAsync = require('../utils/catchAsync'); const { pluginService } = require('../services'); const pick = require('../utils/pick'); @@ -76,8 +76,7 @@ const updatePlugin = catchAsync(async (req, res) => { updated_by: req.user.id, }; const isAdmin = - (Array.isArray(req.user.roles) && req.user.roles.includes('admin')) || - (await pluginService.isAdminUser(req.user.id)); + (Array.isArray(req.user.roles) && req.user.roles.includes('admin')) || (await pluginService.isAdminUser(req.user.id)); const actor = { id: req.user.id, isAdmin, @@ -126,33 +125,169 @@ async function findEntryFile(rootDir, candidates = ['index.js', 'index.html']) { } /* eslint-enable no-await-in-loop, no-continue, no-restricted-syntax */ +/** + * Safely extract a zip archive into a target directory. + * Rejects path traversal (../, absolute paths) and symlink entries + * to prevent arbitrary file write/read (CWE-22, CWE-59). + * + * Transactional: on any failure, the zip file descriptor and any in-flight + * read/write streams are released, and any partially extracted content is + * removed from targetDir. This prevents file-descriptor leaks and orphaned + * partial directories from accumulating on disk over time. targetDir should + * be a fresh, dedicated directory (the caller creates it immediately before + * calling this function). + */ +async function safeExtractZip(zipPath, targetDir) { + const resolvedTarget = path.resolve(targetDir); + return new Promise((resolve, reject) => { + let settled = false; + let zipfile = null; + let activeReadStream = null; + let activeWriteStream = null; + + // Centralized failure path: release the zip fd and any in-flight streams, + // then remove partially extracted content so disk does not accumulate. + const fail = (err) => { + if (settled) return; + settled = true; + if (activeReadStream) { + activeReadStream.destroy(); + } + if (activeWriteStream) { + activeWriteStream.destroy(); + } + if (zipfile) { + try { + zipfile.close(); + } catch (_) { + // Already auto-closed — ignore. + } + } + // eslint-disable-next-line security/detect-non-literal-fs-filename + fsp + .rm(resolvedTarget, { recursive: true, force: true }) + .catch(() => {}) + .finally(() => reject(err)); + }; + + yauzl.open(zipPath, { lazyEntries: true, autoClose: true }, (err, zf) => { + if (err) { + // No zipfile handle yet, but still clean up the (empty) target dir. + return fail(err); + } + zipfile = zf; + + zipfile.readEntry(); + zipfile.on('entry', (entry) => { + // Reject absolute paths and path traversal in entry names + if (path.isAbsolute(entry.fileName) || entry.fileName.includes('..')) { + return fail(new ApiError(httpStatus.BAD_REQUEST, `Unsafe zip entry: ${entry.fileName}`)); + } + + const entryPath = path.resolve(resolvedTarget, entry.fileName); + // Containment check: resolved entry must be within target directory + if (entryPath !== resolvedTarget && !entryPath.startsWith(resolvedTarget + path.sep)) { + return fail(new ApiError(httpStatus.BAD_REQUEST, `Unsafe zip entry: ${entry.fileName}`)); + } + + // Unix file mode: reject symlinks and non-regular files + // eslint-disable-next-line no-bitwise + const mode = (entry.externalFileAttributes >>> 16) & 0o170000; + if (mode === 0o120000) { + return fail(new ApiError(httpStatus.BAD_REQUEST, `Symlink entries are not allowed: ${entry.fileName}`)); + } + + if (/\/$/.test(entry.fileName)) { + // Directory entry + fsp + .mkdir(entryPath, { recursive: true }) + .then(() => zipfile.readEntry()) + .catch(fail); + } else { + // File entry — ensure parent directory exists + fsp + .mkdir(path.dirname(entryPath), { recursive: true }) + .then(() => { + zipfile.openReadStream(entry, (readErr, readStream) => { + if (readErr) return fail(readErr); + activeReadStream = readStream; + readStream.on('error', fail); + const writeStream = fs.createWriteStream(entryPath); + activeWriteStream = writeStream; + writeStream.on('error', fail); + writeStream.on('close', () => { + activeReadStream = null; + activeWriteStream = null; + zipfile.readEntry(); + }); + readStream.pipe(writeStream); + }); + }) + .catch(fail); + } + }); + zipfile.on('end', () => { + if (!settled) { + settled = true; + resolve(); + } + }); + zipfile.on('error', fail); + }); + }); +} + const uploadInternalPlugin = catchAsync(async (req, res) => { const { slug } = req.params; if (!req.file) throw new ApiError(httpStatus.BAD_REQUEST, 'No file uploaded'); + const actor = { + id: req.user.id, + isAdmin: + (Array.isArray(req.user.roles) && req.user.roles.includes('admin')) || (await pluginService.isAdminUser(req.user.id)), + }; + + // Authorization: if a plugin with this slug already exists, verify + // ownership BEFORE extracting any files (CWE-862). + const existing = await pluginService.getPluginBySlug(slug); + if (existing) { + const isOwner = String(existing.created_by) === String(actor.id); + if (!isOwner && !actor.isAdmin) { + throw new ApiError( + httpStatus.FORBIDDEN, + 'This plugin name is already used by another account. Please choose a different name and upload again.', + ); + } + } + // Ensure base plugin directory exists await ensureDir(PLUGIN_DIR); const pluginPath = path.join(PLUGIN_DIR, slug); - await ensureDir(pluginPath); - // Extract zip to target dir using system unzip (no extra npm deps) - await new Promise((resolve, reject) => { - const unzip = spawn('unzip', ['-o', req.file.path, '-d', pluginPath]); - unzip.on('error', reject); - unzip.on('close', (code) => { - if (code === 0) resolve(); - else reject(new Error(`unzip exited with code ${code}`)); - }); - }); + // Defense-in-depth: verify resolved path stays within PLUGIN_DIR (CWE-22) + const resolvedPluginPath = path.resolve(pluginPath); + if (resolvedPluginPath !== PLUGIN_DIR && !resolvedPluginPath.startsWith(PLUGIN_DIR + path.sep)) { + throw new ApiError(httpStatus.BAD_REQUEST, 'Invalid plugin slug'); + } - // Remove uploaded temp file + await ensureDir(pluginPath); + + // Safely extract zip (rejects symlinks and path traversal — CWE-22, CWE-59). + // safeExtractZip is transactional: on failure it releases the zip fd and + // removes any partially extracted content from pluginPath. try { - // Temp upload path is provided by the trusted multer middleware. - // eslint-disable-next-line security/detect-non-literal-fs-filename - fs.unlinkSync(req.file.path); - } catch (e) { - // eslint-disable-next-line no-console - console.error(e); + await safeExtractZip(req.file.path, pluginPath); + } finally { + // Always remove the uploaded temp file (success or failure) so multer + // uploads don't accumulate under static/uploads over time. + try { + // Temp upload path is provided by the trusted multer middleware. + // eslint-disable-next-line security/detect-non-literal-fs-filename + fs.unlinkSync(req.file.path); + } catch (e) { + // eslint-disable-next-line no-console + console.error(e); + } } // Try to detect entry file (index.js preferred, fallback index.html) @@ -164,23 +299,7 @@ const uploadInternalPlugin = catchAsync(async (req, res) => { const safeRel = entryRel.replace(/^\/+/, ''); const pluginUrl = `/plugin/${slug}/${safeRel}`.replace(/\\/g, '/'); - const actor = { - id: req.user.id, - isAdmin: - (Array.isArray(req.user.roles) && req.user.roles.includes('admin')) || - (await pluginService.isAdminUser(req.user.id)), - }; - - const existing = await pluginService.getPluginBySlug(slug); - if (existing) { - const isOwner = String(existing.created_by) === String(actor.id); - if (!isOwner && !actor.isAdmin) { - throw new ApiError( - httpStatus.FORBIDDEN, - 'This plugin name is already used by another account. Please choose a different name and upload again.' - ); - } // Plugin already exists — update its URL in place const plugin = await pluginService.upsertPluginBySlug(slug, { is_internal: true, @@ -199,8 +318,7 @@ const uploadInternalPlugin = catchAsync(async (req, res) => { const removePlugin = catchAsync(async (req, res) => { const isAdmin = - (Array.isArray(req.user.roles) && req.user.roles.includes('admin')) || - (await pluginService.isAdminUser(req.user.id)); + (Array.isArray(req.user.roles) && req.user.roles.includes('admin')) || (await pluginService.isAdminUser(req.user.id)); const actor = { id: req.user.id, isAdmin, @@ -219,4 +337,6 @@ module.exports = { updatePlugin, uploadInternalPlugin, removePlugin, + // Exported for unit testing only (not used by route handlers) + safeExtractZip, }; diff --git a/backend/src/routes/v2/system/plugin.route.js b/backend/src/routes/v2/system/plugin.route.js index 1c3303c8..a7e8f415 100644 --- a/backend/src/routes/v2/system/plugin.route.js +++ b/backend/src/routes/v2/system/plugin.route.js @@ -34,8 +34,6 @@ router.delete('/:id', pluginController.removePlugin); // Upload and extract internal plugin zip router.post( '/upload/:slug', - auth(), - // checkPermission(PERMISSIONS.ADMIN), validate(pluginValidation.uploadInternal), upload.single('file'), pluginController.uploadInternalPlugin, diff --git a/backend/src/validations/plugin.validation.js b/backend/src/validations/plugin.validation.js index 338b5219..923bc966 100644 --- a/backend/src/validations/plugin.validation.js +++ b/backend/src/validations/plugin.validation.js @@ -1,5 +1,5 @@ // Copyright (c) 2025 Eclipse Foundation. -// +// // This program and the accompanying materials are made available under the // terms of the MIT License which is available at // https://opensource.org/licenses/MIT. @@ -72,7 +72,7 @@ const updatePlugin = { const uploadInternal = { params: Joi.object().keys({ - slug: Joi.string().required(), + slug: Joi.string().required().custom(slug), }), }; @@ -84,5 +84,3 @@ module.exports = { updatePlugin, uploadInternal, }; - - diff --git a/backend/tests/fixtures/build_zip.py b/backend/tests/fixtures/build_zip.py new file mode 100644 index 00000000..b8ab7424 --- /dev/null +++ b/backend/tests/fixtures/build_zip.py @@ -0,0 +1,42 @@ +#!/usr/bin/env python3 +"""Build a zip file from a JSON spec for security regression tests. + +Usage: python3 build_zip.py + +Spec is a JSON array of entries: + {"name": "path/in/zip", "content": "bytes-or-str", "type": "file|dir|symlink"} + - file: regular file (mode 0644) + - dir: directory entry (mode 0755, name should end with "/") + - symlink: symbolic link entry (mode 120777), content is the link target +""" +import json +import sys +import zipfile + + +def main(): + out_path = sys.argv[1] + spec = json.loads(sys.argv[2]) + + with zipfile.ZipFile(out_path, "w", zipfile.ZIP_DEFLATED) as z: + for entry in spec: + name = entry["name"] + etype = entry.get("type", "file") + content = entry.get("content", "") + if isinstance(content, str): + content = content.encode("utf-8") + + info = zipfile.ZipInfo(name) + info.create_system = 3 # Unix + if etype == "symlink": + # 0o120777 = symlink; external_attr stores mode in high 16 bits + info.external_attr = (0o120777 << 16) + elif etype == "dir": + info.external_attr = (0o040755 << 16) + else: + info.external_attr = (0o100644 << 16) + z.writestr(info, content) + + +if __name__ == "__main__": + main() \ No newline at end of file diff --git a/backend/tests/unit/controllers/plugin.upload.auth.test.js b/backend/tests/unit/controllers/plugin.upload.auth.test.js new file mode 100644 index 00000000..af4ed32c --- /dev/null +++ b/backend/tests/unit/controllers/plugin.upload.auth.test.js @@ -0,0 +1,144 @@ +// Copyright (c) 2025 Eclipse Foundation. +// +// This program and the accompanying materials are made available under the +// terms of the MIT License which is available at +// https://opensource.org/licenses/MIT. +// +// SPDX-License-Identifier: MIT + +// Regression test for the authorization fix in PR #614 (issue #719, CWE-862): +// the ownership check must run BEFORE any zip extraction, so an unauthorized +// upload to an existing slug owned by another user returns 403 without writing +// files to disk or mutating the plugin record. +// +// The plugin service is mocked so no DB is required. Because the controller +// throws at the authorization gate (before ensureDir/safeExtractZip), no real +// filesystem writes occur, so fs is left un-mocked. + +jest.mock('../../../src/services', () => ({ + pluginService: { + getPluginBySlug: jest.fn(), + isAdminUser: jest.fn(), + upsertPluginBySlug: jest.fn(), + getPluginById: jest.fn(), + queryPlugins: jest.fn(), + queryAdminPlugins: jest.fn(), + createPlugin: jest.fn(), + updatePluginById: jest.fn(), + deletePluginById: jest.fn(), + }, +})); + +const path = require('path'); +const fs = require('fs'); +const httpStatus = require('http-status'); +const { uploadInternalPlugin } = require('../../../src/controllers/plugin.controller'); +const { pluginService } = require('../../../src/services'); + +// PLUGIN_DIR as resolved inside the controller (backend/static/plugin) +const PLUGIN_DIR = path.join(__dirname, '../../../static/plugin'); + +// catchAsync does not return its inner promise, so we wait on next()/res.send() +// (whichever fires first) to know the handler has settled. +function runHandler(req) { + let settle; + const done = new Promise((resolve) => { + settle = resolve; + }); + let nextErr; + let sent; + const res = { + status() { + return res; + }, + send(body) { + sent = body; + settle(); + }, + }; + const next = (err) => { + nextErr = err; + settle(); + }; + uploadInternalPlugin(req, res, next); + return done.then(() => ({ nextErr, sent })); +} + +describe('Plugin upload authorization (PR #614 / issue #719, CWE-862)', () => { + const slug = 'pr614-auth-test-slug'; + + beforeEach(() => { + jest.clearAllMocks(); + pluginService.isAdminUser.mockResolvedValue(false); + }); + + afterAll(() => { + // Defensive cleanup in case any dir was created. + const slugDir = path.join(PLUGIN_DIR, slug); + if (fs.existsSync(slugDir)) { + fs.rmSync(slugDir, { recursive: true, force: true }); + } + }); + + it('returns 403 and writes nothing when the slug is owned by another user', async () => { + pluginService.getPluginBySlug.mockResolvedValue({ created_by: 'other-user-id' }); + + const { nextErr, sent } = await runHandler({ + params: { slug }, + user: { id: 'me-user-id', roles: ['user'] }, + file: { path: '/tmp/pr614-does-not-matter.zip' }, + }); + + // No response was sent; the error was forwarded to next(). + expect(sent).toBeUndefined(); + expect(nextErr).toBeDefined(); + expect(nextErr.statusCode).toBe(httpStatus.FORBIDDEN); + + // Authorization was checked (getPluginBySlug called), but the plugin record + // was NOT mutated (extraction never happened). + expect(pluginService.getPluginBySlug).toHaveBeenCalledWith(slug); + expect(pluginService.upsertPluginBySlug).not.toHaveBeenCalled(); + + // No plugin directory should have been created on disk — the throw happens + // before ensureDir/safeExtractZip. + expect(fs.existsSync(path.join(PLUGIN_DIR, slug))).toBe(false); + }); + + it('returns 403 when the requester is a non-owner non-admin (roles missing admin)', async () => { + pluginService.getPluginBySlug.mockResolvedValue({ created_by: 'other-user-id' }); + pluginService.isAdminUser.mockResolvedValue(false); + + const { nextErr } = await runHandler({ + params: { slug }, + user: { id: 'me-user-id', roles: [] }, + file: { path: '/tmp/pr614-does-not-matter.zip' }, + }); + + expect(nextErr).toBeDefined(); + expect(nextErr.statusCode).toBe(httpStatus.FORBIDDEN); + expect(pluginService.upsertPluginBySlug).not.toHaveBeenCalled(); + }); + + it('allows the owner through the ownership gate (no 403 at the gate)', async () => { + pluginService.getPluginBySlug.mockResolvedValue({ created_by: 'me-user-id' }); + pluginService.isAdminUser.mockResolvedValue(false); + + const { nextErr } = await runHandler({ + params: { slug }, + user: { id: 'me-user-id', roles: ['user'] }, + // Non-existent zip: the request passes the ownership gate and then fails + // at extraction. The failure must NOT be a 403 (it is a file/zip error). + file: { path: '/tmp/pr614-definitely-does-not-exist.zip' }, + }); + + expect(nextErr).toBeDefined(); + // The owner passed the ownership gate; the subsequent error is NOT FORBIDDEN. + expect(nextErr.statusCode).not.toBe(httpStatus.FORBIDDEN); + + // Clean up the plugin dir the gate allowed to be created. + const slugDir = path.join(PLUGIN_DIR, slug); + if (fs.existsSync(slugDir)) { + fs.rmSync(slugDir, { recursive: true, force: true }); + } + }); +}); diff --git a/backend/tests/unit/controllers/plugin.upload.security.test.js b/backend/tests/unit/controllers/plugin.upload.security.test.js new file mode 100644 index 00000000..7328620c --- /dev/null +++ b/backend/tests/unit/controllers/plugin.upload.security.test.js @@ -0,0 +1,212 @@ +// Copyright (c) 2025 Eclipse Foundation. +// +// This program and the accompanying materials are made available under the +// terms of the MIT License which is available at +// https://opensource.org/licenses/MIT. +// +// SPDX-License-Identifier: MIT + +// Regression tests for the security fixes in PR #614 (issue #719): +// 1. CWE-22 — path traversal via slug (validation rejects ../ and absolute paths) +// 2. CWE-22/59 — safeExtractZip rejects path-traversal, absolute, and symlink entries +// +// These tests exercise the PR's real production code: +// - `uploadInternal` Joi schema (validations/plugin.validation.js) +// - `safeExtractZip` (controllers/plugin.controller.js, exported for testing) + +const path = require('path'); +const fs = require('fs'); +const fsp = require('fs/promises'); +const os = require('os'); +const { execFileSync } = require('child_process'); + +const { uploadInternal } = require('../../../src/validations/plugin.validation'); +const { safeExtractZip } = require('../../../src/controllers/plugin.controller'); + +const BUILD_ZIP = path.join(__dirname, '../../fixtures/build_zip.py'); + +// Build a zip from a spec array using the python helper. +function buildZip(zipPath, entries) { + execFileSync('python3', [BUILD_ZIP, zipPath, JSON.stringify(entries)], { stdio: 'pipe' }); + return zipPath; +} + +describe('Plugin upload security (PR #614 / issue #719)', () => { + describe('CWE-22: slug validation rejects path traversal', () => { + const schema = uploadInternal.params; + + const valid = ['my-plugin', 'plugin1', 'abc', 'valid-slug-2']; + valid.forEach((slug) => { + it(`accepts a valid slug: ${slug}`, () => { + const { error, value } = schema.validate({ slug }); + expect(error).toBeUndefined(); + expect(value.slug).toBe(slug); + }); + }); + + const malicious = [ + '../../etc/passwd', + '../foo', + '..%2f..%2fsrc', // URL-encoded ../ (Express decodes before validation) + '/etc/passwd', // absolute path + 'my.plugin', // dots not allowed + 'my plugin', // spaces not allowed + ]; + malicious.forEach((slug) => { + it(`rejects a malicious/invalid slug: ${slug}`, () => { + const { error } = schema.validate({ slug }); + expect(error).toBeDefined(); + }); + }); + + it('rejects an empty slug (required)', () => { + const { error } = schema.validate({ slug: '' }); + expect(error).toBeDefined(); + }); + + it('rejects a missing slug (required)', () => { + const { error } = schema.validate({}); + expect(error).toBeDefined(); + }); + }); + + describe('CWE-22/CWE-59: safeExtractZip', () => { + let tmpRoot; + let zipCount; + + beforeAll(async () => { + tmpRoot = await fsp.mkdtemp(path.join(os.tmpdir(), 'pr614-')); + zipCount = 0; + }); + + afterAll(async () => { + await fsp.rm(tmpRoot, { recursive: true, force: true }); + }); + + // Unique target dir + zip path per test to avoid cross-test interference. + async function nextDirs(name) { + const target = path.join(tmpRoot, `${name}-target`); + await fsp.mkdir(target, { recursive: true }); + zipCount += 1; + const zipPath = path.join(tmpRoot, `${name}-${zipCount}.zip`); + return { target, zipPath }; + } + + it('rejects a path-traversal entry (../evil.txt) and does not escape the target dir', async () => { + const { target, zipPath } = await nextDirs('traversal'); + buildZip(zipPath, [{ name: '../evil.txt', content: 'pwned' }]); + + // Rejected either by yauzl's built-in path validation ("invalid relative path") + // or by safeExtractZip's explicit containment check ("Unsafe zip entry"). + await expect(safeExtractZip(zipPath, target)).rejects.toThrow(/invalid relative path|Unsafe zip entry/); + + // Nothing should have escaped above the target directory. + const escaped = path.join(tmpRoot, 'evil.txt'); + expect(fs.existsSync(escaped)).toBe(false); + // Target dir should remain empty (entry rejected before writing). + expect(fs.existsSync(path.join(target, 'evil.txt'))).toBe(false); + }); + + it('rejects a deeper path-traversal entry (../../etc/evil)', async () => { + const { target, zipPath } = await nextDirs('deep-traversal'); + buildZip(zipPath, [{ name: '../../etc/evil.txt', content: 'pwned' }]); + + await expect(safeExtractZip(zipPath, target)).rejects.toThrow(/invalid relative path|Unsafe zip entry/); + expect(fs.existsSync(path.join(tmpRoot, 'evil.txt'))).toBe(false); + }); + + it('rejects an absolute-path entry (/etc/passwd-evil)', async () => { + const { target, zipPath } = await nextDirs('abs'); + buildZip(zipPath, [{ name: '/etc/passwd-evil', content: 'pwned' }]); + + // Rejected either by yauzl ("absolute path") or safeExtractZip ("Unsafe zip entry"). + await expect(safeExtractZip(zipPath, target)).rejects.toThrow(/absolute path|Unsafe zip entry/); + }); + + it('rejects a non-traversal filename that merely contains ".." (defense-in-depth)', async () => { + // yauzl allows this valid relative path, so the PR's explicit `includes('..')` + // check is the one that must catch it. + const { target, zipPath } = await nextDirs('dots'); + buildZip(zipPath, [{ name: 'foo..bar.txt', content: 'x' }]); + + await expect(safeExtractZip(zipPath, target)).rejects.toThrow(/Unsafe zip entry/); + }); + + it('rejects a symlink entry and does not create a symlink on disk', async () => { + const { target, zipPath } = await nextDirs('symlink'); + buildZip(zipPath, [{ name: 'link.txt', content: '/etc/passwd', type: 'symlink' }]); + + await expect(safeExtractZip(zipPath, target)).rejects.toThrow(/Symlink entries are not allowed/); + + const linkPath = path.join(target, 'link.txt'); + expect(fs.existsSync(linkPath)).toBe(false); + // A symlink pointing outside must never have been created on disk. + expect(fs.existsSync(linkPath) && fs.lstatSync(linkPath).isSymbolicLink()).toBe(false); + }); + + it('extracts a valid zip correctly (files + nested dirs, correct content)', async () => { + const { target, zipPath } = await nextDirs('valid'); + buildZip(zipPath, [ + { name: 'index.js', content: 'console.log("hello");\n' }, + { name: 'sub/style.css', content: 'body { color: red; }\n' }, + { name: 'assets/', type: 'dir' }, + ]); + + await safeExtractZip(zipPath, target); + + const indexJs = path.join(target, 'index.js'); + const styleCss = path.join(target, 'sub/style.css'); + expect(fs.existsSync(indexJs)).toBe(true); + expect(fs.existsSync(styleCss)).toBe(true); + expect(fs.readFileSync(indexJs, 'utf8')).toBe('console.log("hello");\n'); + expect(fs.readFileSync(styleCss, 'utf8')).toBe('body { color: red; }\n'); + expect(fs.existsSync(path.join(target, 'assets'))).toBe(true); + }); + + it('rejects a zip that mixes a valid entry and a traversal entry', async () => { + const { target, zipPath } = await nextDirs('mixed'); + buildZip(zipPath, [ + { name: 'index.js', content: 'ok' }, + { name: '../escape.txt', content: 'bad' }, + ]); + + await expect(safeExtractZip(zipPath, target)).rejects.toThrow(); + // The valid first entry may have been written (sequential extraction), but the + // traversal entry must never escape the target dir. + const escaped = path.join(tmpRoot, 'escape.txt'); + expect(fs.existsSync(escaped)).toBe(false); + }); + + it('removes partially extracted content on rejection (no disk accumulation)', async () => { + // A valid entry is written first, then a traversal entry triggers failure. + // safeExtractZip is transactional: on failure it must remove the whole + // target dir so partial plugin directories don't accumulate on disk. + const { target, zipPath } = await nextDirs('cleanup'); + buildZip(zipPath, [ + { name: 'good.txt', content: 'ok' }, + { name: 'sub/more.txt', content: 'ok' }, + { name: '../escape.txt', content: 'bad' }, + ]); + + await expect(safeExtractZip(zipPath, target)).rejects.toThrow(); + + // Target dir (and the partial good.txt / sub/) must be cleaned up. + expect(fs.existsSync(target)).toBe(false); + expect(fs.existsSync(path.join(target, 'good.txt'))).toBe(false); + expect(fs.existsSync(path.join(target, 'sub', 'more.txt'))).toBe(false); + // And nothing escaped above the target. + expect(fs.existsSync(path.join(tmpRoot, 'escape.txt'))).toBe(false); + }); + + it('keeps the target dir intact on successful extraction', async () => { + // Sanity check: the transactional cleanup must NOT fire on success. + const { target, zipPath } = await nextDirs('success-keeps'); + buildZip(zipPath, [{ name: 'index.js', content: 'ok' }]); + + await safeExtractZip(zipPath, target); + + expect(fs.existsSync(target)).toBe(true); + expect(fs.existsSync(path.join(target, 'index.js'))).toBe(true); + }); + }); +}); diff --git a/backend/yarn.lock b/backend/yarn.lock index 8fd0a10b..f36b93fd 100644 --- a/backend/yarn.lock +++ b/backend/yarn.lock @@ -5615,6 +5615,11 @@ peek-readable@^4.1.0: resolved "https://registry.npmjs.org/peek-readable/-/peek-readable-4.1.0.tgz" integrity sha512-ZI3LnwUv5nOGbQzD9c2iDG6toheuXSZP5esSHBjopsXH4dg19soufvpUGA3uohi5anFtGb2lhAVdHzH6R/Evvg== +pend@~1.2.0: + version "1.2.0" + resolved "https://registry.yarnpkg.com/pend/-/pend-1.2.0.tgz#7a57eb550a6783f9115331fcf4663d5c8e007a50" + integrity sha512-F3asv42UuXchdzt+xXqfW1OGlVBe+mxa2mqI0pg5yAHZPvFmY3Y6drSf/GQ1A86WgWEN9Kzh/WrgKa6iGcHXLg== + phin@^3.7.1: version "3.7.1" resolved "https://registry.npmjs.org/phin/-/phin-3.7.1.tgz" @@ -7377,6 +7382,13 @@ yargs@^17.3.1, yargs@^17.7.2: y18n "^5.0.5" yargs-parser "^21.1.1" +yauzl@^3.4.0: + version "3.4.0" + resolved "https://registry.yarnpkg.com/yauzl/-/yauzl-3.4.0.tgz#88b2a21455f37ca7dccf2eeb33bacb4392322719" + integrity sha512-jIH9yLR9wqr0wOS0TpBvo/g/2UgZH5qePVbjgRliiF0BYvOZyaBknKsF+x9Iht0O6sqgnB93rCICdOZFecJuDw== + dependencies: + pend "~1.2.0" + yocto-queue@^0.1.0: version "0.1.0" resolved "https://registry.npmjs.org/yocto-queue/-/yocto-queue-0.1.0.tgz"