From 97cac91c4f303006253867bdbff19e4abd53c8ad Mon Sep 17 00:00:00 2001
From: Alberto Vena <kennyadsl@gmail.com>
Date: Wed, 13 May 2020 17:13:26 +0200
Subject: [PATCH] Revert "API uploads images via URL"
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This reverts commit 4a1824af29fd873c049376231cc31d555ad4700d.

This commit potentially introduces a security vulnerabilty,
(CVE-2017–0889) as described here:

https://medium.com/in-the-weeds/all-about-paperclips-cve-2017-0889-server-side-request-forgery-ssrf-vulnerability-8cb2b1c96fe8

as kindly reported and described by this PR author in solidusio/solidus#3593.

Also, this PR has been introduced because there wasn't a clear way to
upload images via API, but this can be done by setting the right
Content-Type header in the request, as follow:

curl -v -H "Content-Type: multipart/form-data" -H "Authorization: Bearer YOUR_TOKEN" http://your.host/api/products/1
---
 .../spree/api/images_controller.rb            | 25 ++--------------
 api/openapi/solidus-api.oas.yml               |  1 -
 .../spree/api/images_controller_spec.rb       | 30 -------------------
 3 files changed, 2 insertions(+), 54 deletions(-)

diff --git a/api/app/controllers/spree/api/images_controller.rb b/api/app/controllers/spree/api/images_controller.rb
index 9bb51fe2edb..c18a9b757d1 100644
--- a/api/app/controllers/spree/api/images_controller.rb
+++ b/api/app/controllers/spree/api/images_controller.rb
@@ -15,23 +15,13 @@ def show
 
       def create
         authorize! :create, Image
-
-        @image = scope.images.build(image_params.except(:attachment))
-        @image.attachment = prepared_attachment
-        @image.save
-
+        @image = scope.images.create(image_params)
         respond_with(@image, status: 201, default_template: :show)
       end
 
       def update
         @image = scope.images.accessible_by(current_ability, :update).find(params[:id])
-        if image_params[:attachment].present?
-          @image.assign_attributes(image_params.except(:attachment))
-          @image.attachment = prepared_attachment
-          @image.save
-        else
-          @image.update image_params
-        end
+        @image.update(image_params)
         respond_with(@image, default_template: :show)
       end
 
@@ -54,17 +44,6 @@ def scope
           Spree::Variant.find(params[:variant_id])
         end
       end
-
-      def prepared_attachment
-        uri = URI.parse image_params[:attachment]
-        if uri.is_a? URI::HTTP
-          URI.open(image_params[:attachment])
-        else
-          image_params[:attachment]
-        end
-      rescue URI::InvalidURIError
-        image_params[:attachment]
-      end
     end
   end
 end
diff --git a/api/openapi/solidus-api.oas.yml b/api/openapi/solidus-api.oas.yml
index 194fa9fe688..02bcecbaf29 100644
--- a/api/openapi/solidus-api.oas.yml
+++ b/api/openapi/solidus-api.oas.yml
@@ -6378,7 +6378,6 @@ components:
           type: string
         attachment:
           type: string
-          description: 'This field can be used to pass an image URL. When used in this manner, the image undergoes the standard post upload processing via paperclip.'
         position:
           type: integer
         viewable_type:
diff --git a/api/spec/requests/spree/api/images_controller_spec.rb b/api/spec/requests/spree/api/images_controller_spec.rb
index c42a9a5e7d8..6465005058d 100644
--- a/api/spec/requests/spree/api/images_controller_spec.rb
+++ b/api/spec/requests/spree/api/images_controller_spec.rb
@@ -32,22 +32,6 @@ module Spree
         end.to change(Image, :count).by(1)
       end
 
-      it 'can upload a new image from a valid URL' do
-        expect do
-          post spree.api_product_images_path(product.id), params: {
-            image: {
-              attachment: 'https://github.com/solidusio/brand/raw/1827e7afb7ebcf5a1fc9cf7bf6cf9d277183ef11/PNG/solidus-logo-dark.png',
-              viewable_type: 'Spree::Variant',
-              viewable_id: product.master.to_param,
-              alt: 'just a test'
-            },
-          }
-          expect(response.status).to eq(201)
-          expect(json_response).to have_attributes(attributes)
-          expect(json_response[:alt]).to eq('just a test')
-        end.to change(Image, :count).by(1)
-      end
-
       context "working with an existing product image" do
         let!(:product_image) { product.master.images.create!(attachment: image('thinking-cat.jpg')) }
 
@@ -85,20 +69,6 @@ module Spree
           expect(product_image.reload.position).to eq(2)
         end
 
-        it "can update image URL" do
-          expect(product_image.position).to eq(1)
-          put spree.api_variant_image_path(product.master.id, product_image), params: {
-            image: {
-              position: 2,
-              attachment: 'https://github.com/solidusio/brand/raw/1827e7afb7ebcf5a1fc9cf7bf6cf9d277183ef11/PNG/solidus-logo-dark.png'
-            },
-          }
-          expect(response.status).to eq(200)
-          expect(json_response).to have_attributes(attributes)
-          expect(product_image.reload.position).to eq(2)
-          expect(product_image.reload.attachment_height).to eq(420)
-        end
-
         it "can delete an image" do
           delete spree.api_variant_image_path(product.master.id, product_image)
           expect(response.status).to eq(204)