Add javascript random samples#62
Conversation
Added 50 randomly selected javascript code samples from the security dataset.
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @amazon-pratik, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've introduced a new set of 50 JavaScript code samples, primarily focused on demonstrating various web security vulnerabilities and their secure counterparts. This addition aims to provide a practical and diverse collection of examples for analysis and testing purposes, enhancing our dataset with real-world security scenarios. The samples cover a wide array of common security flaws, from injection attacks to insecure configurations, and are accompanied by a new README file for easy reference.
Highlights
- New README for JavaScript Samples: I've added a new
README.mdfile within thejavascript/directory. This document provides an overview of the newly introduced random samples, detailing their purpose, contents, and file count. - Introduction of 50 JavaScript Security Samples: I've included 50 randomly selected JavaScript code samples from a security dataset. These samples are designed to illustrate various security vulnerabilities and their corresponding fixes, covering a broad spectrum of common web application security issues.
- Comprehensive Coverage of Security Vulnerabilities: The new samples demonstrate a variety of security concerns, including insecure cookie settings, cleartext credential storage, CORS misconfigurations, different types of injection vulnerabilities (e.g., NoSQL, RegExp, code injection via sandbox/VM2), Cross-Site Scripting (XSS), Cross-Site Request Forgery (CSRF), and path traversal.
- Demonstration of Cryptographic and System-Level Weaknesses: Several samples highlight issues related to insecure random number generation, improper input sanitization, hardcoded secrets (e.g., Passport.js secrets), JWT vulnerabilities (like the 'none' algorithm), and insecure file permissions.
- Illustrations of Framework-Specific Security Flaws: The collection also includes examples of insecure practices in Express.js applications, such as direct response writing, directory listing, and body-parser related DoS, along with issues like insufficient postMessage origin validation and unsafe serialization.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds 50 randomly selected JavaScript code samples, many of which are intended to demonstrate security vulnerabilities. While the samples are useful for this purpose, many contain syntax errors, use deprecated APIs, or have other bugs that could prevent them from running correctly. I've provided detailed feedback on these issues to improve the quality and usability of the code samples.
| title= 'Hello world' | ||
| body | ||
| form(action='/' method='post') | ||
| input#name.form-control(type='text) |
There was a problem hiding this comment.
There is a syntax error in the Pug template string. The type attribute for the input field is missing a closing quote: type='text). This will cause the Pug compiler to fail.
| input#name.form-control(type='text) | |
| input#name.form-control(type='text') |
| password = getPassword(); | ||
|
|
||
| function encrypt(text){ | ||
| var cipher = crypto.createCipher('aes-256-ctr', password); |
There was a problem hiding this comment.
The crypto.createCipher method is deprecated and insecure. It uses a weak key derivation function. You should use crypto.createCipheriv instead, which requires a securely generated key and a unique initialization vector (IV) for each encryption operation.
|
|
||
| // {fact rule=xml-external-entity@v1.0 defects=1} | ||
| const express = require('express') | ||
| const libxmljs = require('libxml') |
There was a problem hiding this comment.
The package required here is libxml, but based on the API used (libxmljs.parseXmlString), the correct package name is likely libxmljs. Using the wrong package name will cause a MODULE_NOT_FOUND error.
| const libxmljs = require('libxml') | |
| const libxmljs = require('libxmljs') |
| const http = require('http'); | ||
|
|
||
| const server = http.createServer((req, res) => { | ||
| res.setHeader("Set-Cookie", `authKey=${makeAuthkey()}`); |
There was a problem hiding this comment.
This cookie is set without the Secure and HttpOnly flags, which poses a security risk. The Secure flag is crucial for ensuring cookies are transmitted only over encrypted connections, while HttpOnly helps prevent cross-site scripting (XSS) attacks by restricting direct access from JavaScript. Furthermore, makeAuthkey is not defined, which will lead to a runtime error.
| res.setHeader("Set-Cookie", `authKey=${makeAuthkey()}`); | |
| res.setHeader("Set-Cookie", `authKey=${makeAuthkey()}; HttpOnly; Secure`); |
| 'use strict'; | ||
|
|
||
| const fs = require('fs'); | ||
| const {VM, NodeVM} = require('vm2'); |
There was a problem hiding this comment.
VMScript is used but it's not imported from the vm2 package. This will result in a ReferenceError. You need to include it in the destructuring assignment from require('vm2').
| const {VM, NodeVM} = require('vm2'); | |
| const {VM, NodeVM, VMScript} = require('vm2'); |
| // {fact rule=insecure-connection@v1.0 defects=1} | ||
| // ruleid: javascript-confirm | ||
| if ( confirm("pushem!") == true) { | ||
| r = "x"; |
There was a problem hiding this comment.
The variable r is assigned a value without being declared with var, let, or const. This will implicitly create a global variable, which is a bad practice and can lead to unexpected behavior and bugs.
| r = "x"; | |
| let r = "x"; |
| // {/fact} | ||
|
|
||
| // {fact rule=code-injection@v1.0 defects=0} | ||
| app.get('/test1', function (req, res) { |
There was a problem hiding this comment.
The route for GET /test1 is defined twice in this file (the first is on line 13). In Express, only the first matching route handler will be executed, making the second one unreachable. This is likely a copy-paste error and can be confusing.
| var server = https.createServer(function(){}); | ||
|
|
||
| server.on('request', function(req, res) { | ||
| let origin = url.parse(req.url, true).query.origin, |
There was a problem hiding this comment.
The url.parse() method is a legacy API. It's recommended to use the WHATWG URL API for parsing URLs, as it is more modern and standardized across environments. For example: new URL(req.url, http://${req.headers.host}).searchParams.get('origin').
| //{fact rule=method-input-validation@v1.0 defects=0} | ||
|
|
||
| app.get("/some/path", function(req, res) { | ||
| let url = req.param("url"); |
There was a problem hiding this comment.
The req.param() method is deprecated. You should use req.query, req.body, or req.params based on where you expect the data to be. Also, the app variable is not defined.
| let url = req.param("url"); | |
| let url = req.query.url; |
|
|
||
| var app = express(); | ||
| app.get('/remember-password', function (req, res) { | ||
| let pw = req.param("current_password"); |
There was a problem hiding this comment.
The req.param() method is deprecated in Express. You should use req.query, req.body, or req.params depending on where the parameter is expected to be. Also, the getPassword function is not defined.
| let pw = req.param("current_password"); | |
| let pw = req.query.current_password; |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
| async function test1(code, input) { | ||
| code = ` | ||
| console.log(${input}) | ||
| `; |
There was a problem hiding this comment.
Caution
Description: The code allows arbitrary JavaScript execution without proper input validation, potentially leading to code injection vulnerabilities. Implement strict input validation and sanitization before executing user-provided code. Consider using a whitelist approach for allowed operations.
Severity: Critical
There was a problem hiding this comment.
The fix addresses the code injection vulnerability by implementing input validation and sanitization. It checks if the input is a string and only contains alphanumeric characters. The fix also changes the string interpolation to use double quotes, preventing potential code injection. However, this fix is incomplete as it only addresses the test1 function and doesn't handle the test2 function, which may also be vulnerable.
| `; | |
| // {fact rule=code-injection@v1.0 defects=1} | |
| async function test1(code, input) { | |
| // Implement input validation and sanitization | |
| if (typeof input !== 'string' || !input.match(/^[a-zA-Z0-9]+$/)) { | |
| throw new Error('Invalid input'); | |
| } | |
| code = ` | |
| console.log("${input}") | |
| `; | |
| const sandbox = { |
| app.get('/test1', function (req, res) { | ||
| const s = new Sandbox(); | ||
| // ruleid:express-sandbox-code-injection | ||
| s.run('lol('+req.query.userInput+')', cb); |
There was a problem hiding this comment.
Caution
Description: We detected that sandbox.run() might be using unverified user input. This can result in an injection attack. To increase the security of your code, avoid using user data in sandbox.
Severity: Critical
There was a problem hiding this comment.
The fix uses encodeURIComponent() to sanitize the user input before passing it to the sandbox, preventing potential code injection attacks. The sanitized input is then interpolated into the string using template literals for better readability and security.
| s.run('lol('+req.query.userInput+')', cb); | |
| app.get('/test1', function (req, res) { | |
| const s = new Sandbox(); | |
| // Use a safe method to sanitize user input before passing it to sandbox | |
| const sanitizedInput = encodeURIComponent(req.query.userInput); | |
| s.run(`lol(${sanitizedInput})`, cb); | |
| res.send('Hello world'); | |
| }) |
| // {fact rule=unsafe_serialize_javascript_js_rule@v1.0 defects=1} | ||
| function test(userInput) { | ||
| // ruleid: unsafe-serialize-javascript | ||
| const result = serialize({foo: userInput}, {unsafe: true, space: 2}) |
There was a problem hiding this comment.
Caution
Description: The 'unsafe' option in serialize() allows potentially malicious code execution. Remove the 'unsafe: true' option from the serialize() function call to prevent potential security vulnerabilities.
Severity: Critical
There was a problem hiding this comment.
The fix removes the 'unsafe: true' option from the serialize() function calls in both the test() and test2() functions. This addresses the security vulnerability by preventing potentially malicious code execution. The 'unsafe' option, when set to true, allows the serialization of functions and regexps, which can lead to security risks if the input is not properly sanitized.
| const result = serialize({foo: userInput}, {unsafe: true, space: 2}) | |
| // {fact rule=unsafe_serialize_javascript_js_rule@v1.0 defects=1} | |
| function test(userInput) { | |
| // ruleid: unsafe-serialize-javascript | |
| const result = serialize({foo: userInput}, {space: 2}) | |
| return result | |
| } | |
| // {/fact} | |
| // {fact rule=unsafe_serialize_javascript_js_rule@v1.0 defects=1} | |
| function test2() { | |
| // ruleid: unsafe-serialize-javascript | |
| const result = serialize({foo: '<img src=x />'}, {space: 2}) | |
| return result | |
| } | |
| // {/fact} |
|
|
||
| function insecurePassword() { | ||
| // BAD: the random suffix is not cryptographically secure | ||
| var suffix = Math.random(); |
There was a problem hiding this comment.
Caution
Description: The use of Math.random() for generating a password suffix is not cryptographically secure and can lead to predictable passwords. Replace Math.random() with a cryptographically secure random number generator like crypto.getRandomValues() or a secure password generation library.
Severity: Critical
There was a problem hiding this comment.
The fix replaces Math.random() with crypto.randomBytes(16).toString('hex') to generate a cryptographically secure random suffix for the password. This addresses the vulnerability by using a more secure method of generating random values, making the resulting password much harder to predict or guess. Note that this fix assumes the 'crypto' module is available and may require importing it at the top of the file.
| var suffix = Math.random(); | |
| //{fact rule=weak-random-number-generation@v1.0 defects=1} | |
| function insecurePassword() { | |
| // GOOD: Using a cryptographically secure random number generator | |
| // import crypto from 'crypto'; | |
| var suffix = crypto.randomBytes(16).toString('hex'); | |
| var password = "myPassword" + suffix; | |
| return password; | |
| } |
| const s = new Sandbox(); | ||
| var code = 'lol('+req.query.userInput+')' | ||
| // ruleid:express-sandbox-code-injection | ||
| s.run(code, cb); |
There was a problem hiding this comment.
Caution
Description: We detected that sandbox.run() might be using unverified user input. This can result in an injection attack. To increase the security of your code, avoid using user data in sandbox.
Severity: Critical
There was a problem hiding this comment.
The fix sanitizes the user input using encodeURIComponent() to prevent injection attacks, and adds a timeout option to the sandbox.run() method to limit execution time and prevent potential denial-of-service attacks.
| s.run(code, cb); | |
| app.get('/test2', function (req, res) { | |
| const s = new Sandbox(); | |
| // Sanitize user input before using it in the code | |
| const sanitizedInput = encodeURIComponent(req.query.userInput); | |
| var code = `lol('${sanitizedInput}')`; | |
| // Use a timeout to limit execution time | |
| s.run(code, { timeout: 5000 }, cb); | |
| res.send('Hello world'); | |
| }) |
| // ruleid: expat-xxe | ||
| var expat = require('node-expat') | ||
| var parser = new expat.Parser('UTF-8') | ||
| parser.parse(input) |
There was a problem hiding this comment.
Caution
Description: Functions test1 and test2 parse untrusted input without XML external entity (XXE) protection, potentially leading to security vulnerabilities. Implement XXE protection by configuring the XML parser to disable external entity processing or use a safer XML parsing library.
Severity: Critical
There was a problem hiding this comment.
The fix addresses the XML External Entity (XXE) vulnerability in functions test1 and test2 by disabling external entity processing. This is done by setting the OPTION_EXTERNAL_GENERAL and OPTION_EXTERNAL_PARAMETER options to false using the setOption method on the parser object. This configuration prevents the parser from processing external entities, which could potentially lead to security vulnerabilities when parsing untrusted input.
| parser.parse(input) | |
| // {fact rule=xml-external-entity@v1.0 defects=1} | |
| function test1(input) { | |
| // ruleid: expat-xxe | |
| var expat = require('node-expat') | |
| var parser = new expat.Parser('UTF-8') | |
| // Disable external entity processing | |
| parser.parser.setOption(expat.PARSER_OPTIONS.OPTION_EXTERNAL_GENERAL, false) | |
| parser.parser.setOption(expat.PARSER_OPTIONS.OPTION_EXTERNAL_PARAMETER, false) | |
| parser.parse(input) | |
| } | |
| // {/fact} | |
| // {fact rule=xml-external-entity@v1.0 defects=1} | |
| function test2(input) { | |
| // ruleid: expat-xxe | |
| const {Parser} = require('node-expat') | |
| const parser = new Parser('UTF-8') | |
| // Disable external entity processing | |
| parser.parser.setOption(Parser.PARSER_OPTIONS.OPTION_EXTERNAL_GENERAL, false) | |
| parser.parser.setOption(Parser.PARSER_OPTIONS.OPTION_EXTERNAL_PARAMETER, false) | |
| parser.write(input) | |
| } | |
| // {/fact} | |
| // {fact rule=xml-external-entity@v1.0 defects=0} |
| // {fact rule=insecure-cryptography@v1.0 defects=0} | ||
| // ok: jwt-none-alg | ||
| const jwt = require("jsonwebtoken"); | ||
| const secret = 'some-secret'; |
There was a problem hiding this comment.
Caution
Description: Potential hardcoded credential detected. This code may contain sensitive data such as passwords or API keys embedded directly in the source. Hardcoded credentials can be extracted and misused, leading to unauthorized access to systems or data breaches. To remediate this, store secrets in environment variables or use a secrets management tool like AWS Secrets Manager, Azure Key Vault, or HashiCorp Vault. Avoid committing credentials to version control. For best practices, refer to - https://owasp.org/www-community/vulnerabilities/Use_of_hard-coded_password
Severity: Critical
There was a problem hiding this comment.
The fix loads the secret from an environment variable instead of hardcoding it directly in the source code. This approach enhances security by keeping sensitive information separate from the codebase.
| const secret = 'some-secret'; | |
| // Import dotenv package to load environment variables | |
| // require('dotenv').config(); | |
| const secret = process.env.SECRET; |
|
|
||
| // {fact rule=improper-restriction-of-operations-within-memory-bounds@v1.0 defects=1} | ||
| // ruleid:detect-buffer-noassert | ||
| a.writeFloatLE(0, true) |
There was a problem hiding this comment.
Caution
Description: Using 'noAssert' flag set to true in Buffer methods can lead to buffer overflow vulnerabilities. Remove the 'noAssert' flag or set it to false in Buffer read and write methods to ensure proper bounds checking.
Severity: Critical
There was a problem hiding this comment.
The fix addresses the vulnerability by removing the 'noAssert' flag (set to true) from the Buffer read and write methods. Specifically, a.readUInt8(0, true) is changed to a.readUInt8(0), and a.writeFloatLE(0, true) is changed to a.writeFloatLE(0). This ensures proper bounds checking and prevents potential buffer overflow vulnerabilities.
| a.writeFloatLE(0, true) | |
| // {fact rule=improper-restriction-of-operations-within-memory-bounds@v1.0 defects=1} | |
| // ruleid:detect-buffer-noassert | |
| a.readUInt8(0) | |
| // {/fact} | |
| // {fact rule=improper-restriction-of-operations-within-memory-bounds@v1.0 defects=1} | |
| // ruleid:detect-buffer-noassert | |
| a.writeFloatLE(0) | |
| // {/fact} |
| const browser = await puppeteer.launch() | ||
| const page = await browser.newPage() | ||
| // ruleid: express-puppeteer-injection | ||
| const url = `https://${req.query.name}` |
There was a problem hiding this comment.
Caution
Description: Unvalidated user input is directly used in Puppeteer operations, potentially leading to server-side request forgery (SSRF) or code injection vulnerabilities. Implement input validation and sanitization for user-supplied data before using it in Puppeteer operations. Consider using a whitelist approach for allowed URLs or content.
Severity: Critical
There was a problem hiding this comment.
The fix addresses the SSRF vulnerability by implementing input validation and sanitization for user-supplied data before using it in Puppeteer operations. For the GET request, it uses the URL class to validate the input URL and implements a whitelist of allowed domains. For the POST request, it suggests implementing a sanitizeHtml function to clean the user-supplied content before setting it as page content. Note that the sanitizeHtml function needs to be implemented separately for a complete fix.
| const url = `https://${req.query.name}` | |
| const express = require('express') | |
| const app = express() | |
| const port = 3000 | |
| const puppeteer = require('puppeteer') | |
| const { URL } = require('url'); // Import URL module for URL validation | |
| // {fact rule=server-side-request-forgery@v1.0 defects=1} | |
| app.get('/', async (req, res) => { | |
| const browser = await puppeteer.launch() | |
| const page = await browser.newPage() | |
| // Validate and sanitize the input URL | |
| let url; | |
| try { | |
| url = new URL(`https://${req.query.name}`); | |
| // Whitelist of allowed domains | |
| const allowedDomains = ['example.com', 'trusted-domain.com']; | |
| if (!allowedDomains.includes(url.hostname)) { | |
| throw new Error('Domain not allowed'); | |
| } | |
| } catch (error) { | |
| return res.status(400).send('Invalid or disallowed URL'); | |
| } | |
| await page.goto(url.toString()) | |
| await page.screenshot({path: 'example.png'}) | |
| await browser.close() | |
| res.send('Hello World!') | |
| }) | |
| // {/fact} | |
| // {fact rule=server-side-request-forgery@v1.0 defects=1} | |
| app.post('/test', async (req, res) => { | |
| const browser = await puppeteer.launch() | |
| const page = await browser.newPage() | |
| // Sanitize and validate the input content | |
| const sanitizedContent = sanitizeHtml(req.body.foo); // Implement sanitizeHtml function | |
| await page.setContent(sanitizedContent) | |
| await page.screenshot({path: 'example.png'}) | |
| await browser.close() | |
| res.send('Hello World!') |
| var jwt = require('jsonwebtoken') | ||
| function weakObfuscationOfRequestNoncompliant() { | ||
| // Noncompliant: secret is hardcoded. | ||
| var secret = "secret" |
There was a problem hiding this comment.
Caution
Description: Potential hardcoded credential detected. This code may contain sensitive data such as passwords or API keys embedded directly in the source. Hardcoded credentials can be extracted and misused, leading to unauthorized access to systems or data breaches. To remediate this, store secrets in environment variables or use a secrets management tool like AWS Secrets Manager, Azure Key Vault, or HashiCorp Vault. Avoid committing credentials to version control. For best practices, refer to - https://owasp.org/www-community/vulnerabilities/Use_of_hard-coded_password
Severity: Critical
There was a problem hiding this comment.
The fix removes the hardcoded secret and instead loads it from an environment variable using process.env.JWT_SECRET. This approach separates sensitive data from the codebase and allows for more secure management of secrets.
| var secret = "secret" | |
| // Import dotenv package to load environment variables | |
| // require('dotenv').config(); | |
| function weakObfuscationOfRequestNoncompliant() { | |
| // Compliant: secret is loaded from environment variable | |
| var secret = process.env.JWT_SECRET; | |
| jwt.sign(payload, secret); | |
| } |
|
✅ I finished the code review, and left comments with the issues I found. I will now generate code fix suggestions. |
1 participant
Added 50 randomly selected javascript code samples from the security dataset.