From 26281cbe36ae999f01d24af4cebf62874c3ba76d Mon Sep 17 00:00:00 2001 From: Florian Haas Date: Mon, 6 Jul 2020 09:19:34 +0200 Subject: [PATCH] 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=" 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=" instead if we find a match. The proper way of appending the v= 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 --- openedx/core/djangoapps/user_api/accounts/image_helpers.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/user_api/accounts/image_helpers.py b/openedx/core/djangoapps/user_api/accounts/image_helpers.py index 7b1e555074..40128af60a 100644 --- a/openedx/core/djangoapps/user_api/accounts/image_helpers.py +++ b/openedx/core/djangoapps/user_api/accounts/image_helpers.py @@ -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()}