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.

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

Conversation

umar8hassan
Copy link
Contributor

@umar8hassan umar8hassan commented May 28, 2024

What are the relevant tickets?

closes #2175 and closes #2172

Description (What does it do?)

Limited the use of input url. Format for the subscribe url is pre-defined and only token will be used from the message.

How can this be tested?

The scope of this ticket is to test that url was formatted correctly after using the pre-defined url format

  • Use the RC settings for AWS_ACCOUNT_ID and AWS_REGION
  • Using Postman or curl, make a request to http://localhost:8043/api/transcode-jobs/ with the following data, and headers "Authorization: Bearer <settings.API_BEARER_TOKEN>" and "Content-Type: application/json":
{
 "Type": "SubscriptionConfirmation",
 "MessageId": "88b400b3-2fd3-49a3-b182-f64a4224d3e2",
 "Token": "fake-token",
 "TopicArn": "arn:aws:sns:<AWS_REGION>:<AWS_ACCOUNT_ID>:MediaConvertJobAlert",
 "Message": "To confirm the subscription, visit the SubscribeURL included in this message.",
 "SubscribeURL": "https://sns.<AWS_REGION>.amazonaws.com/?Action=ConfirmSubscription&TopicArn=arn:aws:sns:<AWS_REGION>:<AWS_ACCOUNT_ID>:MediaConvertJobAlert&Token=fake-token",
 "Timestamp": "2021-08-05T14:32:17.423Z",
 "SignatureVersion": "1",
 "Signature": "fake-signature",
 "SigningCertURL": "https://sns.<AWS_REGION>.amazonaws.com/SimpleNotificationService.pem"
}
  • The request should return 200 status_code. Which would indicate that url was formatted correctly.

@umar8hassan umar8hassan linked an issue May 28, 2024 that may be closed by this pull request
1 task
@umar8hassan umar8hassan self-assigned this May 28, 2024
@umar8hassan umar8hassan marked this pull request as ready for review May 28, 2024 13:19
@pt2302
Copy link
Contributor

pt2302 commented May 29, 2024

Could you add a test for this functionality, i.e., making sure that the URL is properly formatted? Also, we are not currently performing validation on some of these fields, such as AWS_ACCOUNT_ID. We should do this, and throw appropriate errors if they are not valid.

@pt2302 pt2302 self-assigned this May 30, 2024
@umar8hassan umar8hassan requested a review from pt2302 May 30, 2024 20:41
@umar8hassan umar8hassan requested a review from pt2302 May 30, 2024 22:16
@@ -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:

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.

@umar8hassan umar8hassan requested a review from pt2302 June 3, 2024 13:08
Copy link
Contributor

@pt2302 pt2302 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@umar8hassan umar8hassan merged commit f9266fe into master Jun 4, 2024
7 checks passed
@umar8hassan umar8hassan deleted the umar/2175-fix-codeql-alert-for-security-risk branch June 4, 2024 08:54
@odlbot odlbot mentioned this pull request Jun 6, 2024
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix CodeQL Alert for security risk Refactor videos/views.py to fix CodeQL Alert
2 participants