[BB-2779] Cherry pick all upstream security fixes#241
Merged
lgp171188 merged 10 commits intoopencraft-release/juniper.2from Aug 20, 2020
Merged
Conversation
Agrendalath
approved these changes
Aug 20, 2020
Member
Agrendalath
left a comment
Agrendalath
left a comment
There was a problem hiding this comment.
@lgp171188, just one nit - I see that only the first commit (f4b70fc9a3258ca82d38536b4020a7ba2df32fa2) contains its source (by using git cherry-pick -x). The other ones don't have it. Would it make sense to reword them and add these original hashes to the commit message (even manually, since there are only 5 upstream commits without this hash) to have an indication that they come from the open-release/juniper.master?
👍
- I tested this: it's a cherry-pick from upstream branch, checked that both b37bab80b21b67727ca3653cace64d5a67aba13c - 37da9e2aa6a6d388f4b1fc8fa201c82ca1ebba5f and c093d472c8ec1c8a75ca8d7fed91d18c15b439eb - 6eeb1846e260173d5e327aa600f5c3897220fd31 are "mirrored" correctly
- I read through the code
-
I checked for accessibility issues -
Includes documentation
Author
|
@Agrendalath, the reason for not mentioning the upstream commit IDs in the cherry-pick is that by default all the commits from people outside OpenCraft are from upstream. That said, I have no problem adding them and will try to do that now. :) |
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 (cherry picked from commit 26281cb)
This commit contains xsslint fixes for the following Jira Tickets: PROD-1661 PROD-1663 PROD-1665 PROD-1727 PROD-1729 PROD-1731 PROD-1732 PROD-1795 (cherry-picked from upstream commit 0e45ecb)
(cherry-picked from upstream commit 91af099)
(cherry-picked from upstream commit 8dd7861)
(cherry-picked from upstream commit 47ab6af)
(cherry-picked from upstream commit c8421f6)
This reverts commit 4383429.
1. PROD-1603 2. PROD-1605 3. PROD-1612 4. PROD-1619 5. PROD-1289 6. PROD-1530 7. PROD-1525 8. PROD-1534 (cherry-picked from upstream commit d9e0ca5)
…Languages (#240) * Fix issue caused by XSS fix in Video > Advanced > Transcript Languages * change append to prepend Co-authored-by: pkulkark <pooja@opencraft.com>
lgp171188
force-pushed
the
guruprasad/BB-2779-cherry-pick-all-upstream-security-fixes
branch
from
6eeb184 to
ca79c6e
Compare
Agrendalath
deleted the
guruprasad/BB-2779-cherry-pick-all-upstream-security-fixes
branch
Agrendalath
pushed a commit
that referenced
this pull request
Apr 20, 2022
Previously, our rate-limiting code trusted the entire `X-Forwarded-For` header, allowing a malicious client to spoof that header and evade rate-limiting. This commit introduces a new module and setting allowing us to make a more conservative choice of IPs. - Create new `openedx.core.djangoapps.util.ip` module for producing the IP "external chain" for requests based on the XFF header and the REMOTE_ADDR. - Include a function that gives the safest choice of IPs. - Add new setting `CLOSEST_CLIENT_IP_FROM_HEADERS` for configuring how the external chain is derived (i.e. setting the trust boundary). Currently has a default, but we may want to make it mandatory in the future. - Change `django-ratelimit` code to use the proximate IP in the external chain -- the one just outside the trust boundary. Also: - Change `XForwardedForMiddleware` to use more conservative choice for its `REMOTE_ADDR` override - Other adjustments to `XForwardedForMiddleware` as needed in order to initialize new module and support code that needs the real `REMOTE_ADDR` value - Metrics for observability into the change (and XFF composition) - Feature switch to restore legacy mode if needed This also gives us a path forward to removing use of the django-ipware package, which is no longer maintained and has a handful of bugs that make it difficult to use safely. Internal ticket: ARCHBOM-2056
pomegranited
pushed a commit
that referenced
this pull request
Apr 27, 2022
Previously, our rate-limiting code trusted the entire `X-Forwarded-For` header, allowing a malicious client to spoof that header and evade rate-limiting. This commit introduces a new module and setting allowing us to make a more conservative choice of IPs. - Create new `openedx.core.djangoapps.util.ip` module for producing the IP "external chain" for requests based on the XFF header and the REMOTE_ADDR. - Include a function that gives the safest choice of IPs. - Add new setting `CLOSEST_CLIENT_IP_FROM_HEADERS` for configuring how the external chain is derived (i.e. setting the trust boundary). Currently has a default, but we may want to make it mandatory in the future. - Change `django-ratelimit` code to use the proximate IP in the external chain -- the one just outside the trust boundary. Also: - Change `XForwardedForMiddleware` to use more conservative choice for its `REMOTE_ADDR` override - Other adjustments to `XForwardedForMiddleware` as needed in order to initialize new module and support code that needs the real `REMOTE_ADDR` value - Metrics for observability into the change (and XFF composition) - Feature switch to restore legacy mode if needed This also gives us a path forward to removing use of the django-ipware package, which is no longer maintained and has a handful of bugs that make it difficult to use safely. Internal ticket: ARCHBOM-2056 Backported from a251d18
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR cherry-picks all the security fixes/changes made to the upstream
open-release/juniper.masterbranch after theopen-release/juniper.2tag was cut, that are not already in our common branch. This also includes a fix for profile image storage on S3, Django version upgrade and a Studio frontend version bump.This PR also reverts a custom commit made by @pkulkark in #237 and a fix related to that and then adds back the corresponding upstream commit and the same fix.
Reviewers:
Testing instructions:
open-release/juniper.masterbranch and verify that everything is okay.