Address review: close the glob validation gap and harden the object reads

- validate_dir! runs the glob branch's directory through the same
  component walk as plain accesses: git ls-tree on a symlinked
  directory or a regular file returns an empty listing with exit 0,
  which would have silently diverged from Dir[] on a checkout instead
  of falling back.
- Shadow mode rescues FallbackRequired ahead of the catch-all so the
  fallback reason taxonomy survives -- shadow is what produces the
  data the rollout gate is evaluated on. Events also carry the active
  mode.
- git_read clears the GIT_* repository-selection environment variables
  it no longer inherits Shipit::Command's scrubbed BASE_ENV protection
  against, and takes pathspecs via :(literal) so directory names with
  glob characters are read verbatim.
- git_object evaluation now guards core.autocrlf/core.attributesfile
  at runtime instead of relying on a point-in-time preflight: those
  convert checkouts without leaving a trace in the tree. autocrlf
  false/input are safe; core.eol alone is inert without text
  attributes, which the .gitattributes guards already cover.

Author: aqeelvn

app/models/shipit/deploy_spec/git_object_file_system.rb

@@ -18,6 +18,12 @@ class DeploySpec
     # escaping the repository, runaway inherit_from chains), FallbackRequired
     # is raised and the caller is expected to fall back to the checkout-based
     # code path.
+    #
+    # Unlike the checkout path (which clones a snapshot first), reads target
+    # the stack's live git cache. A concurrent ClearGitCacheJob or git gc
+    # degrades to a command failure, which the caller treats as a fallback;
+    # since every read is pinned to a single commit sha there is no torn-read
+    # hazard.
     class GitObjectFileSystem < FileSystem
       class FallbackRequired < StandardError
         attr_reader :reason, :detail
@@ -50,6 +56,7 @@ def file(path, root: false)
         pathname = super
         if path.to_s.match?(GLOB_CHARS)
           dir = repo_rel(pathname.dirname)
+          validate_dir!(dir)
           pattern = File.basename(path.to_s)
           entries(dir).each_key do |name|
             next unless File.fnmatch(pattern, name)
@@ -149,6 +156,26 @@ def blob_mode(key)
         (parts.last && entries(prefix)[parts.last]) || :absent
       end
 
+      # The glob branch lists a directory without materializing a file, so the
+      # directory's own path components must be validated explicitly:
+      # `git ls-tree <sha> -- '<dir>/'` on a symlinked directory or on a
+      # regular file returns an empty listing with exit 0, which would
+      # silently diverge from Dir[] on a checkout (which follows symlinks).
+      def validate_dir!(dir)
+        return if dir.empty?
+
+        case blob_mode(dir)
+        when TREE_MODE, :absent
+          nil
+        when SYMLINK_MODE
+          raise FallbackRequired.new(:symlink, dir)
+        when GITLINK_MODE
+          raise FallbackRequired.new(:submodule, dir)
+        else
+          raise FallbackRequired.new(:file_in_path, dir)
+        end
+      end
+
       # Memoized directory listings, keyed by canonical repo-relative dir
       # ("" = root). Any listed ancestor containing a .gitattributes entry
       # forces a fallback: a checkout applies eol/text/smudge attributes,

lib/shipit/stack_commands.rb

@@ -107,6 +107,10 @@ def cacheable_deploy_spec(commit: nil)
                         .reject { |key| old_config[key] == new_config[key] }
             notify_checkout_less(:shadow_mismatch, detail: differing.first(5).join(','))
           end
+        rescue DeploySpec::GitObjectFileSystem::FallbackRequired => e
+          # Preserve the reason taxonomy: shadow mode is what produces the
+          # fallback-rate data the rollout gate is evaluated on.
+          notify_checkout_less(:fallback, reason: e.reason, detail: e.detail)
         rescue StandardError => e
           notify_checkout_less(:fallback, reason: :shadow_error, detail: "#{e.class}: #{e.message}")
         end
@@ -126,7 +130,9 @@ def git_read_object(sha, repo_rel_path)
     # children rather than the directory entry itself.
     def git_ls_dir(sha, repo_rel_dir)
       args = ['ls-tree', '-z', sha]
-      args += ['--', "#{repo_rel_dir}/"] unless repo_rel_dir.empty?
+      # :(literal) disables pathspec magic so directory names containing
+      # glob characters or a leading ':' are taken verbatim.
+      args += ['--', ":(literal)#{repo_rel_dir}/"] unless repo_rel_dir.empty?
       output = git_read(*args)
       output.split("\0").each_with_object({}) do |record, listing|
         next if record.empty?
@@ -211,6 +217,8 @@ def checkout_cacheable_deploy_spec(commit)
     end
 
     def git_object_cacheable_deploy_spec(commit)
+      ensure_no_checkout_conversion_config!
+
       unless fetched?(commit).tap(&:run).success?
         @stack.acquire_git_cache_lock do
           fetch.run! unless fetched?(commit).tap(&:run).success?
@@ -222,6 +230,24 @@ def git_object_cacheable_deploy_spec(commit)
       end
     end
 
+    # A checkout applies core.autocrlf conversions and attributes from a
+    # core.attributesfile, neither of which leaves a trace in the tree, while
+    # `git cat-file` emits raw bytes. Guarding at runtime (rather than a
+    # point-in-time preflight) keeps the guarantee if a base image change
+    # introduces such config later. autocrlf=false and autocrlf=input do not
+    # convert on checkout; core.eol alone is inert without text attributes,
+    # which are covered by the in-tree .gitattributes guard and the
+    # attributesfile check here.
+    def ensure_no_checkout_conversion_config!
+      output = git_read('config', '--get-regexp', '^core\.(autocrlf|attributesfile)$', allow_failure: true)
+      output.split("\n").each do |line|
+        key, value = line.split(' ', 2)
+        next if key == 'core.autocrlf' && %w[false input].include?(value.to_s.downcase)
+
+        raise DeploySpec::GitObjectFileSystem::FallbackRequired.new(:git_config, line)
+      end
+    end
+
     def normalize_spec_config(value, root)
       case value
       when String then value.gsub(root, '$SPEC_ROOT')
@@ -231,15 +257,35 @@ def normalize_spec_config(value, root)
       end
     end
 
-    def git_read(*args)
-      output, error, status = Open3.capture3('git', *args, chdir: @stack.git_path.to_s, binmode: true)
-      raise Command::Failed.new("git #{args.first} failed: #{error.strip}", status.exitstatus) unless status.success?
+    # Environment variables that select which repository/object store git
+    # operates on. Cleared explicitly: git_read bypasses Shipit::Command (and
+    # therefore its scrubbed BASE_ENV), and any of these set on the worker
+    # process would silently override chdir:.
+    GIT_REPO_SELECTION_ENV = {
+      'GIT_DIR' => nil,
+      'GIT_WORK_TREE' => nil,
+      'GIT_INDEX_FILE' => nil,
+      'GIT_OBJECT_DIRECTORY' => nil,
+      'GIT_ALTERNATE_OBJECT_DIRECTORIES' => nil,
+      'GIT_COMMON_DIR' => nil
+    }.freeze
+
+    def git_read(*args, allow_failure: false)
+      output, error, status = Open3.capture3(
+        GIT_REPO_SELECTION_ENV, 'git', *args,
+        chdir: @stack.git_path.to_s, binmode: true
+      )
+      unless status.success?
+        return "" if allow_failure
+
+        raise Command::Failed.new("git #{args.first} failed: #{error.strip}", status.exitstatus)
+      end
 
       output
     end
 
     def notify_checkout_less(event, reason: nil, detail: nil)
-      payload = { stack_id: @stack.id, event:, reason:, detail: }.compact
+      payload = { stack_id: @stack.id, mode: Shipit.checkout_less_deploy_spec, event:, reason:, detail: }.compact
       ActiveSupport::Notifications.instrument('checkout_less_deploy_spec.shipit', payload)
       if event == :hit
         Rails.logger.debug { "[checkout_less_deploy_spec] hit stack=#{@stack.id}" }

test/jobs/cache_deploy_spec_job_test.rb

@@ -45,17 +45,17 @@ class CacheDeploySpecJobTest < ActiveSupport::TestCase
       @stack.stubs(:commits).returns(stub(reachable:))
       @stack.stubs(:update!) # side-effect callbacks are irrelevant to this test
 
-      StackCommands.any_instance.expects(:with_temporary_working_directory)
-                   .with(commit: @last_commit, recursive: false).yields(Pathname(Dir.tmpdir))
+      StackCommands.any_instance.expects(:cacheable_deploy_spec)
+                   .with(commit: @last_commit).returns(DeploySpec.new({}))
 
       assert_enqueued_with(job: CacheDeploySpecJob, args: [@stack]) do
         @job.perform(@stack)
       end
     end
 
     test "#perform does not re-enqueue itself when the head is unchanged" do
-      StackCommands.any_instance.expects(:with_temporary_working_directory)
-                   .with(commit: @last_commit, recursive: false).yields(Pathname(Dir.tmpdir))
+      StackCommands.any_instance.expects(:cacheable_deploy_spec)
+                   .with(commit: @last_commit).returns(DeploySpec.new({}))
 
       assert_no_enqueued_jobs(only: CacheDeploySpecJob) do
         @job.perform(@stack)

test/models/shipit/deploy_spec/git_object_file_system_test.rb

@@ -181,6 +181,42 @@ class GitObjectFileSystemTest < ActiveSupport::TestCase
         assert_equal :gitattributes, error.reason
       end
 
+      test "fallback: glob listing under a symlinked directory" do
+        repo, sha = make_repo('realdir/x.gemspec' => "Gem::Specification.new\n") do |dir|
+          File.symlink('realdir', dir.join('linkdir'))
+        end
+        commands = commands_for(repo)
+
+        Dir.mktmpdir do |dir|
+          fs = GitObjectFileSystem.new(dir, @stack, commands:, sha:)
+          error = assert_raises(GitObjectFileSystem::FallbackRequired) do
+            fs.file('linkdir/*.gemspec', root: true)
+          end
+          assert_equal :symlink, error.reason
+        end
+      end
+
+      test "fallback: repository-level checkout conversion config" do
+        repo, _sha = make_repo('shipit.production.yml' => "deploy:\n  override:\n    - echo x\n")
+        git(repo, 'config', 'core.autocrlf', 'true')
+        commands = commands_for(repo)
+
+        error = assert_raises(GitObjectFileSystem::FallbackRequired) do
+          commands.send(:ensure_no_checkout_conversion_config!)
+        end
+        assert_equal :git_config, error.reason
+      end
+
+      test "checkout conversion config guard tolerates safe autocrlf values" do
+        repo, _sha = make_repo('shipit.production.yml' => "deploy:\n  override:\n    - echo x\n")
+        git(repo, 'config', 'core.autocrlf', 'input')
+        commands = commands_for(repo)
+
+        assert_nothing_raised do
+          commands.send(:ensure_no_checkout_conversion_config!)
+        end
+      end
+
       # --- Idempotency ---
 
       test "repeat access to the same file reads the object database once" do

test/unit/cacheable_deploy_spec_test.rb

@@ -101,6 +101,25 @@ class CacheableDeploySpecTest < ActiveSupport::TestCase
       assert_includes @events.first[:detail], 'deploy'
     end
 
+    test "shadow mode preserves the fallback reason taxonomy" do
+      Shipit.checkout_less_deploy_spec = :shadow
+      error = DeploySpec::GitObjectFileSystem::FallbackRequired.new(:gitattributes, 'app')
+      @commands.expects(:checkout_cacheable_deploy_spec).with(@commit).returns([@old_spec, '/tmp/old'])
+      @commands.expects(:git_object_cacheable_deploy_spec).raises(error)
+
+      assert_equal @old_spec, @commands.cacheable_deploy_spec(commit: @commit)
+      assert_equal :gitattributes, @events.first[:reason]
+      assert_equal 'app', @events.first[:detail]
+    end
+
+    test "events carry the active mode" do
+      Shipit.checkout_less_deploy_spec = :enabled
+      @commands.expects(:git_object_cacheable_deploy_spec).with(@commit).returns([@new_spec, '/tmp/new'])
+
+      @commands.cacheable_deploy_spec(commit: @commit)
+      assert_equal :enabled, @events.first[:mode]
+    end
+
     test "shadow mode never propagates new-path exceptions" do
       Shipit.checkout_less_deploy_spec = :shadow
       @commands.expects(:checkout_cacheable_deploy_spec).with(@commit).returns([@old_spec, '/tmp/old'])