fix(security): patch path traversal and symlink in plugin upload (#719) - #614
Open
tri2510 wants to merge 2 commits into
Open
fix(security): patch path traversal and symlink in plugin upload (#719)#614tri2510 wants to merge 2 commits into
tri2510 wants to merge 2 commits into
Conversation
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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three security vulnerabilities in
POST /v2/plugin/upload/:slugreported in issue #719.Vulnerabilities Fixed
Joi.string().required(), allowing URL-encoded../sequences to extract the zip into arbitrary directories (e.g.backend/src/→ RCE on next process restart)slugcustom validator (rejects non-slug characters) + addpath.resolvecontainment check in the controller (defense in depth)spawn('unzip')recreated symbolic links, andexpress.staticfollowed them, allowing read access to any file on disk (e.g..envcontainingJWT_SECRET)spawn('unzip')withyauzl-basedsafeExtractZip()that rejects symlink entries, absolute paths, and../in entry names. Adddotfiles: 'ignore'toexpress.staticmounts for/plugincheckPermissionguard was commented out, and the ownership check ran after extraction had already completedcheckPermissionand redundantauth()from route (auth already applied viarouter.use(auth())at line 26)Files Changed
backend/src/validations/plugin.validation.js— applyslugcustom validator touploadInternal.params.slugbackend/src/controllers/plugin.controller.js— replacespawn('unzip')withsafeExtractZip(), add path containment check, move ownership check before extractionbackend/src/routes/v2/system/plugin.route.js— remove commented-outcheckPermissionand redundantauth()backend/src/app.js— adddotfiles: 'ignore'toexpress.staticmounts for/pluginand/static/pluginbackend/package.json/backend/yarn.lock— addyauzldependencyTest plan
/plugin/<slug>/index.js)../in slug is rejected by validation (e.g...%2F..%2Fsrc→ 400)yarn lintpasses on changed filesCloses #719