Fix compatibility with `--enable-frozen-string-literal` #1855

casperisfine · 2024-06-27T06:15:43Z

Since Ruby 2.3, Ruby can be started with this options, and it's expected to become the default in Ruby 4.0 (no release date yet).

This commit fixes the callsites that assume string literal are mutable.

Note however that a large part of the test suite is still failing, because of two dependencies:

ruby2ruby: Fix compatibility with --enable-frozen-string-literal seattlerb/ruby2ruby#58
erubis: (this one is abandoned so can hardly be fixed).

dryrunsecurity · 2024-06-27T06:16:07Z

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security	Status	Findings
Server-Side Request Forgery Analyzer	✅	0 findings
Configured Codepaths Analyzer	✅	0 findings
Authn/Authz Analyzer	❕	1 finding
Secrets Analyzer	✅	0 findings
IDOR Analyzer	✅	0 findings
SQL Injection Analyzer	✅	0 findings
Sensitive Files Analyzer	✅	0 findings

Note

🟢 Risk threshold not exceeded.

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The changes in this pull request are primarily focused on improving the functionality and usability of the Brakeman security scanner for Ruby on Rails applications. The changes span several files and cover a range of improvements, including:

Command-line Option Handling: The changes to the Brakeman::Options module aim to improve the handling and consistency of command-line options for the Brakeman tool.
Erubis Template Handling: The changes to the various Brakeman::*Erubis classes focus on enhancing the handling of Erubis templates, including the integration of a custom Brakeman::ErubisPatch module to address compatibility and security-related concerns.
Report Generation: The changes to the Brakeman::Report classes improve the formatting, readability, and efficiency of the security reports generated by Brakeman.
User Input Handling: The changes to the Brakeman::Warning class focus on improving the handling and formatting of user input to ensure consistent and reliable behavior when processing and displaying security warnings.

While these changes do not directly address any known security vulnerabilities, they contribute to the overall security and reliability of the Brakeman tool, which is an important application security tool for Ruby on Rails developers. By improving the usability, performance, and robustness of the Brakeman scanner, these changes can indirectly enhance the security of the applications being analyzed.

Files Changed:

lib/brakeman/options.rb: The changes focus on improving the handling of command-line options for the Brakeman tool, such as ensuring consistent naming of check options and simplifying the output format option.
lib/brakeman/app_tree.rb: The changes involve a minor refactoring of the regex_for_paths method, which is used to generate a regular expression pattern for filtering files and directories during the Brakeman scan.
gem_common.rb: The changes update the version constraint for the "slim" gem dependency, likely to ensure compatibility with the latest version of the gem.
lib/brakeman/parsers/erubis_patch.rb: The changes introduce a new Brakeman::ErubisPatch module to address compatibility issues with frozen string literals in the Erubis library.
lib/brakeman/parsers/rails2_erubis.rb: The changes incorporate the Brakeman::ErubisPatch module into the Brakeman::ScannerErubis class, suggesting further enhancements to the Erubis handling in the Brakeman scanner.
lib/brakeman/parsers/rails2_xss_plugin_erubis.rb: The changes focus on improving the handling of dynamic content within Erubis templates, specifically addressing potential XSS vulnerabilities.
lib/brakeman/processors/alias_processor.rb: The changes are a minor optimization to the process_array_join method, improving the performance of the array-to-string conversion process.
lib/brakeman/report/report_markdown.rb: The changes enhance the generation of Markdown-formatted security reports, improving the readability and usability of the Brakeman output.
lib/brakeman/report/report_text.rb: The changes are minor optimizations to the text-based report generation, focusing on improving the performance of string concatenation operations.
lib/brakeman/parsers/rails3_erubis.rb: The changes modify the Brakeman::Rails3Erubis class to integrate the Brakeman::ErubisPatch module and customize the handling of Erubis templates.
lib/brakeman/report/report_table.rb: The changes are minor optimizations to the string concatenation operations in the Brakeman::Report::Table class.
lib/brakeman/warning.rb: The changes focus on improving the handling of user input and code formatting in the Brakeman::Warning class, ensuring consistent and reliable behavior when processing and displaying security warnings.

Powered by DryRun Security

presidentbeef · 2024-07-05T21:06:37Z

lib/brakeman/warning.rb

@@ -53,6 +53,7 @@ def initialize options = {}
    OPTIONS.each do |key, var|
      self.instance_variable_set(var, options[key])
    end
+    @warning_type = @warning_type.dup


Most of these look okay, but I'm a bit surprised by this one. I don't expect warning_type to get modified. Any clues to where that might be happening?

It fails two tests:

1) Error: TestReportGeneration#test_tabs_sanity: FrozenError: can't modify frozen String: "Cross-Site Scripting", created at /Users/byroot/src/github.com/casperisfine/brakeman/lib/brakeman/checks/check_content_tag.rb:199 lib/brakeman/report/report_tabs.rb:12:in `gsub!' lib/brakeman/report/report_tabs.rb:12:in `block (2 levels) in generate_report' lib/brakeman/report/report_tabs.rb:10:in `map' lib/brakeman/report/report_tabs.rb:10:in `block in generate_report' lib/brakeman/report/report_tabs.rb:8:in `map' lib/brakeman/report/report_tabs.rb:8:in `generate_report' lib/brakeman/report.rb:107:in `generate' lib/brakeman/report.rb:58:in `format' lib/brakeman/report.rb:63:in `method_missing' test/tests/report_generation.rb:67:in `test_tabs_sanity' 2) Error: TestTabsOutput#test_reported_warnings: FrozenError: can't modify frozen String: "Cross-Site Request Forgery", created at /Users/byroot/src/github.com/casperisfine/brakeman/lib/brakeman/checks/check_csrf_token_forgery_cve.rb:19 lib/brakeman/report/report_tabs.rb:12:in `gsub!' lib/brakeman/report/report_tabs.rb:12:in `block (2 levels) in generate_report' lib/brakeman/report/report_tabs.rb:10:in `map' lib/brakeman/report/report_tabs.rb:10:in `block in generate_report' lib/brakeman/report/report_tabs.rb:8:in `map' lib/brakeman/report/report_tabs.rb:8:in `generate_report' lib/brakeman/report.rb:107:in `generate' lib/brakeman/report.rb:58:in `format' lib/brakeman/report.rb:63:in `method_missing' test/tests/tabs_output.rb:9:in `setup'

class Brakeman::Report::Tabs < Brakeman::Report::Table def generate_report [[:generic_warnings, "General"], [:controller_warnings, "Controller"], [:model_warnings, "Model"], [:template_warnings, "Template"]].map do |meth, category| self.send(meth).map do |w| line = w.line || 0 w.warning_type.gsub!(/[^\w\s]/, ' ') "#{(w.file.absolute)}\t#{line}\t#{w.warning_type}\t#{category}\t#{w.format_message}\t#{w.confidence_name}" end.join "\n" end.join "\n" end end

It very explicitly mutate that string in place, hence why I chose to make it mutable. But to be honest it looks like a bug, but I don't know brakeman enough to decide.

If I delete that gsub! the test suite still pass.

Let's delete the gsub!. Not sure what I thought it was doing... but it doesn't. Also I plan to deprecate+remove that report format.

casperisfine · 2024-07-08T07:35:20Z

About the dependencies

ruby2ruby: seattlerb/ruby2ruby#58

Looks like @zenspider merged a fix: seattlerb/ruby2ruby@3d5966e, not sure when we'll get a release thought.

erubis: (this one is abandoned so can hardly be fixed).

I could monkey patch the one erubis method that isn't compatible, not sure if that'd be OK with you.

zenspider · 2024-07-08T07:36:34Z

I can have a release of r2r out this week.

casperisfine · 2024-07-08T07:45:37Z

I could monkey patch the one erubis method that isn't compatible, not sure if that'd be OK with you.

Ok, so I did it without monkey patch, I just include a module in all of Brakeman's erubis based parsers to change the one method.

zenspider · 2024-07-08T21:57:55Z

r2r is out

dryrunsecurity · 2024-07-09T07:28:07Z

DryRun Security Summary

The provided code changes cover a variety of updates and improvements to the Brakeman security scanner for Ruby on Rails applications, including dependency version updates, CircleCI configuration improvements, refactoring and optimizations, and integration of specific features, all focused on improving the functionality, performance, and security of the Brakeman tool.

Expand for full summary

Summary:

The provided code changes cover a variety of updates and improvements to the Brakeman security scanner for Ruby on Rails applications. The changes include:

Dependency version updates to address potential security vulnerabilities.
Improvements to the CircleCI configuration to enable better testing and code quality practices.
Refactoring and optimizations to various Brakeman components, such as the command-line options parser, the file path matching logic, and the report generation functionality.
Integration and handling of specific features, like the Rails 2 XSS plugin and frozen string literals, to ensure comprehensive security analysis.

Overall, the changes appear to be focused on improving the functionality, performance, and security of the Brakeman tool. While none of the individual changes introduce obvious security vulnerabilities, the reviewer should carefully consider the potential impact of each change, especially those related to user input handling, file path management, and report generation, to ensure the ongoing security and reliability of the Brakeman scanner.

Files Changed:

gem_common.rb: The changes update the version constraints for the ruby2ruby and slim dependencies, which is a positive security practice to address potential vulnerabilities in the dependencies.
.circleci/config.yml: The changes in the CircleCI configuration file enable the use of the frozen string literal feature and the debug version of this feature, which can help identify potential issues related to string literals in the codebase.
lib/brakeman/options.rb: The changes focus on improving the consistency and readability of the Brakeman command-line options, without any direct impact on the security analysis performed by the tool.
lib/brakeman/app_tree.rb: The changes are a minor refactoring to improve the readability and maintainability of the regular expression used for matching file paths, without introducing any obvious security concerns.
lib/brakeman/parsers/rails2_erubis.rb: The changes are focused on improving the functionality and performance of the Brakeman security scanner, without raising any immediate security concerns.
lib/brakeman/parsers/erubis_patch.rb: The patch ensures the compatibility of the erubis library with frozen string literals in Ruby, which is an important change to maintain the reliability of the Brakeman tool.
lib/brakeman/parsers/rails2_xss_plugin_erubis.rb: The changes demonstrate Brakeman's effort to integrate with the Rails 2 XSS plugin and handle the specific behavior and functionality of Erubis templates, which is crucial for accurate security analysis.
lib/brakeman/parsers/rails3_erubis.rb: The changes are focused on improving the handling of Erubis templates, which is an important component of the Brakeman security scanner.
lib/brakeman/processors/alias_processor.rb: The changes are a performance optimization and do not introduce any obvious security vulnerabilities.
lib/brakeman/report/report_markdown.rb: The changes focus on enhancing the usability and readability of the Brakeman report, which can help developers better understand and address the identified security issues.
lib/brakeman/report/report_table.rb: The changes are primarily focused on improving the efficiency and performance of the report generation process, without introducing any obvious security vulnerabilities.
lib/brakeman/warning.rb: The changes are a minor improvement to the Brakeman::Warning class and do not raise any significant security concerns.
lib/brakeman/report/report_tabs.rb: The removal of the sanitization of the warning_type field may introduce potential security concerns, such as injection attacks or reduced readability of the report. The reviewer should carefully evaluate the impact of this change.
lib/brakeman/report/report_text.rb: The changes are a performance optimization and do not directly impact the security of the Brakeman tool or the applications it analyzes.

Code Analysis

We ran 7 analyzers against 14 files and 1 analyzer had findings. 6 analyzers had no findings.

Analyzer	Findings
Authn/Authz Analyzer	1 finding

Riskiness

🟢 Risk threshold not exceeded.

View PR in the DryRun Dashboard.

casperisfine · 2024-07-09T07:28:16Z

thanks @zenspider I updated the dependency, now the entire test suite passes with --enable-frozen-string-literal.

zenspider

Having fixed a lot of these now, I fully approve of this PR. One style note that I eschew String#<< which seems in high use here.

zenspider · 2024-07-09T17:48:36Z

lib/brakeman/report/report_markdown.rb

@@ -27,7 +27,7 @@ def initialize *args
  end

  def generate_report
-    out = "# BRAKEMAN REPORT\n\n" <<
+    out = +"# BRAKEMAN REPORT\n\n" <<


Personally I really don't like String#<<... I wound up changing a lot of my old code from this to Array#<< + #join. It led to similarly sized diffs, array mutation (which I don't mind) instead of string mutation (which I do), and fits my current style a lot more.

Not recommending this change here... just something to consider.

Indeed... I'm sure there are a lot of improvements that could be made 😄

lib/brakeman/warning.rb

Since Ruby 2.3, Ruby can be started with this options, and it's expected to become the default in Ruby 4.0 (no release date yet). This commit fixes the callsites that assume string literal are mutable. Note however that a large part of the test suite is still failing, because of two dependencies: - ruby2ruby: seattlerb/ruby2ruby#58 - erubis: (this one is abandoned so can hardly be fixed).

presidentbeef · 2024-07-12T03:41:40Z

Took some time to get all the testing done, but other than one ruby2ruby output that changed (will follow up on that), looks good! Thanks for your contribution!

presidentbeef reviewed Jul 5, 2024

View reviewed changes

casperisfine force-pushed the frozen-string-literals-compat branch from 62904ec to a34e5be Compare July 8, 2024 07:33

casperisfine force-pushed the frozen-string-literals-compat branch from 8675374 to c6344ea Compare July 9, 2024 07:27

casperisfine requested a review from presidentbeef July 9, 2024 07:28

zenspider reviewed Jul 9, 2024

View reviewed changes

casperisfine force-pushed the frozen-string-literals-compat branch from c6344ea to ccd4db3 Compare July 10, 2024 14:43

presidentbeef reviewed Jul 10, 2024

View reviewed changes

lib/brakeman/warning.rb Outdated Show resolved Hide resolved

casperisfine force-pushed the frozen-string-literals-compat branch from ccd4db3 to 491e62c Compare July 10, 2024 17:10

presidentbeef merged commit 987627f into presidentbeef:main Jul 12, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compatibility with `--enable-frozen-string-literal` #1855

Fix compatibility with `--enable-frozen-string-literal` #1855

casperisfine commented Jun 27, 2024

dryrunsecurity bot commented Jun 27, 2024 •

edited

Loading

presidentbeef Jul 5, 2024

casperisfine Jul 8, 2024

presidentbeef Jul 10, 2024

casperisfine Jul 10, 2024

casperisfine commented Jul 8, 2024

zenspider commented Jul 8, 2024

casperisfine commented Jul 8, 2024

zenspider commented Jul 8, 2024

dryrunsecurity bot commented Jul 9, 2024 •

edited

Loading

casperisfine commented Jul 9, 2024

zenspider left a comment

zenspider Jul 9, 2024

presidentbeef Jul 9, 2024

presidentbeef commented Jul 12, 2024

Fix compatibility with --enable-frozen-string-literal #1855

Fix compatibility with --enable-frozen-string-literal #1855

Conversation

casperisfine commented Jun 27, 2024

dryrunsecurity bot commented Jun 27, 2024 • edited Loading

presidentbeef Jul 5, 2024

Choose a reason for hiding this comment

casperisfine Jul 8, 2024

Choose a reason for hiding this comment

presidentbeef Jul 10, 2024

Choose a reason for hiding this comment

casperisfine Jul 10, 2024

Choose a reason for hiding this comment

casperisfine commented Jul 8, 2024

zenspider commented Jul 8, 2024

casperisfine commented Jul 8, 2024

zenspider commented Jul 8, 2024

dryrunsecurity bot commented Jul 9, 2024 • edited Loading

DryRun Security Summary

Code Analysis

Riskiness

casperisfine commented Jul 9, 2024

zenspider left a comment

Choose a reason for hiding this comment

zenspider Jul 9, 2024

Choose a reason for hiding this comment

presidentbeef Jul 9, 2024

Choose a reason for hiding this comment

presidentbeef commented Jul 12, 2024

Fix compatibility with `--enable-frozen-string-literal` #1855

Fix compatibility with `--enable-frozen-string-literal` #1855

dryrunsecurity bot commented Jun 27, 2024 •

edited

Loading

dryrunsecurity bot commented Jul 9, 2024 •

edited

Loading