Merge pull request #256 from puppetlabs/cat_2484_fix_yaml_overwrite

(CAT-2484) Prevent YAML corruption by making PuppetLint instances file-type aware

Author: david22swan

lib/puppet-lint.rb

@@ -235,6 +235,42 @@ def print_problems
     report(@problems)
   end
 
+  # Public: Write fixes back to the file if this file type supports fixes.
+  #
+  # Returns nothing.
+  def write_fixes
+    return unless should_write_fixes?
+
+    File.binwrite(@path, @manifest)
+  end
+
+  # Internal: Determine if fixes should be written for this file.
+  #
+  # Returns true if all conditions are met for writing fixes, false otherwise.
+  def should_write_fixes?
+    # Don't write if file type doesn't support fixes
+    return false unless supports_fixes?
+
+    # Don't write if there are syntax errors (can't safely fix)
+    return false if @problems&.any? { |r| r[:check] == :syntax }
+
+    # Don't write if there's no manifest content
+    return false if @manifest.nil? || @manifest.empty?
+
+    true
+  end
+
+  # Public: Determine if this file type supports automatic fixes.
+  #
+  # Returns true if fixes are supported for this file type, false otherwise.
+  def supports_fixes?
+    return false if @path.nil?
+
+    # Only .pp files support fixes currently
+    # YAML files and other types may support fixes in the future
+    File.extname(@path).match?(%r{\.pp$}i)
+  end
+
   # Public: Define a new check.
   #
   # name  - A unique name for the check as a Symbol.

lib/puppet-lint/bin.rb

@@ -90,9 +90,7 @@ def run
 
         return_val = 1 if l.errors? || (l.warnings? && PuppetLint.configuration.fail_on_warnings)
 
-        next unless PuppetLint.configuration.fix && l.problems.none? { |r| r[:check] == :syntax }
-
-        File.binwrite(f, l.manifest)
+        l.write_fixes if PuppetLint.configuration.fix
       end
 
       if PuppetLint.configuration.sarif

spec/fixtures/test/manifests/issue_254_overwriting_yaml/class_with_dash.pp

@@ -0,0 +1,3 @@
+class foo-bar {
+  notify { 'hello': }
+}

spec/fixtures/test/manifests/issue_254_overwriting_yaml/class_with_dash.yaml

@@ -0,0 +1,7 @@
+---
+classes:
+  foo-bar:
+    parameters: []
+    resources:
+      notify:
+        - title: hello

spec/unit/puppet-lint/bin_spec.rb

@@ -615,9 +615,27 @@ def initialize(args)
     end
   end
 
+  context 'when fixing a directory containing both .pp and .yaml files' do
+    let(:args) { ['--fix', 'spec/fixtures/test/manifests/issue_254_overwriting_yaml'] }
+
+    its(:exitstatus) { is_expected.to eq(1) }
+
+    it 'does not overwrite YAML files' do
+      yaml_file = 'spec/fixtures/test/manifests/issue_254_overwriting_yaml/class_with_dash.yaml'
+      original_yaml = File.read(yaml_file)
+
+      bin # Run the command
+
+      yaml_after = File.read(yaml_file)
+      expect(yaml_after).to eq(original_yaml)
+      expect(yaml_after).to include('classes:')
+      expect(yaml_after).not_to include('class foo-bar {')
+    end
+  end
+
   context 'when overriding config file options with command line options' do
     context 'and config file sets "--only-checks=variable_contains_dash"' do
-      around(:context) do |example|
+      around(:each) do |example|
         Dir.mktmpdir do |tmpdir|
           Dir.chdir(tmpdir) do
             File.open('.puppet-lint.rc', 'wb') do |f|

spec/unit/puppet-lint/puppet-lint_spec.rb

@@ -15,4 +15,53 @@
     linter.run
     expect(linter.manifest).to eq('')
   end
+
+  describe '#supports_fixes?' do
+    context 'with a .pp file' do
+      it 'returns true' do
+        linter.instance_variable_set(:@path, 'test.pp')
+        expect(linter.supports_fixes?).to be true
+      end
+    end
+
+    context 'with a .yaml file' do
+      it 'returns false' do
+        linter.instance_variable_set(:@path, 'test.yaml')
+        expect(linter.supports_fixes?).to be false
+      end
+    end
+
+    context 'with no path set' do
+      it 'returns false' do
+        expect(linter.supports_fixes?).to be false
+      end
+    end
+  end
+
+  describe '#should_write_fixes?' do
+    before(:each) do
+      linter.instance_variable_set(:@path, 'test.pp')
+      linter.instance_variable_set(:@manifest, 'class test { }')
+    end
+
+    context 'when file supports fixes and has no syntax errors' do
+      it 'returns true' do
+        expect(linter.should_write_fixes?).to be true
+      end
+    end
+
+    context 'when file has syntax errors' do
+      it 'returns false' do
+        linter.instance_variable_set(:@problems, [{ check: :syntax }])
+        expect(linter.should_write_fixes?).to be false
+      end
+    end
+
+    context 'when file type does not support fixes' do
+      it 'returns false' do
+        linter.instance_variable_set(:@path, 'test.yaml')
+        expect(linter.should_write_fixes?).to be false
+      end
+    end
+  end
 end