Add typescript random samples#68
Conversation
Added 50 randomly selected typescript 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 added a collection of 50 randomly selected TypeScript code samples, primarily focused on security-related scenarios. This change introduces a new directory structure under typescript/random_samples/ and includes a README.md to document the purpose and contents of these files. The intent is to provide a diverse set of examples for analysis and testing within a security context.
Highlights
- New README for TypeScript Samples: I've added a new
README.mdfile within thetypescript/directory to provide an overview of the newly introduced random samples. This document explains the purpose of these samples, their quantity (50 files), and how they are structured. - Addition of 50 TypeScript Security Samples: I've included 50 randomly selected TypeScript code samples, sourced from a security dataset, into the
typescript/random_samples/directory. These samples cover a wide range of security-related scenarios, including various types of injections (code, SQL, NoSQL), cross-site scripting (XSS), insecure configurations (AWS CDK, JWT secrets), information disclosure, and other common vulnerabilities.
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 a large number of TypeScript code samples for analysis and testing purposes. While this is a valuable addition for testing, it's important to note that many of these samples contain significant security vulnerabilities. My review highlights numerous critical and high-severity issues, including Code Injection, Cross-Site Scripting (XSS), SQL Injection, Command Injection, Prototype Pollution, and insecure configurations. This code should be treated as illustrative of security anti-patterns and must not be used in a production environment without thorough remediation of the identified vulnerabilities.
| height: 600, | ||
| webPreferences: { | ||
| // Noncompliant: 'nodeIntegration' and 'allowRunningInsecureContent' properties are enabled. | ||
| nodeIntegration: true, |
There was a problem hiding this comment.
Enabling nodeIntegration in an Electron renderer process is extremely dangerous as it allows web content to access Node.js APIs, effectively granting it full control over the user's system. This can lead to Remote Code Execution (RCE) if the renderer loads any remote or untrusted content. allowRunningInsecureContent further increases the risk.
nodeIntegration: false,
contextIsolation: true, // It's highly recommended to also enable contextIsolation
allowRunningInsecureContent: false| db.sequelize.query( | ||
| `INSERT INTO user (username, password) VALUES('${username}','${password}')` |
There was a problem hiding this comment.
Constructing a raw SQL query by directly interpolating user input is a critical SQL Injection vulnerability. An attacker can manipulate the username and password variables to alter the query's logic, potentially bypassing authentication, stealing data, or corrupting the database.
| db.sequelize.query( | |
| `INSERT INTO user (username, password) VALUES('${username}','${password}')` | |
| db.sequelize.query( | |
| 'INSERT INTO user (username, password) VALUES(?, ?)', | |
| { replacements: [username, password], type: db.sequelize.QueryTypes.INSERT } | |
| ) |
| var vals = keys.map(function(key) { | ||
| return vars[key]; | ||
| }); | ||
| return Function(keys.join(', '), 'return ' + unparse(node)).apply(null, vals); |
There was a problem hiding this comment.
Using new Function() to dynamically generate and execute code is a significant security risk, equivalent to using eval(). If the AST (node) that is being unparsed can be influenced by user input, this could lead to Remote Code Execution (RCE).
| const childProcess = execSync( | ||
| '"' + opts.sqlclPath + '"' + // Sqlcl path | ||
| ' ' + opts.connectString + // Connect string (user/pass@server:port/sid) | ||
| ' @"' + path.resolve(__dirname, 'lib/script') + '"' + // Sql to execute | ||
| ' "' + path.resolve(__dirname, 'lib/distUpload.js') + '"' + // Param &1 (js to execute) | ||
| ' "' + path.resolve(opts.directory) + '"' + // Param &2 | ||
| ' ' + opts.appID + // Param &3 | ||
| ' "' + opts.destination + '"' + // Param &4 | ||
| ' "' + opts.pluginName + '"' // Param &5 |
There was a problem hiding this comment.
Constructing a shell command by concatenating multiple parts, some of which are options that could be user-controlled, is extremely dangerous and can lead to Command Injection. Even with quoting, it's difficult to prevent all possible shell injection attacks.
| return sh( | ||
| `lsof -i tcp:${port} | grep LISTEN | awk '{print $2}' | xargs kill -9` |
There was a problem hiding this comment.
Interpolating user-provided input directly into a shell command is a critical Command Injection vulnerability. The port variable is not sufficiently sanitized before being used in the command string, which could allow an attacker to execute arbitrary commands on the system.
| // {fact rule=unsafe_serialize_javascript_js_rule@v1.0 defects=1} | ||
| function test(userInput: any) { | ||
| // ruleid: unsafe-serialize-javascript | ||
| const result = serialize({foo: userInput}, {unsafe: true, space: 2}) |
There was a problem hiding this comment.
Using serialize-javascript with the { unsafe: true } option disables escaping of HTML-significant characters. If the resulting string is embedded directly into an HTML page inside a <script> tag, it can lead to Cross-Site Scripting (XSS) vulnerabilities.
| const result = serialize({foo: userInput}, {unsafe: true, space: 2}) | |
| const result = serialize({foo: userInput}, {space: 2}) |
|
|
||
| // {fact rule=incorrect-expression@v1.0 defects=1} | ||
| router.get("/tstMe", (req: { params: { id: string; }; }, res: any) => { | ||
| var r = /([a-z]+)+$/; |
There was a problem hiding this comment.
This regular expression is vulnerable to Regular Expression Denial of Service (ReDoS). The nested quantifiers ([a-z]+)+ can cause catastrophic backtracking when processing certain strings (e.g., a long string of 'a's followed by a character not in the set), leading to excessive CPU usage and potentially freezing the application.
| var r = /([a-z]+)+$/; | |
| var r = /[a-z]+$/; |
| try { | ||
| var parsed = unknownParseFunction(deflt); | ||
| } catch(e) { | ||
| document.write("Had an error: " + e + "."); |
There was a problem hiding this comment.
Writing an error object directly to the document using document.write() can lead to a Cross-Site Scripting (XSS) vulnerability. If the error message contains user-controlled data (in this case, from the URL), an attacker could craft a malicious link that injects and executes arbitrary scripts.
// Sanitize the error message before displaying it, or show a generic error.
console.error(e); // Log the full error for debugging
document.write("An unexpected error occurred.");| @@ -0,0 +1,52 @@ | |||
|
|
|||
| var Test1 = <a target='_blank' href="http://example.com/"></a> | |||
There was a problem hiding this comment.
When opening links in a new tab with target='_blank', it's crucial to add rel='noopener noreferrer' to prevent security vulnerabilities like tabnabbing. Without noopener, the newly opened page can access the original page's window object via window.opener, which can be a security risk.
var Test1 = <a target='_blank' rel="noopener noreferrer" href="http://example.com/"></a>| @@ -0,0 +1,11 @@ | |||
| // ruleid: moment-deprecated | |||
| import moment from 'moment'; | |||
There was a problem hiding this comment.
The moment.js library is in maintenance mode and is no longer recommended for new projects. It has known issues with large bundle size and mutability. Consider migrating to a modern alternative like date-fns, dayjs, or luxon for better performance, smaller bundle size, and an immutable API.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
| var fs = module.parent.require('fs'); | ||
| var path = module.parent.require('path'); | ||
| var async = module.parent.require('async'); | ||
| var winston = module.parent.require('winston'); |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected on this line, where untrusted input is passed to a method that may execute arbitrary code. This issue allows attackers to inject and execute arbitrary code within the application, which could lead to unauthorized access to sensitive data or other malicious actions. To mitigate this, ensure that all user-supplied input is properly sanitized and validated before being processed. Avoid passing untrusted input to methods like eval, send, or system that can execute arbitrary code. Where possible, use safer alternatives such as parameterized queries or more controlled methods for handling user input. Learn more
Severity: Critical
There was a problem hiding this comment.
The fix replaces the use of module.parent.require() with a direct require() statement, eliminating the potential for code injection through the module system. This approach ensures that only the intended 'winston' package is loaded, improving security and reducing the risk of arbitrary code execution.
| var winston = module.parent.require('winston'); | |
| // Import the 'winston' package directly | |
| const winston = require('winston'); |
| var user = require.main.require('./src/user'); | ||
| var groups = require.main.require('./src/groups'); | ||
| var fs = module.parent.require('fs'); | ||
| var path = module.parent.require('path'); |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected on this line, where untrusted input is passed to a method that may execute arbitrary code. This issue allows attackers to inject and execute arbitrary code within the application, which could lead to unauthorized access to sensitive data or other malicious actions. To mitigate this, ensure that all user-supplied input is properly sanitized and validated before being processed. Avoid passing untrusted input to methods like eval, send, or system that can execute arbitrary code. Where possible, use safer alternatives such as parameterized queries or more controlled methods for handling user input. Learn more
Severity: Critical
There was a problem hiding this comment.
The fix replaces the potentially unsafe module.parent.require() with the standard require() function to import the 'path' module directly, eliminating the risk of code injection through the module system.
| var path = module.parent.require('path'); | |
| // Import the 'path' module directly | |
| const path = require('path'); |
| var fs = module.parent.require('fs'); | ||
| var path = module.parent.require('path'); | ||
| var async = module.parent.require('async'); | ||
| var winston = module.parent.require('winston'); |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected on this line, where untrusted input is passed to a method that may execute arbitrary code. This issue allows attackers to inject and execute arbitrary code within the application, which could lead to unauthorized access to sensitive data or other malicious actions. To mitigate this, ensure that all user-supplied input is properly sanitized and validated before being processed. Avoid passing untrusted input to methods like eval, send, or system that can execute arbitrary code. Where possible, use safer alternatives such as parameterized queries or more controlled methods for handling user input. Learn more
Severity: Critical
There was a problem hiding this comment.
The fix replaces the use of module.parent.require() with a direct require() statement, eliminating the potential for code injection through the module.parent object. This ensures that only the intended 'winston' package is loaded, improving security.
| var winston = module.parent.require('winston'); | |
| // Import the 'winston' package directly | |
| const winston = require('winston'); |
| var path = module.parent.require('path'); | ||
| var async = module.parent.require('async'); | ||
| var winston = module.parent.require('winston'); | ||
| var nconf = module.parent.require('nconf'); |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected on this line, where untrusted input is passed to a method that may execute arbitrary code. This issue allows attackers to inject and execute arbitrary code within the application, which could lead to unauthorized access to sensitive data or other malicious actions. To mitigate this, ensure that all user-supplied input is properly sanitized and validated before being processed. Avoid passing untrusted input to methods like eval, send, or system that can execute arbitrary code. Where possible, use safer alternatives such as parameterized queries or more controlled methods for handling user input. Learn more
Severity: Critical
There was a problem hiding this comment.
The fix removes the use of module.parent.require() and instead uses a direct require() statement to import the 'nconf' package. This eliminates the potential for code injection through the use of module.parent.
| var nconf = module.parent.require('nconf'); | |
| // Import the 'nconf' package directly | |
| const nconf = require('nconf'); |
| var topics = require.main.require('./src/topics'); | ||
| var user = require.main.require('./src/user'); | ||
| var groups = require.main.require('./src/groups'); | ||
| var fs = module.parent.require('fs'); |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected on this line, where untrusted input is passed to a method that may execute arbitrary code. This issue allows attackers to inject and execute arbitrary code within the application, which could lead to unauthorized access to sensitive data or other malicious actions. To mitigate this, ensure that all user-supplied input is properly sanitized and validated before being processed. Avoid passing untrusted input to methods like eval, send, or system that can execute arbitrary code. Where possible, use safer alternatives such as parameterized queries or more controlled methods for handling user input. Learn more
Severity: Critical
There was a problem hiding this comment.
The fix removes the use of module.parent.require() and instead uses the standard require() function to import the 'fs' module directly, eliminating the potential for code injection through manipulating the module hierarchy.
| var fs = module.parent.require('fs'); | |
| // Import the 'fs' module directly | |
| const fs = require('fs'); |
|
|
||
| var command = this.test_command+" "+testFile + " " + this.modelFileString + " /tmp/out_" + this.timestamp; | ||
|
|
||
| var output = child_process.execSync(command) |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected on this line, where untrusted input is passed to a method that may execute arbitrary code. This issue allows attackers to inject and execute arbitrary code within the application, which could lead to unauthorized access to sensitive data or other malicious actions. To mitigate this, ensure that all user-supplied input is properly sanitized and validated before being processed. Avoid passing untrusted input to methods like eval, send, or system that can execute arbitrary code. Where possible, use safer alternatives such as parameterized queries or more controlled methods for handling user input. Learn more
Severity: Critical
There was a problem hiding this comment.
The fix replaces child_process.execSync with spawn, which executes the command as separate arguments, preventing potential command injection vulnerabilities by not interpreting the input as a shell command string.
| var output = child_process.execSync(command) | |
| var command = this.test_command+" "+testFile + " " + this.modelFileString + " /tmp/out_" + this.timestamp; | |
| // Use spawn instead of execSync to avoid potential command injection | |
| // import { spawn } from 'child_process'; | |
| var output = spawn(this.test_command, [testFile, this.modelFileString, "/tmp/out_" + this.timestamp]); | |
| console.log(command) | |
| var result = fs.readFileSync("/tmp/out_" + this.timestamp, "utf-8").split(" | |
| ") |
| * @returns mapLabelToMapFeatureToWeight. | ||
| */ | ||
| function modelStringToModelMap(modelString) { | ||
| var matches = LIB_LINEAR_MODEL_PATTERN.exec(modelString); |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected on this line, where untrusted input is passed to a method that may execute arbitrary code. This issue allows attackers to inject and execute arbitrary code within the application, which could lead to unauthorized access to sensitive data or other malicious actions. To mitigate this, ensure that all user-supplied input is properly sanitized and validated before being processed. Avoid passing untrusted input to methods like eval, send, or system that can execute arbitrary code. Where possible, use safer alternatives such as parameterized queries or more controlled methods for handling user input. Learn more
Severity: Critical
There was a problem hiding this comment.
The remediation is made by sanitizing the input modelString using validator.escape() before passing it to the exec() method. This prevents potential code injection by escaping special characters in the input.
| var matches = LIB_LINEAR_MODEL_PATTERN.exec(modelString); | |
| function modelStringToModelMap(modelString) { | |
| // Import the 'validator' package for input sanitization | |
| // validator.escape() is used to sanitize the input string | |
| var sanitizedModelString = validator.escape(modelString); | |
| var matches = LIB_LINEAR_MODEL_PATTERN.exec(sanitizedModelString); | |
| if (!matches) { | |
| console.log(sanitizedModelString); | |
| throw new Error("Model does not match SVM-Linear format"); | |
| }; | |
| var labels = matches[1].split(/\s+/); |
| var topics = require.main.require('./src/topics'); | ||
| var user = require.main.require('./src/user'); | ||
| var groups = require.main.require('./src/groups'); | ||
| var fs = module.parent.require('fs'); |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected on this line, where untrusted input is passed to a method that may execute arbitrary code. This issue allows attackers to inject and execute arbitrary code within the application, which could lead to unauthorized access to sensitive data or other malicious actions. To mitigate this, ensure that all user-supplied input is properly sanitized and validated before being processed. Avoid passing untrusted input to methods like eval, send, or system that can execute arbitrary code. Where possible, use safer alternatives such as parameterized queries or more controlled methods for handling user input. Learn more
Severity: Critical
There was a problem hiding this comment.
The fix removes the use of module.parent.require() and instead uses the standard require() function to import the 'fs' module directly, eliminating the potential for code injection through the module.parent object.
| var fs = module.parent.require('fs'); | |
| // Import the 'fs' module directly | |
| const fs = require('fs'); |
| var user = require.main.require('./src/user'); | ||
| var groups = require.main.require('./src/groups'); | ||
| var fs = module.parent.require('fs'); | ||
| var path = module.parent.require('path'); |
There was a problem hiding this comment.
Caution
Description: A potential code injection vulnerability has been detected on this line, where untrusted input is passed to a method that may execute arbitrary code. This issue allows attackers to inject and execute arbitrary code within the application, which could lead to unauthorized access to sensitive data or other malicious actions. To mitigate this, ensure that all user-supplied input is properly sanitized and validated before being processed. Avoid passing untrusted input to methods like eval, send, or system that can execute arbitrary code. Where possible, use safer alternatives such as parameterized queries or more controlled methods for handling user input. Learn more
Severity: Critical
There was a problem hiding this comment.
The fix replaces the potentially unsafe module.parent.require() with the standard require() function to import the 'path' module directly, eliminating the risk of code injection through the module system.
| var path = module.parent.require('path'); | |
| // Import the 'path' module directly | |
| const path = require('path'); |
| @@ -0,0 +1,5 @@ | |||
| //{fact rule=code-injection@v1.0 defects=0} | |||
|
|
|||
| eval(document.location.href.substring(document.location.href.indexOf("default=")+8)) | |||
There was a problem hiding this comment.
Caution
Description: Passing user-provided inputs to code that is dynamically executed could lead to injection attacks. Make sure you validate user-provided data before using it.
Severity: Critical
There was a problem hiding this comment.
The fix replaces the use of eval() with URLSearchParams to safely parse the 'default' parameter from the URL. This prevents potential code injection attacks by treating the parameter as a string value rather than executable code.
| eval(document.location.href.substring(document.location.href.indexOf("default=")+8)) | |
| // Import the URLSearchParams class to safely parse URL parameters | |
| const params = new URLSearchParams(document.location.search); | |
| const defaultValue = params.get('default'); | |
| if (defaultValue) { | |
| // Use the parsed value directly, avoiding eval() | |
| console.log(defaultValue); | |
| } |
|
✅ 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 typescript code samples from the security dataset.