department-of-veterans-affairs / veterans-employment-center

Veterans Employment Center
https://www.vets.gov/veterans-employment-center
5 stars 4 forks source link

Enable Brakeman and Bundler-Audit during CI builds #336

Closed jeffmaher closed 8 years ago

jeffmaher commented 8 years ago

@cfeeney-va : Could you finish off the work on this branch to enable Brakeman (scans code for security woes) and Bundler-Audit (checks dependencies that have known security issues and can be upgraded)?

jeffmaher commented 8 years ago

(Will require a bundle install since bundler-audit Gem was added in this PR)

To run Brakeman: bundle exec brakeman

To run Bundler-Audit: bundle exec bundle-audit

Both produce errors right now that need to be resolved before this lands.

jeffmaher commented 8 years ago

Brakeman outputs the following:

| Medium     | MigrationHelper | mass_skills_translator_insert | SQL Injection | Possible SQL injection near line 20: ActiveRecord::Base.connection.execute("\n      INSERT IN>>

Not sure if this will be helpful, but it seems like the way the query is constructed could benefit from becoming more of a "prepared statement" (sorry - this is Java terminology, not sure the equivalent lingo in Rails): http://stackoverflow.com/a/13806512/249016

jeffmaher commented 8 years ago

Bundler-Audit outputs:

Name: actionpack
Version: 4.2.5.1
Advisory: CVE-2016-2098
Criticality: Unknown
URL: https://groups.google.com/forum/#!topic/rubyonrails-security/ly-IH-fxr_Q
Title: Possible remote code execution vulnerability in Action Pack
Solution: upgrade to ~> 3.2.22.2, >= 4.2.5.2, ~> 4.2.5, >= 4.1.14.2, ~> 4.1.14

Name: nokogiri
Version: 1.6.7.2
Advisory: CVE-2015-8806
Criticality: Unknown
URL: https://github.com/sparklemotion/nokogiri/issues/1473
Title: Denial of service or RCE from libxml2 and libxslt
Solution: upgrade to >= 1.6.8

Both of these can be fixed in the Gemfile:

However, you might want to give it a sanity check to make sure the common paths still work fine.

jeffmaher commented 8 years ago

@cfeeney-va Thanks for the fixes! Just have one question, but everything else LGTM.

ayaleloehr commented 8 years ago

Merging as I believe I answered @plusjeff 's question and it has a LGTM.