fix: cli commands accept file path arguments directl... in edit.ts#35311
fix: cli commands accept file path arguments directl... in edit.ts#35311orbisai0security wants to merge 1 commit intoiptv-org:masterfrom
Conversation
Automated security fix generated by Orbis Security AI
There was a problem hiding this comment.
Pull request overview
This PR aims to mitigate a high-severity path traversal risk (CWE-22) in the playlist edit CLI command by adding validation for the user-supplied filepath argument before performing storage reads/writes.
Changes:
- Added
path-based resolution + working-directory boundary check for the CLI<filepath>argument. - Exits early with an error when the provided path is detected outside the working directory.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const resolvedPath = path.resolve(filepath) | ||
| const relative = path.relative(process.cwd(), resolvedPath) | ||
| if (relative.startsWith('..') || path.isAbsolute(relative)) { |
There was a problem hiding this comment.
The directory-traversal guard uses relative.startsWith('..'), which can incorrectly reject legitimate in-tree paths whose first path segment begins with .. (e.g. ..cache/file.channels.xml). Safer is to treat only the parent-dir segment as disallowed (e.g. relative === '..' || relative.startsWith('..' + path.sep)).
| if (relative.startsWith('..') || path.isAbsolute(relative)) { | |
| if (relative === '..' || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) { |
| const resolvedPath = path.resolve(filepath) | ||
| const relative = path.relative(process.cwd(), resolvedPath) | ||
| if (relative.startsWith('..') || path.isAbsolute(relative)) { | ||
| console.error(`Error: filepath "${filepath}" is outside the working directory`) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
path.resolve() does not account for symlinks. A filepath inside the working directory can still escape via a symlink (e.g. ./safe/link-to-/etc/passwd), and the subsequent Storage read/write will follow the link. Consider validating against fs.realpathSync()/fs.promises.realpath() (after existence check) and ensure the real path is within the allowed base directory.
| main(filepath) | ||
| nodeCleanup(() => { | ||
| save(filepath) |
There was a problem hiding this comment.
After computing and validating resolvedPath, the script continues to use the original filepath for storage.exists(), parsing, and saving. To ensure the validated path is the one actually used (and to avoid absolute-path handling differences in Storage), pass the validated path (resolvedPath / realpath) through to main() and save() instead of the raw argument string.
| main(filepath) | |
| nodeCleanup(() => { | |
| save(filepath) | |
| main(resolvedPath) | |
| nodeCleanup(() => { | |
| save(resolvedPath) |
|
|
||
| export default async function main(filepath: string) { | ||
| if (!(await storage.exists(filepath))) { | ||
| throw new Error(`File "${filepath}" does not exists`) |
There was a problem hiding this comment.
Grammar in the error message: does not exists -> does not exist. This improves clarity in CLI output.
| throw new Error(`File "${filepath}" does not exists`) | |
| throw new Error(`File "${filepath}" does not exist`) |
Summary
Fix high severity security issue in
scripts/commands/playlist/edit.ts.Vulnerability
V-001scripts/commands/playlist/edit.ts:29Description: CLI commands accept file path arguments directly from process.argv without validation or sanitization. The filepath parameter is passed to storage operations (rootStorage.save, streamsStorage.save) without checking for directory traversal sequences like '../' or absolute paths. An attacker can provide paths like '../../../etc/passwd' or '../../package.json' to read or write files outside the intended directory structure.
Changes
scripts/commands/playlist/edit.tsVerification
Automated security fix by OrbisAI Security