Merge pull request #24 from ethanis/main

Address command injection concerns

.github/workflows/issue_ops.yml

@@ -8,6 +8,10 @@ permissions:
   contents: read
   issues: write
 
+defaults:
+  run:
+    shell: bash
+
 env:
   GITHUB_INSTANCE_URL: ${{ secrets.GITHUB_INSTANCE_URL || 'https://github.com' }}
   GITHUB_ACCESS_TOKEN: ${{ secrets.GH_ACCESS_TOKEN }}
@@ -31,7 +35,6 @@ jobs:
       log-filename: ${{ steps.logs.outputs.filename }}
     steps:
       - name: Install GitHub Actions Importer
-        shell: bash
         run: |
           gh actions-importer version || gh extension install github/gh-actions-importer
           gh actions-importer update
@@ -55,7 +58,9 @@ jobs:
         # env:
         #     ImageOS: ubuntu22
       - name: Install dependencies
-        run: bundle install --without development
+        run: bundle install
+        env:
+          BUNDLE_WITHOUT: development
       - name: Prepare arguments
         id: prepare
         env:

lib/concerns/environment_writer.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+module EnvironmentWriter
+  def set_output(name, value)
+    modify_env("GITHUB_OUTPUT", name, value)
+  end
+
+  def set_environment(name, value)
+    modify_env("GITHUB_ENV", name, value)
+  end
+
+  private
+
+  def modify_env(file, name, value)
+    return if value.nil?
+
+    raise ArgumentError, "Invalid input: #{name}=#{value}" if value.include?("\n")
+
+    File.open(ENV[file], "a") do |f|
+      f.puts "#{name}=#{value}"
+    end
+  end
+end

lib/concerns/output_writer.rb

@@ -1,11 +0,0 @@
-# frozen_string_literal: true
-
-module OutputWriter
-  def set_output(name, value)
-    return if value.nil?
-
-    File.open(ENV["GITHUB_OUTPUT"], "a") do |f|
-      f.puts "#{name}=#{value}"
-    end
-  end
-end

lib/models/arguments.rb

@@ -1,9 +1,9 @@
 # frozen_string_literal: true
 
-require_relative "../concerns/output_writer"
+require_relative "../concerns/environment_writer"
 
 class Arguments
-  include OutputWriter
+  include EnvironmentWriter
 
   def initialize(provider, command, issue_content)
     @args = argument_class(provider, command, issue_content)
@@ -24,12 +24,26 @@ class Arguments
 
     arguments.concat(["--custom-transformers", *@custom_transformers]) if @custom_transformers.length.positive?
 
+    rng = Random.new
+    variable_names = Set.new
+
     set_output(
       "args",
       arguments.map do |a|
-        next a unless a.include?(" ")
+        value = a.include?(" ") ? a.inspect : a
 
-        a.inspect
+        unless value.start_with?("--")
+          name = "variable_#{rng.rand(1000..9999)}"
+          name = "variable_#{rng.rand(1000..9999)}" while variable_names.include?(name)
+
+          variable_names.add(name)
+
+          set_environment(name, value)
+
+          value = "$#{name}"
+        end
+
+        value
       end.join(" ")
     )
   end

lib/models/command.rb

@@ -1,10 +1,10 @@
 # frozen_string_literal: true
 
 require "active_support/core_ext/string"
-require_relative "../concerns/output_writer"
+require_relative "../concerns/environment_writer"
 
 class Command
-  include OutputWriter
+  include EnvironmentWriter
 
   VALID_COMMANDS = %w[audit migrate dry-run].freeze
 

lib/models/provider.rb

@@ -7,7 +7,7 @@ require_rel "./jenkins/**/*.rb"
 require_rel "./travis_ci/**/*.rb"
 
 class Provider
-  include OutputWriter
+  include EnvironmentWriter
 
   PROVIDER_MAP = {
     "azure-devops" => ::AzureDevops,

spec/concerns/environment_writer_spec.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+RSpec.describe EnvironmentWriter do
+  let(:test_class) do
+    class TestClass
+      include EnvironmentWriter
+    end
+
+    TestClass.new
+  end
+
+  describe "#set_output" do
+    let(:name) { "var_name" }
+    let(:value) { "var_value" }
+    let(:output) { "#{name}=#{value}" }
+
+    subject { test_class.set_output(name, value) }
+
+    it { expect { subject }.to change { File.readlines(ENV["GITHUB_OUTPUT"], chomp: true).last }.to(/#{output}/) }
+
+    context "when value is nil" do
+      let(:value) { nil }
+
+      it { expect { subject }.not_to change { File.read(ENV["GITHUB_OUTPUT"], chomp: true).last } }
+    end
+
+    context "when value contains a newline" do
+      let(:value) { "var_value\n" }
+
+      it { expect { subject }.to raise_error(ArgumentError) }
+    end
+  end
+
+  describe "#set_environment" do
+    let(:name) { "env_var_name" }
+    let(:value) { "env_var_value" }
+    let(:output) { "#{name}=#{value}" }
+
+    subject { test_class.set_environment(name, value) }
+
+    it { expect { subject }.to change { File.readlines(ENV["GITHUB_ENV"], chomp: true).last }.to(/#{output}/) }
+
+    context "when value is nil" do
+      let(:value) { nil }
+
+      it { expect { subject }.not_to change { File.read(ENV["GITHUB_ENV"], chomp: true).last } }
+    end
+
+    context "when value contains a newline" do
+      let(:value) { "env_var_value\n" }
+
+      it { expect { subject }.to raise_error(ArgumentError) }
+    end
+  end
+end

spec/concerns/output_writer_spec.rb

@@ -1,27 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.describe OutputWriter do
-  let(:test_class) do
-    class TestClass
-      include OutputWriter
-    end
-
-    TestClass.new
-  end
-
-  describe "#set_output" do
-    let(:name) { "var_name" }
-    let(:value) { "var_value" }
-    let(:output) { "#{name}=#{value}" }
-
-    subject { test_class.set_output(name, value) }
-
-    it { expect { subject }.to change { File.readlines(ENV["GITHUB_OUTPUT"], chomp: true).last }.to(/#{output}/) }
-
-    context "when value is nil" do
-      let(:value) { nil }
-
-      it { expect { subject }.not_to change { File.read(ENV["GITHUB_OUTPUT"], chomp: true).last } }
-    end
-  end
-end

spec/models/arguments_spec.rb

@@ -54,7 +54,8 @@ RSpec.describe Arguments do
       let(:output) { ["--option", "value"] }
 
       it "writes an output variable" do
-        expect(arguments).to receive(:set_output).with("args", "--option value")
+        expect(arguments).to receive(:set_output).with("args", /--option \$variable_\d{4}/)
+        expect(arguments).to receive(:set_environment).with(/variable_\d{4}/, "value")
         subject
       end
     end
@@ -63,7 +64,8 @@ RSpec.describe Arguments do
       let(:output) { ["--option", "some value"] }
 
       it "writes an output variable" do
-        expect(arguments).to receive(:set_output).with("args", "--option \"some value\"")
+        expect(arguments).to receive(:set_output).with("args", /--option \$variable_\d{4}/)
+        expect(arguments).to receive(:set_environment).with(/variable_\d{4}/, "\"some value\"")
         subject
       end
     end
@@ -73,7 +75,10 @@ RSpec.describe Arguments do
       let(:options) { { "custom-transformers" => "transformers/**/*.rb" } }
 
       it "writes an output variable" do
-        expect(arguments).to receive(:set_output).with("args", "--option value --custom-transformers transformers/**/*.rb")
+        expect(arguments).to receive(:set_output).with("args", /--option \$variable_\d{4} --custom-transformers \$variable_\d{4}/)
+        ["value", "transformers/**/*.rb"].each do |value|
+          expect(arguments).to receive(:set_environment).with(/variable_\d{4}/, value)
+        end
         subject
       end
     end
@@ -87,7 +92,11 @@ RSpec.describe Arguments do
       end
 
       it "writes an output variable" do
-        expect(arguments).to receive(:set_output).with("args", "--option value --custom-transformers transformers/jenkins/transformers.rb transformers/all.rb")
+        expect(arguments).to receive(:set_output).with("args", /--option \$variable_\d{4} --custom-transformers \$variable_\d{4} \$variable_\d{4}/)
+        ["value", *files].each do |value|
+          expect(arguments).to receive(:set_environment).with(/variable_\d{4}/, value)
+        end
+
         subject
       end
     end

spec/spec_helper.rb

@@ -29,6 +29,7 @@ RSpec.configure do |config|
     Dir.mkdir("tmp") unless Dir.exist?("tmp")
     FileUtils.touch "tmp/test.txt"
     ENV["GITHUB_OUTPUT"] = "tmp/test.txt"
+    ENV["GITHUB_ENV"] = "tmp/test.txt"
   end
 
   config.after(:suite) do