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

Jump to bottom

Django upgrade 5 #2777

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

Django upgrade 5 #2777

wants to merge 48 commits into from

Conversation

ukanga
Copy link
Member

@ukanga ukanga commented Feb 10, 2025

Changes / Features implemented

Steps taken to verify this change does what is intended

Side effects of implementing this change

Before submitting this PR for review, please make sure you have:

  • Included tests
  • Updated documentation

Closes #

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 12, 2025
View reviewed changes
onadata/apps/main/forms.py
Comment on lines +415 to +419
response = requests.head(
cleaned_url,
allow_redirects=True,
timeout=DEFAULT_REQUEST_TIMEOUT,
)

Check failure

Code scanning / CodeQL

Full server-side request forgery Critical

The full URL of this request depends on a
user-provided value
Loading
.
The full URL of this request depends on a
user-provided value
Loading
.
The full URL of this request depends on a
user-provided value
Loading
.

Copilot Autofix AI 19 days ago

To fix the SSRF vulnerability, we need to ensure that the user-provided URL is properly validated before being used in HTTP requests. One way to achieve this is by using a whitelist of allowed domains and ensuring that the user-provided URL belongs to one of these domains. This approach prevents the user from directing the request to an unintended server.

  1. Define a list of allowed domains.
  2. Parse the user-provided URL and check if its domain is in the allowed list.
  3. Only proceed with the HTTP request if the domain is valid.
Suggested changeset 1
onadata/apps/main/forms.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/onadata/apps/main/forms.py b/onadata/apps/main/forms.py
--- a/onadata/apps/main/forms.py
+++ b/onadata/apps/main/forms.py
@@ -19,2 +19,3 @@
 import requests
+from urllib.parse import urlparse
 from registration.forms import RegistrationFormUniqueEmail
@@ -407,5 +408,9 @@
 
+            allowed_domains = ["example.com", "anotherdomain.com"]
             if cleaned_url:
                 self.validate(cleaned_url)
-                cleaned_xls_file = urlparse(cleaned_url)
+                parsed_url = urlparse(cleaned_url)
+                if parsed_url.netloc not in allowed_domains:
+                    raise forms.ValidationError(_("Invalid URL domain."))
+                cleaned_xls_file = parsed_url
                 cleaned_xls_file = "_".join(cleaned_xls_file.path.split("/")[-2:])
EOF
@@ -19,2 +19,3 @@
import requests
from urllib.parse import urlparse
from registration.forms import RegistrationFormUniqueEmail
@@ -407,5 +408,9 @@

allowed_domains = ["example.com", "anotherdomain.com"]
if cleaned_url:
self.validate(cleaned_url)
cleaned_xls_file = urlparse(cleaned_url)
parsed_url = urlparse(cleaned_url)
if parsed_url.netloc not in allowed_domains:
raise forms.ValidationError(_("Invalid URL domain."))
cleaned_xls_file = parsed_url
cleaned_xls_file = "_".join(cleaned_xls_file.path.split("/")[-2:])
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
onadata/apps/main/forms.py
@@ -420,7 +425,7 @@
cleaned_xls_file = get_filename(response)

cleaned_xls_file = upload_to(None, cleaned_xls_file, user.username)
response = requests.get(cleaned_url)
response = requests.get(cleaned_url, timeout=DEFAULT_REQUEST_TIMEOUT)

Check failure

Code scanning / CodeQL

Full server-side request forgery Critical

The full URL of this request depends on a
user-provided value
Loading
.
The full URL of this request depends on a
user-provided value
Loading
.
The full URL of this request depends on a
user-provided value
Loading
.

Copilot Autofix AI 19 days ago

To fix the problem, we need to ensure that the user-provided URL is properly validated before being used in an HTTP request. One way to do this is to use a whitelist of allowed domains or perform strict validation to ensure the URL points to a trusted domain.

The best way to fix this without changing existing functionality is to add a validation step that checks if the URL belongs to a trusted domain. We can use the URLValidator from Django to validate the URL format and then check the domain against a whitelist.

Suggested changeset 1
onadata/apps/main/forms.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/onadata/apps/main/forms.py b/onadata/apps/main/forms.py
--- a/onadata/apps/main/forms.py
+++ b/onadata/apps/main/forms.py
@@ -24,2 +24,3 @@
 from onadata.apps.logger.models import Project
+
 from onadata.apps.main.models import UserProfile
@@ -408,3 +409,3 @@
             if cleaned_url:
-                self.validate(cleaned_url)
+                if not is_valid_url(cleaned_url):
                 cleaned_xls_file = urlparse(cleaned_url)
EOF
@@ -24,2 +24,3 @@
from onadata.apps.logger.models import Project

from onadata.apps.main.models import UserProfile
@@ -408,3 +409,3 @@
if cleaned_url:
self.validate(cleaned_url)
if not is_valid_url(cleaned_url):
cleaned_xls_file = urlparse(cleaned_url)
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
onadata/apps/main/models/meta_data.py
@@ -157,7 +159,9 @@
filename = media.data_value.split("/")[-1]
data_file = NamedTemporaryFile()
content_type = mimetypes.guess_type(filename)
with closing(requests.get(media.data_value, stream=True)) as resp:
with closing(
requests.get(media.data_value, stream=True, timeout=DEFAULT_REQUEST_TIMEOUT)

Check failure

Code scanning / CodeQL

Full server-side request forgery Critical

The full URL of this request depends on a
user-provided value
Loading
.

Copilot Autofix AI 18 days ago

To fix the problem, we need to ensure that the URL used in the requests.get method is validated against a whitelist of allowed domains. This can be achieved by implementing a function that checks if the URL's domain is in a predefined list of allowed domains before making the request.

  1. Define a list of allowed domains.
  2. Implement a function to validate the URL against this list.
  3. Use this function to validate media.data_value before making the request.
Suggested changeset 1
onadata/apps/main/models/meta_data.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/onadata/apps/main/models/meta_data.py b/onadata/apps/main/models/meta_data.py
--- a/onadata/apps/main/models/meta_data.py
+++ b/onadata/apps/main/models/meta_data.py
@@ -28,2 +28,3 @@
 import requests
+from urllib.parse import urlparse
 
@@ -155,5 +156,12 @@
 
+ALLOWED_DOMAINS = ["example.com", "anotherdomain.com"]
+
+def is_allowed_domain(url):
+    """Check if the URL's domain is in the allowed list."""
+    parsed_url = urlparse(url)
+    return parsed_url.netloc in ALLOWED_DOMAINS
+
 def create_media(media):
     """Download media link"""
-    if is_valid_url(media.data_value):
+    if is_valid_url(media.data_value) and is_allowed_domain(media.data_value):
         filename = media.data_value.split("/")[-1]
EOF
@@ -28,2 +28,3 @@
import requests
from urllib.parse import urlparse

@@ -155,5 +156,12 @@

ALLOWED_DOMAINS = ["example.com", "anotherdomain.com"]

def is_allowed_domain(url):
"""Check if the URL's domain is in the allowed list."""
parsed_url = urlparse(url)
return parsed_url.netloc in ALLOWED_DOMAINS

def create_media(media):
"""Download media link"""
if is_valid_url(media.data_value):
if is_valid_url(media.data_value) and is_allowed_domain(media.data_value):
filename = media.data_value.split("/")[-1]
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
kelvin-muchiri
kelvin-muchiri reviewed Feb 14, 2025
View reviewed changes
onadata/apps/logger/tests/models/test_xform.py

# get exact choices element from choice abbreviated xpath
fruita_o = xform.get_survey_element("a/fruita/orange")
self.assertEqual(fruita_o.get_abbreviated_xpath(), "a/fruita/orange")
# fruita_o = xform.get_survey_element("a/fruita/orange")
Copy link
Contributor

Choose a reason for hiding this comment

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

@ukanga Why were these tests commented?

ukanga added 26 commits February 19, 2025 09:26
@ukanga
initial Django 5 requirements updates
d2c2e87
@ukanga
django.core.files.storage.get_storage_class is deprecated in favor of…
4f65896 
… using django.core.files.storage.storages
@ukanga
upgrade prospector==1.13.3 pylint==3.3.3
2805c26
@ukanga
disable lint checker in ci
3c7f732
@ukanga
Use datetime.timezone.utc
667f2cd
@ukanga
Use local function get_abbreviated_xpath
fd5f175 
Function removed from pyxform codebase
@ukanga
Upload trivy results only if in a public repository
ff060e6
@ukanga
refactor get_abbreviated_xpath to avoid circular imports
9ddf778
@ukanga
Publish Trivy results on slack for PRs
2705961
@ukanga
Use local function get_abbreviated_xpath
be72d17 
Function removed from pyxform codebase
@ukanga
Use datetime.timezone.utc
56f5d23
@ukanga
accommodate changes in pyxform
0f1e5f0
@ukanga
Use datetime.timezone.utc
b4a47b1
@ukanga
accommodate changes in pyxform
a081125
@ukanga
Use datetime.timezone.utc
fb00d25
@ukanga
accommodate changes in pyxform
5caafa1
@ukanga
Use datetime.timezone.utc
6f94818
@ukanga
Update pyxform
19b12c1
@ukanga
choices no longer in survey elements
7020bd2
@ukanga
Formatting ..
021d333
@ukanga
pyxform message changed
4349539
@ukanga
pyxform changes
1c73527
@ukanga
Use datetime.timezone.utc
bbf2b1d
@ukanga
pyxform changes
f02471c
@ukanga
Set use_pk_only_optimization() to False for GenericRelatedField
c403314
@ukanga
pyxform changes
0f77048
ukanga added 22 commits February 19, 2025 09:26
@ukanga
Use datetime.timezone.utc
364d45a
@ukanga
Use moto to mock AWS S3 functionality
fd792db
@ukanga
Choices are not field survey elements
2f3b5cd 
no need to test with build charts functionality
@ukanga
Remove Unused GenericExport imported from onadata.apps.viewer.models.…
4c3b615 
…export
@ukanga
refactor onadata.libs.utils.google to avoid pylint import issues with…
2b352dc 
… google-auth package
@ukanga
pylint cleanup
2a5ae1a
@ukanga
pylint cleanup
2c5b2de
@ukanga
fix mock requests tests
778c92e
@ukanga
Switch to python-deps:3.10.16-20250213
9b2229a 
- Update to python 3.10.16 from 3.10.14
- Add debian bookworm unstable to include newer glibc
@ukanga
formatting
c1e717f
@ukanga
formatting
890fbe7
@ukanga
Pyxform version osm fix
283f48a
@ukanga
Handle the case where survey.choices is None for charts
0be3eb6
@ukanga
pyxform: check that the survey element has "children" attribute
49dac6d
@ukanga
Recreate PyXForm survey from the XLSForm
0d2237c 
JSON structure changed in pyxform==3.0.0, saved JSON might fail to recreate a survey element
@ukanga
pyxform: check that the survey element has "children" attribute
ef3dc27
@ukanga
Skip tesing FLOIP publishing
5810c0d 
Not in use and package needs to be updated.
@ukanga
field.children not necessary
c2f4488 
choices only option for select questions
@ukanga
Skip tesing FLOIP publishing
1c7202d 
Not in use and package needs to be updated.
@ukanga
Get xpath for Option using the parent
8520f7f
@ukanga
Use [element].choices.options
899ae7f
@ukanga
Update choices in survey level for merged xforms
11d1952
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kelvin-muchiri kelvin-muchiri kelvin-muchiri left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ukanga @kelvin-muchiri