Add ruby code quality examples#53
Conversation
amazon-pratik
commented
Aug 5, 2025
amazon-pratik
commented
- Added code quality examples for ruby
- Includes error handling, logging, naming, performance, and readability/maintainability examples
- Contains both compliant and non-compliant code samples
- Added code quality examples for ruby - Includes error handling, logging, naming, performance, and readability/maintainability examples - Contains both compliant and non-compliant code samples
|
⏳ 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 comprehensive set of Ruby code quality examples to the repository. This includes distinct sections covering best and worst practices for error handling, secure logging, effective naming conventions, performance optimization, and improving code readability and maintainability. The goal is to provide clear, practical illustrations for developers to understand and apply these principles in their Ruby projects.
Highlights
- Error Handling Examples: I've introduced a series of examples demonstrating both problematic and robust error handling patterns in Ruby. This includes scenarios like swallowing exceptions, returning generic errors, and hiding specific failure details, contrasted with examples that properly validate arguments, handle specific exception types, and re-raise meaningful errors.
- Logging Practices: I've added examples illustrating common pitfalls in logging, such as inadvertently logging sensitive information like API keys, passwords, or full request bodies. Alongside these, I've provided secure logging counterparts that demonstrate techniques like data redaction, masking, and selective logging to prevent exposure of confidential data.
- Naming Convention Improvements: This PR includes examples that highlight the importance of clear and descriptive naming conventions in Ruby. You'll find instances of poorly named variables, methods, and classes that use cryptic abbreviations or generic terms, contrasted with refactored versions that employ meaningful and self-documenting names.
- Performance Optimization Techniques: I've added a set of examples showcasing common performance bottlenecks in Ruby applications, such as inefficient array searching (linear scan vs. Set), string concatenation in loops (
+=vs.join), and N+1 database queries. Each inefficient example is paired with an optimized version demonstrating better practices for performance. - Readability and Maintainability Enhancements: I've included examples that demonstrate how to improve the readability and maintainability of Ruby code. This covers refactoring complex, deeply nested conditional logic, simplifying long boolean expressions, and breaking down monolithic methods into smaller, more manageable, and testable units using constants and helper methods.
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. ↩
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
| # {fact rule=code-quality-logging@v1.0 defects=0} | ||
| puts "Authentication attempt for user: #{username}" | ||
|
|
||
| if username == 'admin' && password == 'secret123' |
There was a problem hiding this comment.
Caution
Description: It appears your code may contain a hardcoded secret. We recommend replacing it with AWS Secrets Manager references to enhance security and follow best practices. For more information, please refer OWASP password storage cheat sheet.
Severity: Critical
There was a problem hiding this comment.
The fix replaces the hardcoded password with a secure retrieval from AWS Secrets Manager, eliminating the vulnerability of exposing sensitive information in the code.
| if username == 'admin' && password == 'secret123' | |
| # Import the AWS Secrets Manager client library | |
| # This library is used to securely retrieve secrets from AWS Secrets Manager | |
| require 'aws-sdk-secretsmanager' | |
| def authenticate(username, password) | |
| puts "Authentication attempt for user: #{username}" | |
| # Retrieve the admin password from AWS Secrets Manager | |
| client = Aws::SecretsManager::Client.new(region: 'us-west-2') | |
| secret = client.get_secret_value(secret_id: 'admin_password') | |
| admin_password = JSON.parse(secret.secret_string)['password'] | |
| if username == 'admin' && password == admin_password | |
| puts "Authentication successful for user: #{username}" | |
| return true | |
| end | |
| puts "Authentication failed for user: #{username}" | |
| return false | |
| end |
| def get_property(object, property) | ||
| # {fact rule=code-quality-error-handling@v1.0 defects=1} | ||
| begin | ||
| return object.send(property) |
There was a problem hiding this comment.
Caution
Description: Bad Send is a security best practice that identifies and addresses potential vulnerabilities arising from unsafe usage of Object#send, try, send, and public_send in Ruby code.
Severity: Critical
There was a problem hiding this comment.
The remediation replaces the unsafe send method with public_send, adds a whitelist of allowed properties, checks if the object responds to the property, and improves error handling by logging the error instead of silently failing.
| return object.send(property) | |
| def get_property(object, property) | |
| # {fact rule=code-quality-error-handling@v1.0 defects=1} | |
| begin | |
| valid_properties = %i[allowed_property1 allowed_property2 allowed_property3] # Replace with actual property names you want to allow | |
| if valid_properties.include?(property) && object.respond_to?(property) | |
| return object.public_send(property) | |
| else | |
| raise ArgumentError, "Invalid or unauthorized property: #{property}" | |
| end | |
| rescue => e | |
| # Log the error instead of silent failure | |
| Rails.logger.error("Error getting property: #{e.message}") | |
| nil | |
| end | |
| # {/fact} | |
| end |
| raise NoMethodError, "Property '#{property}' does not exist on #{object.class}" | ||
| end | ||
|
|
||
| object.send(property) |
There was a problem hiding this comment.
Caution
Description: Bad Send is a security best practice that identifies and addresses potential vulnerabilities arising from unsafe usage of Object#send, try, send, and public_send in Ruby code.
Severity: Critical
There was a problem hiding this comment.
The vulnerability is fixed by replacing object.send(property) with object.public_send(property). This change ensures that only public methods can be called, improving security by preventing access to private or protected methods.
| object.send(property) | |
| raise NoMethodError, "Property '#{property}' does not exist on #{object.class}" | |
| end | |
| object.public_send(property) | |
| # {/fact} | |
| end |
| # {fact rule=code-quality-logging@v1.0 defects=1} | ||
| puts "User #{username} attempting login with password: #{password}" | ||
|
|
||
| if username == 'admin' && password == 'secret123' |
There was a problem hiding this comment.
Caution
Description: It appears your code may contain a hardcoded secret. We recommend replacing it with AWS Secrets Manager references to enhance security and follow best practices. For more information, please refer OWASP password storage cheat sheet.
Severity: Critical
There was a problem hiding this comment.
The fix replaces hardcoded credentials with AWS Secrets Manager and implements proper logging practices by using a logger instead of puts, removing sensitive information from log messages.
| if username == 'admin' && password == 'secret123' | |
| # Import statements | |
| require 'aws-sdk-secretsmanager' # AWS SDK for Ruby to interact with Secrets Manager | |
| require 'logger' | |
| def authenticate(username, password) | |
| logger = Logger.new(STDOUT) | |
| logger.info("User #{username} attempting login") | |
| secrets_manager = Aws::SecretsManager::Client.new(region: 'us-west-2') | |
| secret = secrets_manager.get_secret_value(secret_id: 'admin_credentials') | |
| admin_credentials = JSON.parse(secret.secret_string) | |
| if username == admin_credentials['username'] && password == admin_credentials['password'] | |
| logger.info("Login successful for #{username}") | |
| return true | |
| end | |
| logger.info("Login failed for #{username}") | |
| return false | |
| end |
| def store_user_credentials(username, password) | ||
| @user_credentials = {} if @user_credentials.nil? | ||
|
|
||
| hashed_password = username + password # Simplified hashing |
There was a problem hiding this comment.
Caution
Description: Simplified hashing method is insecure and misleading. Replace the simplified hashing with a secure hashing algorithm, such as BCrypt, and update the comment to reflect the actual hashing method used.
Severity: Critical
There was a problem hiding this comment.
The fix replaces the insecure "simplified hashing" method with BCrypt, a secure password hashing algorithm. The line hashed_password = username + password is replaced with hashed_password = BCrypt::Password.create(password), which creates a secure hash of the password. This addresses the security vulnerability in the original code by using a proper cryptographic hashing function instead of a simple string concatenation.
| hashed_password = username + password # Simplified hashing | |
| def store_user_credentials(username, password) | |
| @user_credentials = {} if @user_credentials.nil? | |
| # Use BCrypt for secure password hashing | |
| hashed_password = BCrypt::Password.create(password) | |
| @user_credentials[username] = hashed_password | |
| @user_credentials.key?(username) |
|
|
||
| def apply_transformation(item, transformation) | ||
| if transformation.is_a?(Symbol) | ||
| item.send(transformation) |
There was a problem hiding this comment.
Caution
Description: Bad Send is a security best practice that identifies and addresses potential vulnerabilities arising from unsafe usage of Object#send, try, send, and public_send in Ruby code.
Severity: Critical
There was a problem hiding this comment.
The fix introduces a whitelist of allowed transformations and uses public_send instead of send to call the method, ensuring only approved methods can be called on the item. This prevents potential security vulnerabilities from arbitrary method execution.
| item.send(transformation) | |
| def apply_transformation(item, transformation) | |
| valid_transformations = %i[allowed_method1 allowed_method2 allowed_method3] # Replace with actual method names you want to allow | |
| if transformation.is_a?(Symbol) | |
| if valid_transformations.include?(transformation) && item.respond_to?(transformation) | |
| item.public_send(transformation) | |
| else | |
| # Handle the error or invalid method case appropriately | |
| nil | |
| end | |
| else | |
| transformation.call(item) | |
| end |
| return [] if user_ids.empty? | ||
|
|
||
| user_id_list = user_ids.join(',') | ||
| batch_query = "SELECT * FROM users WHERE id IN (#{user_id_list})" |
There was a problem hiding this comment.
Caution
Description: The batch query is constructed using string interpolation, which can lead to SQL injection vulnerabilities. Use parameterized queries or prepared statements to prevent SQL injection.
Severity: Critical
There was a problem hiding this comment.
The fix addresses the SQL injection vulnerability by using a parameterized query instead of string interpolation. The batch_query now uses a placeholder '?' for the user IDs, and the execute_batch_query method is modified to accept the user_ids as a separate parameter. This approach separates the SQL query structure from the data, preventing potential SQL injection attacks. However, the fix is incomplete as it requires implementing an 'execute' method to handle the parameterized query execution, which should be added to the class to complete the security improvement.
| batch_query = "SELECT * FROM users WHERE id IN (#{user_id_list})" | |
| # {fact rule=code-quality-performance@v1.0 defects=0} | |
| return [] if user_ids.empty? | |
| # Use parameterized query to prevent SQL injection | |
| batch_query = "SELECT * FROM users WHERE id IN (?)" | |
| execute_batch_query(batch_query, user_ids) | |
| # {/fact} | |
| end | |
| private | |
| def execute_batch_query(query, user_ids) | |
| # Simulate single batch database call | |
| sleep(0.05) # 50ms delay | |
| # Assuming the execute method handles parameterized queries | |
| execute(query, user_ids).map do |user_data| | |
| "User data from batch query: #{user_data}" | |
| end | |
| end | |
| # Implement execute(query, params) method to handle parameterized queries | |
| end |
| raise ArgumentError, "File is not readable: #{filename}" unless File.readable?(filename) | ||
|
|
||
| begin | ||
| File.read(filename) |
There was a problem hiding this comment.
Warning
Description: The read_file method takes a filename parameter which is used directly in File.read(filename) without proper path sanitization. While there are checks for file existence and readability (File.exist? and File.readable?), these do not prevent path traversal attacks. An attacker could potentially provide a filename like "../../../../etc/passwd" to access files outside the intended directory. The severity of this vulnerability depends on the broader context of how this class is used and the system's directory permissions.
Severity: High
There was a problem hiding this comment.
The vulnerability is addressed by using Pathname.new(filename).cleanpath.to_s to sanitize the input filename, preventing path traversal attacks by resolving and cleaning up the path before use.
| File.read(filename) | |
| # require 'pathname' | |
| # Pathname is used to safely handle file paths and prevent path traversal attacks | |
| def read_file(filename) | |
| # {fact rule=code-quality-error-handling@v1.0 defects=0} | |
| raise ArgumentError, "Filename cannot be nil" if filename.nil? | |
| safe_path = Pathname.new(filename).cleanpath.to_s | |
| raise ArgumentError, "File does not exist: #{safe_path}" unless File.exist?(safe_path) | |
| raise ArgumentError, "File is not readable: #{safe_path}" unless File.readable?(safe_path) | |
| begin | |
| File.read(safe_path) | |
| rescue Errno::ENOENT => e | |
| raise RuntimeError, "File not found: #{safe_path}" | |
| rescue Errno::EACCES => e | |
| raise RuntimeError, "Permission denied: #{safe_path}" | |
| rescue IOError => e | |
| raise RuntimeError, "IO error reading file: #{e.message}" | |
| end | |
| # {/fact} | |
| end |
| def debug_request(request) | ||
| # {fact rule=code-quality-logging@v1.0 defects=1} | ||
| puts "Request URL: #{request[:url]}" | ||
| puts "Request headers: #{request[:headers].inspect}" |
There was a problem hiding this comment.
Warning
Description: Potential log injection detected. Ensure all untrusted input is properly sanitized before logging. Use parameterized logging or validate input against an allow list to prevent log injection vulnerabilities. Consider using a dedicated logging library's built-in sanitization features when available. Learn more - https://cwe.mitre.org/data/definitions/117.html
Severity: High
There was a problem hiding this comment.
The fix uses Ruby's Logger class to handle logging securely, sanitizes header output by removing newlines, and redacts sensitive information like authorization headers, request body, and session data to prevent potential log injection and exposure of sensitive data.
| puts "Request headers: #{request[:headers].inspect}" | |
| # require 'logger' | |
| # Logger is used for secure logging and sanitization of input | |
| def debug_request(request) | |
| logger = Logger.new(STDOUT) | |
| logger.info("Request URL: #{request[:url]}") | |
| logger.info("Request headers: #{request[:headers].to_s.gsub(/\r | |
| |\r| | |
| /, ' ')}") | |
| logger.info("Authorization header: [REDACTED]") | |
| logger.info("Request body: [REDACTED]") | |
| logger.info("Session data: [REDACTED]") | |
| end |
1 participant
