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

Upgrade Django to 4.2.13 #9493

Merged
merged 6 commits into from
Jun 13, 2024
Merged

Upgrade Django to 4.2.13 #9493

merged 6 commits into from
Jun 13, 2024

Conversation

kiblik
Copy link
Contributor

@kiblik kiblik commented Feb 6, 2024

Upgrade to Django 5.x #9258 might be quite complated but we need to go to at least 4.2.13 because 4.1.x is not supported anymore (check https://www.djangoproject.com/download/)

Needs to be checked/considered:

Copy link

dryrunsecurity bot commented Feb 6, 2024

Contextual Security Analysis

As DryRun Security performs checks, we’ll summarize them here. You can always dive into the detailed results in the section below for checks.

Status DryRun Security Check
Sensitive Functions Analyzer
Configured Sensitive Files Analyzer
Sensitive Files Analyzer

Chat with your AI-powered Security Buddy by typing @dryrunsecurity followed by your question into a comment.
Example: @dryrunsecurity What are common security issues with web application cookies?

Install and configure more repositories at DryRun Security

@Maffooch
Copy link
Contributor

Maffooch commented Feb 6, 2024

I think the best approach here is to remove tests for MySQL by removing the following entry from the integration tests and then replace all the mysql references with Postgres

@kiblik
Copy link
Contributor Author

kiblik commented Feb 7, 2024

@Maffooch, noticed that you implemented #5899.
Now errors.append(ValidationError('Query "{}" has invalid format - It contains the NULL character. The following action was taken: {}'.format(old_value, action_string))) breaks test.
If correction (query.replace(remove_str, '%00')) was performed, is it still necessary to raise ValidationError?
I'm considering change it to logger.error.

@Maffooch
Copy link
Contributor

Maffooch commented Feb 7, 2024

I guess it doesn't really make sense to raise a validation error on a successful correction.. Changing to a logger statement makes sense. Good call!

@github-actions github-actions bot added the settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR label Feb 8, 2024
@kiblik
Copy link
Contributor Author

kiblik commented Feb 9, 2024

Help needed

I'm facing unusual behavior. Based on unittests, in test_rest_framework.py I adjusted results for deleted_objects. For Language_Type from 2 to 1 and for TestType from 18 to 5.
At first, this kind of adjustment should not be done (if there is no change in the data of the algorithm). But I wanted to see the results.

Now, the complicated part:

  • Why it happen?
  • Why unittests for Alpine pass but for Debian fail (with a request to roll back from 5 to 18 for TestsTest and not mentioning Language_Type)

@Maffooch
Copy link
Contributor

@kiblik this is a very strange error, and I do not understand why it is happening. I am hoping to get some time in the near future to pull this PR down and play around with it

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link

dryrunsecurity bot commented Mar 4, 2024

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Authn/Authz Analyzer 0 findings
Configured Codepaths Analyzer 0 findings
Sensitive Files Analyzer 1 finding
AppSec Analyzer 0 findings
Secrets Analyzer 0 findings

Note

🟢 Risk threshold not exceeded.

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The code changes in this pull request focus on several key areas that have a direct impact on the security and reliability of the application:

  1. Dependency Updates: The changes update the Django version from 4.1.13 to 4.2.13, which is a positive security improvement as it likely addresses known vulnerabilities in the previous version. The changes also update the dependencies of the DefectDojo application, including security-related libraries like cryptography and argon2-cffi, which is a good security practice.

  2. Integration Testing Workflow: The changes to the GitHub Actions workflow for running integration tests introduce several security-relevant updates, such as the switch from MySQL to PostgreSQL as the database backend, the change from RabbitMQ to Redis as the message queue, and the phased startup approach to ensure the application's dependencies are properly configured before running the tests. These changes help improve the overall security and reliability of the testing process.

  3. User Authorization Tests: The changes to the unit tests for the get_authorized_users function in the dojo.user.queries module focus on validating the correct authorization and access control mechanisms for different user roles and permissions. This is a good security practice to ensure the application's authorization logic is working as expected.

  4. Remote User Authentication: The changes to the test_remote_user.py file demonstrate a well-designed and secure implementation of the remote user authentication functionality, with a comprehensive set of test cases covering various scenarios, such as trusted/untrusted proxy handling and API schema visibility.

Overall, the changes in this pull request appear to be focused on improving the security and reliability of the application by updating dependencies, enhancing the testing workflow, validating authorization logic, and implementing secure remote user authentication. These are all important aspects of maintaining a secure and well-functioning application.

Files Changed:

  1. requirements.txt: The changes update the Django version from 4.1.13 to 4.2.13, which is a positive security improvement.
  2. .github/workflows/integration-tests.yml: The changes update the database configuration from MySQL to PostgreSQL and the message queue configuration from RabbitMQ to Redis, which could have security implications that should be reviewed. The changes also introduce a phased startup approach and improve the handling of exit codes for the integration tests.
  3. unittests/test_user_queries.py: The changes focus on improving the unit tests for the get_authorized_users function, which is important for validating the application's authorization and access control mechanisms.
  4. unittests/test_remote_user.py: The changes demonstrate a well-designed and secure implementation of the remote user authentication functionality, with a comprehensive set of test cases covering various scenarios.

Powered by DryRun Security

Copy link
Contributor

github-actions bot commented Apr 3, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@kiblik
Copy link
Contributor Author

kiblik commented Apr 5, 2024

@Maffooch, it looks like the mentioned issue is not connected to the upgrade to Django 4.2 but to migration tests from MySQL to Postgres
This fails in the same way #9885

Copy link
Contributor

github-actions bot commented Apr 9, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed conflicts-detected settings_changes Needs changes to settings.py based on changes in settings.dist.py included in this PR labels Apr 9, 2024
@kiblik kiblik marked this pull request as ready for review June 5, 2024 07:46
@kiblik kiblik changed the title Upgrade Django to 4.2.10 Upgrade Django to 4.2.13 Jun 5, 2024
@kiblik
Copy link
Contributor Author

kiblik commented Jun 5, 2024

I'm happy that we are ready, just be careful with timing:

Dropped support for MySQL 5.7
Upstream support for MySQL 5.7 ends in October 2023. Django 4.2 supports MySQL 8 and higher.
Source: https://docs.djangoproject.com/en/5.0/releases/4.2/#dropped-support-for-mysql-5-7

Docker-compose.yml uses mysql:5.7.44 right now.

@Maffooch
Copy link
Contributor

Maffooch commented Jun 5, 2024

@kiblik I am glad you mentioned this in this PR :) this is a good opportunity to link to the discussion where this change is announced by @mtesauro

#9690

@kiblik kiblik mentioned this pull request Jun 6, 2024
@cneill
Copy link
Contributor

cneill commented Jun 6, 2024

Since our version of MySQL isn't supported by the version of Django we're moving to and it's our intention to deprecate MySQL with the next release, should we go ahead and remove it entirely as part of this PR?

@mtesauro
Copy link
Contributor

mtesauro commented Jun 6, 2024

Since our version of MySQL isn't supported by the version of Django we're moving to and it's our intention to deprecate MySQL with the next release, should we go ahead and remove it entirely as part of this PR?

I'd prefer to keep this PR only about the Django update and have a separate PR to remove MySQL from the various places.
This PR is complicated enough without adding to it's scope.

FWIW, I'll be working on a PR to remove MySQL & RabbitMQ from the compose files over the weekend.

Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

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

Approved

@Maffooch Maffooch merged commit 665c6b0 into DefectDojo:dev Jun 13, 2024
123 checks passed
@kiblik kiblik deleted the django_4.2.10 branch June 13, 2024 22:10
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.

5 participants