Skip to content

Commit

Permalink
Revert "API uploads images via URL"
Browse files Browse the repository at this point in the history
This reverts commit 4a1824a.

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#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
  • Loading branch information
kennyadsl authored and mamhoff committed Feb 1, 2021
1 parent 0fa4d8b commit ae22587
Show file tree
Hide file tree
Showing 3 changed files with 2 additions and 54 deletions.
25 changes: 2 additions & 23 deletions api/app/controllers/spree/api/images_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
1 change: 0 additions & 1 deletion api/openapi/solidus-api.oas.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
30 changes: 0 additions & 30 deletions api/spec/requests/spree/api/images_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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')) }

Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit ae22587

Please sign in to comment.