Reject directory traversal in store report processor

lib/puppet/reports/store.rb

@@ -2,6 +2,8 @@
 require 'fileutils'
 require 'tempfile'
 
+SEPARATOR = [Regexp.escape(File::SEPARATOR.to_s), Regexp.escape(File::ALT_SEPARATOR.to_s)].join
+
 Puppet::Reports.register_report(:store) do
   desc "Store the yaml report on disk.  Each host sends its report as a YAML dump
     and this just stores the file on disk, in the `reportdir` directory.
@@ -13,9 +15,11 @@
   def process
     # We don't want any tracking back in the fs.  Unlikely, but there
     # you go.
-    client = self.host.gsub("..",".")
+    if host =~ Regexp.union(/[#{SEPARATOR}]/, /\A\.\.?\Z/)
+      raise ArgumentError, "Invalid node name #{host.inspect}"
+    end
 
-    dir = File.join(Puppet[:reportdir], client)
+    dir = File.join(Puppet[:reportdir], host)
 
     if ! FileTest.exists?(dir)
       FileUtils.mkdir_p(dir)
@@ -42,17 +46,20 @@ def process
       FileUtils.mv(f.path, file)
     rescue => detail
       puts detail.backtrace if Puppet[:trace]
-      Puppet.warning "Could not write report for #{client} at #{file}: #{detail}"
+      Puppet.warning "Could not write report for #{host} at #{file}: #{detail}"
     end
 
     # Only testing cares about the return value
     file
   end
 
-  # removes all reports for a given host
+  # removes all reports for a given host?
   def self.destroy(host)
-    client = host.gsub("..",".")
-    dir = File.join(Puppet[:reportdir], client)
+    if host =~ Regexp.union(/[#{SEPARATOR}]/, /\A\.\.?\Z/)
+      raise ArgumentError, "Invalid node name #{host.inspect}"
+    end
+
+    dir = File.join(Puppet[:reportdir], host)
 
     if File.exists?(dir)
       Dir.entries(dir).each do |file|

spec/unit/reports/store_spec.rb

@@ -44,5 +44,33 @@
       FileUtils.expects(:mv).in_sequence(writeseq).with(File.join(Dir.tmpdir, "foo123"), File.join(Puppet[:reportdir], @report.host, "201101061200.yaml"))
       @report.process
     end
+
+    ['..', 'hello/', '/hello', 'he/llo', 'hello/..', '.'].each do |node|
+      it "rejects #{node.inspect}" do
+        @report.host = node
+        expect { @report.process }.to raise_error(ArgumentError, /Invalid node/)
+      end
+    end
+
+    ['.hello', 'hello.', '..hi', 'hi..'].each do |node|
+      it "accepts #{node.inspect}" do
+        @report.host = node
+        @report.process
+      end
+    end
+  end
+
+  describe "::destroy" do
+    ['..', 'hello/', '/hello', 'he/llo', 'hello/..', '.'].each do |node|
+      it "rejects #{node.inspect}" do
+        expect { processor.destroy(node) }.to raise_error(ArgumentError, /Invalid node/)
+      end
+    end
+
+    ['.hello', 'hello.', '..hi', 'hi..'].each do |node|
+      it "accepts #{node.inspect}" do
+        processor.destroy(node)
+      end
+    end
   end
 end