Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for namespaced safe_methods option #1916

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

evpgh
Copy link

@evpgh evpgh commented Jan 27, 2025

Based on the issue or feature request #1738 I created a safe method handler to do the parsing of the command line argument --safe-methods and comparisons in checks.

Copy link

dryrunsecurity bot commented Jan 27, 2025

DryRun Security Summary

The code changes aim to improve Brakeman's security scanner for Ruby on Rails by introducing a SafeMethodHandler module that enhances method identifier handling, vulnerability detection accuracy, and overall security analysis capabilities.

Expand for full summary

Summary:

The provided code changes are related to the Brakeman security scanner for Ruby on Rails applications. The changes aim to improve the security analysis capabilities of Brakeman by enhancing the handling and normalization of method identifiers, which is an important aspect of detecting vulnerabilities such as cross-site scripting (XSS).

The key changes include:

  1. Introducing the SafeMethodHandler module to centralize the logic for determining which methods should be considered "safe" and ignored during the security analysis.
  2. Improving the CheckSelectTag, CheckLinkToHref, and CheckCrossSiteScripting checks to leverage the SafeMethodHandler for more accurate and flexible detection of potential vulnerabilities.
  3. Adding comprehensive unit tests for the SafeMethodHandler class to ensure the robustness and reliability of the method normalization and comparison functionality.
  4. Enhancing the handling and parsing of method identifiers, including support for different delimiter formats and namespace detection.

These changes demonstrate a focused effort to enhance the security analysis capabilities of the Brakeman tool, making it more effective at identifying and mitigating security vulnerabilities in Ruby on Rails applications. The improvements to the method handling and normalization functionality are particularly important, as they can help prevent false positives and improve the overall accuracy of the security scans.

Files Changed:

  1. lib/brakeman/checks/check_select_tag.rb: Improvements to the CheckSelectTag check to leverage the SafeMethodHandler for more accurate detection of the CVE-2012-3463 vulnerability.
  2. lib/brakeman/options.rb: Modifications to the command-line option handling to use the SafeMethodHandler for normalizing "safe methods" specified by the user.
  3. lib/brakeman/checks/check_link_to_href.rb: Updates to the CheckLinkToHref check to utilize the SafeMethodHandler for determining if a method should be ignored during the security analysis.
  4. lib/brakeman/checks/check_cross_site_scripting.rb: Changes to the CheckCrossSiteScripting check to leverage the SafeMethodHandler for handling "safe" methods.
  5. test/tests/options.rb: Addition of a test case for the safe_methods_option feature, including an extra safe method.
  6. test/tests/safe_method_handler.rb: Comprehensive unit tests for the SafeMethodHandler class, covering various normalization, parsing, and matching scenarios.
  7. lib/brakeman/safe_method_handler.rb: Implementation of the SafeMethodHandler class, including new functionality for normalizing, parsing, and matching method identifiers.

Code Analysis

We ran 9 analyzers against 7 files and 0 analyzers had findings. 9 analyzers had no findings.

View PR in the DryRun Dashboard.

@evpgh
Copy link
Author

evpgh commented Feb 5, 2025

Hey @presidentbeef I know this is a low priority but could you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant