-
Notifications
You must be signed in to change notification settings - Fork 1.8k
🐛 Robustify create_user to handle None value #13572
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
Conversation
🔴 Risk threshold exceeded.This pull request modifies sensitive codepaths (notably dojo/pipeline.py and dojo/middleware.py) triggering configured-codepath alerts, and also includes broad exception logging in dojo/middleware.py that may disclose sensitive information (e.g., PII or tokens) by logging raw exception objects. Review and restrict who can edit those files via .dryrunsecurity.yaml and sanitize or redact logged exceptions to avoid information leakage.
🔴 Configured Codepaths Edit in
|
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/pipeline.py
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/middleware.py
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/pipeline.py
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
🔴 Configured Codepaths Edit in dojo/pipeline.py
| Vulnerability | Configured Codepaths Edit |
|---|---|
| Description | Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml. |
Information Disclosure via Error Logging in dojo/middleware.py
| Vulnerability | Information Disclosure via Error Logging |
|---|---|
| Description | The code logs the full string representation of any unhandled exception during social login using logger.error(f"Unhandled exception during social login: {exception}"). This practice carries a significant risk of information disclosure. Exception objects, especially those originating from authentication libraries or external identity providers, can contain sensitive data such as user PII (e.g., email addresses, full names), authentication tokens, session IDs, or internal system details (e.g., file paths, configuration values, stack traces). Logging the raw exception object without sanitization or redaction creates a risk of exposing this sensitive information in application logs, which could be accessed by unauthorized individuals or systems. While specific examples of social_core exceptions leaking PII were not found with the available tools, the general principle of secure logging dictates that such broad logging of exception details in a security-sensitive context is a vulnerability. |
django-DefectDojo/dojo/middleware.py
Lines 101 to 104 in fe01743
| logger.error(f"Unhandled exception during social login: {exception}") | |
| return super().process_exception(request, exception) | |
We've notified @mtesauro.
All finding details can be found in the DryRun Security Dashboard.
quirinziessler
left a 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.
Are you sure you only want to log this as warning? Error and gracefully stopping the login process would be better imo.
Thank you @quirinziessler for your review |
|
Would it be possible to raise this error in the UI by using the middleware introduced in your other PR? Not sure if this pipeline function would be too late in the process or not |
|
Ok, merge the other PR and I will check if I can add it there with rebasing this PR. |
|
I don't want to add an additional functionality after everyone has approved and right before it gets merged. |
5210c61 to
5fa87c7
Compare
|
Done @Maffooch, it works solid, you can review |
Maffooch
left a 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.
@manuel-sommer nice work 😄
mtesauro
left a 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.
Approved
Uh oh!
There was an error while loading. Please reload this page.