diff options
author | Lamont Granquist <lamont@scriptkiddie.org> | 2013-11-26 14:19:50 -0800 |
---|---|---|
committer | Lamont Granquist <lamont@scriptkiddie.org> | 2013-11-26 14:19:50 -0800 |
commit | 12d7ffe818efcbe7cfabde4a86fedcdf96c576df (patch) | |
tree | f1fd20ec9ae9153a9567b092b8988af7a2ec6b88 | |
parent | 5ca7571cc1a39d11082e08f787c6c6d87ea859c2 (diff) | |
download | chef-12d7ffe818efcbe7cfabde4a86fedcdf96c576df.tar.gz |
backport of CHEF-4662 to 10-stable
- insecure Tempfile perms fixes
-rw-r--r-- | chef/lib/chef/knife/core/node_editor.rb | 52 | ||||
-rw-r--r-- | chef/lib/chef/knife/core/ui.rb | 22 | ||||
-rw-r--r-- | chef/spec/unit/knife/core/ui_spec.rb | 118 | ||||
-rw-r--r-- | chef/spec/unit/knife/node_edit_spec.rb | 39 |
4 files changed, 176 insertions, 55 deletions
diff --git a/chef/lib/chef/knife/core/node_editor.rb b/chef/lib/chef/knife/core/node_editor.rb index 22ba3eaa25..073492197c 100644 --- a/chef/lib/chef/knife/core/node_editor.rb +++ b/chef/lib/chef/knife/core/node_editor.rb @@ -18,6 +18,7 @@ require 'chef/json_compat' require 'chef/node' +require 'tempfile' class Chef class Knife @@ -35,11 +36,24 @@ class Chef abort "You specified the --disable_editing option, nothing to edit" if config[:disable_editing] assert_editor_set! - updated_node_data = edit_data(view) + updated_node_data = @ui.edit_data(view) apply_updates(updated_node_data) @updated_node end + def updated? + pristine_copy = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json(node), :create_additions => false) + updated_copy = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json(@updated_node), :create_additions => false) + unless pristine_copy == updated_copy + updated_properties = %w{name normal chef_environment run_list default override automatic}.reject do |key| + pristine_copy[key] == updated_copy[key] + end + end + ( pristine_copy != updated_copy ) && updated_properties + end + + private + def view result = {} result["name"] = node.name @@ -52,12 +66,7 @@ class Chef result["override"] = node.override_attrs result["automatic"] = node.automatic_attrs end - Chef::JSONCompat.to_json_pretty(result) - end - - def edit_data(text) - edited_data = tempfile_for(text) {|filename| system("#{config[:editor]} #{filename}")} - Chef::JSONCompat.from_json(edited_data) + result end def apply_updates(updated_data) @@ -84,19 +93,6 @@ class Chef end end - def updated? - pristine_copy = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json(node), :create_additions => false) - updated_copy = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json(@updated_node), :create_additions => false) - unless pristine_copy == updated_copy - updated_properties = %w{name normal chef_environment run_list default override automatic}.reject do |key| - pristine_copy[key] == updated_copy[key] - end - end - ( pristine_copy != updated_copy ) && updated_properties - end - - private - def abort(message) ui.error(message) exit 1 @@ -108,22 +104,6 @@ class Chef end end - def tempfile_for(data) - # TODO: include useful info like the node name in the temp file - # name - basename = "knife-edit-" << rand(1_000_000_000_000_000).to_s.rjust(15, '0') << '.js' - filename = File.join(Dir.tmpdir, basename) - File.open(filename, "w+") do |f| - f.sync = true - f.puts data - end - - yield filename - - IO.read(filename) - ensure - File.unlink(filename) - end end end end diff --git a/chef/lib/chef/knife/core/ui.rb b/chef/lib/chef/knife/core/ui.rb index 4c80e9e2ea..1dacab66c6 100644 --- a/chef/lib/chef/knife/core/ui.rb +++ b/chef/lib/chef/knife/core/ui.rb @@ -21,6 +21,7 @@ require 'forwardable' require 'chef/platform' require 'chef/knife/core/generic_presenter' +require 'tempfile' class Chef class Knife @@ -148,19 +149,14 @@ class Chef output = Chef::JSONCompat.to_json_pretty(data) if (!config[:disable_editing]) - filename = "knife-edit-" - 0.upto(20) { filename += rand(9).to_s } - filename << ".js" - filename = File.join(Dir.tmpdir, filename) - tf = File.open(filename, "w") - tf.sync = true - tf.puts output - tf.close - raise "Please set EDITOR environment variable" unless system("#{config[:editor]} #{tf.path}") - tf = File.open(filename, "r") - output = tf.gets(nil) - tf.close - File.unlink(filename) + Tempfile.open([ 'knife-edit-', '.json' ]) do |tf| + tf.sync = true + tf.puts output + tf.close + raise "Please set EDITOR environment variable" unless system("#{config[:editor]} #{tf.path}") + + output = IO.read(tf.path) + end end parse_output ? Chef::JSONCompat.from_json(output) : output diff --git a/chef/spec/unit/knife/core/ui_spec.rb b/chef/spec/unit/knife/core/ui_spec.rb index 3cce709949..c71aa32817 100644 --- a/chef/spec/unit/knife/core/ui_spec.rb +++ b/chef/spec/unit/knife/core/ui_spec.rb @@ -27,6 +27,124 @@ describe Chef::Knife::UI do @ui = Chef::Knife::UI.new(@out, @err, @in, @config) end + describe "edit" do + ruby_for_json = { 'foo' => 'bar' } + json_from_ruby = "{\n \"foo\": \"bar\"\n}" + json_from_editor = "{\n \"bar\": \"foo\"\n}" + ruby_from_editor = { 'bar' => 'foo' } + my_editor = "veeeye" + temp_path = "/tmp/bar/baz" + + let(:subject) { @ui.edit_data(ruby_for_json, parse_output) } + let(:parse_output) { false } + + context "when editing is disabled" do + before do + @ui.config[:disable_editing] = true + stub_const("Tempfile", double) # Tempfiles should never be invoked + end + context "when parse_output is false" do + it "returns pretty json string" do + expect(subject).to eql(json_from_ruby) + end + end + context "when parse_output is true" do + let(:parse_output) { true } + it "returns a ruby object" do + expect(subject).to eql(ruby_for_json) + end + end + + end + + context "when editing is enabled" do + before do + @ui.config[:disable_editing] = false + @ui.config[:editor] = my_editor + @mock = mock('Tempfile') + @mock.should_receive(:sync=).with(true) + @mock.should_receive(:puts).with(json_from_ruby) + @mock.should_receive(:close) + @mock.should_receive(:path).at_least(:once).and_return(temp_path) + Tempfile.should_receive(:open).with([ 'knife-edit-', '.json' ]).and_yield(@mock) + end + context "and the editor works" do + before do + @ui.should_receive(:system).with("#{my_editor} #{temp_path}").and_return(true) + IO.should_receive(:read).with(temp_path).and_return(json_from_editor) + end + + context "when parse_output is false" do + it "returns an edited pretty json string" do + expect(subject).to eql(json_from_editor) + end + end + context "when parse_output is true" do + let(:parse_output) { true } + it "returns an edited ruby object" do + expect(subject).to eql(ruby_from_editor) + end + end + end + context "when running the editor fails with nil" do + before do + @ui.should_receive(:system).with("#{my_editor} #{temp_path}").and_return(nil) + IO.should_not_receive(:read) + end + it "throws an exception" do + expect{ subject }.to raise_error(RuntimeError) + end + end + context "when running the editor fails with false" do + before do + @ui.should_receive(:system).with("#{my_editor} #{temp_path}").and_return(false) + IO.should_not_receive(:read) + end + it "throws an exception" do + expect{ subject }.to raise_error(RuntimeError) + end + end + end + context "when editing and not stubbing Tempfile (semi-functional test)" do + before do + @ui.config[:disable_editing] = false + @ui.config[:editor] = my_editor + @tempfile = Tempfile.new([ 'knife-edit-', '.json' ]) + Tempfile.should_receive(:open).with([ 'knife-edit-', '.json' ]).and_yield(@tempfile) + end + + context "and the editor works" do + before do + @ui.should_receive(:system).with("#{my_editor} #{@tempfile.path}").and_return(true) + IO.should_receive(:read).with(@tempfile.path).and_return(json_from_editor) + end + + context "when parse_output is false" do + it "returns an edited pretty json string" do + expect(subject).to eql(json_from_editor) + end + it "the tempfile should have mode 0600" do + # XXX: this looks odd because we're really testing Tempfile.new here + expect(File.stat(@tempfile.path).mode & 0777).to eql(0600) + expect(subject).to eql(json_from_editor) + end + end + + context "when parse_output is true" do + let(:parse_output) { true } + it "returns an edited ruby object" do + expect(subject).to eql(ruby_from_editor) + end + it "the tempfile should have mode 0600" do + # XXX: this looks odd because we're really testing Tempfile.new here + expect(File.stat(@tempfile.path).mode & 0777).to eql(0600) + expect(subject).to eql(ruby_from_editor) + end + end + end + end + end + describe "format_list_for_display" do it "should print the full hash if --with-uri is true" do @ui.config[:with_uri] = true diff --git a/chef/spec/unit/knife/node_edit_spec.rb b/chef/spec/unit/knife/node_edit_spec.rb index 0ba2e90cfe..3651103b81 100644 --- a/chef/spec/unit/knife/node_edit_spec.rb +++ b/chef/spec/unit/knife/node_edit_spec.rb @@ -20,6 +20,12 @@ require 'spec_helper' Chef::Knife::NodeEdit.load_deps describe Chef::Knife::NodeEdit do + + # helper to convert the view from Chef objects into Ruby objects representing JSON + def deserialized_json_view + actual = Chef::JSONCompat.from_json(Chef::JSONCompat.to_json_pretty(@knife.node_editor.send(:view))) + end + before(:each) do Chef::Config[:node_name] = "webmonkey.example.com" @knife = Chef::Knife::NodeEdit.new @@ -49,7 +55,7 @@ describe Chef::Knife::NodeEdit do end it "creates a view of the node without attributes from roles or ohai" do - actual = Chef::JSONCompat.from_json(@knife.node_editor.view) + actual = deserialized_json_view actual.should_not have_key("automatic") actual.should_not have_key("override") actual.should_not have_key("default") @@ -61,7 +67,7 @@ describe Chef::Knife::NodeEdit do it "shows the extra attributes when given the --all option" do @knife.config[:all_attributes] = true - actual = Chef::JSONCompat.from_json(@knife.node_editor.view) + actual = deserialized_json_view actual["automatic"].should == {"go" => "away"} actual["override"].should == {"dont" => "show"} actual["default"].should == {"hide" => "me"} @@ -71,18 +77,39 @@ describe Chef::Knife::NodeEdit do end it "does not consider unedited data updated" do - view = Chef::JSONCompat.from_json( @knife.node_editor.view ) - @knife.node_editor.apply_updates(view) + view = deserialized_json_view + @knife.node_editor.send(:apply_updates, view) @knife.node_editor.should_not be_updated end it "considers edited data updated" do - view = Chef::JSONCompat.from_json( @knife.node_editor.view ) + view = deserialized_json_view view["run_list"] << "role[fuuu]" - @knife.node_editor.apply_updates(view) + @knife.node_editor.send(:apply_updates, view) @knife.node_editor.should be_updated end end + + describe "edit_node" do + + before do + @knife.stub!(:node).and_return(@node) + end + + let(:subject) { @knife.node_editor.edit_node } + + it "raises an exception when editing is disabled" do + @knife.config[:disable_editing] = true + expect{ subject }.to raise_error(SystemExit) + end + + it "raises an exception when the editor is not set" do + @knife.config[:editor] = nil + expect{ subject }.to raise_error(SystemExit) + end + + end + end |