Skip to content

Commit

Permalink
Avoid Double URL Encoding S3 Keys
Browse files Browse the repository at this point in the history 
When performing a copy operation with the S3 Object resource, the object
key would be URL encoded before being sent on to the S3 client, which
URL encodes the key a second time. This was causing a 404 to
short-circuit the copy operation.

Tests were checking the inputs into the client call, which would not
catch the second step of the double encoding.
  • Loading branch information
@awood45
awood45 committed Mar 29, 2016
1 parent 386df32 commit 7dd105a
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 21 deletions.
12 changes: 4 additions & 8 deletions aws-sdk-resources/lib/aws-sdk-resources/services/s3/object_copier.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,15 @@ def copy_object(source, target, options)

def copy_source(source)
case source
when String then escape(source)
when String then source
when Hash
src = "#{source[:bucket]}/#{escape(source[:key])}"
src = "#{source[:bucket]}/#{source[:key]}"
src += "?versionId=#{source[:version_id]}" if source.key?(:version_id)
src
when S3::Object, S3::ObjectSummary
"#{source.bucket_name}/#{escape(source.key)}"
"#{source.bucket_name}/#{source.key}"
when S3::ObjectVersion
"#{source.bucket_name}/#{escape(source.object_key)}?versionId=#{source.id}"
"#{source.bucket_name}/#{source.object_key}?versionId=#{source.id}"
else
msg = "expected source to be an Aws::S3::Object, Hash, or String"
raise ArgumentError, msg
Expand Down Expand Up @@ -90,10 +90,6 @@ def apply_source_client(source, options)

end

def escape(str)
Seahorse::Util.uri_path_escape(str)
end

end
end
end
26 changes: 13 additions & 13 deletions aws-sdk-resources/spec/services/s3/object/multipart_copy_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'target-bucket',
key: 'target-key',
copy_source: 'bucket/unescaped/key%20path',
copy_source: 'bucket/unescaped/key path',
})
object.copy_to('target-bucket/target-key')
end
Expand All @@ -20,7 +20,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'target-bucket',
key: 'target-key',
copy_source: 'bucket/unescaped/key%20path',
copy_source: 'bucket/unescaped/key path',
})
object.copy_to(bucket:'target-bucket', key:'target-key')
end
Expand All @@ -29,7 +29,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'target-bucket',
key: 'target-key',
copy_source: 'bucket/unescaped/key%20path',
copy_source: 'bucket/unescaped/key path',
})
object.copy_to(bucket:'target-bucket', key:'target-key')
end
Expand All @@ -38,7 +38,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'target-bucket',
key: 'target-key',
copy_source: 'bucket/unescaped/key%20path',
copy_source: 'bucket/unescaped/key path',
content_type: 'text/plain',
})
object.copy_to(
Expand All @@ -52,7 +52,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'target-bucket',
key: 'target-key',
copy_source: 'bucket/unescaped/key%20path',
copy_source: 'bucket/unescaped/key path',
})
target = S3::Object.new('target-bucket', 'target-key', stub_responses:true)
object.copy_to(target)
Expand All @@ -62,7 +62,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'target-bucket',
key: 'target-key',
copy_source: 'bucket/unescaped/key%20path',
copy_source: 'bucket/unescaped/key path',
acl: 'public-read',
})
object.copy_to('target-bucket/target-key', acl: 'public-read')
Expand All @@ -81,16 +81,16 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'bucket',
key: 'unescaped/key path',
copy_source: 'source-bucket/escaped/source/key%20path',
copy_source: 'source-bucket/escaped/source/key path',
})
object.copy_from(copy_source: 'source-bucket/escaped/source/key%20path')
object.copy_from(copy_source: 'source-bucket/escaped/source/key path')
end

it 'accepts a string source' do
expect(client).to receive(:copy_object).with({
bucket: 'bucket',
key: 'unescaped/key path',
copy_source: 'source-bucket/source/key%20path',
copy_source: 'source-bucket/source/key path',
})
object.copy_from('source-bucket/source/key path')
end
Expand All @@ -99,7 +99,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'bucket',
key: 'unescaped/key path',
copy_source: 'source-bucket/unescaped/source/key%20path'
copy_source: 'source-bucket/unescaped/source/key path'
})
object.copy_from(bucket:'source-bucket', key:'unescaped/source/key path')
end
Expand Down Expand Up @@ -136,7 +136,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'bucket',
key: 'unescaped/key path',
copy_source: 'source-bucket/unescaped/source/key%20path',
copy_source: 'source-bucket/unescaped/source/key path',
})
object.copy_from(src)
end
Expand All @@ -146,7 +146,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'bucket',
key: 'unescaped/key path',
copy_source: 'source-bucket/unescaped/source/key%20path',
copy_source: 'source-bucket/unescaped/source/key path',
})
object.copy_from(src)
end
Expand All @@ -160,7 +160,7 @@ module S3
expect(client).to receive(:copy_object).with({
bucket: 'bucket',
key: 'unescaped/key path',
copy_source: 'source-bucket/unescaped/source/key%20path?versionId=source-version-id',
copy_source: 'source-bucket/unescaped/source/key path?versionId=source-version-id',
})
object.copy_from(src)
end
Expand Down

0 comments on commit 7dd105a

Please sign in to comment.