Fixed Multiple command injection and path traversal vulnerabilities#714
Open
Aryan-Agarwal-creator wants to merge 1 commit into
Open
Fixed Multiple command injection and path traversal vulnerabilities#714Aryan-Agarwal-creator wants to merge 1 commit into
Aryan-Agarwal-creator wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR remediates three critical command injection vulnerabilities across the TermUI repository by introducing input validation, replacing unsafe ChangesCommand Injection Vulnerability Remediation
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
type:docs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/generate-docs.js`:
- Around line 16-29: The validation currently only blocks '..' and allows
leading slashes, so absolute paths like '/tmp/pwn' can escape docs; update the
validation in scripts/generate-docs.js to reject absolute paths by using Node's
path.isAbsolute(target) (or checking target.startsWith('/') and Windows drive
letters) in addition to the existing checks (VALID_DOC_TARGET_RE and the '..'
check) and throw an error like "Documentation target cannot be absolute"; apply
the same absolute-path rejection to the other duplicate validation block that
repeats these checks.
In `@SECURITY_ANALYSIS.md`:
- Around line 374-377: Remove the unrelated CWE entry CWE-434 from the
compliance mapping list (the bullet that currently reads "**CWE-434**:
Unrestricted Upload of File with Dangerous Type") since the remediation scope
only covers command/argument/path injection; update the list to contain only
CWE-78, CWE-88, and CWE-22 and ensure any references or cross-links elsewhere in
SECURITY_ANALYSIS.md that point to CWE-434 for this section are also removed or
redirected to the appropriate CWEs.
- Around line 143-145: Update SECURITY_ANALYSIS.md to reflect the actual,
domain-agnostic validator behavior used in
packages/create-termui-app/src/git-init.ts: state that SSH
(git@<host>:user/repo.git) and HTTPS (https://<host>/user/repo.git) URL formats
are allowed (use "e.g." examples) and that other protocols (http, ftp, file://)
are disallowed; reference the validator in
packages/create-termui-app/src/git-init.ts (the Git URL validation
function/regex) when making the change so the wording matches the implemented
rule.
In `@SECURITY_FIX_SUMMARY.txt`:
- Around line 164-166: The example comment misstates parsing semantics: update
the secure-path example around execFileSync('npm', ['install', userInput]) to
explain that execFileSync passes userInput as a single literal argument to npm
(not shell-split); change the malformed line that shows npm receiving a shell
command to a correct explanation such as "npm receives the literal package name
including quotes/semicolon (e.g. '\"\"; rm -rf /') and does not execute the
shell command", referencing execFileSync, userInput and the npm install
invocation to make the intended behavior explicit.
In `@VULNERABILITY_DEMONSTRATION.md`:
- Around line 435-439: The markdown contains a machine-specific absolute cd path
in the code block that prevents others from running the test steps; edit
VULNERABILITY_DEMONSTRATION.md to remove or replace the absolute path line with
a repo-relative approach (e.g., remove the cd line and run the existing vitest
invocation from the repository root, or change it to a relative/path-agnostic
command using the repo root), updating the code block so it no longer hardcodes
/Users/... and instead uses a portable instruction for running the package
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f72c155-87cb-4518-8191-9d5d9dea7001
📒 Files selected for processing (9)
SECURITY_ANALYSIS.mdSECURITY_FIX_SUMMARY.txtVULNERABILITY_DEMONSTRATION.mdpackages/create-termui-app/src/git-init.test.tspackages/create-termui-app/src/git-init.tspackages/create-termui-app/src/input-validation.test.tspackages/create-termui-app/src/input-validation.tsscripts/generate-docs.jsscripts/install-theme.js
Comment on lines
+16
to
+29
| const VALID_DOC_TARGET_RE = /^[a-z0-9_/-]+$/; | ||
|
|
||
| if (!target || target.length === 0) { | ||
| throw new Error('Documentation target cannot be empty'); | ||
| } | ||
|
|
||
| if (target.length > 256) { | ||
| throw new Error('Documentation target too long (max 256 characters)'); | ||
| } | ||
|
|
||
| // Reject path traversal attempts | ||
| if (target.includes('..')) { | ||
| throw new Error('Documentation target cannot contain ".."'); | ||
| } |
There was a problem hiding this comment.
Block absolute output paths in documentation target validation.
Line 16 currently permits /, and Line 27 only blocks ... Inputs like /tmp/pwn can still escape docs/<target> and write outside the repo docs directory.
🔒 Suggested hardening
-import { join } from 'node:path';
+import { join, resolve, sep } from 'node:path';
@@
function validateDocTarget(target) {
@@
+ if (target.startsWith('/')) {
+ throw new Error('Documentation target cannot be an absolute path');
+ }
@@
function generateDocs(target) {
@@
validateDocTarget(target);
+ const docsRoot = resolve('docs');
+ const outputDir = resolve(docsRoot, target);
+ if (outputDir !== docsRoot && !outputDir.startsWith(`${docsRoot}${sep}`)) {
+ throw new Error('Documentation target must stay within docs/');
+ }
@@
- execFileSync('typedoc', ['--out', join('docs', target), 'src'], {
+ execFileSync('typedoc', ['--out', outputDir, 'src'], {
@@
- execFileSync('jsdoc', ['-d', join('docs', target), 'src/**/*.ts'], {
+ execFileSync('jsdoc', ['-d', outputDir, 'src/**/*.ts'], {Also applies to: 57-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate-docs.js` around lines 16 - 29, The validation currently only
blocks '..' and allows leading slashes, so absolute paths like '/tmp/pwn' can
escape docs; update the validation in scripts/generate-docs.js to reject
absolute paths by using Node's path.isAbsolute(target) (or checking
target.startsWith('/') and Windows drive letters) in addition to the existing
checks (VALID_DOC_TARGET_RE and the '..' check) and throw an error like
"Documentation target cannot be absolute"; apply the same absolute-path
rejection to the other duplicate validation block that repeats these checks.
Comment on lines
+143
to
+145
| - SSH format only: `git@github.com:user/repo.git` | ||
| - HTTPS format only: `https://github.com/user/repo.git` | ||
| - No other protocols (HTTP, FTP, file://) |
There was a problem hiding this comment.
Align Git URL rule wording with actual validator behavior.
Line 143 and Line 144 read as GitHub-only accepted formats, but packages/create-termui-app/src/git-init.ts validates SSH/HTTPS patterns for any domain. Update wording to “e.g.” examples or document the real domain-agnostic rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SECURITY_ANALYSIS.md` around lines 143 - 145, Update SECURITY_ANALYSIS.md to
reflect the actual, domain-agnostic validator behavior used in
packages/create-termui-app/src/git-init.ts: state that SSH
(git@<host>:user/repo.git) and HTTPS (https://<host>/user/repo.git) URL formats
are allowed (use "e.g." examples) and that other protocols (http, ftp, file://)
are disallowed; reference the validator in
packages/create-termui-app/src/git-init.ts (the Git URL validation
function/regex) when making the change so the wording matches the implemented
rule.
Comment on lines
+374
to
+377
| - **CWE-78**: Improper Neutralization of Special Elements used in an OS Command | ||
| - **CWE-88**: Improper Neutralization of Argument Delimiters in a Command | ||
| - **CWE-434**: Unrestricted Upload of File with Dangerous Type | ||
| - **CWE-22**: Improper Limitation of a Pathname to a Restricted Directory |
There was a problem hiding this comment.
Remove unrelated CWE-434 from compliance mapping.
Line 376 lists CWE-434 (unrestricted file upload), which is not evidenced by the remediation scope here (command/argument/path injection). Keeping it may weaken audit clarity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SECURITY_ANALYSIS.md` around lines 374 - 377, Remove the unrelated CWE entry
CWE-434 from the compliance mapping list (the bullet that currently reads
"**CWE-434**: Unrestricted Upload of File with Dangerous Type") since the
remediation scope only covers command/argument/path injection; update the list
to contain only CWE-78, CWE-88, and CWE-22 and ensure any references or
cross-links elsewhere in SECURITY_ANALYSIS.md that point to CWE-434 for this
section are also removed or redirected to the appropriate CWEs.
Comment on lines
+164
to
+166
| execFileSync('npm', ['install', userInput]); | ||
| // npm receives: npm install ""; rm -rf /" | ||
| // Result: Package name is literal string, not command |
There was a problem hiding this comment.
Fix the secure-path example output to avoid misleading parsing semantics.
Line 165 currently shows a malformed command string. With execFileSync('npm', ['install', userInput]), npm receives the payload as a single literal argument; it is not shell-split into multiple commands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SECURITY_FIX_SUMMARY.txt` around lines 164 - 166, The example comment
misstates parsing semantics: update the secure-path example around
execFileSync('npm', ['install', userInput]) to explain that execFileSync passes
userInput as a single literal argument to npm (not shell-split); change the
malformed line that shows npm receiving a shell command to a correct explanation
such as "npm receives the literal package name including quotes/semicolon (e.g.
'\"\"; rm -rf /') and does not execute the shell command", referencing
execFileSync, userInput and the npm install invocation to make the intended
behavior explicit.
Comment on lines
+435
to
+439
| ```bash | ||
| # Run validation tests | ||
| cd /Users/aryanagarwal/Documents/GSSOC/TermUI | ||
| bun vitest run packages/create-termui-app | ||
|
|
There was a problem hiding this comment.
Remove machine-specific absolute path from test instructions.
Line 437 hardcodes a local filesystem path, so these steps won’t run for other contributors as written.
✏️ Suggested doc fix
-# Run validation tests
-cd /Users/aryanagarwal/Documents/GSSOC/TermUI
-bun vitest run packages/create-termui-app
+# Run validation tests (from repository root)
+bun vitest run packages/create-termui-app📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```bash | |
| # Run validation tests | |
| cd /Users/aryanagarwal/Documents/GSSOC/TermUI | |
| bun vitest run packages/create-termui-app |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@VULNERABILITY_DEMONSTRATION.md` around lines 435 - 439, The markdown contains
a machine-specific absolute cd path in the code block that prevents others from
running the test steps; edit VULNERABILITY_DEMONSTRATION.md to remove or replace
the absolute path line with a repo-relative approach (e.g., remove the cd line
and run the existing vitest invocation from the repository root, or change it to
a relative/path-agnostic command using the repo root), updating the code block
so it no longer hardcodes /Users/... and instead uses a portable instruction for
running the package tests.
Karanjot786
added
gssoc:approved
Owner
Review: Absolute path validation missingIn Add this check after the existing validation: if (path.isAbsolute(target)) {
throw new Error('Documentation target cannot be an absolute path');
}Apply this to both validation blocks in the file. |
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
2 participants
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.
Description
This PR fixes multiple command injection and path traversal vulnerabilities across the TermUI tooling ecosystem. Unsafe
execSync()usage has been replaced with secure command execution patterns, strict input validation has been added, and comprehensive security tests have been introduced to prevent arbitrary command execution and filesystem abuse.Related Issue
Closes #708
Which package(s)?
packages/create-termui-appscripts/install-theme.jsscripts/generate-docs.jsType of Change
type:bug)type:testing)type:docs)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheck[CONTRIBUTING.md](https://chatgpt.com/c/CONTRIBUTING.md).type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/____Screenshots / Recordings (UI changes)
N/A – Security, validation, testing, and documentation changes only.
Notes for the Reviewer
Vulnerabilities Fixed
1. Theme Installation Script
File:
scripts/install-theme.jsIssue:
Fix:
execFileSync()and argument arrays.2. Documentation Generation Script
File:
scripts/generate-docs.jsIssue:
Fix:
3. Git Repository Initialization
File:
packages/create-termui-app/src/git-init.tsIssue:
Fix:
execFileSync()calls.Security Utilities Added
File:
packages/create-termui-app/src/input-validation.tsIntroduced:
validateThemeName()validateRepoName()validateProjectName()Validation rules:
Blocks:
Security Test Coverage
Added 42 new security-focused test cases:
input-validation.test.ts
git-init.test.ts
Attack Vectors Mitigated
Blocked examples include:
Compliance
Addresses:
Validation Results
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests