From 9373e133224442103913141e84796b9318101670 Mon Sep 17 00:00:00 2001
From: Pieter van de Bruggen <pvande@gmail.com>
Date: Wed, 31 Jul 2013 14:30:25 -0700
Subject: [PATCH] (#14333) Ensure module permissions are sane.

Prior to this change, tarballs unpacked by the root user would be unpacked
with the permissions and the ownership information present in the tarball.
In the best case, this made some modules fail to function after installation
since the newly installed module was owned by a different user. In the worst
case, this could be seen as a fairly serious security vulnerability.

This change ensures that the module's permissions are always owner-writable,
world-readable, and that the owner is always the same as the module's parent
directory.

Backporting acceptance tests.

(Maint) Use dirname instead of regexes

A sequence of substitutions to strip off the trailing filename from a
path. This is actually just File.dirname in disguise.

(Maint) Clean up specs

The specs were distractingly bad. There was usage of instance variables
that were created in a loop, but caused the final value, which was used
by the tests, to not be what the author expected. This also pulls out
specific tests around how add() behaves in very important circumstances,
which the previous way of automatic test generation did not make clear.

(#21971) Check for possible directory traversal

The class name that is being looked for is translated to the directory
structure to search. In order to prevent directory traversal attacks we
need to make sure that the requested class name is valid and therefore
doesn't contain any directory traversal shortcuts.

(#21971) Split import and autoloading code paths

The codepaths for import and autoloading of manifest files were combined
at many levels, which made them share behaviors that were not wanted.
The import ability to load absolute paths leaked through into how
autoloading would try to find files for a particular class name, which
resulted in a possible vulnerability to injected class names (ENC
values, hiera data, etc). This splits those code paths to ensure that
the one used for autoloading only will ever look in modules that are
registered on the modulepath, while still allowing the import path to
look at absolute and relative file paths.

(#21971) Create system for safely dealing with path patterns

Correctly checking that a given string is safe to use for looking for
files on the filesystem is tricky. This adds a class called
Puppet::FileSystem::PathPattern that encapsulates a validated path
pattern free of defects such as directory traversal '..' entries and
zero byte delimeters. This is then used to protect finding manifest
files in modules.

(#21971) Fix how import_all loads files

The backport of changes had an incorrect import_all that called
load_files with a single string, when an array is expected. This mistake
would not be obvious since ruby 1.8 implements #each on String.

(#21971) Fixes PathPattern's usage of Dir.glob for Windows

Dir.glob seems to perform an impilicit to_s on what is passed.  Except
on Windows.  This change explicitly converts Pathname to a String, so we
don't have any surprises.

Fixing broken requires in the acceptance tests.

Three acceptance tests were improperly backported, and as such couldn't require some dependencies they had; this change fixes the requires and backports the
helpers.

(Maint) Confine module tool tests off of windows

The module tool on puppet 2.7 isn't supported on windows. The tests
shouldn't run there.

(#21971) Allow paths that contain .. as part of a name

The PathPattern code was incorrectly flagging paths like /tmp/foo..bar
as being directory traversals. This updates that to exclude those names
by checking each path component separately instead of looking at the
path as a whole.

In addition this fixes up the error reporting about these kinds of
errors in Puppet::Module by creating the absolute PathPattern for the
manifests directory of the module at initialization time, so that any
errors from it are not swallowed by the error handling for the relative
path searching of manifests.

This also removed some duplicate handling of init.pp and init.rb where
in the case of having no pattern to search for the init files were
returned twice and also seemed to be loaded twice. This does not remove
the behavior in which the init.pp and rb files are *always* returned
when they exist, because that seems to be explicit behavior added for
issue #4220.

Conflicts:
	lib/puppet/module.rb
	spec/unit/module_spec.rb

Had to move absolute path to manifests back into wanted_manifests_from
because module initializes with a path optionally.

Ensure that PMT uses the correct group membership.

Previously, modules installed by the root user would retain the
original group membership information.  This change ensures that the
group will match the module directory being installed to.

Improving testing around PMT module install permissions.

Previously, the tests simply asserted that certain files had correct permissions
and belonged to the appropriate users and groups.  This worked well enough in common
cases, but could also yield a number of false positives.  This change works to remove
a great deal of the ambiguity by specifically assigning an unusual group to the modules
directory, and by asserting the permissions for every file in the installed module.

(Maint) Update expectations for older rspec

These tests were backported from a commit that was for a newer version
of rspec. The older version of rspec used for this branch does not
support the expect().to syntax.

(Maint) Update test to reflect chown using the gid too

When the gid control code was backported, the spec tests were not
updated to reflect this. This changes the expectation to match the code.

(maint) Fix module_utils regex tests for module file perms

Corrects a few bugs in the assert_module_installed_on_disk method.

Also specifically rejects '..' files from the test, since moduledir
itself, as seen by "#{moduledir}/#{module_name}/..", may have
different permissions.

And finally tests 644 for any non-directory file, and allows for a non
space character following the symbolic mode list, because SELinux
(rhel6) may have a '.' rather than a space here.
---
 acceptance/tests/module_utils.rb                  |  183 +++++++++++++++++++++
 acceptance/tests/modules/install/basic_install.rb |   26 +++
 acceptance/tests/temp_file_utils.rb               |  113 +++++++++++++
 lib/puppet.rb                                     |    2 +
 lib/puppet/file_system.rb                         |    3 +
 lib/puppet/file_system/path_pattern.rb            |   97 +++++++++++
 lib/puppet/module.rb                              |   29 +++-
 lib/puppet/module_tool/applications/unpacker.rb   |   11 +-
 lib/puppet/parser/files.rb                        |   35 +++--
 lib/puppet/parser/parser_support.rb               |   11 +-
 lib/puppet/parser/type_loader.rb                  |   77 ++++++----
 spec/unit/file_system/path_pattern_spec.rb        |  139 ++++++++++++++++
 spec/unit/module_spec.rb                          |    9 +-
 spec/unit/parser/files_spec.rb                    |   73 +--------
 spec/unit/parser/parser_spec.rb                   |   20 ++-
 spec/unit/parser/type_loader_spec.rb              |   47 ++----
 spec/unit/resource/type_collection_spec.rb        |   94 +++++------
 17 files changed, 758 insertions(+), 211 deletions(-)
 create mode 100644 acceptance/tests/module_utils.rb
 create mode 100644 acceptance/tests/modules/install/basic_install.rb
 create mode 100644 acceptance/tests/temp_file_utils.rb
 create mode 100644 lib/puppet/file_system.rb
 create mode 100644 lib/puppet/file_system/path_pattern.rb
 create mode 100644 spec/unit/file_system/path_pattern_spec.rb

Index: puppet-2.6.18/acceptance/tests/module_utils.rb
===================================================================
--- /dev/null
+++ puppet-2.6.18/acceptance/tests/module_utils.rb
@@ -0,0 +1,183 @@
+module Puppet
+  module Acceptance
+    module ModuleUtils
+
+      # Return an array of paths to installed modules for a given host.
+      #
+      # Example return value:
+      #
+      # [
+      #   "/opt/puppet/share/puppet/modules/apt",
+      #   "/opt/puppet/share/puppet/modules/auth_conf",
+      #   "/opt/puppet/share/puppet/modules/concat",
+      # ]
+      #
+      # @param host [String] hostname
+      # @return [Array] paths for found modules
+      def get_installed_modules_for_host (host)
+        on host, puppet("module list --render-as s")
+        str  = stdout.lines.to_a.last
+        pat = /\(([^()]+)\)/
+        mods =  str.scan(pat).flatten
+        return mods
+      end
+
+      # Return a hash of array of paths to installed modules for a hosts.
+      # The individual hostnames are the keys of the hash. The only value
+      # for a given key is an array of paths for the found modules.
+      #
+      # Example return value:
+      #
+      # {
+      #   "my_master" =>
+      #     [
+      #       "/opt/puppet/share/puppet/modules/apt",
+      #       "/opt/puppet/share/puppet/modules/auth_conf",
+      #       "/opt/puppet/share/puppet/modules/concat",
+      #     ],
+      #   "my_agent01" =>
+      #     [
+      #       "/opt/puppet/share/puppet/modules/apt",
+      #       "/opt/puppet/share/puppet/modules/auth_conf",
+      #       "/opt/puppet/share/puppet/modules/concat",
+      #     ],
+      # }
+      #
+      # @param hosts [Array] hostnames
+      # @return [Hash] paths for found modules indexed by hostname
+      def get_installed_modules_for_hosts (hosts)
+        mods  = {}
+        hosts.each do |host|
+          mods[host] = get_installed_modules_for_host host
+        end
+        return mods
+      end
+
+      # Compare the module paths in given hashes and remove paths that
+      # are were not present in the first hash. The use case for this
+      # method is to remove any modules that were installed during the
+      # course of a test run.
+      #
+      # Installed module hashes would be gathered using the
+      # `get_+installed_module_for_hosts` command in the setup stage
+      # and teardown stages of a test. These hashes would be passed into
+      # this method in order to find modules installed during the test
+      # and delete them in order to return the SUT environments to their
+      # initial state.
+      #
+      # TODO: Enhance to take versions into account, so that upgrade/
+      # downgrade events during a test does not persist in the SUT
+      # environment.
+      #
+      # @param beginning_hash [Hash] paths for found modules indexed
+      #   by hostname. Taken in the setup stage of a test.
+      # @param ending_hash [Hash] paths for found modules indexed
+      #   by hostname. Taken in the teardown stage of a test.
+      def rm_installed_modules_from_hosts (beginning_hash, ending_hash)
+        ending_hash.each do |host, mod_array|3
+          mod_array.each do |mod|
+            if ! beginning_hash[host].include? mod
+              on host, "rm -rf #{mod}"
+            end
+          end
+        end
+      end
+
+      # Convert a semantic version number string to an integer.
+      #
+      # Example return value given an input of '1.2.42':
+      #
+      #   10242
+      #
+      # @param semver [String] semantic version number
+      def semver_to_i ( semver )
+        # semver assumed to be in format <major>.<minor>.<patch>
+        # calculation assumes that each segment is < 100
+        tmp = semver.split('.')
+        tmp[0].to_i * 10000 + tmp[1].to_i * 100 + tmp[2].to_i
+      end
+
+      # Compare two given semantic version numbers.
+      #
+      # Returns an integer indicating the relationship between the two:
+      #   0 indicates that both are equal
+      #   a value greater than 0 indicates that the semver1 is greater than semver2
+      #   a value less than 0 indicates that the semver1 is less than semver2
+      #
+      def semver_cmp ( semver1, semver2 )
+        semver_to_i(semver1) - semver_to_i(semver2)
+      end
+
+      # Assert that a module was installed according to the UI..
+      #
+      # This is a wrapper to centralize the validation about how
+      # the UI responded that a module was installed.
+      # It is called after a call # to `on ( host )` and inspects
+      # STDOUT for specific content.
+      #
+      # @param stdout [String]
+      # @param module_author [String] the author portion of a module name
+      # @param module_name [String] the name portion of a module name
+      # @param module_verion [String] the version of the module to compare to
+      #     installed version
+      # @param compare_op [String] the operator for comparing the verions of
+      #     the installed module
+      def assert_module_installed_ui ( stdout, module_author, module_name, module_version = nil, compare_op = nil )
+        valid_compare_ops = {'==' => 'equal to', '>' => 'greater than', '<' => 'less than'}
+        assert_match(/#{module_author}-#{module_name}/, stdout,
+              "Notice that module '#{module_author}-#{module_name}' was installed was not displayed")
+        if version
+          /#{module_author}-#{module_name} \(.*v(\d+\.\d+\.\d+)/ =~ stdout
+          installed_version = Regexp.last_match[1]
+          if valid_compare_ops.include? compare_op
+            assert_equal( true, semver_cmp(installed_version, module_version).send(compare_op, 0),
+              "Installed version '#{installed_version}' of '#{module_name}' was not #{valid_compare_ops[compare_op]} '#{module_version}'")
+          end
+        end
+      end
+
+      # Assert that a module is installed on disk.
+      #
+      # @param host [HOST] the host object to make the remote call on
+      # @param moduledir [String] the path where the module should be
+      # @param module_name [String] the name portion of a module name
+      def assert_module_installed_on_disk ( host, moduledir, module_name )
+        # module directory should exist
+        on host, "[ -d #{moduledir}/#{module_name} ]"
+
+        owner = ''
+        group = ''
+        on host, "ls -ld #{moduledir}" do
+          listing = stdout.split(' ')
+          owner = listing[2]
+          group = listing[3]
+        end
+
+        # A module's files should have:
+        #     * a mode of 644 (755, if they're a directory)
+        #     * owner == owner of moduledir
+        #     * group == group of moduledir
+        on host, "ls -alR #{moduledir}/#{module_name}" do
+          listings = stdout.split("\n")
+          listings = listings.grep(/^[bcdlsp-]/)
+          listings = listings.reject { |l| l =~ /\.\.$/ }
+
+          listings.each do |line|
+            assert_match /(drwxr-xr-x|[^d]rw-r--r--)[^\d]+\d+\s+#{owner}\s+#{group}/, line,
+              "bad permissions for '#{line[/\S+$/]}' - expected 644/755, #{owner}, #{group}"
+          end
+        end
+      end
+
+      # Assert that a module is not installed on disk.
+      #
+      # @param host [HOST] the host object to make the remote call on
+      # @param moduledir [String] the path where the module should be
+      # @param module_name [String] the name portion of a module name
+      def assert_module_not_installed_on_disk ( host, moduledir, module_name )
+        on host, "[ ! -d #{moduledir}/#{module_name} ]"
+      end
+
+    end
+  end
+end
Index: puppet-2.6.18/acceptance/tests/modules/install/basic_install.rb
===================================================================
--- /dev/null
+++ puppet-2.6.18/acceptance/tests/modules/install/basic_install.rb
@@ -0,0 +1,26 @@
+test_name "puppet module install (agent)"
+require File.join(File.dirname(__FILE__), '..', '..', 'module_utils')
+extend Puppet::Acceptance::ModuleUtils
+
+confine :except, :platform => 'windows'
+
+agents.each do |agent|
+  step 'setup'
+  stub_forge_on(agent)
+
+  modulesdir = agent.tmpdir('puppet_module_build')
+
+  on(agent, 'groups') do
+    on(agent, "chgrp #{stdout.split(' ').pop} #{modulesdir}")
+  end
+
+  teardown do
+    on agent, "rm -rf #{modulesdir}"
+  end
+
+  step "install module to '#{modulesdir}'"
+  on(agent, puppet("module install pmtacceptance-nginx  --target-dir='#{modulesdir}'")) do
+    assert_match(/#{modulesdir}\n└── pmtacceptance-nginx \(.*\)/, stdout)
+    assert_module_installed_on_disk(agent, modulesdir, 'nginx')
+  end
+end
Index: puppet-2.6.18/acceptance/tests/temp_file_utils.rb
===================================================================
--- /dev/null
+++ puppet-2.6.18/acceptance/tests/temp_file_utils.rb
@@ -0,0 +1,113 @@
+module Puppet
+  module Acceptance
+    module TempFileUtils
+
+      # Create a file on the host.
+      # Parameters:
+      # [host] the host to create the file on
+      # [file_path] the path to the file to be created
+      # [file_content] a string containing the contents to be written to the file
+      # [options] a hash containing additional behavior options.  Currently supported:
+      # * :mkdirs (default false) if true, attempt to create the parent directories on the remote host before writing
+      #       the file
+      # * :owner (default 'root') the username of the user that the file should be owned by
+      # * :group (default 'puppet') the name of the group that the file should be owned by
+      # * :mode (default '644') the mode (file permissions) that the file should be created with
+      def create_test_file(host, file_rel_path, file_content, options = {})
+
+        # set default options
+        options[:mkdirs] ||= false
+        options[:owner] ||= host['user']
+        options[:group] ||= host['group'] || "puppet"
+        options[:mode] ||= "755"
+
+        file_path = get_test_file_path(host, file_rel_path)
+
+        mkdirs(host, File.dirname(file_path)) if (options[:mkdirs] == true)
+        create_remote_file(host, file_path, file_content)
+
+        #
+        # NOTE: we need these chown/chmod calls because the acceptance framework connects to the nodes as "root", but
+        #  puppet 'master' runs as user 'puppet'.  Therefore, in order for puppet master to be able to read any files
+        #  that we've created, we have to carefully set their permissions
+        #
+
+        chown(host, options[:owner], options[:group], file_path)
+        chmod(host, options[:mode], file_path)
+
+      end
+
+
+      # Given a relative path, returns an absolute path for a test file.  Basically, this just prepends the
+      # a unique temp dir path (specific to the current test execution) to your relative path.
+      def get_test_file_path(host, file_rel_path)
+        File.join(@host_test_tmp_dirs[host.name], file_rel_path)
+      end
+
+
+      # Check for the existence of a temp file for the current test; basically, this just calls file_exists?(),
+      # but prepends the path to the current test's temp dir onto the file_rel_path parameter.  This allows
+      # tests to be written using only a relative path to specify file locations, while still taking advantage
+      # of automatic temp file cleanup at test completion.
+      def test_file_exists?(host, file_rel_path)
+        file_exists?(host, get_test_file_path(host, file_rel_path))
+      end
+
+      def file_exists?(host, file_path)
+        host.execute("test -f \"#{file_path}\"",
+                     :acceptable_exit_codes => [0, 1])  do |result|
+          return result.exit_code == 0
+        end
+      end
+
+      def file_contents(host, file_path)
+        host.execute("cat \"#{file_path}\"") do |result|
+          return result.stdout
+        end
+      end
+
+      def tmpdir(host, basename)
+        host_tmpdir = host.tmpdir(basename)
+        # we need to make sure that the puppet user can traverse this directory...
+        chmod(host, "755", host_tmpdir)
+        host_tmpdir
+      end
+
+      def mkdirs(host, dir_path)
+        on(host, "mkdir -p #{dir_path}")
+      end
+
+      def chown(host, owner, group, path)
+        on(host, "chown #{owner}:#{group} #{path}")
+      end
+
+      def chmod(host, mode, path)
+        on(host, "chmod #{mode} #{path}")
+      end
+
+
+      def initialize_temp_dirs()
+        # pluck this out of the test case environment; not sure if there is a better way
+        @cur_test_file = @path
+        @cur_test_file_shortname = File.basename(@cur_test_file, File.extname(@cur_test_file))
+
+        # we need one list of all of the hosts, to assist in managing temp dirs.  It's possible
+        # that the master is also an agent, so this will consolidate them into a unique set
+        @all_hosts = Set[master, *agents]
+
+        # now we can create a hash of temp dirs--one per host, and unique to this test--without worrying about
+        # doing it twice on any individual host
+        @host_test_tmp_dirs = Hash[@all_hosts.map do |host| [host.name, tmpdir(host, @cur_test_file_shortname)] end ]
+      end
+
+      def remove_temp_dirs()
+        @all_hosts.each do |host|
+          on(host, "rm -rf #{@host_test_tmp_dirs[host.name]}")
+        end
+      end
+
+      # a silly variable for keeping track of whether or not all of the tests passed...
+      @all_tests_passed = false
+    end
+  end
+end
\ No newline at end of file
Index: puppet-2.6.18/lib/puppet.rb
===================================================================
--- puppet-2.6.18.orig/lib/puppet.rb
+++ puppet-2.6.18/lib/puppet.rb
@@ -26,6 +26,7 @@ require 'puppet/util/run_mode'
 # it's also a place to find top-level commands like 'debug'
 
 module Puppet
+  require 'puppet/file_system'
   PUPPETVERSION = '2.6.18'
 
   def Puppet.version
Index: puppet-2.6.18/lib/puppet/file_system.rb
===================================================================
--- /dev/null
+++ puppet-2.6.18/lib/puppet/file_system.rb
@@ -0,0 +1,3 @@
+module Puppet::FileSystem
+  require 'puppet/file_system/path_pattern'
+end
Index: puppet-2.6.18/lib/puppet/file_system/path_pattern.rb
===================================================================
--- /dev/null
+++ puppet-2.6.18/lib/puppet/file_system/path_pattern.rb
@@ -0,0 +1,97 @@
+require 'pathname'
+
+module Puppet::FileSystem
+  class PathPattern
+    class InvalidPattern < Puppet::Error; end
+
+    TRAVERSAL = /^\.\.$/
+    ABSOLUTE_UNIX = /^\//
+    ABSOLUTE_WINDOWS = /^[a-z]:/i
+    #ABSOLUT_VODKA #notappearinginthisclass
+    CURRENT_DRIVE_RELATIVE_WINDOWS = /^\\/
+
+    def self.relative(pattern)
+      RelativePathPattern.new(pattern)
+    end
+
+    def self.absolute(pattern)
+      AbsolutePathPattern.new(pattern)
+    end
+
+    class << self
+      protected :new
+    end
+
+    # @param prefix [AbsolutePathPattern] An absolute path pattern instance
+    # @return [AbsolutePathPattern] A new AbsolutePathPattern prepended with
+    #   the passed prefix's pattern.
+    def prefix_with(prefix)
+      new_pathname = prefix.pathname + pathname
+      self.class.absolute(new_pathname.to_s)
+    end
+
+    def glob
+      Dir.glob(pathname.to_s)
+    end
+
+    def to_s
+      pathname.to_s
+    end
+
+    protected
+
+    attr_reader :pathname
+
+    private
+
+    def validate
+      @pathname.each_filename do |e|
+        if e =~ TRAVERSAL
+          raise(InvalidPattern, "PathPatterns cannot be created with directory traversals.")
+        end
+      end
+      case @pathname.to_s
+      when CURRENT_DRIVE_RELATIVE_WINDOWS
+        raise(InvalidPattern, "A PathPattern cannot be a Windows current drive relative path.")
+      end
+    end
+
+    def initialize(pattern)
+      begin
+        @pathname = Pathname.new(pattern.strip)
+      rescue ArgumentError => error
+        raise InvalidPattern.new("PathPatterns cannot be created with a zero byte.", error)
+      end
+      validate
+    end
+  end
+
+  class RelativePathPattern < PathPattern
+    def absolute?
+      false
+    end
+
+    def validate
+      super
+      case @pathname.to_s
+      when ABSOLUTE_WINDOWS
+        raise(InvalidPattern, "A relative PathPattern cannot be prefixed with a drive.")
+      when ABSOLUTE_UNIX
+        raise(InvalidPattern, "A relative PathPattern cannot be an absolute path.")
+      end
+    end
+  end
+
+  class AbsolutePathPattern < PathPattern
+    def absolute?
+      true
+    end
+
+    def validate
+      super
+      if @pathname.to_s !~ ABSOLUTE_UNIX and @pathname.to_s !~ ABSOLUTE_WINDOWS
+        raise(InvalidPattern, "An absolute PathPattern cannot be a relative path.")
+      end
+    end
+  end
+end
Index: puppet-2.6.18/lib/puppet/module.rb
===================================================================
--- puppet-2.6.18.orig/lib/puppet/module.rb
+++ puppet-2.6.18/lib/puppet/module.rb
@@ -9,6 +9,7 @@ class Puppet::Module
   class IncompatiblePlatform < Error; end
   class MissingMetadata < Error; end
   class InvalidName < Error; end
+  class InvalidFilePattern < Error; end
 
   include Puppet::Util::Logging
 
@@ -123,10 +124,16 @@ class Puppet::Module
   # Return the list of manifests matching the given glob pattern,
   # defaulting to 'init.{pp,rb}' for empty modules.
   def match_manifests(rest)
-    pat = File.join(path, MANIFESTS, rest || 'init')
-    [manifest("init.pp"),manifest("init.rb")].compact + Dir.
-      glob(pat + (File.extname(pat).empty? ? '.{pp,rb}' : '')).
-      reject { |f| FileTest.directory?(f) }
+    if rest
+      wanted_manifests = wanted_manifests_from(rest)
+      searched_manifests = wanted_manifests.glob.reject { |f| FileTest.directory?(f) }
+    else
+      searched_manifests = []
+    end
+
+    # (#4220) Always ensure init.pp in case class is defined there.
+    init_manifests = [manifest("init.pp"), manifest("init.rb")].compact
+    init_manifests + searched_manifests
   end
 
   def metadata_file
@@ -183,6 +190,20 @@ class Puppet::Module
 
   private
 
+  def wanted_manifests_from(pattern)
+    begin
+      extended = File.extname(pattern).empty? ? "#{pattern}.{pp,rb}" : pattern
+      relative_pattern = Puppet::FileSystem::PathPattern.relative(extended)
+    rescue Puppet::FileSystem::PathPattern::InvalidPattern => error
+      raise Puppet::Module::InvalidFilePattern.new(
+        "The pattern \"#{pattern}\" to find manifests in the module \"#{name}\" " +
+        "is invalid and potentially unsafe.", error)
+    end
+
+    absolute_path_to_manifests = Puppet::FileSystem::PathPattern.absolute(subpath(MANIFESTS))
+    relative_pattern.prefix_with(absolute_path_to_manifests)
+  end
+
   def subpath(type)
     return File.join(path, type) unless type.to_s == "plugins"
 
Index: puppet-2.6.18/lib/puppet/parser/files.rb
===================================================================
--- puppet-2.6.18.orig/lib/puppet/parser/files.rb
+++ puppet-2.6.18/lib/puppet/parser/files.rb
@@ -8,23 +8,25 @@ require 'puppet/parser/parser'
 module Puppet::Parser::Files
   module_function
 
-  # Return a list of manifests (as absolute filenames) that match +pat+
-  # with the current directory set to +cwd+. If the first component of
-  # +pat+ does not contain any wildcards and is an existing module, return
-  # a list of manifests in that module matching the rest of +pat+
-  # Otherwise, try to find manifests matching +pat+ relative to +cwd+
-  def find_manifests(start, options = {})
-    cwd = options[:cwd] || Dir.getwd
-    module_name, pattern = split_file_path(start)
+  # Return a list of manifests as absolute filenames matching the given 
+  # pattern.
+  #
+  # @param pattern [String] A reference for a file in a module. It is the format "<modulename>/<file glob>"
+  # @param environment [Puppet::Node::Environment] the environment of modules
+  #
+  # @return [Array(String, Array<String>)] the module name and the list of files found
+  # @api private
+  def find_manifests_in_modules(pattern, environment)
+    module_name, file_pattern = split_file_path(pattern)
     begin
-      if mod = Puppet::Module.find(module_name, options[:environment])
-        return [mod.name, mod.match_manifests(pattern)]
+      if mod = Puppet::Module.find(module_name, environment)
+        return [mod.name, mod.match_manifests(file_pattern)]
       end
     rescue Puppet::Module::InvalidName
-      # Than that would be a "no."
+      # one of the modules being loaded might have an invalid name and so
+      # looking for one might blow up since we load them lazily.
     end
-    abspat = File::expand_path(start, cwd)
-    [nil, Dir.glob(abspat + (File.extname(abspat).empty? ? '{.pp,.rb}' : '' )).uniq.reject { |f| FileTest.directory?(f) }]
+    [nil, []]
   end
 
   # Find the concrete file denoted by +file+. If +file+ is absolute,
@@ -80,9 +82,12 @@ module Puppet::Parser::Files
 
   # Split the path into the module and the rest of the path, or return
   # nil if the path is empty or absolute (starts with a /).
-  # This method can return nil & anyone calling it needs to handle that.
   def split_file_path(path)
-    path.split(File::SEPARATOR, 2) unless path == "" or path == File.expand_path(path)
+    if path == "" or Puppet::Util.absolute_path?(path)
+      nil
+    else
+      path.split(File::SEPARATOR, 2)
+    end
   end
 
 end
Index: puppet-2.6.18/lib/puppet/parser/parser_support.rb
===================================================================
--- puppet-2.6.18.orig/lib/puppet/parser/parser_support.rb
+++ puppet-2.6.18/lib/puppet/parser/parser_support.rb
@@ -110,7 +110,16 @@ class Puppet::Parser::Parser
   end
 
   def import(file)
-    known_resource_types.loader.import(file, @lexer.file)
+    if @lexer.file
+      # use a path relative to the file doing the importing
+      dir = File.dirname(@lexer.file)
+    else
+      # otherwise assume that everything needs to be from where the user is
+      # executing this command. Normally, this would be in a "puppet apply -e"
+      dir = Dir.pwd
+    end
+
+    known_resource_types.loader.import(file, dir)
   end
 
   def initialize(env)
Index: puppet-2.6.18/lib/puppet/parser/type_loader.rb
===================================================================
--- puppet-2.6.18.orig/lib/puppet/parser/type_loader.rb
+++ puppet-2.6.18/lib/puppet/parser/type_loader.rb
@@ -58,36 +58,29 @@ class Puppet::Parser::TypeLoader
     end
   end
 
-  # Import our files.
-  def import(file, current_file = nil)
+  # Import manifest files that match a given file glob pattern.
+  #
+  # @param pattern [String] the file glob to apply when determining which files
+  #   to load
+  # @param dir [String] base directory to use when the file is not
+  #   found in a module
+  # @api private
+  def import(pattern, dir)
     return if Puppet[:ignoreimport]
 
-    # use a path relative to the file doing the importing
-    if current_file
-      dir = current_file.sub(%r{[^/]+$},'').sub(/\/$/, '')
-    else
-      dir = "."
-    end
-    if dir == ""
-      dir = "."
-    end
+    modname, files = Puppet::Parser::Files.find_manifests_in_modules(pattern, environment)
+    if files.empty?
+      abspat = File.expand_path(pattern, dir)
+      file_pattern = abspat + (File.extname(abspat).empty? ? '{.pp,.rb}' : '' )
 
-    pat = file
-    modname, files = Puppet::Parser::Files.find_manifests(pat, :cwd => dir, :environment => environment)
-    if files.size == 0
-      raise Puppet::ImportError.new("No file(s) found for import of '#{pat}'")
-    end
+      files = Dir.glob(file_pattern).uniq.reject { |f| FileTest.directory?(f) }
+      modname = nil
 
-    files.each do |file|
-      unless file =~ /^#{File::SEPARATOR}/
-        file = File.join(dir, file)
-      end
-      @loading_helper.do_once(file) do
-        parse_file(file)
+      if files.empty?
+        raise_no_files_found(pattern)
       end
-    end
-
-    modname
+	end
+    load_files(modname, files)
   end
 
   def known_resource_types
@@ -118,6 +111,31 @@ class Puppet::Parser::TypeLoader
     end
     nil
   end
+  def import_from_modules(pattern)
+    modname, files = Puppet::Parser::Files.find_manifests_in_modules(pattern, environment)
+    if files.empty?
+      raise_no_files_found(pattern)
+    end
+
+    load_files(modname, files)
+  end
+
+  def raise_no_files_found(pattern)
+    raise Puppet::ImportError, "No file(s) found for import of '#{pattern}'"
+  end
+
+  def load_files(modname, files)
+    loaded_asts = []
+   files.each do |file|
+      @loading_helper.do_once(file) do
+        loaded_asts << parse_file(file)
+      end
+    end
+
+    loaded_asts.collect do |ast|
+      known_resource_types.import_ast(ast, modname)
+    end.flatten
+  end
 
   def name2files(namespaces, name)
     return [name.sub(/^::/, '').gsub("::", File::SEPARATOR)] if name =~ /^::/
Index: puppet-2.6.18/spec/unit/file_system/path_pattern_spec.rb
===================================================================
--- /dev/null
+++ puppet-2.6.18/spec/unit/file_system/path_pattern_spec.rb
@@ -0,0 +1,139 @@
+require 'spec_helper'
+require 'puppet_spec/files'
+require 'puppet/file_system'
+
+describe Puppet::FileSystem::PathPattern do
+  include PuppetSpec::Files
+  InvalidPattern = Puppet::FileSystem::PathPattern::InvalidPattern
+
+  describe 'relative' do
+    it "can not be created with a traversal up the directory tree" do
+      expect do
+        Puppet::FileSystem::PathPattern.relative("my/../other")
+      end.to raise_error(InvalidPattern, "PathPatterns cannot be created with directory traversals.")
+    end
+
+    it "can be created with a '..' prefixing a filename" do
+      Puppet::FileSystem::PathPattern.relative("my/..other").to_s.should eq("my/..other")
+    end
+
+    it "can be created with a '..' suffixing a filename" do
+      Puppet::FileSystem::PathPattern.relative("my/other..").to_s.should eq("my/other..")
+    end
+
+    it "can be created with a '..' embedded in a filename" do
+      Puppet::FileSystem::PathPattern.relative("my/ot..her").to_s.should eq("my/ot..her")
+    end
+
+    it "can not be created with a \\0 byte embedded" do
+      expect do
+        Puppet::FileSystem::PathPattern.relative("my/\0/other")
+      end.to raise_error(InvalidPattern, /PathPatterns cannot be created with a zero byte./)
+    end
+
+    it "can not be created with a windows drive" do
+      expect do
+        Puppet::FileSystem::PathPattern.relative("c:\\relative\\path")
+      end.to raise_error(InvalidPattern, "A relative PathPattern cannot be prefixed with a drive.")
+    end
+
+    it "can not be created with a windows drive (with space)" do
+      expect do
+        Puppet::FileSystem::PathPattern.relative(" c:\\relative\\path")
+      end.to raise_error(InvalidPattern, "A relative PathPattern cannot be prefixed with a drive.")
+    end
+
+    it "can not create an absolute relative path" do
+      expect do
+        Puppet::FileSystem::PathPattern.relative("/no/absolutes")
+      end.to raise_error(InvalidPattern, "A relative PathPattern cannot be an absolute path.")
+    end
+
+    it "can not create an absolute relative path (with space)" do
+      expect do
+        Puppet::FileSystem::PathPattern.relative("\t/no/absolutes")
+      end.to raise_error(InvalidPattern, "A relative PathPattern cannot be an absolute path.")
+    end
+
+    it "can not create a relative path that is a windows path relative to the current drive" do
+      expect do
+        Puppet::FileSystem::PathPattern.relative("\\no\relatives")
+      end.to raise_error(InvalidPattern, "A PathPattern cannot be a Windows current drive relative path.")
+    end
+
+    it "creates a relative PathPattern from a valid relative path" do
+      Puppet::FileSystem::PathPattern.relative("a/relative/path").to_s.should eq("a/relative/path")
+    end
+
+    it "is not absolute" do
+      Puppet::FileSystem::PathPattern.relative("a/relative/path").should_not be_absolute
+    end
+  end
+
+  describe 'absolute' do
+    it "can not create a relative absolute path" do
+      expect do
+        Puppet::FileSystem::PathPattern.absolute("no/relatives")
+      end.to raise_error(InvalidPattern, "An absolute PathPattern cannot be a relative path.")
+    end
+
+    it "can not create an absolute path that is a windows path relative to the current drive" do
+      expect do
+        Puppet::FileSystem::PathPattern.absolute("\\no\\relatives")
+      end.to raise_error(InvalidPattern, "A PathPattern cannot be a Windows current drive relative path.")
+    end
+
+    it "creates an absolute PathPattern from a valid absolute path" do
+      Puppet::FileSystem::PathPattern.absolute("/an/absolute/path").to_s.should eq("/an/absolute/path")
+    end
+
+    it "creates an absolute PathPattern from a valid Windows absolute path" do
+      Puppet::FileSystem::PathPattern.absolute("c:/absolute/windows/path").to_s.should eq("c:/absolute/windows/path")
+    end
+
+    it "is absolute" do
+      Puppet::FileSystem::PathPattern.absolute("c:/absolute/windows/path").should be_absolute
+    end
+
+    it "can be created with a '..' embedded in a filename on windows", :if => Puppet.features.microsoft_windows? do
+      Puppet::FileSystem::PathPattern.absolute(%q{c:\..my\ot..her\one..}).to_s.should eq(%q{c:\..my\ot..her\one..})
+    end
+  end
+
+  it "prefixes the relative path pattern with another path" do
+    pattern = Puppet::FileSystem::PathPattern.relative("docs/*_thoughts.txt")
+    prefix = Puppet::FileSystem::PathPattern.absolute("/prefix")
+
+    absolute_pattern = pattern.prefix_with(prefix)
+
+    absolute_pattern.should be_absolute
+    absolute_pattern.to_s.should eq(File.join("/prefix", "docs/*_thoughts.txt"))
+  end
+
+  it "refuses to prefix with a relative pattern" do
+    pattern = Puppet::FileSystem::PathPattern.relative("docs/*_thoughts.txt")
+    prefix = Puppet::FileSystem::PathPattern.relative("prefix")
+
+    expect do
+      pattern.prefix_with(prefix)
+    end.to raise_error(InvalidPattern, "An absolute PathPattern cannot be a relative path.")
+  end
+
+  it "applies the pattern to the filesystem as a glob" do
+    dir = tmpdir('globtest')
+    create_file_in(dir, "found_one")
+    create_file_in(dir, "found_two")
+    create_file_in(dir, "third_not_found")
+
+    pattern = Puppet::FileSystem::PathPattern.relative("found_*").prefix_with(
+      Puppet::FileSystem::PathPattern.absolute(dir))
+
+    pattern.glob.should include(File.join(dir, "found_one"))
+    pattern.glob.should include(File.join(dir, "found_two"))
+    pattern.glob.should_not include(File.join(dir, "third_not_found"))
+  end
+
+  def create_file_in(dir, name)
+    File.open(File.join(dir, name), "w") { |f| f.puts "data" }
+  end
+end
Index: puppet-2.6.18/spec/unit/module_spec.rb
===================================================================
--- puppet-2.6.18.orig/spec/unit/module_spec.rb
+++ puppet-2.6.18/spec/unit/module_spec.rb
@@ -421,7 +421,8 @@ describe Puppet::Module, "when finding m
   end
 
   it "should default to the 'init' file if no glob pattern is specified" do
-    Dir.expects(:glob).with("/a/manifests/init.{pp,rb}").returns(%w{/a/manifests/init.pp})
+    FileTest.expects(:exist?).with("/a/manifests/init.pp").returns(true)
+    FileTest.expects(:exist?).with("/a/manifests/init.rb").returns(false)
 
     @mod.match_manifests(nil).should == %w{/a/manifests/init.pp}
   end
@@ -443,6 +444,12 @@ describe Puppet::Module, "when finding m
 
     @mod.match_manifests(@pq_glob_with_extension).should == []
   end
+
+  it "should raise an error if the pattern tries to leave the manifest directory" do
+    expect do
+      @mod.match_manifests("something/../../*")
+    end.to raise_error(Puppet::Module::InvalidFilePattern, /The pattern "something\/\.\.\/\.\.\/\*" to find manifests in the module "mymod" is invalid and potentially unsafe./)
+  end
 end
 
 describe Puppet::Module do
Index: puppet-2.6.18/spec/unit/parser/files_spec.rb
===================================================================
--- puppet-2.6.18.orig/spec/unit/parser/files_spec.rb
+++ puppet-2.6.18/spec/unit/parser/files_spec.rb
@@ -10,14 +10,6 @@ describe Puppet::Parser::Files do
     @basepath = Puppet.features.posix? ? "/somepath" : "C:/somepath"
   end
 
-  it "should have a method for finding a template" do
-    Puppet::Parser::Files.should respond_to(:find_template)
-  end
-
-  it "should have a method for finding manifests" do
-    Puppet::Parser::Files.should respond_to(:find_manifests)
-  end
-
   describe "when searching for templates" do
     it "should return fully-qualified templates directly" do
       Puppet::Parser::Files.expects(:modulepath).never
@@ -126,74 +118,21 @@ describe Puppet::Parser::Files do
   describe "when searching for manifests" do
     it "should ignore invalid modules" do
       mod = mock 'module'
-      Puppet::Node::Environment.new.expects(:module).with("mymod").raises(Puppet::Module::InvalidName, "name is invalid")
+	  env = Puppet::Node::Environment.new
+      env.expects(:module).with("mymod").raises(Puppet::Module::InvalidName, "name is invalid")
       Puppet.expects(:value).with(:modulepath).never
       Dir.stubs(:glob).returns %w{foo}
-
-      Puppet::Parser::Files.find_manifests("mymod/init.pp")
-    end
-  end
-
-  describe "when searching for manifests when no module is found" do
-    before do
-      File.stubs(:find).returns(nil)
-    end
-
-    it "should not look for modules when paths are fully qualified" do
-      Puppet.expects(:value).with(:modulepath).never
-      file = @basepath + "/fully/qualified/file.pp"
-      Dir.stubs(:glob).with(file).returns([file])
-      Puppet::Parser::Files.find_manifests(file)
-    end
-
-    it "should return nil and an array of fully qualified files" do
-      file = @basepath + "/fully/qualified/file.pp"
-      Dir.stubs(:glob).with(file).returns([file])
-      Puppet::Parser::Files.find_manifests(file).should == [nil, [file]]
-    end
-
-    it "should match against provided fully qualified patterns" do
-      pattern = @basepath + "/fully/qualified/pattern/*"
-      Dir.expects(:glob).with(pattern+'{.pp,.rb}').returns(%w{my file list})
-      Puppet::Parser::Files.find_manifests(pattern)[1].should == %w{my file list}
-    end
-
-    it "should look for files relative to the current directory" do
-      cwd = Dir.getwd
-      Dir.expects(:glob).with("#{cwd}/foobar/init.pp").returns(["#{cwd}/foobar/init.pp"])
-      Puppet::Parser::Files.find_manifests("foobar/init.pp")[1].should == ["#{cwd}/foobar/init.pp"]
-    end
-
-    it "should only return files, not directories" do
-      pattern = @basepath + "/fully/qualified/pattern/*"
-      file = @basepath + "/my/file"
-      dir = @basepath + "/my/directory"
-      Dir.expects(:glob).with(pattern+'{.pp,.rb}').returns([file, dir])
-      FileTest.expects(:directory?).with(file).returns(false)
-      FileTest.expects(:directory?).with(dir).returns(true)
-      Puppet::Parser::Files.find_manifests(pattern)[1].should == [file]
-    end
-
-    it "should return files once only" do
-      pattern = @basepath + "/fully/qualified/pattern/*"
-      Dir.expects(:glob).with(pattern+'{.pp,.rb}').returns(%w{one two one})
-      Puppet::Parser::Files.find_manifests(pattern)[1].should == %w{one two}
+      Puppet::Parser::Files.find_manifests_in_modules("mymod/init.pp", env)
     end
   end
 
   describe "when searching for manifests in a found module" do
     it "should return the name of the module and the manifests from the first found module" do
       mod = Puppet::Module.new("mymod")
-      Puppet::Node::Environment.new.expects(:module).with("mymod").returns mod
-      mod.expects(:match_manifests).with("init.pp").returns(%w{/one/mymod/manifests/init.pp})
-      Puppet::Parser::Files.find_manifests("mymod/init.pp").should == ["mymod", ["/one/mymod/manifests/init.pp"]]
-    end
-
-    it "should use the node environment if specified" do
-      mod = Puppet::Module.new("mymod")
-      Puppet::Node::Environment.new("myenv").expects(:module).with("mymod").returns mod
+      env = Puppet::Node::Environment.new
+      env.expects(:module).with("mymod").returns mod
       mod.expects(:match_manifests).with("init.pp").returns(%w{/one/mymod/manifests/init.pp})
-      Puppet::Parser::Files.find_manifests("mymod/init.pp", :environment => "myenv")[1].should == ["/one/mymod/manifests/init.pp"]
+      Puppet::Parser::Files.find_manifests_in_modules("mymod/init.pp", env).should == ["mymod", ["/one/mymod/manifests/init.pp"]]
     end
 
     after { Puppet.settings.clear }
Index: puppet-2.6.18/spec/unit/parser/parser_spec.rb
===================================================================
--- puppet-2.6.18.orig/spec/unit/parser/parser_spec.rb
+++ puppet-2.6.18/spec/unit/parser/parser_spec.rb
@@ -33,11 +33,21 @@ describe Puppet::Parser do
     parser.known_resource_types.should equal(rtc)
   end
 
-  it "should delegate importing to the known resource type loader" do
-    parser = Puppet::Parser::Parser.new "development"
-    parser.known_resource_types.loader.expects(:import).with("newfile", "current_file")
-    parser.lexer.expects(:file).returns "current_file"
-    parser.import("newfile")
+  context "when importing" do
+    it "uses the directory of the currently parsed file" do
+      @parser.lexer.stubs(:file).returns "/tmp/current_file"
+
+      @parser.known_resource_types.loader.expects(:import).with("newfile", "/tmp")
+
+      @parser.import("newfile")
+    end
+
+    it "uses the current working directory, when there is no file being parsed" do
+      @parser.known_resource_types.loader.expects(:import).with('one', Dir.pwd)
+      @parser.known_resource_types.loader.expects(:import).with('two', Dir.pwd)
+
+      @parser.parse("import 'one', 'two'")
+    end
   end
 
   describe "when parsing files" do
Index: puppet-2.6.18/spec/unit/parser/type_loader_spec.rb
===================================================================
--- puppet-2.6.18.orig/spec/unit/parser/type_loader_spec.rb
+++ puppet-2.6.18/spec/unit/parser/type_loader_spec.rb
@@ -178,10 +178,11 @@ describe Puppet::Parser::TypeLoader do
     end
   end
 
+
   it "should be able to add classes to the current resource type collection" do
     file = tmpfile("simple_file.pp")
     File.open(file, "w") { |f| f.puts "class foo {}" }
-    @loader.import(file)
+    @loader.import(File.basename(file), File.dirname(file))
 
     @loader.known_resource_types.hostclass("foo").should be_instance_of(Puppet::Resource::Type)
   end
Index: puppet-2.6.18/spec/unit/resource/type_collection_spec.rb
===================================================================
--- puppet-2.6.18.orig/spec/unit/resource/type_collection_spec.rb
+++ puppet-2.6.18/spec/unit/resource/type_collection_spec.rb
@@ -55,6 +55,14 @@ describe Puppet::Resource::TypeCollectio
     @code.node("foo").should equal(node)
   end
 
+  it "should fail if a duplicate node is added" do
+    @code.add(Puppet::Resource::Type.new(:node, "foo"))
+
+    expect do
+      @code.add(Puppet::Resource::Type.new(:node, "foo"))
+    end.to raise_error(Puppet::ParseError, /cannot redefine/)
+  end
+
   it "should store hostclasses as hostclasses" do
     klass = Puppet::Resource::Type.new(:hostclass, "foo")
 
@@ -62,6 +70,16 @@ describe Puppet::Resource::TypeCollectio
     @code.hostclass("foo").should equal(klass)
   end
 
+  it "merge together hostclasses of the same name" do
+    klass1 = Puppet::Resource::Type.new(:hostclass, "foo", :doc => "first")
+    klass2 = Puppet::Resource::Type.new(:hostclass, "foo", :doc => "second")
+
+    @code.add(klass1)
+    @code.add(klass2)
+
+    @code.hostclass("foo").doc.should == "firstsecond"
+  end
+
   it "should store definitions as definitions" do
     define = Puppet::Resource::Type.new(:definition, "foo")
 
@@ -69,13 +87,12 @@ describe Puppet::Resource::TypeCollectio
     @code.definition("foo").should equal(define)
   end
 
-  it "should merge new classes with existing classes of the same name" do
-    loader = Puppet::Resource::TypeCollection.new("env")
-    first = Puppet::Resource::Type.new(:hostclass, "foo")
-    second = Puppet::Resource::Type.new(:hostclass, "foo")
-    loader.add first
-    first.expects(:merge).with(second)
-    loader.add(second)
+  it "should fail if a duplicate definition is added" do
+    @code.add(Puppet::Resource::Type.new(:definition, "foo"))
+
+    expect do
+      @code.add(Puppet::Resource::Type.new(:definition, "foo"))
+    end.to raise_error(Puppet::ParseError, /cannot be redefined/)
   end
 
   it "should remove all nodes, classes, and definitions when cleared" do
@@ -136,58 +153,27 @@ describe Puppet::Resource::TypeCollectio
   end
 
   %w{hostclass node definition}.each do |data|
-    before do
-      @instance = Puppet::Resource::Type.new(data, "foo")
-    end
+    describe "behavior of add for #{data}" do
 
-    it "should have a method for adding a #{data}" do
-      Puppet::Resource::TypeCollection.new("env").should respond_to("add_#{data}")
-    end
-
-    it "should use the name of the instance to add it" do
-      loader = Puppet::Resource::TypeCollection.new("env")
-      loader.send("add_#{data}", @instance)
-      loader.send(data, @instance.name).should equal(@instance)
-    end
-
-    unless data == "hostclass"
-      it "should fail to add a #{data} when one already exists" do
+      it "should return the added #{data}" do
         loader = Puppet::Resource::TypeCollection.new("env")
-        loader.add @instance
-        lambda { loader.add(@instance) }.should raise_error(Puppet::ParseError)
-      end
-    end
+        instance = Puppet::Resource::Type.new(data, "foo")
 
-    it "should return the added #{data}" do
-      loader = Puppet::Resource::TypeCollection.new("env")
-
-      loader.add(@instance).should equal(@instance)
-    end
+        loader.add(instance).should equal(instance)
+      end
 
-    it "should be able to retrieve #{data} by name" do
-      loader = Puppet::Resource::TypeCollection.new("env")
-      instance = Puppet::Resource::Type.new(data, "bar")
-      loader.add instance
-      loader.send(data, "bar").should equal(instance)
-    end
+      it "should retrieve #{data} insensitive to case" do
+        loader = Puppet::Resource::TypeCollection.new("env")
+        instance = Puppet::Resource::Type.new(data, "Bar")
 
-    it "should retrieve #{data} insensitive to case" do
-      loader = Puppet::Resource::TypeCollection.new("env")
-      instance = Puppet::Resource::Type.new(data, "Bar")
-      loader.add instance
-      loader.send(data, "bAr").should equal(instance)
-    end
+        loader.add instance
 
-    it "should return nil when asked for a #{data} that has not been added" do
-      Puppet::Resource::TypeCollection.new("env").send(data, "foo").should be_nil
-    end
+        loader.send(data, "bAr").should equal(instance)
+      end
 
-    it "should be able to retrieve all #{data}s" do
-      plurals = { "hostclass" => "hostclasses", "node" => "nodes", "definition" => "definitions" }
-      loader = Puppet::Resource::TypeCollection.new("env")
-      instance = Puppet::Resource::Type.new(data, "foo")
-      loader.add instance
-      loader.send(plurals[data]).should == { "foo" => instance }
+      it "should return nil when asked for a #{data} that has not been added" do
+        Puppet::Resource::TypeCollection.new("env").send(data, "foo").should be_nil
+      end
     end
   end
 
@@ -457,7 +443,5 @@ describe Puppet::Resource::TypeCollectio
 
       lambda { @code.version }.should raise_error(Puppet::ParseError)
     end
-
   end
-
 end
