-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(settings): Add integrity checker #10212
Conversation
Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and 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 improving the security and integrity of the DefectDojo application's configuration settings. The key changes include:
Overall, these code changes demonstrate a strong focus on enhancing the security and maintainability of the DefectDojo application's configuration settings, which is a crucial aspect of application security. Files Changed:
Powered by DryRun Security |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Just a few small suggestions. Given that people probably won't know about this if they're editing settings.dist.py
as part of a PR, and we don't call this out explicitly for people who might be editing this on their instance, could you add a comment about this at the top of that file?
- Explain that this file shouldn't be edited for modifying a running instance (you can use the same message you used in the
ValueError
) - Inform developers that they will need to update the hash in
.settings.dist.py.sha256sum
if they are modifying this file as part of a PR and explain how to do so (so they don't have to wait for a test failure to know about this)
Co-authored-by: Charles Neill <1749665+cneill@users.noreply.github.com>
Thank you for your feedback. I agree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to ask for changes after approving, but since this will immediately break anyone who has modified their |
a633b79
to
c3494b2
Compare
No problem. @cneill, can you check the wording? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small suggestions. Thanks for adding this
Co-authored-by: Charles Neill <1749665+cneill@users.noreply.github.com>
@kiblik how extensive have you tested this? I am not able to build even I enabled the Debug mode.
|
I have tested the local change of I fixed this issue now: #10241 |
In many cases, users are changing
dojo/settings/settings.dist.py
file (they expect it is the way how the application should be configured).This PR adds a checker that does not allow to start app if the change is detected. The error message points the user to documentation.
It is allowed only in debug mode when changes are expected.
Unittest which helps you in debug mode was added as well.