From 7baf174b1100a9e7e11776788c7a761e7adbedae Mon Sep 17 00:00:00 2001
From: Lamont Granquist <lamont@scriptkiddie.org>
Date: Fri, 14 Aug 2020 15:12:08 -0700
Subject: more tests found a bug

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
---
 chef-config/lib/chef-config/path_helper.rb |  38 +++++----
 chef-config/spec/unit/path_helper_spec.rb  | 129 ++++++++++++++++++++++-------
 2 files changed, 119 insertions(+), 48 deletions(-)

(limited to 'chef-config')

diff --git a/chef-config/lib/chef-config/path_helper.rb b/chef-config/lib/chef-config/path_helper.rb
index 6b501cb9cb..d22858485e 100644
--- a/chef-config/lib/chef-config/path_helper.rb
+++ b/chef-config/lib/chef-config/path_helper.rb
@@ -55,15 +55,14 @@ module ChefConfig
       end
     end
 
-    path_separator_regex = [Regexp.escape(File::SEPARATOR), Regexp.escape(path_separator)].uniq.join
-
-    TRAILING_SLASHES_REGEX = /[#{path_separator_regex}]+$/.freeze
-    LEADING_SLASHES_REGEX = /^[#{path_separator_regex}]+/.freeze
-
     def self.join(*args, windows: ChefUtils.windows?)
+      path_separator_regex = Regexp.escape(windows ? "#{File::SEPARATOR}#{BACKSLASH}" : File::SEPARATOR)
+      trailing_slashes_regex = /[#{path_separator_regex}]+$/.freeze
+      leading_slashes_regex = /^[#{path_separator_regex}]+/.freeze
+
       args.flatten.inject do |joined_path, component|
-        joined_path = joined_path.sub(TRAILING_SLASHES_REGEX, "")
-        component = component.sub(LEADING_SLASHES_REGEX, "")
+        joined_path = joined_path.sub(trailing_slashes_regex, "")
+        component = component.sub(leading_slashes_regex, "")
         joined_path + "#{path_separator(windows: windows)}#{component}"
       end
     end
@@ -126,17 +125,22 @@ module ChefConfig
       abs_path
     end
 
-    # This is the INVERSE of Pathname#cleanpath, it converts forward
-    # slashes to backslashes for Windows.  Since the Ruby API and the
-    # Windows APIs all consume forward slashes, this helper function
-    # should only be used for *DISPLAY* logic to send strings back
-    # to the user with backslashes.  Internally, filename paths should
-    # generally be stored with forward slashes for consistency.  It is
-    # not necessary or desired to blindly convert pathnames to have
-    # backslashes on Windows.
+    # The built in ruby Pathname#cleanpath method does not clean up forward slashes and
+    # backslashes.  This is a wrapper around that which does.  In general this is NOT
+    # recommended for internal use within ruby/chef since ruby does not care about forward slashes
+    # vs. backslashes, even on Windows.  Where this generally matters is when being rendered
+    # to the user, or being rendered into things like the windows PATH or to commands that
+    # are being executed.  In some cases it may be easier on windows to render paths to
+    # unix-style for being eventually eval'd by ruby in the future (templates being rendered
+    # with code to be consumed by ruby) where forcing unix-style forward slashes avoids the
+    # issue of needing to escape the backslashes in rendered strings.  This has a boolean
+    # operator to force windows-style or non-windows style operation, where the default is
+    # determined by the underlying node['platform'] value.
+    #
+    # In general if you don't know if you need this routine, do not use it, best practice
+    # within chef/ruby itself is not to care.  Only use it to force windows or unix style
+    # when it really matters.
     #
-    # Generally, if the user isn't going to be seeing it, you should be
-    # using Pathname#cleanpath instead of this function.
     def self.cleanpath(path, windows: ChefUtils.windows?)
       path = Pathname.new(path).cleanpath.to_s
       if windows
diff --git a/chef-config/spec/unit/path_helper_spec.rb b/chef-config/spec/unit/path_helper_spec.rb
index 8d5eb61166..3eedb8bbf5 100644
--- a/chef-config/spec/unit/path_helper_spec.rb
+++ b/chef-config/spec/unit/path_helper_spec.rb
@@ -23,9 +23,8 @@ RSpec.describe ChefConfig::PathHelper do
 
   let(:path_helper) { described_class }
 
-  shared_examples_for "common_functionality" do
-    describe "join" do
-
+  context "common functionality" do
+    context "join" do
       it "joins starting with '' resolve to absolute paths" do
         expect(path_helper.join("", "a", "b")).to eq("#{path_helper.path_separator}a#{path_helper.path_separator}b")
       end
@@ -33,10 +32,9 @@ RSpec.describe ChefConfig::PathHelper do
       it "joins ending with '' add a / to the end" do
         expect(path_helper.join("a", "b", "")).to eq("a#{path_helper.path_separator}b#{path_helper.path_separator}")
       end
-
     end
 
-    describe "dirname" do
+    context "dirname" do
       it "dirname('abc') is '.'" do
         expect(path_helper.dirname("abc")).to eq(".")
       end
@@ -55,42 +53,109 @@ RSpec.describe ChefConfig::PathHelper do
     end
   end
 
+  context "forcing windows/non-windows" do
+    context "forcing windows" do
+      it "path_separator is \\" do
+        expect(path_helper.path_separator(windows: true)).to eq('\\')
+      end
+
+      context "platform-specific #join behavior" do
+        it "joins components on Windows when some end with unix separators" do
+          expected = "C:\\foo\\bar\\baz"
+          expect(path_helper.join('C:\\foo/', "bar", "baz", windows: true)).to eq(expected)
+        end
+
+        it "joins components when some end with separators" do
+          expected = "C:\\foo\\bar\\baz"
+          expect(path_helper.join('C:\\foo\\', "bar", "baz", windows: true)).to eq(expected)
+        end
+
+        it "joins components when some end and start with separators" do
+          expected = "C:\\foo\\bar\\baz"
+          expect(path_helper.join('C:\\foo\\', "bar/", "/baz", windows: true)).to eq(expected)
+        end
+
+        it "joins components that don't end in separators" do
+          expected = "C:\\foo\\bar\\baz"
+          expect(path_helper.join('C:\\foo', "bar", "baz", windows: true)).to eq(expected)
+        end
+      end
+
+      it "cleanpath changes slashes into backslashes and leaves backslashes alone" do
+        expect(path_helper.cleanpath('/a/b\\c/d/', windows: true)).to eq('\\a\\b\\c\\d')
+      end
+
+      it "cleanpath does not remove leading double backslash" do
+        expect(path_helper.cleanpath('\\\\a/b\\c/d/', windows: true)).to eq('\\\\a\\b\\c\\d')
+      end
+    end
+
+    context "forcing unix" do
+      it "path_separator is /" do
+        expect(path_helper.path_separator(windows: false)).to eq("/")
+      end
+
+      it "cleanpath removes extra slashes alone" do
+        expect(path_helper.cleanpath("/a///b/c/d/", windows: false)).to eq("/a/b/c/d")
+      end
+
+      context "platform-specific #join behavior" do
+        it "joins components when some end with separators" do
+          expected = "/foo/bar/baz"
+          expect(path_helper.join("/foo/", "bar", "baz", windows: false)).to eq(expected)
+        end
+
+        it "joins components when some end and start with separators" do
+          expected = "/foo/bar/baz"
+          expect(path_helper.join("/foo/", "bar/", "/baz", windows: false)).to eq(expected)
+        end
+
+        it "joins components that don't end in separators" do
+          expected = "/foo/bar/baz"
+          expect(path_helper.join("/foo", "bar", "baz", windows: false)).to eq(expected)
+        end
+      end
+
+      it "cleanpath changes backslashes into slashes and leaves slashes alone" do
+        expect(path_helper.cleanpath('/a/b\\c/d/', windows: false)).to eq("/a/b/c/d")
+      end
+
+      it "cleanpath does not remove leading double backslash" do
+        expect(path_helper.cleanpath('\\\\a/b\\c/d/', windows: false)).to eq("//a/b/c/d")
+      end
+    end
+  end
+
   context "on windows", :windows_only do
 
     before(:each) do
       allow(ChefUtils).to receive(:windows?).and_return(true)
     end
 
-    include_examples("common_functionality")
-
     it "path_separator is \\" do
       expect(path_helper.path_separator).to eq('\\')
     end
 
-    describe "platform-specific #join behavior" do
-
+    context "platform-specific #join behavior" do
       it "joins components on Windows when some end with unix separators" do
-        expect(path_helper.join('C:\\foo/', "bar", "baz")).to eq('C:\\foo\\bar\\baz')
+        expected = "C:\\foo\\bar\\baz"
+        expect(path_helper.join('C:\\foo/', "bar", "baz")).to eq(expected)
       end
 
       it "joins components when some end with separators" do
-        expected = path_helper.cleanpath("/foo/bar/baz")
-        expected = "C:#{expected}"
+        expected = "C:\\foo\\bar\\baz"
         expect(path_helper.join('C:\\foo\\', "bar", "baz")).to eq(expected)
       end
 
       it "joins components when some end and start with separators" do
-        expected = path_helper.cleanpath("/foo/bar/baz")
-        expected = "C:#{expected}"
+        expected = "C:\\foo\\bar\\baz"
         expect(path_helper.join('C:\\foo\\', "bar/", "/baz")).to eq(expected)
       end
 
       it "joins components that don't end in separators" do
-        expected = path_helper.cleanpath("/foo/bar/baz")
-        expected = "C:#{expected}"
+        expected = "C:\\foo\\bar\\baz"
         expect(path_helper.join('C:\\foo', "bar", "baz")).to eq(expected)
       end
-
     end
 
     it "cleanpath changes slashes into backslashes and leaves backslashes alone" do
@@ -100,17 +165,13 @@ RSpec.describe ChefConfig::PathHelper do
     it "cleanpath does not remove leading double backslash" do
       expect(path_helper.cleanpath('\\\\a/b\\c/d/')).to eq('\\\\a\\b\\c\\d')
     end
-
   end
 
   context "on unix", :unix_only do
-
     before(:each) do
       allow(ChefUtils).to receive(:windows?).and_return(false)
     end
 
-    include_examples("common_functionality")
-
     it "path_separator is /" do
       expect(path_helper.path_separator).to eq("/")
     end
@@ -119,8 +180,7 @@ RSpec.describe ChefConfig::PathHelper do
       expect(path_helper.cleanpath("/a///b/c/d/")).to eq("/a/b/c/d")
     end
 
-    describe "platform-specific #join behavior" do
-
+    context "platform-specific #join behavior" do
       it "joins components when some end with separators" do
         expected = path_helper.cleanpath("/foo/bar/baz")
         expect(path_helper.join("/foo/", "bar", "baz")).to eq(expected)
@@ -135,12 +195,19 @@ RSpec.describe ChefConfig::PathHelper do
         expected = path_helper.cleanpath("/foo/bar/baz")
         expect(path_helper.join("/foo", "bar", "baz")).to eq(expected)
       end
+    end
 
+    it "cleanpath changes backslashes into slashes and leaves slashes alone" do
+      expect(path_helper.cleanpath('/a/b\\c/d/', windows: false)).to eq("/a/b/c/d")
     end
 
+    # NOTE: this seems a bit weird to me, but this is just the way Pathname#cleanpath works
+    it "cleanpath does not remove leading double backslash" do
+      expect(path_helper.cleanpath('\\\\a/b\\c/d/')).to eq("//a/b/c/d")
+    end
   end
 
-  describe "validate_path" do
+  context "validate_path" do
     context "on windows" do
       before(:each) do
         # pass by default
@@ -171,7 +238,7 @@ RSpec.describe ChefConfig::PathHelper do
     end
   end
 
-  describe "windows_max_length_exceeded?" do
+  context "windows_max_length_exceeded?" do
     it "returns true if the path is too long (259 + NUL) for the API" do
       expect(path_helper.windows_max_length_exceeded?("C:\\" + "a" * 250 + "\\" + "b" * 6)).to be_truthy
     end
@@ -185,7 +252,7 @@ RSpec.describe ChefConfig::PathHelper do
     end
   end
 
-  describe "printable?" do
+  context "printable?" do
     it "returns true if the string contains no non-printable characters" do
       expect(path_helper.printable?("C:\\Program Files (x86)\\Microsoft Office\\Files.lst")).to be_truthy
     end
@@ -208,7 +275,7 @@ RSpec.describe ChefConfig::PathHelper do
     end
   end
 
-  describe "canonical_path" do
+  context "canonical_path" do
     context "on windows", :windows_only do
       it "returns an absolute path with backslashes instead of slashes" do
         expect(path_helper.canonical_path("\\\\?\\C:/windows/win.ini")).to eq("\\\\?\\c:\\windows\\win.ini")
@@ -230,7 +297,7 @@ RSpec.describe ChefConfig::PathHelper do
     end
   end
 
-  describe "paths_eql?" do
+  context "paths_eql?" do
     it "returns true if the paths are the same" do
       allow(path_helper).to receive(:canonical_path).with("bandit", windows: ChefUtils.windows?).and_return("c:/bandit/bandit")
       allow(path_helper).to receive(:canonical_path).with("../bandit/bandit", windows: ChefUtils.windows?).and_return("c:/bandit/bandit")
@@ -244,7 +311,7 @@ RSpec.describe ChefConfig::PathHelper do
     end
   end
 
-  describe "escape_glob" do
+  context "escape_glob" do
     it "escapes characters reserved by glob" do
       path = "C:\\this\\*path\\[needs]\\escaping?"
       escaped_path = "C:\\\\this\\\\\\*path\\\\\\[needs\\]\\\\escaping\\?"
@@ -264,7 +331,7 @@ RSpec.describe ChefConfig::PathHelper do
     end
   end
 
-  describe "escape_glob_dir" do
+  context "escape_glob_dir" do
     it "escapes characters reserved by glob without using backslashes for path separators" do
       path = "C:/this/*path/[needs]/escaping?"
       escaped_path = "C:/this/\\*path/\\[needs\\]/escaping\\?"
@@ -281,7 +348,7 @@ RSpec.describe ChefConfig::PathHelper do
     end
   end
 
-  describe "all_homes" do
+  context "all_homes" do
     before do
       stub_const("ENV", env)
       allow(ChefUtils).to receive(:windows?).and_return(is_windows)
-- 
cgit v1.2.1