(GH-1759) Handle non-UTF-8 task output

Previously, we treated non-UTF-8 task output as simply non-JSON output,
putting it in a hash under the `_output` key. This violated a key
constraint of the task specification (task output must always be UTF-8)
and caused problems when we tried to print the result as JSON (either as
output or for logging).

We now explicitly check for non-UTF-8 characters when interpreting the
result and we throw away stdout entirely and replace it with an error if
it is invalid.

!bug

* **Task output that contains invalid UTF-8 is now rejected**
  ([#1759](#1759))

  Tasks are defined as returning UTF-8, but Bolt didn't handle the
  non-UTF-8 case explicitly, leading to messy error messages and stack
  traces. The error should now be clear and meaningful.

Author: nicklewis

lib/bolt/result.rb

@@ -40,18 +40,23 @@ def self.for_command(target, stdout, stderr, exit_code, action, command)
     end
 
     def self.for_task(target, stdout, stderr, exit_code, task)
-      begin
-        value = JSON.parse(stdout)
-        unless value.is_a? Hash
-          value = nil
-        end
-      rescue JSON::ParserError
-        value = nil
-      end
-      value ||= { '_output' => stdout }
+      stdout.force_encoding('utf-8') unless stdout.encoding == Encoding::UTF_8
+      value = if stdout.valid_encoding?
+                parse_hash(stdout) || { '_output' => stdout }
+              else
+                { '_error' => { 'kind' => 'puppetlabs.tasks/task-error',
+                                'issue_code' => 'TASK_ERROR',
+                                'msg' => 'The task result contained invalid UTF-8 on stdout',
+                                'details' => {} } }
+              end
+
       if exit_code != 0 && value['_error'].nil?
         msg = if stdout.empty?
-                "The task failed with exit code #{exit_code}:\n#{stderr}"
+                if stderr.empty?
+                  "The task failed with exit code #{exit_code} and no output"
+                else
+                  "The task failed with exit code #{exit_code} and no stdout, but stderr contained:\n#{stderr}"
+                end
               else
                 "The task failed with exit code #{exit_code}"
               end
@@ -63,6 +68,13 @@ def self.for_task(target, stdout, stderr, exit_code, task)
       new(target, value: value, action: 'task', object: task)
     end
 
+    def self.parse_hash(string)
+      value = JSON.parse(string)
+      value if value.is_a? Hash
+    rescue JSON::ParserError
+      nil
+    end
+
     def self.for_upload(target, source, destination)
       new(target, message: "Uploaded '#{source}' to '#{target.host}:#{destination}'", action: 'upload', object: source)
     end

spec/bolt/result_spec.rb

@@ -109,5 +109,25 @@
       expect(result.value).to eq(obj)
       expect(result.error_hash).to eq(obj['_error'])
     end
+
+    it 'uses the unparsed value of stdout if it is not valid JSON' do
+      stdout = 'just some string'
+      result = Bolt::Result.for_task(target, stdout, '', 0, 'atask')
+      expect(result.value).to eq('_output' => 'just some string')
+    end
+
+    it 'generates an error for binary data' do
+      stdout = "\xFC].\xF9\xA8\x85f\xDF{\x11d\xD5\x8E\xC6\xA6"
+      result = Bolt::Result.for_task(target, stdout, '', 0, 'atask')
+      expect(result.value.keys).to eq(['_error'])
+      expect(result.error_hash['msg']).to match(/The task result contained invalid UTF-8/)
+    end
+
+    it 'generates an error for non-UTF-8 output' do
+      stdout = "☃".encode('utf-32')
+      result = Bolt::Result.for_task(target, stdout, '', 0, 'atask')
+      expect(result.value.keys).to eq(['_error'])
+      expect(result.error_hash['msg']).to match(/The task result contained invalid UTF-8/)
+    end
   end
 end