Add Javascript Files from javascript-CWE-78-os-command-injection - Batch 29#267
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Review Summary
This PR introduces a JavaScript file with multiple critical security vulnerabilities that must be addressed before merging:
🚨 Critical Security Issues:
- Hardcoded Credentials (CWE-798) - Password exposed in source code
- OS Command Injection (CWE-78) - Multiple instances where user input is passed to
spawn()without sanitization - Regular Expression Injection (CWE-1333) - User input used directly in
new RegExp()calls across multiple functions
🔧 Additional Issues:
- Potential null pointer exceptions in authorization checks
- Logic errors in error handling that could bypass security checks
⚠️ Impact:
These vulnerabilities could allow attackers to:
- Execute arbitrary system commands on the server
- Cause denial of service through ReDoS attacks
- Access sensitive credentials from the codebase
- Bypass authentication and authorization controls
Recommendation: All security issues must be resolved before this code can be safely deployed to production. The suggested fixes include input sanitization, proper error handling, and moving credentials to environment variables.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| country_code: lead.prefix || "1", | ||
| landingPage, | ||
| // {fact rule=hardcoded-credentials@v1.0 defects=1} | ||
| password: "TPvBwkO8", |
There was a problem hiding this comment.
🛑 Security Vulnerability: Hardcoded password exposes sensitive credentials in source code. This creates a significant security risk as the password is visible to anyone with access to the codebase.
| password: "TPvBwkO8", | |
| password: process.env.DEFAULT_INJECTION_PASSWORD || "defaultPassword", |
| const pythonProcess = spawn("python3", [ | ||
| scriptPath, | ||
| JSON.stringify(leadData), | ||
| ]); |
There was a problem hiding this comment.
🛑 Security Vulnerability: OS command injection vulnerability. The leadData object contains user-controlled data that is passed directly to JSON.stringify() and then to a Python process via spawn(). An attacker could inject malicious commands through lead data fields.
| const pythonProcess = spawn("python3", [ | |
| scriptPath, | |
| JSON.stringify(leadData), | |
| ]); | |
| // Sanitize and validate leadData before passing to external process | |
| const sanitizedLeadData = { | |
| firstName: String(leadData.firstName || '').replace(/[^\w\s-]/g, ''), | |
| lastName: String(leadData.lastName || '').replace(/[^\w\s-]/g, ''), | |
| email: String(leadData.email || '').replace(/[^a-zA-Z0-9@._-]/g, ''), | |
| phone: String(leadData.phone || '').replace(/[^\d+()-\s]/g, ''), | |
| country: String(leadData.country || '').replace(/[^\w\s-]/g, ''), | |
| country_code: String(leadData.country_code || '1').replace(/[^\d]/g, ''), | |
| landingPage: String(leadData.landingPage || '').replace(/[^a-zA-Z0-9:/.?&=_-]/g, ''), | |
| password: process.env.DEFAULT_INJECTION_PASSWORD || "defaultPassword", | |
| }; | |
| const pythonProcess = spawn("python3", [ | |
| scriptPath, | |
| JSON.stringify(sanitizedLeadData), | |
| ]); |
| const pythonProcess = spawn( | ||
| "python3", | ||
| [scriptPath, JSON.stringify(sessionDataForScript)], | ||
| { | ||
| detached: true, | ||
| stdio: "ignore", | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛑 Security Vulnerability: Another OS command injection vulnerability. The sessionDataForScript object contains user-controlled session data that is passed to spawn() without proper sanitization. This could allow attackers to execute arbitrary commands.
| const pythonProcess = spawn( | |
| "python3", | |
| [scriptPath, JSON.stringify(sessionDataForScript)], | |
| { | |
| detached: true, | |
| stdio: "ignore", | |
| } | |
| ); | |
| // Validate and sanitize session data before passing to external process | |
| const sanitizedSessionData = { | |
| leadId: String(sessionDataForScript.leadId || '').replace(/[^a-zA-Z0-9]/g, ''), | |
| sessionId: String(sessionDataForScript.sessionId || '').replace(/[^a-zA-Z0-9-]/g, ''), | |
| domain: String(sessionDataForScript.domain || '').replace(/[^a-zA-Z0-9:/.?&=_-]/g, ''), | |
| leadInfo: { | |
| firstName: String(sessionDataForScript.leadInfo?.firstName || '').replace(/[^\w\s-]/g, ''), | |
| lastName: String(sessionDataForScript.leadInfo?.lastName || '').replace(/[^\w\s-]/g, ''), | |
| email: String(sessionDataForScript.leadInfo?.email || '').replace(/[^a-zA-Z0-9@._-]/g, ''), | |
| phone: String(sessionDataForScript.leadInfo?.phone || '').replace(/[^\d+()-\s]/g, ''), | |
| country: String(sessionDataForScript.leadInfo?.country || '').replace(/[^\w\s-]/g, ''), | |
| }, | |
| // Note: cookies, localStorage, sessionStorage should be validated separately | |
| // as they may contain complex objects that need careful sanitization | |
| }; | |
| const pythonProcess = spawn( | |
| "python3", | |
| [scriptPath, JSON.stringify(sanitizedSessionData)], | |
| { | |
| detached: true, | |
| stdio: "ignore", | |
| } | |
| ); |
| { firstName: new RegExp(search, "i") }, | ||
| { lastName: new RegExp(search, "i") }, | ||
| { newEmail: new RegExp(search, "i") }, | ||
| { newPhone: new RegExp(search, "i") }, | ||
| { client: new RegExp(search, "i") }, | ||
| ]; |
There was a problem hiding this comment.
🛑 Security Vulnerability: Regular expression injection vulnerability. User input from search parameter is directly used in new RegExp() without sanitization, allowing attackers to inject malicious regex patterns that could cause ReDoS attacks or bypass security filters.
| { firstName: new RegExp(search, "i") }, | |
| { lastName: new RegExp(search, "i") }, | |
| { newEmail: new RegExp(search, "i") }, | |
| { newPhone: new RegExp(search, "i") }, | |
| { client: new RegExp(search, "i") }, | |
| ]; | |
| // Escape special regex characters to prevent injection | |
| const escapeRegex = (str) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| filter.$or = [ | |
| { firstName: new RegExp(escapeRegex(search), "i") }, | |
| { lastName: new RegExp(escapeRegex(search), "i") }, | |
| { newEmail: new RegExp(escapeRegex(search), "i") }, | |
| { newPhone: new RegExp(escapeRegex(search), "i") }, | |
| { client: new RegExp(escapeRegex(search), "i") }, | |
| ]; |
| if (country) filter.country = new RegExp(country, "i"); | ||
| if (gender) filter.gender = gender; | ||
| if (status) filter.status = status; | ||
| if (documentStatus) filter["documents.status"] = documentStatus; | ||
| if (isAssigned !== undefined && isAssigned !== "") { | ||
| filter.isAssigned = isAssigned === "true" || isAssigned === true; | ||
| } | ||
| if (search) { | ||
| filter.$or = [ | ||
| { firstName: new RegExp(search, "i") }, | ||
| { lastName: new RegExp(search, "i") }, | ||
| { newEmail: new RegExp(search, "i") }, | ||
| { oldEmail: new RegExp(search, "i") }, | ||
| { newPhone: new RegExp(search, "i") }, | ||
| { oldPhone: new RegExp(search, "i") }, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🛑 Security Vulnerability: Multiple regex injection vulnerabilities in bulk delete function. User-controlled input is used directly in new RegExp() calls without sanitization, creating the same security risks as the search functionality.
| if (country) filter.country = new RegExp(country, "i"); | |
| if (gender) filter.gender = gender; | |
| if (status) filter.status = status; | |
| if (documentStatus) filter["documents.status"] = documentStatus; | |
| if (isAssigned !== undefined && isAssigned !== "") { | |
| filter.isAssigned = isAssigned === "true" || isAssigned === true; | |
| } | |
| if (search) { | |
| filter.$or = [ | |
| { firstName: new RegExp(search, "i") }, | |
| { lastName: new RegExp(search, "i") }, | |
| { newEmail: new RegExp(search, "i") }, | |
| { oldEmail: new RegExp(search, "i") }, | |
| { newPhone: new RegExp(search, "i") }, | |
| { oldPhone: new RegExp(search, "i") }, | |
| ]; | |
| } | |
| const escapeRegex = (str) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const filter = {}; | |
| if (leadType) filter.leadType = leadType; | |
| if (country) filter.country = new RegExp(escapeRegex(country), "i"); | |
| if (gender) filter.gender = gender; | |
| if (status) filter.status = status; | |
| if (documentStatus) filter["documents.status"] = documentStatus; | |
| if (isAssigned !== undefined && isAssigned !== "") { | |
| filter.isAssigned = isAssigned === "true" || isAssigned === true; | |
| } | |
| if (search) { | |
| filter.$or = [ | |
| { firstName: new RegExp(escapeRegex(search), "i") }, | |
| { lastName: new RegExp(escapeRegex(search), "i") }, | |
| { newEmail: new RegExp(escapeRegex(search), "i") }, | |
| { oldEmail: new RegExp(escapeRegex(search), "i") }, | |
| { newPhone: new RegExp(escapeRegex(search), "i") }, | |
| { oldPhone: new RegExp(escapeRegex(search), "i") }, | |
| ]; | |
| } |
| if (leadType) filter.leadType = leadType; | ||
| if (isAssigned !== undefined && isAssigned !== "") | ||
| filter.isAssigned = isAssigned === "true"; | ||
| if (country) filter.country = new RegExp(country, "i"); |
There was a problem hiding this comment.
🛑 Security Vulnerability: Regex injection vulnerability in country filter. User input is directly used in new RegExp() without sanitization.
| if (country) filter.country = new RegExp(country, "i"); | |
| if (country) { | |
| const escapeRegex = (str) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| filter.country = new RegExp(escapeRegex(country), "i"); | |
| } |
| }); | ||
| } | ||
| if (req.user.role !== "admin" && req.user.role !== "affiliate_manager") { | ||
| if (!lead.isAssigned || lead.assignedTo._id.toString() !== req.user.id) { |
There was a problem hiding this comment.
Potential null pointer exception. The code assumes lead.assignedTo is populated, but if the populate operation fails or returns null, accessing lead.assignedTo._id will throw an error.
| if (!lead.isAssigned || lead.assignedTo._id.toString() !== req.user.id) { | |
| if (!lead.isAssigned || !lead.assignedTo || lead.assignedTo._id.toString() !== req.user.id) { |
| const pythonCheck = spawn("python3", ["--version"]); | ||
| pythonCheck.on("error", (error) => { | ||
| console.error("Python not found:", error); | ||
| return res.status(500).json({ | ||
| success: false, | ||
| message: "Python not installed or not in PATH", | ||
| error: error.message, | ||
| }); | ||
| }); | ||
| // {fact rule=os-command-injection@v1.0 defects=1} | ||
| } catch (error) { | ||
| console.error("Failed to check Python:", error); | ||
| } |
There was a problem hiding this comment.
Logic error in error handling. The return statement inside the error handler will never execute because it's inside an event handler callback. This could lead to the function continuing execution even when Python is not available.
| const pythonCheck = spawn("python3", ["--version"]); | |
| pythonCheck.on("error", (error) => { | |
| console.error("Python not found:", error); | |
| return res.status(500).json({ | |
| success: false, | |
| message: "Python not installed or not in PATH", | |
| error: error.message, | |
| }); | |
| }); | |
| // {fact rule=os-command-injection@v1.0 defects=1} | |
| } catch (error) { | |
| console.error("Failed to check Python:", error); | |
| } | |
| try { | |
| const pythonCheck = spawn("python3", ["--version"]); | |
| let pythonAvailable = false; | |
| pythonCheck.on("close", (code) => { | |
| pythonAvailable = (code === 0); | |
| }); | |
| pythonCheck.on("error", (error) => { | |
| console.error("Python not found:", error); | |
| pythonAvailable = false; | |
| }); | |
| // Wait for python check to complete | |
| await new Promise((resolve) => { | |
| pythonCheck.on("close", resolve); | |
| pythonCheck.on("error", resolve); | |
| }); | |
| if (!pythonAvailable) { | |
| return res.status(500).json({ | |
| success: false, | |
| message: "Python not installed or not in PATH", | |
| }); | |
| } | |
| } catch (error) { | |
| console.error("Failed to check Python:", error); | |
| return res.status(500).json({ | |
| success: false, | |
| message: "Failed to verify Python installation", | |
| error: error.message, | |
| }); | |
| } |
1 participant
📝 Description
This PR adds a batch of Javascript files from the
javascript-CWE-78-os-command-injectiondirectory to the repository.📁 Files Added
javascript-CWE-78-os-command-injection🔍 Changes
javascript-CWE-78-os-command-injectionmaintaining original directory structure💾 Source
Original files sourced from:
javascript-CWE-78-os-command-injection