Skip to content

Conversation

dblandin
Copy link
Contributor

This PR updates the engine analysis to use bundler-audit's internal Scanner class which offers better access to gem and advisory information.

This particular change is also necessary to disable the gem's network access attempt in order to determine whether a Gemfile source is internal or external, an update which will be introduced following this PR.

This PR also takes a few refactoring steps to improve overall structure and clarify remediation point calculation.

There should be no behavioral changes as a result of the updates in this PR.

@codeclimate/review 🔎

Before

{
    "categories": [
        "Security"
    ],
    "check_name": "Insecure Dependency",
    "content": {
        "body": "**Advisory**: CVE-2013-1800\n\n**Criticality**: High\n\n**URL**: http://osvdb.org/show/osvdb/90742\n\n**Solution**: upgrade to >= 0.3.2"
    },
    "description": "crack Gem for Ruby Type Casting Parameter Parsing Remote Code Execution",
    "location": {
        "lines": {
            "begin": 87,
            "end": 87
        },
        "path": "Gemfile.lock"
    },
    "remediation_points": 500000,
    "severity": "critical",
    "type": "Issue"
}

After

{
    "categories": [
        "Security"
    ],
    "check_name": "Insecure Dependency",
    "content": {
        "body": "**Advisory**: CVE-2013-1800\n\n**Criticality**: High\n\n**URL**: http://osvdb.org/show/osvdb/90742\n\n**Solution**: upgrade to >= 0.3.2"
    },
    "description": "crack Gem for Ruby Type Casting Parameter Parsing Remote Code Execution",
    "location": {
        "lines": {
            "begin": 87,
            "end": 87
        },
        "path": "Gemfile.lock"
    },
    "remediation_points": 500000,
    "severity": "critical",
    "type": "Issue"
}

Dir.chdir(directory) do
Bundler::Audit::Scanner.new.scan do |vulnerability|
result = Result.new(vulnerability, File.open(gemfile_lock_path))
issue = result.to_issue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of changing to_issue below to as_json and then you could just do result.to_json below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd feel more comfortable with that if we renamed the class to Issue. Then we would have:

issue = Issue.new(vulnerability, File.open(gemfile_lock_path))
io.print("#{issue.to_json}\0")

WDYT?

@gdiggs
Copy link
Contributor

gdiggs commented Feb 11, 2016

I like this direction. Did we lose the insecure source checks entirely?

@dblandin
Copy link
Contributor Author

I like this direction. Did we lose the insecure source checks entirely?

I don't think those checks ever worked as the output looks completely different and isn't picked up by our currently handling of the output on master.

Here's an example of that output. We currently just ignore it. I'd like to bring in proper support for insecure source checks in a separate PR.

Insecure Source URI found: http://rubygems.org/

@gdiggs
Copy link
Contributor

gdiggs commented Feb 11, 2016

I'd like to bring in proper support for insecure source checks in a separate PR.

Sounds good to me!

@dblandin dblandin force-pushed the devon/use-bundler-audit-scanner-for-analysis branch from a86125f to e85e0ac Compare February 11, 2016 23:31
@dblandin
Copy link
Contributor Author

@codeclimate/review This is ready for another 🔎

ratings:
paths:
- "**.rb"
exclude_paths:
- bin/bundler_audit.rb
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we excluding this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubocop generated an error on the shebang line. I think we usually drop the extension on executables in other engines, which is why this doesn't often need to be excluded. Might be worth doing that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind. That doesn't seem to be the issue. I get the false positive locally but not on dotcom. Strange.

❯ codeclimate analyze -e rubocop                                         codeclimate-bundler-audit/git/devon/use-bundler-audit-scanner-for-analysis
Starting analysis
Running rubocop: Done!

== bin/bundler-audit (1 issue) ==
1: Use snake_case for source file names. [rubocop]

Analysis complete! Found 1 issue.

@dblandin dblandin force-pushed the devon/use-bundler-audit-scanner-for-analysis branch 3 times, most recently from 5cf3ad7 to 1f3c4b9 Compare February 12, 2016 16:32
@dblandin dblandin force-pushed the devon/use-bundler-audit-scanner-for-analysis branch from 1f3c4b9 to 8efb4de Compare February 12, 2016 16:33
@dblandin
Copy link
Contributor Author

@gordondiggs Updated to remove that exclude path.

@gdiggs
Copy link
Contributor

gdiggs commented Feb 12, 2016

LGTM!

dblandin added a commit that referenced this pull request Feb 12, 2016
…er-for-analysis

Use Bundler::Audit::Scanner for analysis
@dblandin dblandin merged commit 7d1b766 into master Feb 12, 2016
@dblandin dblandin deleted the devon/use-bundler-audit-scanner-for-analysis branch February 12, 2016 16:37
dblandin added a commit that referenced this pull request Feb 12, 2016
A small update to bring our rubocop config up-to-date with our styleguide.
Following the refactor in #16, few changes in the codebase were needed.
dblandin added a commit that referenced this pull request Feb 12, 2016
A small update to bring our rubocop config up-to-date with our styleguide.
Following the refactor in #16, few changes in the codebase were needed.
dblandin added a commit that referenced this pull request Feb 12, 2016
A small update to bring our rubocop config up-to-date with our styleguide.
Following the refactor in #16, few changes in the codebase were needed.
dblandin added a commit that referenced this pull request Feb 12, 2016
A small update to bring our rubocop config up-to-date with our styleguide.
Following the refactor in #16, few changes in the codebase were needed.
dblandin added a commit that referenced this pull request Feb 12, 2016
A small update to bring our rubocop config up-to-date with our styleguide.
Following the refactor in #16, few changes in the codebase were needed.
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.

3 participants