-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(disclaimers): Split disclaimers #10902
Conversation
DryRun Security SummaryThe pull request focuses on improving the handling and display of disclaimers in the Defect Dojo application by introducing separate settings for notifications and reports, enhancing security through input sanitization, and updating templates to use more granular disclaimer controls. Expand for full summarySummary: The code changes in this pull request are primarily focused on improving the handling and display of disclaimers in various parts of the Defect Dojo application, particularly in the context of notifications and reports. The key changes include:
Files Changed:
Code AnalysisWe ran
Overall Riskiness🟡 Please give this pull request extra attention during review. |
e3a271b
to
c9a2893
Compare
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.
This could be consolidated into the other migration
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.
I'm not sure I understand this comment
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.
migrations 0220 and 0219 could be consolidated into the same plugin
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.
When (for any reason) RunPython
fails, migration is not marked as done. You can try to rerun migrations when you fix the issue (it might be any issue). The problem is if RunPython
is part of other migration steps (like RenameField
, AddField
, ...) that have been performed successfully. You can try to rerun migration but it will fail because you are not able to run the same RenameField
or AddField
.
RunPython
is the most sensitive step failure.
Because of this, we agreed that in this project, we will always put RunPython
to the separated migration.
Same as:
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.
Ahh okay that makes sense! I'll keep this in mind going forward (for review and my own migrations)
Thanks for bringing the receipts 😄
migrations.RenameField( | ||
model_name='system_settings', | ||
old_name='disclaimer', | ||
new_name='disclaimer_notifications', |
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.
I don't know how common this is, but it might affects users setting the disclaimer via the api. A little note in the upgrade notes? It also affects people who are using customized notification templates.
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
Until now, all disclaimers have been the same which is not the best.
Now you can choose between:
disclaimer_notifications
disclaimer_reports
There is one more to inform users that they should not include any personal information in notes:
disclaimer_notes
Plus if there is an internal policy to have a disclaimer in all reports, it is possible to use
disclaimer_reports_forced
to not allow users to be excluded it.E.g.: