Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Parameter escaping fix for auth_path generation in boto.s3.connection.generate_url #646

Closed
wants to merge 2 commits into from

3 participants

@jonny5532

The auth_path value generated inside boto.s3.connection.generate_url (and subsequently fed to boto.utils.canonical_string) is made up from unescaped key=value pairs joined by ampersands. If one of values contains a filename with an ampersand in (for the response-content-disposition, for example), canonical_string truncates that value and the routine results in an incorrect signature.

Since canonical_string urllib.unquotes the values internally, it makes sense to urllib.quote them before passing them in so the value is not truncated on unescaped ampersands.

The resulting behaviour works - Amazon is happy with the signature and the file downloads with the correct filename (inclusive of ampersands).

@tpodowd

Hi Jonny, can you make a unit test for this.

Tom.

@jonny5532

Have added a test case as requested (and fixed another test case in the same file).

It is perhaps redundant to perform two similar tests as it now does, one with a normal filename and one with a filename containing ampersands - better to just perform one test with ampersands in the filename?

jonny

@tpodowd

Thanks Jonny. This looks good to me.

Tom.

@tpodowd tpodowd referenced this pull request from a commit in tpodowd/boto
Thomas O'Dowd Add version_id support to generate_url and fix headers/response_headers.
- support version_id in the key or as passed from generate_url params
- Don't add non-provider headers to the generated url (see below)
- Fixed missing url encoding before passing to sig generation so that
  it can correctly decode the params. This is similar to pull request #646
  which can now also be closed.
- Fixed the headers dictionary polution with response-headers that was
  happening as part of calling the generate_url function.
- Added small unit test as provided by pull request #646 also.

I also confirmed that everything still works for the PUT case described
in the following url. Note though that, the url no longer contains the
Content-Length nor is it required at all as it's not part of the signature
calculation anyway. I confirmed also that adding other headers such as
--header "Content-Type: text/plain" to curl work when you specify same
header what you pass to generate_url via headers dict.

http://stackoverflow.com/questions/10044151/how-to-generate-a-temporary-url-to-upload-file-to-amazon-s3-with-boto-library

Both s3 and gs unit tests pass.
1f72bf7
@tpodowd

Jonny, I was reworking the generate_url() code and fixed the issue raised in this pull request so it should be working for you now also. In addition I added your unit test. You can see the history in pull request #746. I don't have permission to close this pull request as I'm not a main developer but I guess @garnaat will do that shortly.

Thanks for your patch and unit test!

Tom.

@garnaat garnaat closed this
@hardys hardys referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@msabramo msabramo referenced this pull request from a commit in msabramo/boto
Thomas O'Dowd Add version_id support to generate_url and fix headers/response_headers.
- support version_id in the key or as passed from generate_url params
- Don't add non-provider headers to the generated url (see below)
- Fixed missing url encoding before passing to sig generation so that
  it can correctly decode the params. This is similar to pull request #646
  which can now also be closed.
- Fixed the headers dictionary polution with response-headers that was
  happening as part of calling the generate_url function.
- Added small unit test as provided by pull request #646 also.

I also confirmed that everything still works for the PUT case described
in the following url. Note though that, the url no longer contains the
Content-Length nor is it required at all as it's not part of the signature
calculation anyway. I confirmed also that adding other headers such as
--header "Content-Type: text/plain" to curl work when you specify same
header what you pass to generate_url via headers dict.

http://stackoverflow.com/questions/10044151/how-to-generate-a-temporary-url-to-upload-file-to-amazon-s3-with-boto-library

Both s3 and gs unit tests pass.
85a4d79
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 19, 2012
Commits on Mar 29, 2012
This page is out of date. Refresh to see the latest.
Showing with 7 additions and 1 deletion.
  1. +1 −1  boto/s3/connection.py
  2. +6 −0 tests/s3/test_connection.py
View
2  boto/s3/connection.py
@@ -305,7 +305,7 @@ def generate_url(self, expires_in, method, bucket='', key='', headers=None,
# Arguments to override response headers become part of the canonical
# string to be signed.
if response_headers:
- response_hdrs = ["%s=%s" % (k, v) for k, v in
+ response_hdrs = ["%s=%s" % (k, urllib.quote(v)) for k, v in
response_headers.items()]
delimiter = '?' if '?' not in auth_path else '&'
auth_path = "%s%s" % (auth_path, delimiter)
View
6 tests/s3/test_connection.py
@@ -72,6 +72,12 @@ def test_1_basic(self):
file = urllib.urlopen(url)
rh = {'response-content-disposition': 'attachment; filename="foo.txt"'}
url = k.generate_url(60, response_headers=rh)
+ file = urllib.urlopen(url)
+ assert s1 == file.read(), 'invalid URL %s' % url
+ #test whether amperands and to-be-escaped characters work in header filename
+ rh = {'response-content-disposition': 'attachment; filename="foo&z%20ar&ar&zar&bar.txt"'}
+ url = k.generate_url(60, response_headers=rh)
+ file = urllib.urlopen(url)
assert s1 == file.read(), 'invalid URL %s' % url
bucket.delete_key(k)
# test a few variations on get_all_keys - first load some data
Something went wrong with that request. Please try again.