Explicitly remove authorization token and make sure that invalid addresses are properly handled

3 files changed, 122 insertions, 51 deletions

diff --git a/lib/container_registry/client.rb b/lib/container_registry/client.rb
index 50ddf79601d..3dda60f607c 100644
--- a/lib/container_registry/client.rb
+++ b/lib/container_registry/client.rb
@@ -10,54 +10,41 @@ module ContainerRegistry
     # Taken from: FaradayMiddleware::FollowRedirects
     REDIRECT_CODES  = Set.new [301, 302, 303, 307]
 
-    # Regex that matches characters that need to be escaped in URLs, sans
-    # the "%" character which we assume already represents an escaped sequence.
-    URI_UNSAFE = /[^\-_.!~*'()a-zA-Z\d;\/?:@&=+$,\[\]%]/
-
     def initialize(base_uri, options = {})
       @base_uri = base_uri
-      @faraday = Faraday.new(@base_uri) do |conn|
-        initialize_connection(conn, options)
-      end
+      @options = options
     end
 
     def repository_tags(name)
-      response_body @faraday.get("/v2/#{name}/tags/list")
+      response_body faraday.get("/v2/#{name}/tags/list")
     end
 
     def repository_manifest(name, reference)
-      response_body @faraday.get("/v2/#{name}/manifests/#{reference}")
+      response_body faraday.get("/v2/#{name}/manifests/#{reference}")
     end
 
     def repository_tag_digest(name, reference)
-      response = @faraday.head("/v2/#{name}/manifests/#{reference}")
+      response = faraday.head("/v2/#{name}/manifests/#{reference}")
       response.headers['docker-content-digest'] if response.success?
     end
 
     def delete_repository_tag(name, reference)
-      @faraday.delete("/v2/#{name}/manifests/#{reference}").success?
+      faraday.delete("/v2/#{name}/manifests/#{reference}").success?
     end
 
     def blob(name, digest, type = nil)
-      headers = {}
-      headers['Accept'] = type if type
-      response_body @faraday.get("/v2/#{name}/blobs/#{digest}", nil, headers), allow_redirect: true
+      type ||= 'application/octet-stream'
+      response_body faraday_blob.get("/v2/#{name}/blobs/#{digest}", nil, 'Accept' => type), allow_redirect: true
     end
 
     def delete_blob(name, digest)
-      @faraday.delete("/v2/#{name}/blobs/#{digest}").success?
+      faraday.delete("/v2/#{name}/blobs/#{digest}").success?
     end
-    
+
     private
-    
+
     def initialize_connection(conn, options)
       conn.request :json
-      conn.headers['Accept'] = MANIFEST_VERSION
-
-      conn.response :json, content_type: 'application/json'
-      conn.response :json, content_type: 'application/vnd.docker.distribution.manifest.v1+prettyjws'
-      conn.response :json, content_type: 'application/vnd.docker.distribution.manifest.v1+json'
-      conn.response :json, content_type: 'application/vnd.docker.distribution.manifest.v2+json'
 
       if options[:user] && options[:password]
         conn.request(:basic_auth, options[:user].to_s, options[:password].to_s)
@@ -68,32 +55,43 @@ module ContainerRegistry
       conn.adapter :net_http
     end
 
+    def accept_manifest(conn)
+      conn.headers['Accept'] = MANIFEST_VERSION
+
+      conn.response :json, content_type: 'application/json'
+      conn.response :json, content_type: 'application/vnd.docker.distribution.manifest.v1+prettyjws'
+      conn.response :json, content_type: 'application/vnd.docker.distribution.manifest.v1+json'
+      conn.response :json, content_type: 'application/vnd.docker.distribution.manifest.v2+json'
+    end
+
     def response_body(response, allow_redirect: false)
       if allow_redirect && REDIRECT_CODES.include?(response.status)
-        response = redirect_response(response.env['url'], response.headers['location'])
+        response = redirect_response(response.headers['location'])
       end
 
       response.body if response && response.success?
     end
 
-    def redirect_response(url, location)
+    def redirect_response(location)
       return unless location
 
-      url += safe_escape(location)
+      # We explicitly remove authorization token
+      faraday_blob.get(location) do |req|
+        req['Authorization'] = ''
+      end
+    end
 
-      # We use HTTParty due to fact that @faraday contains internal authorization token
-      HTTParty.get(url)
+    def faraday
+      @faraday ||= Faraday.new(@base_uri) do |conn|
+        initialize_connection(conn, @options)
+        accept_manifest(conn)
+      end
     end
 
-    # Taken from: FaradayMiddleware::FollowRedirects
-    # Internal: escapes unsafe characters from an URL which might be a path
-    # component only or a fully qualified URI so that it can be joined onto an
-    # URI:HTTP using the `+` operator. Doesn't escape "%" characters so to not
-    # risk double-escaping.
-    def safe_escape(uri)
-      uri.to_s.gsub(URI_UNSAFE) { |match|
-        '%' + match.unpack('H2' * match.bytesize).join('%').upcase
-      }
+    def faraday_blob
+      @faraday_blob ||= Faraday.new(@base_uri) do |conn|
+        initialize_connection(conn, @options)
+      end
     end
   end
 end
diff --git a/spec/lib/container_registry/blob_spec.rb b/spec/lib/container_registry/blob_spec.rb
index 4d8cb787dde..bbacdc67ebd 100644
--- a/spec/lib/container_registry/blob_spec.rb
+++ b/spec/lib/container_registry/blob_spec.rb
@@ -9,8 +9,9 @@ describe ContainerRegistry::Blob do
       'size' => 1000
     }
   end
+  let(:token) { 'authorization-token' }
   
-  let(:registry) { ContainerRegistry::Registry.new('http://example.com') }
+  let(:registry) { ContainerRegistry::Registry.new('http://example.com', token: token) }
   let(:repository) { registry.repository('group/test') }
   let(:blob) { repository.blob(config) }
 
@@ -58,4 +59,53 @@ describe ContainerRegistry::Blob do
 
     it { is_expected.to be_truthy }
   end
+
+  context '#data' do
+    let(:data) { '{"key":"value"}' }
+
+    subject { blob.data }
+
+    context 'when locally stored' do
+      before do
+        stub_request(:get, 'http://example.com/v2/group/test/blobs/sha256:0123456789012345').
+          to_return(
+            status: 200,
+            headers: { 'Content-Type' => 'application/json' },
+            body: data)
+      end
+
+      it { is_expected.to eq(data) }
+    end
+
+    context 'when externally stored' do
+      before do
+        stub_request(:get, 'http://example.com/v2/group/test/blobs/sha256:0123456789012345').
+          with(headers: { 'Authorization' => "bearer #{token}" }).
+          to_return(
+            status: 307,
+            headers: { 'Location' => location })
+      end
+
+      context 'for a valid address' do
+        let(:location) { 'http://external.com/blob/file' }
+
+        before do
+          stub_request(:get, location).
+            with(headers: { 'Authorization' => nil }).
+            to_return(
+              status: 200,
+              headers: { 'Content-Type' => 'application/json' },
+              body: data)
+        end
+
+        it { is_expected.to eq(data) }
+      end
+
+      context 'for invalid file' do
+        let(:location) { 'file:///etc/passwd' }
+
+        it { expect{ subject }.to raise_error(ArgumentError, 'invalid address') }
+      end
+    end
+  end
 end
diff --git a/spec/lib/container_registry/tag_spec.rb b/spec/lib/container_registry/tag_spec.rb
index c7324c2bf77..c5e31ae82b6 100644
--- a/spec/lib/container_registry/tag_spec.rb
+++ b/spec/lib/container_registry/tag_spec.rb
@@ -77,24 +77,47 @@ describe ContainerRegistry::Tag do
       end
 
       context 'config processing' do
-        before do
-          stub_request(:get, 'http://example.com/v2/group/test/blobs/sha256:d7a513a663c1a6dcdba9ed832ca53c02ac2af0c333322cd6ca92936d1d9917ac').
-            with(headers: { 'Accept' => 'application/octet-stream' }).
-            to_return(
-              status: 200,
-              body: File.read(Rails.root + 'spec/fixtures/container_registry/config_blob.json'))
-        end
+        shared_examples 'a processable' do
+          context '#config' do
+            subject { tag.config }
 
-        context '#config' do
-          subject { tag.config }
+            it { is_expected.not_to be_nil }
+          end
+
+          context '#created_at' do
+            subject { tag.created_at }
 
-          it { is_expected.not_to be_nil }
+            it { is_expected.not_to be_nil }
+          end
         end
 
-        context '#created_at' do
-          subject { tag.created_at }
+        context 'when locally stored' do
+          before do
+            stub_request(:get, 'http://example.com/v2/group/test/blobs/sha256:d7a513a663c1a6dcdba9ed832ca53c02ac2af0c333322cd6ca92936d1d9917ac').
+              with(headers: { 'Accept' => 'application/octet-stream' }).
+              to_return(
+                status: 200,
+                body: File.read(Rails.root + 'spec/fixtures/container_registry/config_blob.json'))
+          end
+
+          it_behaves_like 'a processable'
+        end
 
-          it { is_expected.not_to be_nil }
+        context 'when externally stored' do
+          before do
+            stub_request(:get, 'http://example.com/v2/group/test/blobs/sha256:d7a513a663c1a6dcdba9ed832ca53c02ac2af0c333322cd6ca92936d1d9917ac').
+              with(headers: { 'Accept' => 'application/octet-stream' }).
+              to_return(
+                status: 307,
+                headers: { 'Location' => 'http://external.com/blob/file' })
+
+            stub_request(:get, 'http://external.com/blob/file').
+              to_return(
+                status: 200,
+                body: File.read(Rails.root + 'spec/fixtures/container_registry/config_blob.json'))
+          end
+
+          it_behaves_like 'a processable'
         end
       end
     end