Skip to content

Commit

Permalink
Include Content-Length in signature for ActiveStorage direct upload
Browse files Browse the repository at this point in the history
  • Loading branch information
travisp authored and tenderlove committed May 15, 2020
1 parent 092a9dd commit c0ab9a7
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
3 changes: 2 additions & 1 deletion activestorage/lib/active_storage/service/s3_service.rb
Expand Up @@ -80,7 +80,8 @@ def exist?(key)
def url_for_direct_upload(key, expires_in:, content_type:, content_length:, checksum:)
instrument :url, key: key do |payload|
generated_url = object_for(key).presigned_url :put, expires_in: expires_in.to_i,
content_type: content_type, content_length: content_length, content_md5: checksum, **upload_options
content_type: content_type, content_length: content_length, content_md5: checksum
whitelist_headers: ['content-length'], **upload_options

This comment has been minimized.

Copy link
@robzolkos

robzolkos May 26, 2020

Contributor

@rafaelfranca should this commit have been included in 6.0.3.1? Looks like it wasn't (I could be wrong)

This comment has been minimized.

This comment has been minimized.

Copy link
@robzolkos

robzolkos May 26, 2020

Contributor

@jonathanhefner yeah - its different to this commit....

This comment has been minimized.

Copy link
@jonathanhefner

jonathanhefner May 26, 2020

Member

@robzolkos I'm not sure I follow. See 17507e8.

This comment has been minimized.

Copy link
@robzolkos

robzolkos May 26, 2020

Contributor

@jonathanhefner **upload_options param is not in the commit used in 6.0.3.1 It is in this commit.

This comment has been minimized.

Copy link
@jonathanhefner

jonathanhefner May 26, 2020

Member

@robzolkos That is because this commit did not add **upload_options. **upload_options was added by cf7c27f, which has not been backported.

This comment has been minimized.

Copy link
@robzolkos

robzolkos May 26, 2020

Contributor

@jonathanhefner I see. Sorry to waste your time. Just trying to figure out #39334 This is causing issues in production after a 6.0.3.1 upgrade. The fix is in place in master now. Hoping a 6.0.3.2 can be cut soon as its going to bite peeps who upgraded to 6.0.3.1 to fix this CVE.

This comment has been minimized.

Copy link
@jonathanhefner

jonathanhefner May 26, 2020

Member

@robzolkos #39334 should not apply to 6.0.3.1. It was due to a missing comma (see the fix: 393df74), but that comma was included in 17507e8 and is present in 6.0.3.1: https://github.com/rails/rails/blob/v6.0.3.1/activestorage/lib/active_storage/service/s3_service.rb#L84


payload[:url] = generated_url

Expand Down
23 changes: 23 additions & 0 deletions activestorage/test/service/s3_service_test.rb
Expand Up @@ -55,6 +55,29 @@ class ActiveStorage::Service::S3ServiceTest < ActiveSupport::TestCase
@service.delete key
end

test "directly uploading file larger than the provided content-length does not work" do
key = SecureRandom.base58(24)
data = "Some text that is longer than the specified content length"
checksum = Digest::MD5.base64digest(data)
url = @service.url_for_direct_upload(key, expires_in: 5.minutes, content_type: "text/plain", content_length: data.size - 1, checksum: checksum)

uri = URI.parse url
request = Net::HTTP::Put.new uri.request_uri
request.body = data
request.add_field "Content-Type", "text/plain"
request.add_field "Content-MD5", checksum
upload_result = Net::HTTP.start(uri.host, uri.port, use_ssl: true) do |http|
http.request request
end

assert_equal "403", upload_result.code
assert_raises ActiveStorage::FileNotFoundError do
@service.download(key)
end
ensure
@service.delete key
end

test "upload a zero byte file" do
blob = directly_upload_file_blob filename: "empty_file.txt", content_type: nil
user = User.create! name: "DHH", avatar: blob
Expand Down

0 comments on commit c0ab9a7

Please sign in to comment.