(GH-1759) Strip non-UTF-8 characters from command/script results

We explicitly fail if a task returns non-UTF-8 characters, but those
characters are generally allowed from commands and scripts. However,
they still can't be printed or converted to JSON. We now define a
"safe_value" that is the scrubbed version of the input and use that for
printing and JSON.

This commit removes the status_hash method and replaces it in all cases with
the to_data method which is otherwise identical except for having string
keys. The to_data method now returns the safe_value, as to_data is only
used to print the result, deal with it as JSON equivalent in Puppet, or
emit it as JSON. Each of those cases requires it to be UTF-8 only.

!bug

* **Non-UTF-8 characters in command and script output are removed before printing**
  ([#1759](#1759))

  Commands and scripts are allowed to return UTF-8, but Bolt would error
  when trying to print those results or return them as JSON. Now,
  accessing fields of the result from a Puppet plan will return the
  values unmodified, but invalid characters will be replaced by their
  hex-escaped equivalents when printing the result or converting it to
  JSON.

Author: nicklewis

lib/bolt/outputter/human.rb

@@ -107,13 +107,14 @@ def print_result(result)
 
           # Use special handling if the result looks like a command or script result
           if result.generic_value.keys == %w[stdout stderr exit_code]
-            unless result['stdout'].strip.empty?
+            safe_value = result.safe_value
+            unless safe_value['stdout'].strip.empty?
               @stream.puts(indent(2, "STDOUT:"))
-              @stream.puts(indent(4, result['stdout']))
+              @stream.puts(indent(4, safe_value['stdout']))
             end
-            unless result['stderr'].strip.empty?
+            unless safe_value['stderr'].strip.empty?
               @stream.puts(indent(2, "STDERR:"))
-              @stream.puts(indent(4, result['stderr']))
+              @stream.puts(indent(4, safe_value['stderr']))
             end
           elsif result.generic_value.any?
             @stream.puts(indent(2, ::JSON.pretty_generate(result.generic_value)))

lib/bolt/outputter/json.rb

@@ -28,7 +28,7 @@ def handle_event(event)
 
       def print_result(result)
         @stream.puts ',' if @preceding_item
-        @stream.puts result.status_hash.to_json
+        @stream.puts result.to_json
         @preceding_item = true
       end
 

lib/bolt/rerun.rb

@@ -45,7 +45,7 @@ def update(result_set)
         end
 
         if result_set.is_a?(Bolt::ResultSet)
-          data = result_set.map { |res| res.status_hash.select { |k, _| %i[target status].include? k } }
+          data = result_set.map { |res| { target: res.target.name, status: res.status } }
           FileUtils.mkdir_p(File.dirname(@path))
           File.write(@path, data.to_json)
         elsif File.exist?(@path)

lib/bolt/result.rb

@@ -122,18 +122,8 @@ def message?
       message && !message.strip.empty?
     end
 
-    def status_hash
-      {
-        target: @target.name,
-        action: action,
-        object: object,
-        status: status,
-        value: @value
-      }
-    end
-
     def generic_value
-      value.reject { |k, _| %w[_error _output].include? k }
+      safe_value.reject { |k, _| %w[_error _output].include? k }
     end
 
     def eql?(other)
@@ -151,15 +141,36 @@ def ==(other)
     end
 
     def to_json(opts = nil)
-      status_hash.to_json(opts)
+      safe_value.to_json(opts)
     end
 
     def to_s
       to_json
     end
 
+    # This is the value with all non-UTF-8 characters removed, suitable for
+    # printing or converting to JSON. It *should* only be possible to have
+    # non-UTF-8 characters in stdout/stderr keys as they are not allowed from
+    # tasks but we scrub the whole thing just in case.
+    def safe_value
+      Bolt::Util.walk_vals(value) do |val|
+        if val.is_a?(String)
+          # Replace invalid bytes with hex codes, ie. \xDE\xAD\xBE\xEF
+          val.scrub { |c| c.bytes.map { |b| "\\x" + b.to_s(16).upcase }.join }
+        else
+          val
+        end
+      end
+    end
+
     def to_data
-      Bolt::Util.walk_keys(status_hash, &:to_s)
+      {
+        "target" => @target.name,
+        "action" => action,
+        "object" => object,
+        "status" => status,
+        "value" => safe_value
+      }
     end
 
     def status

lib/bolt/result_set.rb

@@ -99,17 +99,14 @@ def eql?(other)
       self.class == other.class && @results == other.results
     end
 
-    def to_a
-      @results.map(&:status_hash)
-    end
-
     def to_json(opts = nil)
-      @results.map(&:status_hash).to_json(opts)
+      to_data.to_json(opts)
     end
 
     def to_data
       @results.map(&:to_data)
     end
+    alias to_a to_data
 
     def to_s
       to_json

lib/bolt_server/transport_app.rb

@@ -57,8 +57,8 @@ def initialize(config)
     end
 
     def scrub_stack_trace(result)
-      if result.dig(:value, '_error', 'details', 'stack_trace')
-        result[:value]['_error']['details'].reject! { |k| k == 'stack_trace' }
+      if result.dig('value', '_error', 'details', 'stack_trace')
+        result['value']['_error']['details'].reject! { |k| k == 'stack_trace' }
       end
       result
     end
@@ -87,14 +87,14 @@ def validate_schema(schema, body)
     # If the `result_set` contains only one item, it will be returned
     # as a single result object. Set `aggregate` to treat it as a set
     # of results with length 1 instead.
-    def result_set_to_status_hash(result_set, aggregate: false)
+    def result_set_to_data(result_set, aggregate: false)
       scrubbed_results = result_set.map do |result|
-        scrub_stack_trace(result.status_hash)
+        scrub_stack_trace(result.to_data)
       end
 
       if aggregate || scrubbed_results.length > 1
         # For actions that act on multiple targets, construct a status hash for the aggregate result
-        all_succeeded = scrubbed_results.all? { |r| r[:status] == 'success' }
+        all_succeeded = scrubbed_results.all? { |r| r['status'] == 'success' }
         {
           status: all_succeeded ? 'success' : 'failure',
           result: scrubbed_results
@@ -297,7 +297,7 @@ def make_ssh_target(target_hash)
       return [400, error.to_json] unless error.nil?
 
       aggregate = body['target'].nil?
-      [200, result_set_to_status_hash(result_set, aggregate: aggregate).to_json]
+      [200, result_set_to_data(result_set, aggregate: aggregate).to_json]
     end
 
     def make_winrm_target(target_hash)
@@ -337,7 +337,7 @@ def make_winrm_target(target_hash)
       return [400, error.to_json] if error
 
       aggregate = body['target'].nil?
-      [200, result_set_to_status_hash(result_set, aggregate: aggregate).to_json]
+      [200, result_set_to_data(result_set, aggregate: aggregate).to_json]
     end
 
     # Fetches the metadata for a single plan

spec/bolt/rerun_spec.rb

@@ -32,8 +32,8 @@
 
   let(:failure_array) do
     result_set.map do |r|
-      r = r.status_hash
-      { 'target' => r[:target], 'status' => r[:status] }
+      r = r.to_data
+      { 'target' => r['target'], 'status' => r['status'] }
     end
   end
 

spec/bolt_server/transport_app_spec.rb

@@ -70,7 +70,7 @@ def mock_plan_info(full_name)
       }
     end
     let(:action) { 'run_task' }
-    let(:result) { double(Bolt::Result, status_hash: { status: 'test_status' }) }
+    let(:result) { double(Bolt::Result, to_data: { 'status': 'test_status' }) }
 
     before(:each) do
       allow_any_instance_of(BoltServer::TransportApp)
@@ -235,7 +235,7 @@ def mock_plan_info(full_name)
         } }
 
         expect_any_instance_of(BoltServer::TransportApp)
-          .to receive(:scrub_stack_trace).with(result.status_hash).and_return({})
+          .to receive(:scrub_stack_trace).with(result.to_data).and_return({})
 
         post(path, JSON.generate(body), 'CONTENT_TYPE' => 'text/json')
 
@@ -255,7 +255,7 @@ def mock_plan_info(full_name)
         } }
 
         expect_any_instance_of(BoltServer::TransportApp)
-          .to receive(:scrub_stack_trace).with(result.status_hash).and_return({})
+          .to receive(:scrub_stack_trace).with(result.to_data).and_return({})
 
         post(path, JSON.generate(body), 'CONTENT_TYPE' => 'text/json')
 
@@ -349,7 +349,7 @@ def mock_plan_info(full_name)
         } }
 
         expect_any_instance_of(BoltServer::TransportApp)
-          .to receive(:scrub_stack_trace).with(result.status_hash).and_return({})
+          .to receive(:scrub_stack_trace).with(result.to_data).and_return({})
 
         post(path, JSON.generate(body), 'CONTENT_TYPE' => 'text/json')