Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CodeQL Alert for SSRF #2197

Merged
merged 6 commits into from
Jun 4, 2024
Merged

Fix CodeQL Alert for SSRF #2197

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eef3103
added predefined format for subscribe url
umar8hassan May 28, 2024
feb4914
added unit tests and token validation
umar8hassan May 30, 2024
52f902f
renamed variable to remove linter warning
umar8hassan May 30, 2024
fe70f63
replaced `open()` with `Path.open()` to remove linter warning
umar8hassan May 30, 2024
39b96c4
resolved change requests
umar8hassan May 30, 2024
a6d5146
meeting change request
umar8hassan May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion videos/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
WEBVTT_FORMAT_ID = 51

TRANSCODE_JOB_SUBSCRIPTION_URL = "https://sns.{AWS_REGION}.amazonaws.com/?Action=ConfirmSubscription&TopicArn=arn:aws:sns:{AWS_REGION}:{AWS_ACCOUNT_ID}:MediaConvertJobAlert&Token={TOKEN}"
BAD_REQUEST_MSG = "Token Cannot be empty!"
BAD_REQUEST_MSG = "Token cannot be empty!"


class VideoStatus:
Expand Down
2 changes: 1 addition & 1 deletion videos/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@


class BadRequest(APIException):
"""Exception for Invalid request data"""
"""Exception for invalid request data"""

status_code = status.HTTP_400_BAD_REQUEST
default_detail = "Bad request."
Expand Down
2 changes: 1 addition & 1 deletion videos/utils_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def test_create_new_content(mocker):


def test_get_subscribe_url(mocker):
"""Test for get_subscribe_url method"""
"""Test get_subscribe_url to format ConfirmSubscription url correctly"""

mocker.patch("django.conf.settings.AWS_REGION", "us-east-1")
mocker.patch("django.conf.settings.AWS_ACCOUNT_ID", "1234567890")
Expand Down
5 changes: 3 additions & 2 deletions videos/views_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import pytest
from django.http.response import HttpResponse
from django.urls import reverse
from rest_framework.status import HTTP_400_BAD_REQUEST
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are using imports from rest_framework.status here, why would we define them as constants here:

ocw-studio/external_resources/constants.py

Lines 4 to 10 in d6d5cd1

HTTP_BAD_REQUEST = 400
HTTP_UNAUTHORIZED = 401
HTTP_PAYMENT_REQUIRED = 402
HTTP_FORBIDDEN = 403
HTTP_TOO_MANY_REQUESTS = 429
HTTP_REQUEST_TIMEOUT = 408
HTTP_SERVICE_UNAVAILABLE = 503
For consistency, I think we should make those imports from rest_framework.status as well.

Also, commit messages should ideally contain a summary of what the commit addresses, rather than a more generic message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pt2302 I agree with you on this. I'd have changed it if it had been in the same module and scope of work. Changing the constants and testing things again for external resources module for any breaking change, should be done in separate refactoring ticket in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I've opened #2198 and put up #2199 to resolve this issue.


from gdrive_sync.factories import DriveFileFactory
from users.factories import UserFactory
Expand Down Expand Up @@ -102,7 +103,7 @@ def test_transcode_jobs_wrong_account(drf_client):


def test_transcode_jobs_subscribe(settings, mocker, drf_client):
"""TranscodeJobView should confirm a subcsription request"""
"""TranscodeJobView should confirm a subscription request"""
mock_get = mocker.patch("videos.views.requests.get")
with open( # noqa: PTH123
f"{TEST_VIDEOS_WEBHOOK_PATH}/subscribe.json", encoding="utf-8"
Expand Down Expand Up @@ -149,7 +150,7 @@ def test_transcode_jobs_subscribe_bad_request(settings, mocker, drf_client):
data["Token"] = ""

response = drf_client.post(reverse("transcode_jobs"), data=data)
assert response.status_code == 400
assert response.status_code == HTTP_400_BAD_REQUEST
mock_get.assert_not_called()


Expand Down
Loading