update immutable API blacklist and whitelist

adds the whitelist so that when we update to new ruby versions it forces
us to audit the new Array and Hash methods and figure out if they are
mutators or not.

this change closes the Hash#store loophole that was being abused by
some cookbook consumers to directly modify merged ImmutableHashes
(producing inherently undefined behavior that we 'broke' at one
point).

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

3 files changed, 231 insertions, 9 deletions

diff --git a/lib/chef/node/mixin/immutablize_array.rb b/lib/chef/node/mixin/immutablize_array.rb
index bd330cf8a9..72273d5333 100644
--- a/lib/chef/node/mixin/immutablize_array.rb
+++ b/lib/chef/node/mixin/immutablize_array.rb
@@ -1,5 +1,5 @@
 #--
-# Copyright:: Copyright 2016-2017, Chef Software Inc.
+# Copyright:: Copyright 2016-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -19,15 +19,118 @@ class Chef
   class Node
     module Mixin
       module ImmutablizeArray
+        # Allowed methods that MUST NOT mutate the object
+        # (if any of these methods mutate the underlying object that is a bug that needs to be fixed)
+        ALLOWED_METHODS = [
+          :&,
+          :*,
+          :+,
+          :-,
+          :[],
+          :all?,
+          :any?,
+          :assoc,
+          :at,
+          :bsearch,
+          :bsearch_index,
+          :chunk,
+          :chunk_while,
+          :collect,
+          :collect_concat,
+          :combination,
+          :compact,
+          :count,
+          :cycle,
+          :detect,
+          :dig,
+          :drop,
+          :drop_while,
+          :each,
+          :each_cons,
+          :each_entry,
+          :each_index,
+          :each_slice,
+          :each_with_index,
+          :each_with_object,
+          :empty?,
+          :entries,
+          :fetch,
+          :find,
+          :find_all,
+          :find_index,
+          :first,
+          :flat_map,
+          :flatten,
+          :grep,
+          :grep_v,
+          :group_by,
+          :include?,
+          :index,
+          :inject,
+          :join,
+          :last,
+          :lazy,
+          :length,
+          :map,
+          :max,
+          :max_by,
+          :member?,
+          :min,
+          :min_by,
+          :minmax,
+          :minmax_by,
+          :none?,
+          :one?,
+          :pack,
+          :partition,
+          :permutation,
+          :product,
+          :rassoc,
+          :reduce,
+          :reject,
+          :repeated_combination,
+          :repeated_permutation,
+          :reverse,
+          :reverse_each,
+          :rindex,
+          :rotate,
+          :sample,
+          :save_plist,
+          :select,
+          :shelljoin,
+          :shuffle,
+          :size,
+          :slice,
+          :slice_after,
+          :slice_before,
+          :slice_when,
+          :sort,
+          :sort_by,
+          :sum,
+          :take,
+          :take_while,
+          :to_a,
+          :to_ary,
+          :to_h,
+          :to_plist,
+          :to_set,
+          :transpose,
+          :uniq,
+          :values_at,
+          :zip,
+          :|,
+        ]
         # A list of methods that mutate Array. Each of these is overridden to
         # raise an error, making this instances of this class more or less
         # immutable.
         DISALLOWED_MUTATOR_METHODS = [
           :<<,
           :[]=,
+          :append,
           :clear,
           :collect!,
           :compact!,
+          :concat,
           :default=,
           :default_proc=,
           :delete,
@@ -40,18 +143,21 @@ class Chef
           :map!,
           :merge!,
           :pop,
+          :prepend,
           :push,
-          :update,
           :reject!,
-          :reverse!,
           :replace,
+          :reverse!,
+          :rotate!,
           :select!,
           :shift,
+          :shuffle!,
           :slice!,
           :sort!,
           :sort_by!,
           :uniq!,
           :unshift,
+          :update,
         ]
 
         # Redefine all of the methods that mutate a Hash to raise an error when called.
diff --git a/lib/chef/node/mixin/immutablize_hash.rb b/lib/chef/node/mixin/immutablize_hash.rb
index f6b22ed7d7..cd2bbf7bf6 100644
--- a/lib/chef/node/mixin/immutablize_hash.rb
+++ b/lib/chef/node/mixin/immutablize_hash.rb
@@ -1,5 +1,5 @@
 #--
-# Copyright:: Copyright 2016-2017, Chef Software Inc.
+# Copyright:: Copyright 2016-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -19,10 +19,112 @@ class Chef
   class Node
     module Mixin
       module ImmutablizeHash
+        # allowed methods that MUST NOT mutate the object
+        # (if any of these methods mutate the underlying object that is a bug that needs to be fixed)
+        ALLOWED_METHODS = [
+          :<,
+          :<=,
+          :>,
+          :>=,
+          :[],
+          :all?,
+          :any?,
+          :assoc,
+          :chunk,
+          :chunk_while,
+          :collect,
+          :collect_concat,
+          :compact,
+          :compare_by_identity,
+          :compare_by_identity?,
+          :count,
+          :cycle,
+          :default,
+          :default_proc,
+          :detect,
+          :dig,
+          :drop,
+          :drop_while,
+          :each,
+          :each_cons,
+          :each_entry,
+          :each_key,
+          :each_pair,
+          :each_slice,
+          :each_value,
+          :each_with_index,
+          :each_with_object,
+          :empty?,
+          :entries,
+          :fetch,
+          :fetch_values,
+          :find,
+          :find_all,
+          :find_index,
+          :first,
+          :flat_map,
+          :flatten,
+          :grep,
+          :grep_v,
+          :group_by,
+          :has_key?,
+          :has_value?,
+          :include?,
+          :index,
+          :inject,
+          :invert,
+          :key,
+          :key?,
+          :keys,
+          :lazy,
+          :length,
+          :map,
+          :max,
+          :max_by,
+          :member?,
+          :merge,
+          :min,
+          :min_by,
+          :minmax,
+          :minmax_by,
+          :none?,
+          :one?,
+          :partition,
+          :rassoc,
+          :reduce,
+          :reject,
+          :reverse_each,
+          :save_plist,
+          :select,
+          :size,
+          :slice,
+          :slice_after,
+          :slice_before,
+          :slice_when,
+          :sort,
+          :sort_by,
+          :sum,
+          :take,
+          :take_while,
+          :to_a,
+          :to_h,
+          :to_hash,
+          :to_plist,
+          :to_proc,
+          :to_set,
+          :transform_keys,
+          :transform_values,
+          :uniq,
+          :value?,
+          :values,
+          :values_at,
+          :zip,
+        ]
         DISALLOWED_MUTATOR_METHODS = [
           :[]=,
           :clear,
           :collect!,
+          :compact!,
           :default=,
           :default_proc=,
           :delete,
@@ -30,15 +132,19 @@ class Chef
           :keep_if,
           :map!,
           :merge!,
-          :update,
+          :rehash,
           :reject!,
           :replace,
           :select!,
           :shift,
-          :write,
-          :write!,
-          :unlink,
+          :store,
+          :transform_keys!,
+          :transform_values!,
           :unlink!,
+          :unlink,
+          :update,
+          :write!,
+          :write,
         ]
 
         # Redefine all of the methods that mutate a Hash to raise an error when called.
diff --git a/spec/unit/node/immutable_collections_spec.rb b/spec/unit/node/immutable_collections_spec.rb
index 96adedddcb..29ae8799e9 100644
--- a/spec/unit/node/immutable_collections_spec.rb
+++ b/spec/unit/node/immutable_collections_spec.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Daniel DeLeo (<dan@chef.io>)
-# Copyright:: Copyright 2012-2017, Chef Software Inc.
+# Copyright:: Copyright 2012-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -34,6 +34,11 @@ describe Chef::Node::ImmutableMash do
     @immutable_mash = @node["key"]
   end
 
+  it "does not have any unaudited methods" do
+    unaudited_methods = Hash.instance_methods - Object.instance_methods - Chef::Node::Mixin::ImmutablizeHash::DISALLOWED_MUTATOR_METHODS - Chef::Node::Mixin::ImmutablizeHash::ALLOWED_METHODS
+    expect(unaudited_methods).to be_empty
+  end
+
   it "element references like regular hash" do
     expect(@immutable_mash[:top][:second_level]).to eq("some value")
   end
@@ -228,6 +233,11 @@ describe Chef::Node::ImmutableArray do
     end
   end
 
+  it "does not have any unaudited methods" do
+    unaudited_methods = Array.instance_methods - Object.instance_methods - Chef::Node::Mixin::ImmutablizeArray::DISALLOWED_MUTATOR_METHODS - Chef::Node::Mixin::ImmutablizeArray::ALLOWED_METHODS
+    expect(unaudited_methods).to be_empty
+  end
+
   it "can be duped even if some elements can't" do
     @immutable_array.dup
   end