fix: address remaining PR C review feedback (security, robustness, docs)

Post-revision review surfaced 7 new actionable items. All fixed.

Code:

1. PREVIOUS_BUNDLE_HASHES path-traversal (claude): env values are used
   as directory names under the renderer cache; a malicious or mistyped
   value like "../../../etc" would escape the cache dir. Now reject
   anything that doesn't match /\A[A-Za-z0-9_\-.]+\z/ with a warning.

2. node_renderer? guard in publish_current_bundle_if_configured
   (claude): returns early when ExecJS is configured rather than
   crashing on NodeRenderingPool.server_bundle_hash. Mirrors the
   guard used by PreSeedRendererCache.

5. Top-level rescue in publish_current_bundle_if_configured (cursor
   Medium): collect_assets / server_bundle_hash / rsc_bundle_js_file_path
   can all raise from the setup code that ran before the per-upload
   rescue. Extracted publish_bundles helper and wrapped the whole
   publication path with StandardError rescue so a failure degrades
   next-deploy seeding but doesn't fail this deploy's assets:precompile.

7. Partial-staging cleanup on asset failure (codex P1): previously if
   stage_file succeeded on the bundle but failed on an asset, the
   cache held a bundle file without its companion manifests. The
   renderer would then find the bundle and serve without those
   manifests, causing hydration breakage instead of the clean 410
   retry fallback. Now rm_rf the hash directory on any failure inside
   seed_previous_hash — restores the 410-retry safety net.

Docs:

3. Control Plane fetch example: replaced Dir[tmp/*.json] with an
   explicit allowlist so stray JSON metadata in the image layer
   isn't silently picked up.

4. S3 client mutex: @s3 ||= is not thread-safe under concurrent
   callers (parallel workers running precompile hooks). Example now
   uses a Mutex-synchronized memoizer with a doc comment.

6. S3 manifest RETENTION alignment: write and read both trim to
   RETENTION now, so the persisted manifest matches what discovery
   returns. Added a comment explaining the intent.

Tests: 40 rolling-deploy + pre-seed + shim specs (was 38, +2 for
path-traversal reject and partial-staging cleanup). Rubocop clean.

Refs: #3122, #3173

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: justin808

docs/pro/rolling-deploy-adapters.md

@@ -158,8 +158,14 @@ class S3RollingDeployAdapter
 
   # -- helpers (private by convention) --
 
+  S3_CLIENT_MUTEX = Mutex.new
+  private_constant :S3_CLIENT_MUTEX
+
+  # Thread-safe memoization. Without the mutex, concurrent callers (e.g. parallel
+  # Sidekiq workers running precompile hooks) could each create a separate client;
+  # only the last survives, and the earlier ones are silently discarded after use.
   def self.s3
-    @s3 ||= Aws::S3::Client.new
+    S3_CLIENT_MUTEX.synchronize { @s3 ||= Aws::S3::Client.new }
   end
 
   def self.download_to(dir, name, hash)
@@ -181,10 +187,13 @@ class S3RollingDeployAdapter
   def self.update_manifest!(hash)
     hashes = previous_bundle_hashes
     hashes << hash unless hashes.include?(hash)
+    # Write and read both trim to RETENTION so the persisted manifest matches
+    # what discovery returns. If you want a soft buffer of historical entries,
+    # increase RETENTION rather than diverging these two trim lengths.
     s3.put_object(
       bucket: bucket,
       key: MANIFEST_KEY,
-      body: JSON.generate(hashes: hashes.last(RETENTION + 2))
+      body: JSON.generate(hashes: hashes.last(RETENTION))
     )
   end
 end
@@ -233,7 +242,12 @@ class ControlPlaneRollingDeployAdapter
     bundle = Dir[File.join(tmp, "*.js")].first
     return nil unless bundle
 
-    { bundle: bundle, assets: Dir[File.join(tmp, "*.json")] }
+    # Explicit allowlist — don't sweep up unrelated JSON files that may be
+    # bundled in the image layer (lock files, health-check payloads, etc.).
+    asset_names = %w[loadable-stats.json react-client-manifest.json react-server-client-manifest.json]
+    assets = asset_names.map { |name| File.join(tmp, name) }.select { |path| File.exist?(path) }
+
+    { bundle: bundle, assets: assets }
   end
 
   def self.upload(_hash, bundle:, assets:)

react_on_rails_pro/lib/react_on_rails_pro/assets_precompile.rb

@@ -83,8 +83,22 @@ def self.call
     def self.publish_current_bundle_if_configured
       adapter = ReactOnRailsPro.configuration.rolling_deploy_adapter
       return if adapter.nil?
+      # NodeRendererPool.server_bundle_hash is only available under the NodeRenderer
+      # renderer mode. With ExecJS, skip publication rather than crash.
+      return unless ReactOnRailsPro.configuration.node_renderer?
       return if Rails.env.development? || Rails.env.test?
 
+      publish_bundles(adapter)
+    rescue StandardError => e
+      # Outer rescue catches anything raised by the setup-side calls below
+      # (collect_assets, server_bundle_hash, rsc_bundle_js_file_path). Per the
+      # rolling-deploy contract, a failed upload must degrade the next deploy's
+      # seeding — not fail *this* deploy's assets:precompile.
+      warn "[ReactOnRailsPro] rolling_deploy_adapter publication failed: #{e.class}: #{e.message}. " \
+           "Next deploy's rolling-deploy seeding may degrade; precompile continuing."
+    end
+
+    def self.publish_bundles(adapter)
       pool = ReactOnRailsPro::ServerRenderingPool::NodeRenderingPool
       assets = ReactOnRailsPro::RendererCacheHelpers.collect_assets.map(&:to_s)
 

react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_cache_stager.rb

@@ -58,8 +58,21 @@ def self.handle_missing_adapter
     end
     private_class_method :handle_missing_adapter
 
+    # Bundle hashes are used as directory names under the renderer cache path
+    # (<cache>/<hash>/<hash>.js). Values coming from config.rolling_deploy_adapter
+    # are trusted, but PREVIOUS_BUNDLE_HASHES comes from the environment and
+    # could contain path separators (e.g. "../../etc") that would escape the
+    # cache directory. Reject anything that isn't a safe hash slug.
+    SAFE_HASH_PATTERN = /\A[A-Za-z0-9_\-.]+\z/
+
     def self.resolve_previous_hashes(adapter, current_hashes)
       explicit = ENV["PREVIOUS_BUNDLE_HASHES"].to_s.split(",").map(&:strip).reject(&:empty?)
+      invalid = explicit.grep_v(SAFE_HASH_PATTERN)
+      if invalid.any?
+        warn "[ReactOnRailsPro] PREVIOUS_BUNDLE_HASHES contains invalid hash values (rejected): " \
+             "#{invalid.inspect}. Hashes must match /#{SAFE_HASH_PATTERN.source}/ to prevent path traversal."
+        explicit -= invalid
+      end
       hashes = explicit.any? ? explicit : fetch_hashes_from_adapter(adapter)
       # Deduplicate against the hashes we just staged so we don't re-fetch the current build.
       hashes - Array(current_hashes).map(&:to_s)
@@ -86,8 +99,14 @@ def self.seed_previous_hash(adapter, hash, cache_dir, mode)
         stage_file(asset_path, File.join(bundle_dir, File.basename(asset_path)), mode)
       end
     rescue StandardError => e
+      # Roll back the entire hash directory. Leaving the bundle file in place
+      # without its companion assets would cause the renderer to find the bundle
+      # (skipping its 410 path) and emit HTML referencing chunks from a manifest
+      # that never got staged — producing hydration failures instead of the clean
+      # 410-retry fallback that we rely on for degradation.
+      FileUtils.rm_rf(File.join(cache_dir, hash))
       warn "[ReactOnRailsPro] Failed to seed previous bundle hash #{hash}: #{e.class}: #{e.message}. " \
-           "Runtime 410-retry path remains available as fallback."
+           "Rolled back partially-staged files. Runtime 410-retry remains the fallback."
     end
     private_class_method :seed_previous_hash
 

react_on_rails_pro/spec/dummy/spec/rolling_deploy_cache_stager_spec.rb

@@ -146,6 +146,43 @@ def source_file(name, contents: "// #{name}")
     end
   end
 
+  context "when PREVIOUS_BUNDLE_HASHES contains an unsafe path-traversal value" do
+    let(:src_bundle) { source_file("bundle-ok.js") }
+
+    before do
+      ENV["PREVIOUS_BUNDLE_HASHES"] = "../../../etc,safe-hash"
+      allow(adapter).to receive(:previous_bundle_hashes)
+      allow(adapter).to receive(:fetch).with("safe-hash").and_return(bundle: src_bundle, assets: [])
+    end
+
+    it "rejects the unsafe value with a warning and stages only the safe one" do
+      expect { described_class.call(cache_dir: cache_dir, current_hashes: [], mode: :copy) }
+        .to output(/invalid hash values \(rejected\).*etc/m).to_stderr
+
+      expect(adapter).not_to have_received(:fetch).with("../../../etc")
+      expect(File.exist?(File.join(cache_dir, "safe-hash", "safe-hash.js"))).to be(true)
+    end
+  end
+
+  context "when an asset stage fails mid-way" do
+    let(:src_bundle) { source_file("bundle-partial.js") }
+
+    before do
+      allow(adapter).to receive_messages(previous_bundle_hashes: ["abc123"])
+      allow(adapter).to receive(:fetch).with("abc123").and_return(
+        bundle: src_bundle,
+        assets: ["/nonexistent/loadable-stats.json"]
+      )
+    end
+
+    it "rolls back the entire hash directory so the renderer sees 410, not a bundle without manifests" do
+      described_class.call(cache_dir: cache_dir, current_hashes: [], mode: :copy)
+
+      bundle_dir = File.join(cache_dir, "abc123")
+      expect(File.exist?(bundle_dir)).to be(false)
+    end
+  end
+
   context "when adapter returns payload without :bundle" do
     before do
       allow(adapter).to receive_messages(previous_bundle_hashes: ["bad-hash"])