Rubocop security fix #253

xeviknal · 2017-12-04T10:00:10Z

This PR is fixing a small vulnerability for Rubocop < 0.48.1.
Here the vulnerability details:
https://www.cvedetails.com/cve/CVE-2017-8418/
rubocop/rubocop#4336

It is related to unsecure metadata stored in a /tmp folder. Allowing any user of the machine to tamper that caching data.

In order to fix it, I just upgrade rubocop version to the latest available one.
On the other hand, I have updated the minimum supported ruby version for rubocop to analyze. This stopped to show errors on 2.2+ new syntax.

xeviknal · 2017-12-04T10:01:12Z

cc @abonas

xeviknal · 2017-12-04T10:10:46Z

okay, I see there is some work in progress..

xeviknal · 2017-12-04T10:47:33Z

Upgrading the gem, adds new rules to check.
Using 0.48.1 (which is the version where the vulnerability was fixed) adds 171 offenses.
Using 0.51.0 (latest) adds 209 offences.

What do you think it would be better: fix the offences? adjust the rubocop rules to silent them?

abonas · 2017-12-04T10:50:22Z

.rubocop.yml

@@ -2,6 +2,7 @@
 # https://github.com/bbatsov/rubocop/blob/master/config/

 AllCops:
+  TargetRubyVersion: 2.2.0


why is this parameter required?
does it mean that it should "=" 2.2? or >= 2.2 ? the gemspec defines it as >= 2.2, and miq is already at a higher version

This tag is used to describe the minimum supported version. So it is a '>= 2.2'.

When running rubocop on my local, it uses ruby 2.1 parser instead that the current ruby version (i.e. 2.4.1). Adding this line, it ensures that Rubocop doesn't use a parser for a version below 2.2.0.

abonas · 2017-12-04T10:57:29Z

Upgrading the gem, adds new rules to check.
Using 0.48.1 (which is the version where the vulnerability was fixed) adds 171 offenses.
Using 0.51.0 (latest) adds 209 offences.

What do you think it would be better: fix the offences? adjust the rubocop rules to silent them?

I think it varies. some most likely should be fixed, and some should be silenced.
For example "Block has too many lines" takes a limit of 25. if that limit is increased to lets say 35, that will solve a lot of warnings of that type. (but not all)
The "Dependencies should be sorted in an alphabetical order within their section of the gemspec" sounds worth to take care of for their readability.
and etc. so I would analyze each "type" of offense to evaluate whether it should be silenced or resolved.

xeviknal · 2017-12-04T11:36:19Z

Cool! Then, it would be great than contributors here would say which rules keep and which one disable.
Maybe someone with more expertise on rubocop than me would know how to approach this easily 👯‍♂️

josejulio · 2017-12-04T15:02:39Z

Block has too many lines its a problem in the specs, maybe we should ignore these on the specs? Is possible to silence some warnings on the specs?
Trivial ones should probably be fixed, such as aligning code, freeze immutable, unnecessary spacing, literals should be delimited, etc.

cfcosta · 2017-12-04T15:27:47Z

LGTM. Specs are failing though, can you check why?

josejulio · 2017-12-04T15:29:04Z

Upgrading the gem, adds new rules to check.
Using 0.48.1 (which is the version where the vulnerability was fixed) adds 171 offenses.
Using 0.51.0 (latest) adds 209 offences.

What do you think it would be better: fix the offences? adjust the rubocop rules to silent them?

@cfcosta ^

xeviknal · 2017-12-04T16:14:56Z

I have tried to apply same cops as ManageIQ, but it raises 1.1k offenses.
I will fix the trivial ones 👍

israel-hdez · 2017-12-04T23:28:49Z

What do you think it would be better: fix the offences? adjust the rubocop rules to silent them?

@xeviknal I agree with @abonas that each warning should be evaluated.
IMO, these should be silented or rubocop config should be changed to be more tolerable:

Block has too many lines. As @josejulio said, should be silented for specs and I would also add an exception for the gemspec file. For the main code, I think it's OK to have.
Avoid rescuing without specifying an error class. It's only a warning. I would silent it, since I think is more error prone. Somebody may rescue Exception, which is bad.
Always use raise to signal exceptions. This one comes from the ruby style guide. I would silent it since I don't see it too important.

I think everything else should be fixed... But it's a lot of work :S

xeviknal · 2017-12-05T16:39:50Z

@josejulio @israel-hdez @cfcosta @abonas I totally agree with what you said before. I also introduced the following disabling:

Lint/BooleanSymbol[doc] it warns when you use a boolean but it is written as a symbol. I removed it because there was a test using specifically this scenario.

Apart from that, looking at Travis, there are some tricky scenarios I would appreciate some of your advice:

C: Style/MethodMissing: When using method_missing, fall back on super. - IMO, I would disable that cop. I think is not that common to have definitions of method_missing (although it is handy).
W: Lint/DuplicateMethods: Method Hawkular::Inventory::Resource#children is defined at both [...] - I don't know how is the usage of those methods nor the instance variable. So I don't know what can I change.
C: Naming/VariableName: Use snake_case for variable names. - I understand that those variables come from the API consumer, so chaging those variables names is actually a change of the API signature. I would suggest to disable the cop for this file.

Any suggestion?

israel-hdez · 2017-12-05T17:33:32Z

@xeviknal

For W: Lint/DuplicateMethods:, remove the attr_reader declarations. They are not working.
For C: Style/MethodMissing: When using method_missing, fall back on super I think this time rubocop is right. Instead of the fail instruction, it should be super. Of course it will fail anyway, but will fail in a more standard way (But, p robably, you may need to fix some specs).
For Lint/BooleanSymbol I'd keep it enabled and disable the cop by comments.
For C: Naming/VariableName: Use snake_case for variable names.... Yes, if you fix them, it'll be a change in the signature. So... I think you'll need to disable the cops by comments around those methods.

See in rubocop manual: disabling cops within the source

israel-hdez · 2017-12-05T17:38:26Z

lib/hawkular/alerts/alerts_api.rb

-      else
-        plugins = [action_plugin]
-      end
+      plugins = if action_plugin.nil?


Ternary operator will look better:

plugins = action_plugin.nil? ? http_get('plugins') : [action_plugin]

israel-hdez · 2017-12-05T17:43:32Z

lib/hawkular/hawkular_client.rb

@@ -18,6 +16,10 @@ def initialize(hash)
      @state = hash
    end

+    def respond_to_missing?
+      true


I think this is wrong. It should only return true if method_missing will process the call (won't fail).

israel-hdez · 2017-12-05T17:48:30Z

lib/hawkular/operations/operations_api.rb

-      else
-        ret = subs[0]
-      end
+      ret = if subs.length > 1


Here, I also think ternary operator looks better:

ret = subs.length > 1 ? subs.collect(&:capitalize).join : subs[0]

xeviknal · 2017-12-11T11:26:24Z

@israel-hdez for C: Style/MethodMissing, falling back to super will change the behavior of method missing. If a consumer expected to rescue a Hawkular::ArgumentError, it would fail since now the error thrown is a NoMethodError.
Correct me if I am wrong, I understand this is a minor change and we could assume this signature difference.

Conflicts: spec/integration/prometheus_spec.rb

coveralls · 2017-12-11T11:36:12Z

Coverage decreased (-0.1%) to 96.791% when pulling 85b9cdb on xeviknal:rubocop-security-fix into f149df1 on hawkular:master.

israel-hdez · 2017-12-11T15:29:56Z

@xeviknal yes, it'll be a small difference.
Hmm... With all the inventory changes, next version will be a major version. So... I'd say... don't worry too much about signature changes... go ahead and change it.

xeviknal · 2017-12-11T17:38:17Z

@israel-hdez changes are introduced. But specs are not passing. I see they are not passing as well in master. Do you think I could help you with that?

israel-hdez · 2017-12-11T18:09:35Z

@xeviknal Those errors are because of the new inventory work. Still pending to fix.
But your changes look good. If they are ready, I can merge.

xeviknal · 2017-12-11T18:14:32Z

Cool! For me, they are ready :)
Thanks

israel-hdez · 2017-12-11T19:19:10Z

lib/hawkular/hawkular_client.rb

@@ -18,6 +16,10 @@ def initialize(hash)
      @state = hash
    end

+    def respond_to_missing?(method_name, include_private = false)
+      method_name.to_s.start_with?('inventory_', 'alerts_', 'operations_', 'tokens_', 'prometheus_') || super


I rechecked this one, and I think that you still need to delegate to respond_to? to the relevant sub_client. Since the prefix won't guarantee that the method exists in the subclient.

@xeviknal ^

@israel-hdez I got what you said. Then answer to respond_to? becomes more precise when checking to the corresponding subclient.

In the proposed solution, i used respond_to? inside method_missing. I am wondering if this is a bad approach. Does anybody know?

For me, if it is okay, it is ready to merge.

coveralls · 2017-12-14T12:16:22Z

Coverage decreased (-0.1%) to 96.805% when pulling 62fee56 on xeviknal:rubocop-security-fix into f149df1 on hawkular:master.

israel-hdez

Now looks 👍

Upgradinig to latest rubocop version

839889e

abonas reviewed Dec 4, 2017

View reviewed changes

abonas requested review from israel-hdez and josejulio December 4, 2017 10:55

xeviknal force-pushed the rubocop-security-fix branch 4 times, most recently from 8c8347b to 7ebec18 Compare December 5, 2017 16:08

israel-hdez reviewed Dec 5, 2017

View reviewed changes

israel-hdez added this to the 5.0.0.pre2 milestone Dec 5, 2017

Disabling new cops added on the upgrade

4a677cc

xeviknal force-pushed the rubocop-security-fix branch from 7ebec18 to 0b0fc98 Compare December 11, 2017 11:01

Remove offenses

6ac022c

xeviknal force-pushed the rubocop-security-fix branch from 0b0fc98 to 6ac022c Compare December 11, 2017 11:09

Merge branch 'master' into rubocop-security-fix

85b9cdb

Conflicts: spec/integration/prometheus_spec.rb

israel-hdez reviewed Dec 11, 2017

View reviewed changes

Better approach for respond_to? implementation

62fee56

israel-hdez approved these changes Dec 14, 2017

View reviewed changes

israel-hdez merged commit 3b07729 into hawkular:master Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rubocop security fix #253

Rubocop security fix #253

xeviknal commented Dec 4, 2017

xeviknal commented Dec 4, 2017

xeviknal commented Dec 4, 2017

xeviknal commented Dec 4, 2017

abonas Dec 4, 2017

xeviknal Dec 4, 2017

abonas commented Dec 4, 2017 •

edited

Loading

xeviknal commented Dec 4, 2017

josejulio commented Dec 4, 2017

cfcosta commented Dec 4, 2017

josejulio commented Dec 4, 2017

xeviknal commented Dec 4, 2017

israel-hdez commented Dec 4, 2017

xeviknal commented Dec 5, 2017 •

edited

Loading

israel-hdez commented Dec 5, 2017

israel-hdez Dec 5, 2017

israel-hdez Dec 5, 2017

israel-hdez Dec 5, 2017

xeviknal commented Dec 11, 2017

coveralls commented Dec 11, 2017 •

edited

Loading

israel-hdez commented Dec 11, 2017

xeviknal commented Dec 11, 2017

israel-hdez commented Dec 11, 2017

xeviknal commented Dec 11, 2017

israel-hdez Dec 11, 2017

israel-hdez Dec 13, 2017

xeviknal Dec 14, 2017 •

edited

Loading

coveralls commented Dec 14, 2017 •

edited

Loading

israel-hdez left a comment

Rubocop security fix #253

Rubocop security fix #253

Conversation

xeviknal commented Dec 4, 2017

xeviknal commented Dec 4, 2017

xeviknal commented Dec 4, 2017

xeviknal commented Dec 4, 2017

abonas Dec 4, 2017

Choose a reason for hiding this comment

xeviknal Dec 4, 2017

Choose a reason for hiding this comment

abonas commented Dec 4, 2017 • edited Loading

xeviknal commented Dec 4, 2017

josejulio commented Dec 4, 2017

cfcosta commented Dec 4, 2017

josejulio commented Dec 4, 2017

xeviknal commented Dec 4, 2017

israel-hdez commented Dec 4, 2017

xeviknal commented Dec 5, 2017 • edited Loading

israel-hdez commented Dec 5, 2017

israel-hdez Dec 5, 2017

Choose a reason for hiding this comment

israel-hdez Dec 5, 2017

Choose a reason for hiding this comment

israel-hdez Dec 5, 2017

Choose a reason for hiding this comment

xeviknal commented Dec 11, 2017

coveralls commented Dec 11, 2017 • edited Loading

israel-hdez commented Dec 11, 2017

xeviknal commented Dec 11, 2017

israel-hdez commented Dec 11, 2017

xeviknal commented Dec 11, 2017

israel-hdez Dec 11, 2017

Choose a reason for hiding this comment

israel-hdez Dec 13, 2017

Choose a reason for hiding this comment

xeviknal Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Dec 14, 2017 • edited Loading

israel-hdez left a comment

Choose a reason for hiding this comment

abonas commented Dec 4, 2017 •

edited

Loading

xeviknal commented Dec 5, 2017 •

edited

Loading

coveralls commented Dec 11, 2017 •

edited

Loading

xeviknal Dec 14, 2017 •

edited

Loading

coveralls commented Dec 14, 2017 •

edited

Loading