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

Code Climate engine: fix `include_paths` when using custom app path #1126

Merged
merged 3 commits into from Dec 6, 2017

Conversation

Projects
None yet
2 participants
@wfleming
Contributor

wfleming commented Nov 30, 2017

Brakeman (very reasonably) expects the only_files options to be
relative to the app_path option. The support that was added to the
Code Climate engine for a custom app_path option did not take
include_paths into account, so the normal result of using a custom
app_path would have ended up being that nothing would get analyzed:
because with a custom app_path of, say, foo, an include path might
include something like foo/bar when it needs to be adjusted to just
bar.

The fix here is to strip the app_path prefix from all matching
include_paths, and drop non-matching include_paths entirely.

Code Climate engine: fix `include_paths` when using custom app path
Brakeman (very reasonably) expects the `only_files` options to be
relative to the `app_path` option. The support that was added to the
Code Climate engine for a custom `app_path` option did not take
`include_paths` into account, so the normal result of using a custom
`app_path` would have ended up being that *nothing* would get analyzed:
because with a custom `app_path` of, say, `foo`, an include path might
include something like `foo/bar` when it needs to be adjusted to just
`bar`.

The fix here is to strip the `app_path` prefix from all matching
`include_paths`, and drop non-matching `include_paths` entirely.
@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Dec 3, 2017

Owner

Looking at the tests...does this mean include_paths must include the app_path if both are specified?

Owner

presidentbeef commented Dec 3, 2017

Looking at the tests...does this mean include_paths must include the app_path if both are specified?

@wfleming

This comment has been minimized.

Show comment
Hide comment
@wfleming

wfleming Dec 4, 2017

Contributor

User's don't specify include_paths explicitly: they specify exclude_patterns, and the Code Climate CLI effectively inverts those into the include_paths that an engine can report on.

So for the include_paths to not include the app_path, a user would have probably supplied a config like:

plugins:
  brakeman:
    enabled: true
    config:
      app_path: "foo"
exclude_patterns:
  - "foo/"

That's not really a sensible config, and in that case ending up with an empty @configured_options[:only_files] seems like the correct behavior to me.

You've made me realize there is, however, an edge case with my current implementation that would incorrectly set an empty only_files: if the app_path was something deeply nested like foo/bar/baz, and the include paths included foo/ but hadn't expanded below that, this would incorrectly result in an empty file list right now, when it should actually be something like ["."]. I'll try to fix that edge case today.

Contributor

wfleming commented Dec 4, 2017

User's don't specify include_paths explicitly: they specify exclude_patterns, and the Code Climate CLI effectively inverts those into the include_paths that an engine can report on.

So for the include_paths to not include the app_path, a user would have probably supplied a config like:

plugins:
  brakeman:
    enabled: true
    config:
      app_path: "foo"
exclude_patterns:
  - "foo/"

That's not really a sensible config, and in that case ending up with an empty @configured_options[:only_files] seems like the correct behavior to me.

You've made me realize there is, however, an edge case with my current implementation that would incorrectly set an empty only_files: if the app_path was something deeply nested like foo/bar/baz, and the include paths included foo/ but hadn't expanded below that, this would incorrectly result in an empty file list right now, when it should actually be something like ["."]. I'll try to fix that edge case today.

@wfleming

This comment has been minimized.

Show comment
Hide comment
@wfleming

wfleming Dec 4, 2017

Contributor

Thanks for noticing that edge case @presidentbeef, just pushed up a slightly tweaked approach.

Contributor

wfleming commented Dec 4, 2017

Thanks for noticing that edge case @presidentbeef, just pushed up a slightly tweaked approach.

Code Climate engine: handle deeply nested app paths
If an `app_path` is *below* the expanded `include_paths`, the simple
approach of "delete the prefix from the include paths" would incorrectly
result in no include paths at all.

The fix here is, when stripping the include paths, to consider a path
that is one of the `app_paths` direct ancestors to map to "." for the
include paths.
@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Dec 4, 2017

Owner

Oh.

Then shouldn't this use skip_files instead? I don't like only_files in general, as Brakeman needs to see the whole application to be effective.

Owner

presidentbeef commented Dec 4, 2017

Oh.

Then shouldn't this use skip_files instead? I don't like only_files in general, as Brakeman needs to see the whole application to be effective.

@wfleming

This comment has been minimized.

Show comment
Hide comment
@wfleming

wfleming Dec 5, 2017

Contributor
Contributor

wfleming commented Dec 5, 2017

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Dec 6, 2017

Owner

If a pull request from Code Climate fails the Code Climate check...? 😬

Owner

presidentbeef commented Dec 6, 2017

If a pull request from Code Climate fails the Code Climate check...? 😬

@wfleming

This comment has been minimized.

Show comment
Hide comment
@wfleming

wfleming Dec 6, 2017

Contributor

Heh, my bad, I didn't check locally before I pushed. Fixed.

Contributor

wfleming commented Dec 6, 2017

Heh, my bad, I didn't check locally before I pushed. Fixed.

@presidentbeef presidentbeef merged commit afc1382 into presidentbeef:master Dec 6, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/diff-coverage 100% (80% threshold)
Details
codeclimate/total-coverage 94% (0.0% change)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Dec 6, 2017

Owner

Thanks!

Owner

presidentbeef commented Dec 6, 2017

Thanks!

@wfleming wfleming deleted the codeclimate-community:will/app-path-bug branch Dec 7, 2017

@wfleming

This comment has been minimized.

Show comment
Hide comment
@wfleming

wfleming Dec 7, 2017

Contributor

Thank you!

Contributor

wfleming commented Dec 7, 2017

Thank you!

Repository owner locked and limited conversation to collaborators Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.