Merge pull request #1051 from puppetlabs/maint-move_apt_mark_to_provider

Harden apt-mark defined type

Author: chelnak

examples/mark.pp

@@ -0,0 +1,3 @@
+apt::mark { 'vim':
+  setting => 'auto',
+}

manifests/mark.pp

@@ -8,16 +8,31 @@
 define apt::mark (
   Enum['auto','manual','hold','unhold'] $setting,
 ) {
-  case $setting {
-    'unhold': {
-      $unless_cmd = undef
-    }
-    default: {
-      $unless_cmd = "/usr/bin/apt-mark show${setting} ${title} | /bin/fgrep -qs ${title}"
-    }
+  if $title !~ /^[a-zA-Z0-9\-_]+$/ {
+    fail("Invalid package name: ${title}")
   }
-  exec { "/usr/bin/apt-mark ${setting} ${title}":
-    onlyif => "/usr/bin/dpkg -l ${title}",
-    unless => $unless_cmd,
+
+  if $setting == 'unhold' {
+    $unless_cmd = undef
+  } else {
+    $action = "show${setting}"
+
+    # It would be ideal if we could break out this command in to an array of args, similar
+    # to $onlyif_cmd and $command. However, in this case it wouldn't work as expected due
+    # to the inclusion of a pipe character.
+    # When passed to the exec function, the posix provider will strip everything to the right of the pipe,
+    # causing the command to return a full list of packages for the given action.
+    # The trade off is to use an interpolated string knowing that action is built from an enum value and
+    # title is pre-validated.
+    $unless_cmd = ["/usr/bin/apt-mark ${action} ${title} | grep ${title} -q"]
+  }
+
+  $onlyif_cmd = [['/usr/bin/dpkg', '-l', $title]]
+  $command = ['/usr/bin/apt-mark', $setting, $title]
+
+  exec { "apt-mark ${setting} ${title}":
+    command => $command,
+    onlyif  => $onlyif_cmd,
+    unless  => $unless_cmd,
   }
 }

spec/defines/mark_spec.rb

@@ -32,7 +32,7 @@
     end
 
     it {
-      is_expected.to contain_exec('/usr/bin/apt-mark manual my_source')
+      is_expected.to contain_exec('apt-mark manual my_source')
     }
   end
 
@@ -47,4 +47,50 @@
       is_expected.to raise_error(Puppet::PreformattedError, %r{expects a match for Enum\['auto', 'hold', 'manual', 'unhold'\], got 'foobar'})
     end
   end
+
+  [
+    'package',
+    'package1',
+    'package_name',
+    'package-name',
+  ].each do |value|
+    describe 'with a valid resource title' do
+      let :title do
+        value
+      end
+
+      let :params do
+        {
+          'setting' => 'manual',
+        }
+      end
+
+      it do
+        is_expected.to contain_exec("apt-mark manual #{title}")
+      end
+    end
+  end
+
+  [
+    '|| ls -la ||',
+    'packakge with space',
+    'package<>|',
+    '|| touch /tmp/foo.txt ||',
+  ].each do |value|
+    describe 'with an invalid resource title' do
+      let :title do
+        value
+      end
+
+      let :params do
+        {
+          'setting' => 'manual',
+        }
+      end
+
+      it do
+        is_expected.to raise_error(Puppet::PreformattedError, %r{Invalid package name: #{title}})
+      end
+    end
+  end
 end