Fix profile image URLs for image storage on non-public S3 buckets
In image_helpers.py, the _get_profile_image_urls() method would append "?v=<version>" to the query string for serving profile images. This might break serving profile images if * EDXAPP_PROFILE_IMAGE_BACKEND was configured with its class option set to django.storages.s3boto3.S3Boto3Storage (or its deprecated predecedessor, django.storages.s3boto.S3BotoStorage), and * that backend used signed URLs with query-string authentication (i.e. was *not* configured with an S3 custom domain). When both the above conditions are met, then the URL returned by the storage backend's url() method already contains "?", and _get_profile_image_urls() would add another. This results in a query string that doesn't exactly violate RFC 3986, but is discouraged by it.[1] Amazon S3 itself may be able to parse these query strings correctly, but other S3 API implementations (such as Ceph radosgw[2]) may not, and the problem is easily avoided by just looking for "?" in the rendered URL, and using "&v=<version>" instead if we find a match. The proper way of appending the v=<version> query parameter would probably be to pull the URL and the query string apart and then back together[3], but that's most likely overdoing it. [1] https://tools.ietf.org/html/rfc3986#section-3.4 says: "However, as query components are often used to carry identifying information in the form of "key=value" pairs and one frequently used value is a reference to another URI, it is sometimes better for usability to avoid percent- encoding those characters." ("Those characters" being "/" and "?".) [2] https://docs.ceph.com/docs/master/radosgw/s3/ [3] https://docs.python.org/3/library/urllib.parse.html
This commit is contained in:
@@ -55,7 +55,12 @@ def _get_profile_image_urls(name, storage, file_extension=PROFILE_IMAGE_FILE_EXT
|
||||
url = storage.url(
|
||||
_get_profile_image_filename(name, size, file_extension=file_extension)
|
||||
)
|
||||
return '{}?v={}'.format(url, version) if version is not None else url
|
||||
# Return the URL, with the "v" parameter added as its query
|
||||
# string with "?v=". If the original URL already includes a
|
||||
# query string (such as signed S3 URLs), append to the query
|
||||
# string with "&v=" instead.
|
||||
separator = '&' if '?' in url else '?'
|
||||
return '{}{}v={}'.format(url, separator, version) if version is not None else url
|
||||
|
||||
return {size_display_name: _make_url(size) for size_display_name, size in settings.PROFILE_IMAGE_SIZES_MAP.items()}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user