fix: port security advisory patches to teak (BB-10886)#843
Conversation
The set_course_mode_price view had no authorization check beyond @login_required, meaning any authenticated user could POST to it and rewrite the honor-mode price for any course — a privilege escalation vulnerability. Ideally this endpoint would be removed: it has no known callers in the UI (no templates or JS reference it), no tests, and targets the legacy 'honor' mode. However, it is publicly routed and external consumers may depend on it, so removal requires going through the DEPR process before we can act. In the meantime, this commit closes the security hole regardless of how active the endpoint is. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> (cherry picked from commit fd93ef5)
validate_timestamp_and_nonce previously returned True unconditionally, allowing any captured LTI launch request to be replayed indefinitely. Now rejects requests whose oauth_timestamp falls outside a ±5-minute window, then atomically records the nonce in the Django cache via cache.add() (OEP-0022 key generation via get_cache_key). A replay returns False immediately because cache.add() only writes when the key is absent. TieredCache is intentionally not used here: it has no atomic add primitive, so a separate get-then-set would leave a race window that defeats the protection. See the updated docstring for details. Documents the requirement for a shared cache backend (Redis or Memcached) in multi-node deployments in both the app and repo READMEs. Fixes GHSA-6gm5-c49g-p3h9 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> (cherry picked from commit 0a92cf2)
The PUT /api/contentstore/v1/videos/{course_id}/download endpoint fetched
every client-supplied files[].url server-side with
requests.get(url, allow_redirects=True) and returned the bytes inside the
ZIP response. Because the URLs were never validated, an authenticated user
with studio read access could point them at internal services or cloud
metadata endpoints and exfiltrate the responses (GHSA-fpf9-9rpr-jvrx).
By design these URLs are always a subset of the course's own VAL
encoded_videos[].url values (the same data the video listing hands the
frontend). Restrict fetches to that allowlist: build the set of legitimate
URLs for the course and reject any request containing a URL outside it
before any HTTP request is made. This eliminates the SSRF rather than
merely narrowing it.
Adds VideoDownloadViewTest (the endpoint previously had no test coverage)
covering the allowed-URL success path, rejection of disallowed URLs without
any outbound request, mixed allowed/disallowed batches, and the non-staff
permission gate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(cherry picked from commit 241b914)
|
@xitij2000 Note regarding CI. The Pylint checks are failing with the following error
I think that's safe to ignore for the purposes of this PR. What do you think? |
That particular error is safe to ignore, however it does mean that the CI doesn't run any tests at all. If the tests are running locally then it's fine but might be worth adding a simple docstring to get the tests to run. |
|
@xitij2000 Hmm..
Are the CI results not loading for you? Because this is what I see. Only the Pylint checks and coverage upload haven't run. Everything else has completed successfully. 
|
|
@tecoholic Ah! sorry I misread the |
xitij2000
left a comment
xitij2000
left a comment
There was a problem hiding this comment.
Approved as cherry-picked commits.
3 participants
Ports three upstream Open edX security advisory fixes to
opencraft/teak.Patches (cherry-picked with
-x, oldest → newest)fd93ef5f991f1346fac3set_course_mode_priceendpoint0a92cf25de69336c402f241b914a1930f277f0c0Verification
All three cherry-picks applied cleanly (no conflicts); working tree clean. Equivalence verified via
git patch-id --stable:fd93ef5f99→1f1346fac3: MATCH241b914a19→30f277f0c0: MATCH0a92cf25de→69336c402f: patch-id differs only because ofREADME.rstdocumentation context lines (the added doc block lands at a different line number on teak than upstream). The substantive change — including the security-criticallti_provider/signature_validator.pynonce-replay logic — was verified byte-for-byte identical to the upstream commit.An addition commit:
Important
Do not use anything other than "Create a merge commit" for this PR. And do not use the > "Update with*" button. We want to preserve original commit IDs.
Partially generated with Claude Code