[rubygems/rubygems] Quote Etag in `If-None-Match` header of compact index request

https://github.com/rubygems/rubygems/commit/d26bcd7551

4 files changed, 26 insertions, 23 deletions

diff --git a/lib/bundler/compact_index_client/updater.rb b/lib/bundler/compact_index_client/updater.rb
index 84a606dc34..36f6b81db8 100644
--- a/lib/bundler/compact_index_client/updater.rb
+++ b/lib/bundler/compact_index_client/updater.rb
@@ -42,7 +42,7 @@ module Bundler
           else
             file.write(response.body)
           end
-          CacheFile.write(etag_path, etag(response))
+          CacheFile.write(etag_path, etag_from_response(response))
           true
         end
       end
@@ -53,13 +53,13 @@ module Bundler
         response = @fetcher.call(remote_path, request_headers(etag))
         return true if response.is_a?(Gem::Net::HTTPNotModified)
         CacheFile.write(local_path, response.body, parse_digests(response))
-        CacheFile.write(etag_path, etag(response))
+        CacheFile.write(etag_path, etag_from_response(response))
       end
 
       def request_headers(etag, range_start = nil)
         headers = {}
         headers["Range"] = "bytes=#{range_start}-" if range_start
-        headers["If-None-Match"] = etag if etag
+        headers["If-None-Match"] = %("#{etag}") if etag
         headers
       end
 
@@ -77,7 +77,7 @@ module Bundler
         etag
       end
 
-      def etag(response)
+      def etag_from_response(response)
         return unless response["ETag"]
         etag = response["ETag"].delete_prefix("W/")
         return if etag.delete_prefix!('"') && !etag.delete_suffix!('"')
diff --git a/spec/bundler/bundler/compact_index_client/updater_spec.rb b/spec/bundler/bundler/compact_index_client/updater_spec.rb
index 51b838d2d2..6eed88ca9e 100644
--- a/spec/bundler/bundler/compact_index_client/updater_spec.rb
+++ b/spec/bundler/bundler/compact_index_client/updater_spec.rb
@@ -21,7 +21,7 @@ RSpec.describe Bundler::CompactIndexClient::Updater do
     before do
       allow(response).to receive(:[]).with("Repr-Digest") { nil }
       allow(response).to receive(:[]).with("Digest") { nil }
-      allow(response).to receive(:[]).with("ETag") { "thisisanetag" }
+      allow(response).to receive(:[]).with("ETag") { '"thisisanetag"' }
     end
 
     it "downloads the file without attempting append" do
@@ -57,7 +57,7 @@ RSpec.describe Bundler::CompactIndexClient::Updater do
 
       let(:headers) do
         {
-          "If-None-Match" => "LocalEtag",
+          "If-None-Match" => '"LocalEtag"',
           "Range" => "bytes=2-",
         }
       end
@@ -76,7 +76,7 @@ RSpec.describe Bundler::CompactIndexClient::Updater do
       it "appends the file if etags do not match" do
         expect(fetcher).to receive(:call).once.with(remote_path, headers).and_return(response)
         allow(response).to receive(:[]).with("Repr-Digest") { "sha-256=:#{digest}:" }
-        allow(response).to receive(:[]).with("ETag") { "NewEtag" }
+        allow(response).to receive(:[]).with("ETag") { '"NewEtag"' }
         allow(response).to receive(:is_a?).with(Gem::Net::HTTPPartialContent) { true }
         allow(response).to receive(:is_a?).with(Gem::Net::HTTPNotModified) { false }
         allow(response).to receive(:body) { "c123" }
@@ -90,7 +90,7 @@ RSpec.describe Bundler::CompactIndexClient::Updater do
       it "replaces the file if response ignores range" do
         expect(fetcher).to receive(:call).once.with(remote_path, headers).and_return(response)
         allow(response).to receive(:[]).with("Repr-Digest") { "sha-256=:#{digest}:" }
-        allow(response).to receive(:[]).with("ETag") { "NewEtag" }
+        allow(response).to receive(:[]).with("ETag") { '"NewEtag"' }
         allow(response).to receive(:body) { full_body }
 
         updater.update(remote_path, local_path, etag_path)
@@ -107,8 +107,8 @@ RSpec.describe Bundler::CompactIndexClient::Updater do
 
         full_response = double(:full_response, body: full_body, is_a?: false)
         allow(full_response).to receive(:[]).with("Repr-Digest") { "sha-256=:#{digest}:" }
-        allow(full_response).to receive(:[]).with("ETag") { "NewEtag" }
-        expect(fetcher).to receive(:call).once.with(remote_path, { "If-None-Match" => "LocalEtag" }).and_return(full_response)
+        allow(full_response).to receive(:[]).with("ETag") { '"NewEtag"' }
+        expect(fetcher).to receive(:call).once.with(remote_path, { "If-None-Match" => '"LocalEtag"' }).and_return(full_response)
 
         updater.update(remote_path, local_path, etag_path)
 
@@ -123,7 +123,7 @@ RSpec.describe Bundler::CompactIndexClient::Updater do
           "Range" => "bytes=2-",
           # This MD5 feature should be deleted after sufficient time has passed since release.
           # From then on, requests that still don't have a saved etag will be made without this header.
-          "If-None-Match" => Digest::MD5.hexdigest(local_body),
+          "If-None-Match" => %("#{Digest::MD5.hexdigest(local_body)}"),
         }
       end
 
@@ -135,13 +135,13 @@ RSpec.describe Bundler::CompactIndexClient::Updater do
         updater.update(remote_path, local_path, etag_path)
 
         expect(local_path.read).to eq("abc")
-        expect(etag_path.read).to eq(headers["If-None-Match"])
+        expect(%("#{etag_path.read}")).to eq(headers["If-None-Match"])
       end
 
       it "appends the file" do
         expect(fetcher).to receive(:call).once.with(remote_path, headers).and_return(response)
         allow(response).to receive(:[]).with("Repr-Digest") { "sha-256=:#{digest}:" }
-        allow(response).to receive(:[]).with("ETag") { "OpaqueEtag" }
+        allow(response).to receive(:[]).with("ETag") { '"OpaqueEtag"' }
         allow(response).to receive(:is_a?).with(Gem::Net::HTTPPartialContent) { true }
         allow(response).to receive(:is_a?).with(Gem::Net::HTTPNotModified) { false }
         allow(response).to receive(:body) { "c123" }
@@ -156,7 +156,7 @@ RSpec.describe Bundler::CompactIndexClient::Updater do
         expect(fetcher).to receive(:call).once.with(remote_path, headers).and_return(response)
         allow(response).to receive(:[]).with("Repr-Digest") { nil }
         allow(response).to receive(:[]).with("Digest") { nil }
-        allow(response).to receive(:[]).with("ETag") { "OpaqueEtag" }
+        allow(response).to receive(:[]).with("ETag") { '"OpaqueEtag"' }
         allow(response).to receive(:is_a?).with(Gem::Net::HTTPPartialContent) { false }
         allow(response).to receive(:is_a?).with(Gem::Net::HTTPNotModified) { false }
         allow(response).to receive(:body) { full_body }
@@ -180,8 +180,8 @@ RSpec.describe Bundler::CompactIndexClient::Updater do
 
         full_response = double(:full_response, body: full_body, is_a?: false)
         allow(full_response).to receive(:[]).with("Repr-Digest") { "sha-256=:#{digest}:" }
-        allow(full_response).to receive(:[]).with("ETag") { "NewEtag" }
-        expect(fetcher).to receive(:call).once.with(remote_path, { "If-None-Match" => "LocalEtag" }).and_return(full_response)
+        allow(full_response).to receive(:[]).with("ETag") { '"NewEtag"' }
+        expect(fetcher).to receive(:call).once.with(remote_path, { "If-None-Match" => '"LocalEtag"' }).and_return(full_response)
 
         updater.update(remote_path, local_path, etag_path)
 
diff --git a/spec/bundler/install/gems/compact_index_spec.rb b/spec/bundler/install/gems/compact_index_spec.rb
index e3c891e4c1..1375d8cab1 100644
--- a/spec/bundler/install/gems/compact_index_spec.rb
+++ b/spec/bundler/install/gems/compact_index_spec.rb
@@ -924,15 +924,19 @@ RSpec.describe "compact index api" do
       gem 'rack', '0.9.1'
     G
 
-    rake_info_path = File.join(Bundler.rubygems.user_home, ".bundle", "cache", "compact_index",
-      "localgemserver.test.80.dd34752a738ee965a2a4298dc16db6c5", "info", "rack")
-
     bundle :install, artifice: "compact_index"
 
+    # We must remove the etag so that we don't ignore the range and get a 304 Not Modified.
+    rake_info_etag_path = File.join(Bundler.rubygems.user_home, ".bundle", "cache", "compact_index",
+      "localgemserver.test.80.dd34752a738ee965a2a4298dc16db6c5", "info-etags", "rack-11690b09f16021ff06a6857d784a1870")
+    File.unlink(rake_info_etag_path) if File.exist?(rake_info_etag_path)
+
+    rake_info_path = File.join(Bundler.rubygems.user_home, ".bundle", "cache", "compact_index",
+      "localgemserver.test.80.dd34752a738ee965a2a4298dc16db6c5", "info", "rack")
     expected_rack_info_content = File.read(rake_info_path)
 
-    # Modify the cache files. We expect them to be reset to the normal ones when we re-run :install
-    File.open(rake_info_path, "a") {|f| f << "this is different" }
+    # Modify the cache files to make the range not satisfiable
+    File.open(rake_info_path, "a") {|f| f << "0.9.2 |checksum:c55b525b421fd833a93171ad3d7f04528ca8e87d99ac273f8933038942a5888c" }
 
     # Update the Gemfile so the next install does its normal things
     gemfile <<-G
diff --git a/spec/bundler/support/artifice/helpers/compact_index.rb b/spec/bundler/support/artifice/helpers/compact_index.rb
index cf8bb34c5a..a803a2d30a 100644
--- a/spec/bundler/support/artifice/helpers/compact_index.rb
+++ b/spec/bundler/support/artifice/helpers/compact_index.rb
@@ -19,8 +19,8 @@ class CompactIndexAPI < Endpoint
     def etag_response
       response_body = yield
       etag = Digest::MD5.hexdigest(response_body)
-      return if not_modified?(etag)
       headers "ETag" => quote(etag)
+      return if not_modified?(etag)
       headers "Repr-Digest" => "sha-256=:#{Digest::SHA256.base64digest(response_body)}:"
       headers "Surrogate-Control" => "max-age=2592000, stale-while-revalidate=60"
       content_type "text/plain"
@@ -35,7 +35,6 @@ class CompactIndexAPI < Endpoint
       etags = parse_etags(request.env["HTTP_IF_NONE_MATCH"])
 
       return unless etags.include?(etag)
-      headers "ETag" => quote(etag)
       status 304
       body ""
     end