From 3d7aef4f78f8e042d7c8cb057751dbde26f1e318 Mon Sep 17 00:00:00 2001 From: Hua Minh Tri Date: Fri, 7 Aug 2026 10:36:06 +0700 Subject: [PATCH 1/5] chore: track dev-stage env sample file --- .gitignore | 1 + dev-stage/.env.dev-stage.sample | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 dev-stage/.env.dev-stage.sample diff --git a/.gitignore b/.gitignore index 5fdcd3b6..00fda2da 100644 --- a/.gitignore +++ b/.gitignore @@ -59,6 +59,7 @@ frontend/.env.* !backend/.env.example !frontend/.env.example !instance-setup/.env.prod.sample +!dev-stage/.env.dev-stage.sample ### Package managers (do not ignore lockfiles) .pnpm-store/ diff --git a/dev-stage/.env.dev-stage.sample b/dev-stage/.env.dev-stage.sample new file mode 100644 index 00000000..3d14cc61 --- /dev/null +++ b/dev-stage/.env.dev-stage.sample @@ -0,0 +1,20 @@ +# AutoWRX Dev Stage Environment Configuration +# Copy to backend/.env and fill in your values +# Used by: /opt/dev/autowrx/backend (PM2 autowrx-dev-stage, test.digital.auto:3202) + +# Server +PORT=3202 +NODE_ENV=production + +# MongoDB (autowrx-dev-mongodb container on port 27020) +MONGODB_URL=mongodb://localhost:27020/autowrx-dev + +# Security - CHANGE THESE! +JWT_SECRET=your-secure-random-secret-here + +# CORS - Add your test domain (escape dots with \.) +CORS_ORIGINS=test\\.digital\\.auto,.*\\.test\\.digital\\.auto,localhost:\\d+,127\\.0\\.0\\.1:\\d+ + +# Admin user (created on first run) +ADMIN_EMAILS=admin@email.com +ADMIN_PASSWORD=change-this-password From d976cc64a46f507bc785c69b4ffcde8cbc5584ff Mon Sep 17 00:00:00 2001 From: Hua Minh Tri Date: Fri, 7 Aug 2026 11:04:29 +0700 Subject: [PATCH 2/5] fix(security): patch path traversal and symlink in plugin upload (#719) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes three vulnerabilities in POST /v2/plugin/upload/:slug: 1. Path traversal via slug (CWE-22): slug was validated only as Joi.string().required(), allowing URL-encoded ../ sequences to extract the zip into arbitrary directories (e.g. backend/src/ → RCE). - Apply the existing slug custom validator (rejects non-slug chars) - Add path.resolve containment check in the controller (defense in depth) 2. Symlink-based arbitrary file read (CWE-59): spawn('unzip') recreated symbolic links, and express.static followed them, allowing read access to any file (e.g. .env containing JWT_SECRET). - Replace spawn('unzip') with safe yauzl-based extraction that rejects symlink entries, absolute paths, and ../ in entry names - Add dotfiles: 'ignore' to express.static mounts for /plugin 3. Missing authorization (CWE-862): the admin checkPermission guard was commented out, and the ownership check ran after extraction. - Move ownership check before extraction so files are never written for unauthorized users - Remove commented-out checkPermission and redundant auth() from route (auth() already applied via router.use(auth()) at line 26) --- backend/package.json | 3 +- backend/src/app.js | 4 +- backend/src/controllers/plugin.controller.js | 120 ++++++++++++++----- backend/src/routes/v2/system/plugin.route.js | 2 - backend/src/validations/plugin.validation.js | 6 +- backend/yarn.lock | 12 ++ 6 files changed, 108 insertions(+), 39 deletions(-) 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..da135173 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,24 +125,102 @@ 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). + */ +async function safeExtractZip(zipPath, targetDir) { + const resolvedTarget = path.resolve(targetDir); + return new Promise((resolve, reject) => { + yauzl.open(zipPath, { lazyEntries: true, autoClose: true }, (err, zipfile) => { + if (err) return reject(err); + + zipfile.readEntry(); + zipfile.on('entry', (entry) => { + // Reject absolute paths and path traversal in entry names + if (path.isAbsolute(entry.fileName) || entry.fileName.includes('..')) { + return reject(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 reject(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 reject(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(reject); + } else { + // File entry — ensure parent directory exists + fsp + .mkdir(path.dirname(entryPath), { recursive: true }) + .then(() => { + zipfile.openReadStream(entry, (readErr, readStream) => { + if (readErr) return reject(readErr); + const writeStream = fs.createWriteStream(entryPath); + writeStream.on('error', reject); + writeStream.on('close', () => zipfile.readEntry()); + readStream.pipe(writeStream); + }); + }) + .catch(reject); + } + }); + zipfile.on('end', resolve); + zipfile.on('error', reject); + }); + }); +} + 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); + + // 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'); + } + 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}`)); - }); - }); + // Safely extract zip (rejects symlinks and path traversal — CWE-22, CWE-59) + await safeExtractZip(req.file.path, pluginPath); // Remove uploaded temp file try { @@ -164,23 +241,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 +260,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, 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/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" From 86c101cee342592805b71ed769b82742b5164e0b Mon Sep 17 00:00:00 2001 From: NhanLuongBGSV Date: Thu, 13 Aug 2026 06:31:41 +0000 Subject: [PATCH 3/5] test(security): add regression tests for plugin upload fixes (#719) Add unit tests exercising the real production code from PR #614: - slug validation rejects path traversal (../, absolute, URL-encoded) - safeExtractZip rejects path-traversal, absolute, and symlink entries, and does not escape the target dir or create symlinks on disk - authorization gate (CWE-862) returns 403 without writing files or mutating the plugin record when the slug is owned by another user Export safeExtractZip from the controller for testability (not used by route handlers). Includes a Python helper to build malicious zip fixtures. Co-Authored-By: Claude --- backend/src/controllers/plugin.controller.js | 2 + backend/tests/fixtures/build_zip.py | 42 ++++ .../controllers/plugin.upload.auth.test.js | 144 ++++++++++++++ .../plugin.upload.security.test.js | 180 ++++++++++++++++++ 4 files changed, 368 insertions(+) create mode 100644 backend/tests/fixtures/build_zip.py create mode 100644 backend/tests/unit/controllers/plugin.upload.auth.test.js create mode 100644 backend/tests/unit/controllers/plugin.upload.security.test.js diff --git a/backend/src/controllers/plugin.controller.js b/backend/src/controllers/plugin.controller.js index da135173..21ac2489 100644 --- a/backend/src/controllers/plugin.controller.js +++ b/backend/src/controllers/plugin.controller.js @@ -279,4 +279,6 @@ module.exports = { updatePlugin, uploadInternalPlugin, removePlugin, + // Exported for unit testing only (not used by route handlers) + safeExtractZip, }; 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..885bbad3 --- /dev/null +++ b/backend/tests/unit/controllers/plugin.upload.security.test.js @@ -0,0 +1,180 @@ +// 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); + }); + }); +}); From 1f7dfc8bf5565c7f0d098a434a899f3b57a6b705 Mon Sep 17 00:00:00 2001 From: NhanLuongBGSV Date: Thu, 13 Aug 2026 06:53:40 +0000 Subject: [PATCH 4/5] chore: drop unrelated dev-stage env sample from security PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the dev-stage/.env.dev-stage.sample + .gitignore change (commit 3d7aef4) — it is unrelated to the plugin-upload security fix (#719) and should ship in its own PR. Co-Authored-By: Claude --- .gitignore | 1 - dev-stage/.env.dev-stage.sample | 20 -------------------- 2 files changed, 21 deletions(-) delete mode 100644 dev-stage/.env.dev-stage.sample diff --git a/.gitignore b/.gitignore index 00fda2da..5fdcd3b6 100644 --- a/.gitignore +++ b/.gitignore @@ -59,7 +59,6 @@ frontend/.env.* !backend/.env.example !frontend/.env.example !instance-setup/.env.prod.sample -!dev-stage/.env.dev-stage.sample ### Package managers (do not ignore lockfiles) .pnpm-store/ diff --git a/dev-stage/.env.dev-stage.sample b/dev-stage/.env.dev-stage.sample deleted file mode 100644 index 3d14cc61..00000000 --- a/dev-stage/.env.dev-stage.sample +++ /dev/null @@ -1,20 +0,0 @@ -# AutoWRX Dev Stage Environment Configuration -# Copy to backend/.env and fill in your values -# Used by: /opt/dev/autowrx/backend (PM2 autowrx-dev-stage, test.digital.auto:3202) - -# Server -PORT=3202 -NODE_ENV=production - -# MongoDB (autowrx-dev-mongodb container on port 27020) -MONGODB_URL=mongodb://localhost:27020/autowrx-dev - -# Security - CHANGE THESE! -JWT_SECRET=your-secure-random-secret-here - -# CORS - Add your test domain (escape dots with \.) -CORS_ORIGINS=test\\.digital\\.auto,.*\\.test\\.digital\\.auto,localhost:\\d+,127\\.0\\.0\\.1:\\d+ - -# Admin user (created on first run) -ADMIN_EMAILS=admin@email.com -ADMIN_PASSWORD=change-this-password From 86002b0f2447352c254e8356c376804174c88ff3 Mon Sep 17 00:00:00 2001 From: NhanLuongBGSV Date: Thu, 13 Aug 2026 07:04:27 +0000 Subject: [PATCH 5/5] fix(security): make safeExtractZip transactional (fd + disk cleanup) On rejection, safeExtractZip previously leaked the yauzl file descriptor (autoClose only fires on 'end', which never happens after a rejected entry) and left partially extracted files in the plugin directory. Repeated malicious/corrupt uploads would accumulate orphan dirs and exhaust fds. - Close the zipfile fd and destroy in-flight read/write streams on failure - Remove any partially extracted content from targetDir on failure (transactional: full extract or no output) - Always remove the multer temp upload in the caller via try/finally (previously skipped on extraction failure, leaking files under static/uploads) Adds regression tests for the cleanup and that success keeps the target. Co-Authored-By: Claude --- backend/src/controllers/plugin.controller.js | 102 ++++++++++++++---- .../plugin.upload.security.test.js | 32 ++++++ 2 files changed, 112 insertions(+), 22 deletions(-) diff --git a/backend/src/controllers/plugin.controller.js b/backend/src/controllers/plugin.controller.js index 21ac2489..26ba20d0 100644 --- a/backend/src/controllers/plugin.controller.js +++ b/backend/src/controllers/plugin.controller.js @@ -129,31 +129,72 @@ async function findEntryFile(rootDir, candidates = ['index.js', 'index.html']) { * 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) => { - yauzl.open(zipPath, { lazyEntries: true, autoClose: true }, (err, zipfile) => { - if (err) return reject(err); + 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 reject(new ApiError(httpStatus.BAD_REQUEST, `Unsafe zip entry: ${entry.fileName}`)); + 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 reject(new ApiError(httpStatus.BAD_REQUEST, `Unsafe zip entry: ${entry.fileName}`)); + 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 reject(new ApiError(httpStatus.BAD_REQUEST, `Symlink entries are not allowed: ${entry.fileName}`)); + return fail(new ApiError(httpStatus.BAD_REQUEST, `Symlink entries are not allowed: ${entry.fileName}`)); } if (/\/$/.test(entry.fileName)) { @@ -161,25 +202,37 @@ async function safeExtractZip(zipPath, targetDir) { fsp .mkdir(entryPath, { recursive: true }) .then(() => zipfile.readEntry()) - .catch(reject); + .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 reject(readErr); + if (readErr) return fail(readErr); + activeReadStream = readStream; + readStream.on('error', fail); const writeStream = fs.createWriteStream(entryPath); - writeStream.on('error', reject); - writeStream.on('close', () => zipfile.readEntry()); + activeWriteStream = writeStream; + writeStream.on('error', fail); + writeStream.on('close', () => { + activeReadStream = null; + activeWriteStream = null; + zipfile.readEntry(); + }); readStream.pipe(writeStream); }); }) - .catch(reject); + .catch(fail); + } + }); + zipfile.on('end', () => { + if (!settled) { + settled = true; + resolve(); } }); - zipfile.on('end', resolve); - zipfile.on('error', reject); + zipfile.on('error', fail); }); }); } @@ -219,17 +272,22 @@ const uploadInternalPlugin = catchAsync(async (req, res) => { await ensureDir(pluginPath); - // Safely extract zip (rejects symlinks and path traversal — CWE-22, CWE-59) - await safeExtractZip(req.file.path, pluginPath); - - // Remove uploaded temp file + // 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) diff --git a/backend/tests/unit/controllers/plugin.upload.security.test.js b/backend/tests/unit/controllers/plugin.upload.security.test.js index 885bbad3..7328620c 100644 --- a/backend/tests/unit/controllers/plugin.upload.security.test.js +++ b/backend/tests/unit/controllers/plugin.upload.security.test.js @@ -176,5 +176,37 @@ describe('Plugin upload security (PR #614 / issue #719)', () => { 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); + }); }); });