Skip to content

prevent request forgery - #3743

Draft
shanbady wants to merge 2 commits into
mainfrom
shanbady/prevent-request-forgery
Draft

prevent request forgery#3743
shanbady wants to merge 2 commits into
mainfrom
shanbady/prevent-request-forgery

Conversation

@shanbady

@shanbady shanbady commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/12773

Description (What does it do?)

This PR fixes an issue that codeql has flagged where without validation, an attacker could potentially send us malicious links that end up being user-facing.

This branch rejects links that are not from an expected host or that does not match the expected format.

How can this be tested?

  1. checkout main
  2. run the following script (outside of the web container - in a non-django regular python shell). it sends webhooks with malicious hosts and prints the responses from learn (the first one is a good case and the rest are malicious cases):
import hashlib
import hmac
import json
import os
import sys
import urllib.error
import urllib.request

SECRET = os.environ.get("WEBHOOK_SECRET", "please-change-this")
URL = os.environ.get("LEARN_URL", "http://localhost:8063") + "/api/v1/webhooks/ovs_videos/"
CF = "https://du3yhovcx8dht.cloudfront.net"
EVIL = os.environ.get("EVIL_HOST", "https://evil.io")
CASES = ["good", "evil-source", "evil-thumb", "evil-cta", "evil-key"]
def build_payload(case, key):
    data = {
        "key": key,
        "created_at": "2026-04-28T17:35:42.065326Z",
        "title": "SSRF review video",
        "description": "",
        "status": "Complete",
        "is_public": True,
        "duration": 5.28,
        "cta_link": None,
        "sources": [
            {"src": f"{CF}/transcoded/{key}/video__index.m3u8", "label": "HLS"}
        ],
        "videothumbnail_set": [
            {"cloudfront_url": f"{CF}/thumbnails/{key}/t.jpg"}
        ],
        "videosubtitle_set": [
            {
                "s3_object_key": f"subtitles/{key}/en.vtt",
                "language": "en",
                "language_name": "English",
            }
        ],
        "collection": {
            "key": "review_col",
            "title": "Review Collection",
            "description": "",
            "is_public": True,
            "for_shorts": False,
        },
    }
    if case == "evil-source":
        # streaming url pointed off the allowlist -- this is a url the server
        # never fetches itself, but would be persisted and served to browsers
        data["sources"][0]["src"] = f"{EVIL}/video__index.m3u8"
    elif case == "evil-thumb":
        data["videothumbnail_set"][0]["cloudfront_url"] = f"{EVIL}/t.jpg"
    elif case == "evil-cta":
        data["cta_link"] = f"{EVIL}/watch"
    elif case == "evil-key":
        # a structural key that could break out of a url/index path if unescaped
        data["key"] = "../../evil"
    return data

def send(case):
    key = "review_ok" if case == "good" else "review_ssrf"
    body = json.dumps(build_payload(case, key)).encode()
    sig = hmac.new(SECRET.encode(), body, hashlib.sha256).hexdigest()
    req = urllib.request.Request(
        URL,
        data=body,
        headers={
            "Content-Type": "application/json",
            "X-MITLearn-Signature": sig,
        },
    )
    try:
        with urllib.request.urlopen(req) as resp:
            return resp.status, resp.read().decode()
    except urllib.error.HTTPError as exc:
        return exc.code, exc.read().decode()

for case in CASES:
    status, text = send(case)
    print(f"{case:12} -> {status} {text[:200]}")
  1. note that they all go through
  2. checkout this branch and re-run the script and observe the malicious requests are blocked.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

OpenAPI Changes

2 changes: 2 error, 0 warning, 0 info

View full changelog

Unexpected changes? Ensure your branch is up-to-date with main (consider rebasing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant