Skip to content
This repository was archived by the owner on Nov 12, 2023. It is now read-only.

[SE-3463] Apply correction to the last XSS patch and a new XSS patch - #40

Merged
clemente merged 4 commits into
pooja/cherry-pick-pymongo-upgradefrom
clemente/se-3463-apply-corrected-xss-patch-and-others
Nov 2, 2020
Merged

[SE-3463] Apply correction to the last XSS patch and a new XSS patch#40
clemente merged 4 commits into
pooja/cherry-pick-pymongo-upgradefrom
clemente/se-3463-apply-corrected-xss-patch-and-others

Conversation

@clemente

@clemente clemente commented Oct 28, 2020

Copy link
Copy Markdown

This combines several security patches, mainly from open-craft#273 but also open-craft#272 and it also has a small original patch.
We backport all that to the current Campus branch.

Fix the previous XSS patch that broke video uploads

We previously reverted part of a XSS patch: https://github.com/edx-olive/edx-platform/pull/35
The proper fix was done upstream: edx@6a0cdf4
This PR cherry-picks that commit, so it should fix the XSS without breaking the uploads.

More fixes

The rest of the fixes in this PR come from https://github.com/open-craft/edx-platform/pull/273/commits
I'll explain each one in the following sections.

new XSS patches

„sustaining security fixes 6“ (XSS patch), edx@e5d7128

This was applied cleanly. When I applied I found another line that would need this change (I guess) but that wasn't in the Juniper patch because it just exists in Ginkgo:

-  <input type="hidden" name="video_list" value='<%= JSON.stringify(video_list) %>'>
+  <input type="hidden" name="video_list" value='<%- JSON.stringify(video_list) %>'>

I did the change in this PR too.

Password reset token leakage

open-craft@a83fa85

I have trouble backporting edx@4b39327 to Ginkgo. There are merge conflicts because edx-platform/openedx/core/djangoapps/user_authn doesn't exist in Ginkgo.
PasswordResetConfirmWrapper (the class used and modified by that patch) doesn't even exist in Ginkgo, and the equivalent code is implemented by a function with different code than the code seen in the patch.

Seeing that this issue doesn't affect Campus (see screenshots and description at SE-3412) I think we shouldn't backport it. We wouldn't notice any difference if we did. We should just be careful not to add external links to untrusted sites in the footer.

Upgrade Django to 2.2.16

open-craft@7c6f286

It doesn't make sense to apply the patch to upgrade to Django 2.2.15 to 2.2.16 since we're using: django==1.8.18

jenkins-worker

open-craft@328411c

The patch about renaming jenkins-workerjuniper-worker obviously isn't desired since we're not on Juniper. But the modified code doesn't even exist in Ginkgo, so we have nothing to change. There's no edx-platform/scripts/Jenkinsfiles folder, nor references to jenkins-worker

certificate URLs

open-craft@5ee006d

There are all sorts of non-trivial merge conflicts when trying to apply the certificate URL patch, involving six, code to handle circular imports, tests and obviously the logic pass parameters in URLs.
I copied them here.
Therefore I think we can't backport this so easily and we'd need to do much more work.

For now, we'd have to stay with this issue until we upgrade.
The issue is not critical, it just shows the user ID as part of the certificate URL.

How to test this

The main things to test are video transcript uploads (since it's what was broken by the XSS patches) and something affected by the XSS patch. We can trust that the edX XSS patch is mostly correct and test the video transcript uploads.

Test in Campus stage, and if it works deploy to production.

Campus stage is being used for other deployments, so we'll need to synchronize.

awaisdar001 and others added 3 commits October 27, 2020 03:11
Cherry-picked from 6a0cdf4 (open-release/juniper.master)
Cherry-picked from 8f861dc (open-release/juniper.master)
@clemente clemente changed the title [WIP] [SE-3463] Apply corrected XSS patch and other patches [SE-3463] Apply correction to the last XSS patch and a new XSS patch Oct 29, 2020
@clemente
clemente requested a review from mtyaka October 29, 2020 16:12
@mtyaka

mtyaka commented Oct 30, 2020

Copy link
Copy Markdown

@clemente Thanks for the detailed explanation on which patches to include or ignore. It all makes sense.

I tried testing this on stage, but I ran into some issues. I am not able to add any components to a course in the Studio. It looks like some JS is broken. I'm not sure if this is related to the patches from this PR.

Screenshot 2020-10-30 at 08 45 11

Screenshot 2020-10-30 at 08 46 24

Another place where the upload problem occurs is the "Schedule & Details" course page in the Studio where uploading any of the course images uses the same upload widget as video transcripts.
I tried uploading a "Course Card Image", but the upload image doesn't work. I can see an error in the JS console, which indicates that this could be related to the patch from this PR: "Uncaught ReferenceError: HtmlUtils is not defined".

Screenshot 2020-10-30 at 08 46 55

@clemente

clemente commented Nov 1, 2020

Copy link
Copy Markdown
Author

The error about not being able to add a component is tracked in SE-3534 and unrelated to this patch. (But it's blocking the testing!).

The other one (HTMLUtils): comes from the re-fixed XSS patch described in the first section. I had applied edx@6a0cdf4 but it looks like Ginkgo was missing some other lines that were applied by previous patches. I added this other patch from master, edx@71593b140c55, with the HtmlUtils loading and with an extra change.

I'll launch a new server with the changes.

@clemente

clemente commented Nov 1, 2020

Copy link
Copy Markdown
Author

@mtyaka I enabled it and the file upload works, both in course banners (schedule & details) and in video transcripts. I'm using this section where there's already a video block.
To test the 2nd one, go to studio → video → edit → advanced → transcript languages → add → and then upload a .srt file It must have some content, like:

1
00:02:31,567 --> 00:02:37,164 
It's a test

@mtyaka Could you review that nothing else is broken?

After this is verified, I'll merge and deploy to production. (I'll deploy this PR. Note that the one in stage is set on top of some changes done for SE-2888).

@clemente

clemente commented Nov 2, 2020

Copy link
Copy Markdown
Author

The code was already reviewed by @mtyaka, and @pkulkark reviewed that files upload still works fine on stage. (Serenity channel in mattermost). Thanks.
Merging, and I'll deploy it to production.

@clemente
clemente merged commit 186e6a5 into pooja/cherry-pick-pymongo-upgrade Nov 2, 2020
@clemente
clemente deleted the clemente/se-3463-apply-corrected-xss-patch-and-others branch November 2, 2020 14:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants