Merge 1cadfb8 into 6859d76

lib/inspec/profile.rb

@@ -325,10 +325,9 @@ def info(res = params.dup) # rubocop:disable Metrics/CyclomaticComplexity, Metri
       res
     end
 
-    # Check if the profile is internally well-structured. The logger will be
-    # used to print information on errors and warnings which are found.
+    # Sanity check the profile.  This is the core of `inspec check`
     #
-    # @return [Boolean] true if no errors were found, false otherwise
+    # @return [Hash] check results.  See below for structure.
     def check # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength
       # initial values for response object
       result = {
@@ -411,8 +410,8 @@ def check # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metri
         @logger.info("Found #{count} controls.")
       end
 
-      # iterate over hash of groups
-      params[:controls].each { |id, control|
+      # iterate over hash of controls
+      params[:controls].each do |id, control|
         sfile = control[:source_location][:ref]
         sline = control[:source_location][:line]
         error.call(sfile, sline, nil, id, 'Avoid controls with empty IDs') if id.nil? or id.empty?
@@ -422,7 +421,24 @@ def check # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metri
         warn.call(sfile, sline, nil, id, "Control #{id} has impact > 1.0") if control[:impact].to_f > 1.0
         warn.call(sfile, sline, nil, id, "Control #{id} has impact < 0.0") if control[:impact].to_f < 0.0
         warn.call(sfile, sline, nil, id, "Control #{id} has no tests defined") if control[:checks].nil? or control[:checks].empty?
-      }
+
+        # Check for duplicate IDs within the same profile
+        rule = control[:rule_obj]
+        next unless Inspec::Rule.merge_count(rule) > 0
+        profile_id = Inspec::Rule.profile_id(rule)
+        merges = Inspec::Rule.merge_changes(rule)
+        merges = merges.select do |merge_location|
+          # Look for rules that had a merge within the same profile...
+          merge_location[:profile_id] == profile_id &&
+            !merge_location[:in_include] # and the merge did not happen in an include/require_controls
+        end
+        merges.each do |location|
+          cfile = location[:ref]
+          cline = location[:line]
+          msg = "Control #{id} is duplicated in profile #{profile_id} - clobbered by #{cfile}:#{cline}"
+          error.call(sfile, sline, nil, id, msg)
+        end
+      end
 
       # profile is valid if we could not find any error
       result[:summary][:valid] = result[:errors].empty?
@@ -607,6 +623,7 @@ def load_rule_filepath(prefix, rule)
     def load_rule(rule, file, controls, groups)
       id = Inspec::Rule.rule_id(rule)
       location = rule.instance_variable_get(:@__source_location)
+      # TODO: why are we constructiong this thing, when the rule object already has this info?
       controls[id] = {
         title: rule.title,
         desc: rule.desc,
@@ -617,6 +634,7 @@ def load_rule(rule, file, controls, groups)
         checks: Inspec::Rule.checks(rule),
         code: Inspec::MethodSource.code_at(location, source_reader),
         source_location: location,
+        rule_obj: rule, # We need the actual object to interrogate merge history
       }
 
       # try and grab code text from merge locations

lib/inspec/rule.rb

@@ -39,7 +39,7 @@ def initialize(id, profile_id, opts, &block)
       # not changeable by the user:
       @__code = nil
       @__block = block
-      @__source_location = __get_block_source_location(&block)
+      @__source_location = __get_block_source_location(&block).merge(profile_id: profile_id, in_include: __check_for_include)
       @__rule_id = id
       @__profile_id = profile_id
       @__checks = []
@@ -178,6 +178,10 @@ def self.set_rule_id(rule, value)
       rule.instance_variable_set(:@__rule_id, value)
     end
 
+    def self.source_location(rule)
+      rule.instance_variable_get(:@__source_location)
+    end
+
     def self.profile_id(rule)
       rule.instance_variable_get(:@__profile_id)
     end
@@ -312,5 +316,18 @@ def __get_block_source_location(&block)
     rescue MethodSource::SourceNotFoundError
       {}
     end
+
+    # For duplicate control detection (github 822),
+    # we must only warn on a control being duplicated when
+    # it is *not* being overridden.  Overrides happen within
+    # include_control / require control blocks.
+    # HACK: detect that using the call stack.
+    def __check_for_include
+      # A require_contols / include_controls call will be at least 8 frames back;
+      # the 6 is just a guess for an upper limit.  We don't want to look too far
+      # back - each level of dependency adds about 12 frames.
+      frames = caller_locations(8, 6).map(&:base_label)
+      frames.include?('include_controls') || frames.include?('require_controls')
+    end
   end
 end

lib/inspec/runner.rb

@@ -75,7 +75,7 @@ def reset
       @rules = []
     end
 
-    def load
+    def load # rubocop: disable Metrics/AbcSize
       all_controls = []
 
       @target_profiles.each do |profile|
@@ -102,6 +102,25 @@ def load
 
       all_controls.each do |rule|
         register_rule(rule) unless rule.nil?
+        # Check for duplicate controls and warn
+        next if Inspec::Rule.merge_count(rule).nil?
+        next unless Inspec::Rule.merge_count(rule) > 0
+        profile_id = Inspec::Rule.profile_id(rule)
+        merges = Inspec::Rule.merge_changes(rule)
+        merges = merges.select do |merge_location|
+          # Look for rules that had a merge within the same profile...
+          merge_location[:profile_id] == profile_id &&
+            !merge_location[:in_include] # and the merge did not happen in an include/require_controls
+        end
+        merges.each do |location|
+          sfile = Inspec::Rule.source_location(rule)[:ref]
+          sline = Inspec::Rule.source_location(rule)[:line]
+          cfile = location[:ref]
+          cline = location[:line]
+          id = Inspec::Rule.rule_id(rule)
+          msg = "Control #{id} at #{sfile}:#{sline} is duplicated within the same profile '#{profile_id}' - clobbered by #{cfile}:#{cline}"
+          Inspec::Log.warn(msg)
+        end
       end
     end
 

test/functional/helper.rb

@@ -4,6 +4,7 @@
 
 require 'helper'
 require 'rbconfig'
+require 'byebug'
 
 require 'minitest/hell'
 class Minitest::Test

test/functional/inspec_check_test.rb

@@ -88,6 +88,50 @@
       out.exit_status.must_equal 1
       out.stdout.must_include 'inspec.yml and inspec.lock are out-of-sync. Please re-vendor with `inspec vendor`.'
       out.stdout.must_include 'Cannot find linux-baseline in lockfile. Please re-vendor with `inspec vendor`.'
-    end    
+    end
+  end
+
+  describe 'inspec check should catch a duplicate control' do
+    describe 'when a simple profile has duplicates' do
+      it 'can detect a duplicate control' do
+        dupe_profile = File.join(profile_path, 'dupe-controls', 'simple')
+        run_result = inspec('check ' + dupe_profile + ' --format json')
+        run_result.exit_status.must_equal 1
+        json_result = JSON.parse(run_result.stdout)
+
+        # Must detect exactly one error
+        json_result['errors'].count.must_equal 1
+
+        json_result['errors'][0]['control_id'].must_equal 'dupe-01' # Find the right control
+        json_result['errors'][0]['msg'].must_include 'is duplicated in profile dupe-controls' # The kernel of the error message
+        json_result['errors'][0]['msg'].must_include 'clobbered by' # And we tell you that is was overwritten
+        json_result['errors'][0]['msg'].must_include 'duplicates.rb:14' # And we tell you what overwrote it
+      end
+    end
+
+    describe 'when a profile uses include_controls without a block' do
+      it 'should not detect a duplicate control' do
+        dupe_profile = File.join(profile_path, 'dupe-controls', 'wrapper-simple-include')
+        run_result = inspec('check ' + dupe_profile + ' --format json')
+        run_result.exit_status.must_equal 0
+        run_result.stderr.must_be_empty
+        json_result = JSON.parse(run_result.stdout)
+
+        # Must detect no errors
+        json_result['errors'].count.must_equal 0
+      end
+    end
+
+    describe 'when a profile uses include_controls with a block to modify a control' do
+      it 'should not detect a duplicate control' do
+        dupe_profile = File.join(profile_path, 'dupe-controls', 'wrapper-block-include')
+        run_result = inspec('check ' + dupe_profile + ' --format json')
+        json_result = JSON.parse(run_result.stdout)
+        run_result.exit_status.must_equal 0
+
+        # Must detect no errors
+        json_result['errors'].count.must_equal 0
+      end
+    end
   end
 end

test/functional/inspec_exec_test.rb

@@ -524,4 +524,43 @@
       run_result.stdout.wont_include('1 deprecation warning total')
     end
   end
+
+  describe 'when using a profile that contains duplicate controls' do
+    describe 'when it is a simple profile' do
+      it 'merges the controls and emits a warning' do
+        dupe_profile = File.join(profile_path, 'dupe-controls', 'simple')
+        run_result = inspec('exec ' + dupe_profile)
+        run_result.exit_status.must_equal 0
+
+        warning = run_result.stdout.split("\n").grep(/WARN.+duplicate/).first
+        warning.wont_be_nil
+        warning.must_include 'dupe-01' # Which control
+        warning.must_include "is duplicated within the same profile 'dupe-controls'"
+        warning.must_include 'duplicates.rb:1' # the original location
+        warning.must_include 'duplicates.rb:12' # the duplicate location
+      end
+    end
+
+    describe 'when it is a wrapper profile with a simple include' do
+      it 'no duplicate is detected' do
+        dupe_profile = File.join(profile_path, 'dupe-controls', 'wrapper-simple-include')
+        run_result = inspec('exec ' + dupe_profile)
+        run_result.exit_status.must_equal 0
+
+        warning = run_result.stdout.split("\n").grep(/WARN.+duplicate/).first
+        warning.must_be_nil
+      end
+    end
+
+    describe 'when it is a wrapper profile with a block include modifying an inherited control' do
+      it 'no duplicate is detected' do
+        dupe_profile = File.join(profile_path, 'dupe-controls', 'wrapper-block-include')
+        run_result = inspec('exec ' + dupe_profile)
+        run_result.exit_status.must_equal 0
+
+        warning = run_result.stdout.split("\n").grep(/WARN.+duplicate/).first
+        warning.must_be_nil
+      end
+    end
+  end
 end

test/unit/mock/profiles/dupe-controls/child/controls/child.rb

@@ -0,0 +1,7 @@
+control 'control-01' do
+  title 'the title set in child'
+  description 'something'
+  describe 'describe-01' do
+    it { should include '01' }
+  end
+end

test/unit/mock/profiles/dupe-controls/child/inspec.yml

@@ -0,0 +1,10 @@
+name: child
+title: A simple profile to test inheritance with duplicate control ids
+maintainer: The Authors
+copyright: The Authors
+copyright_email: you@example.com
+license: Apache-2.0
+summary: An InSpec Compliance Profile
+version: 0.1.0
+supports:
+  platform: os

test/unit/mock/profiles/dupe-controls/simple/controls/duplicates.rb

@@ -0,0 +1,25 @@
+control 'dupe-01' do
+  impact 1
+  title 'dupe-01-title-one'
+  description 'something'
+
+  desc 'collision', 'one'
+  desc 'uniq-one', 'one'
+
+  describe true do
+    it { should eq false }
+  end
+end
+
+control 'dupe-01' do
+  impact 0
+  title 'dupe-01-title-two'
+  description 'something'
+
+  desc 'collision', 'two'
+  desc 'uniq-two', 'two'
+
+  describe true do
+    it { should eq true }
+  end
+end

test/unit/mock/profiles/dupe-controls/simple/inspec.yml

@@ -0,0 +1,10 @@
+name: dupe-controls
+title: A profile containing duplicated controls, refs github issue 822
+maintainer: The Authors
+copyright: The Authors
+copyright_email: you@example.com
+license: Apache-2.0
+summary: An InSpec Compliance Profile
+version: 0.1.0
+supports:
+  platform: os

test/unit/mock/profiles/dupe-controls/wrapper-block-include/controls/include.rb

@@ -0,0 +1,14 @@
+include_controls 'child' do
+  control 'control-01' do
+    title 'title set in wrapper-block-include, inside the block'
+    description 'something'
+  end
+end
+
+control 'control-01' do
+  title 'the title set in wrapper-block-include, outside the block'
+  description 'something'
+  describe 'describe-01' do
+    it { should include '01' }
+  end
+end

test/unit/mock/profiles/dupe-controls/wrapper-block-include/inspec.yml

@@ -0,0 +1,13 @@
+name: wrapper-simple-include
+title: A simple InSpec Profile to test inheritance with duplicate control IDs with an include block
+maintainer: The Authors
+copyright: The Authors
+copyright_email: you@example.com
+license: Apache-2.0
+summary: An InSpec Compliance Profile
+version: 0.1.0
+supports:
+  platform: os
+depends:
+  - name: child
+    path: ../child

test/unit/mock/profiles/dupe-controls/wrapper-simple-include/controls/include.rb

@@ -0,0 +1,9 @@
+include_controls 'child'
+
+control 'control-01' do
+  title 'the title set in wrapper-simple-include'
+  description 'something'
+  describe 'describe-01' do
+    it { should include '01' }
+  end
+end

test/unit/mock/profiles/dupe-controls/wrapper-simple-include/inspec.yml

@@ -0,0 +1,13 @@
+name: wrapper-simple-include
+title: A simple InSpec Profile to test inheritance with duplicate control IDs with a bare include
+maintainer: The Authors
+copyright: The Authors
+copyright_email: you@example.com
+license: Apache-2.0
+summary: An InSpec Compliance Profile
+version: 0.1.0
+supports:
+  platform: os
+depends:
+  - name: child
+    path: ../child

test/unit/profiles/profile_test.rb

@@ -49,26 +49,26 @@
   end
 
   describe 'code info' do
-    let(:profile_id) { 'complete-profile' }
+    let(:profile_path) { 'complete-profile' } # Id is 'complete'
     let(:code) { "control 'test01' do\n  impact 0.5\n  title 'Catchy title'\n  desc '\n    example.com should always exist.\n  '\n  describe host('example.com') do\n    it { should be_resolvable }\n  end\nend\n" }
-    let(:loc) { {:ref=>"controls/host_spec.rb", :line=>6} }
+    let(:loc) { {:ref=>"controls/host_spec.rb", :line=>6, :profile_id => 'complete', :in_include => false } }
 
     it 'gets code from an uncompressed profile' do
-      info = MockLoader.load_profile(profile_id).info
+      info = MockLoader.load_profile(profile_path).info
       info[:controls][0][:code].must_equal code
-      loc[:ref] = File.join(MockLoader.profile_path(profile_id), loc[:ref])
+      loc[:ref] = File.join(MockLoader.profile_path(profile_path), loc[:ref])
       info[:controls][0][:source_location].must_equal loc
     end
 
     it 'gets code on zip profiles' do
-      path = MockLoader.profile_zip(profile_id)
+      path = MockLoader.profile_zip(profile_path)
       info = MockLoader.load_profile(path).info
       info[:controls][0][:code].must_equal code
       info[:controls][0][:source_location].must_equal loc
     end
 
     it 'gets code on tgz profiles' do
-      path = MockLoader.profile_tgz(profile_id)
+      path = MockLoader.profile_tgz(profile_path)
       info = MockLoader.load_profile(path).info
       info[:controls][0][:code].must_equal code
       info[:controls][0][:source_location].must_equal loc