Opened 4 years ago
Closed 5 days ago
#31604 closed Uncategorized (wontfix)
Should SafeExceptionReporterFilter cleanse setting keys whose name matches "URL" in part, too?
Reported by: | Sebastian Pipping | Owned by: | |
---|---|---|---|
Component: | Error reporting | Version: | dev |
Severity: | Normal | Keywords: | security debug |
Cc: | Sebastian Pipping | Triage Stage: | Unreviewed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hi!
Since #23004 is fixed (commit 581ba5a9486ed73cb81031d85b3ce1b27a960109, Django 3.1a1 and later) setting .DEFAULT_EXCEPTION_REPORTER_FILTER
allows customizing the filter that hides away values of variables whose name match a certain pattern, API|TOKEN|KEY|SECRET|PASS|SIGNATURE
(ignoring case) by default. So if I want to filter away more than the default, I could do that for my own Django project.
I noticed that URL
is not in that list while people seem to put credentials into the authority part of URLs — not just database URLs but also RabbitMQ and Celery connection URLs, used in so-called "development environments" with DEBUG=True
enabled, accessible by public internet. While the real fix is DEBUG=False
of course, I consider getting more people on safe ground. Filtering URLs away altogether is probably inconvenient and maybe more than needed. I consider adding code to parse URLs, replace user and password by SafeExceptionReporterFilter.cleansed_substitute
each where present, and display the result for a value.
Would that be considered an anti-feature or should I go for a pull request?
Best, Sebastian
Change History (4)
comment:1 by , 4 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
comment:2 by , 6 days ago
Cc: | added |
---|---|
Has patch: | set |
Keywords: | security debug added |
Resolution: | wontfix |
Status: | closed → new |
Re-opening because it took part in allowing potential arbitrary remote code execution as explained at https://github.com/climateconnect/climateconnect/pull/1331#issuecomment-2397881433 in practice … Pull request upcoming…
comment:4 by , 5 days ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Please don't reopen "wontfix" tickets. TicketClosingReasons/DontReopenTickets
Hi. Thanks for the report.
I think the idea is precisely that after #23004 you customise this for your own project. The underlying issue is that there are any number of possible customisations that would be appropriate for some given project, and it's simply not feasible to keep adding them all.
In this particular case, the additional benefit to parsing sensitive URLs (vs simply filtering them entirely in a subclass) doesn't seem worth the complexity. (Given that URLs end up in log files, which is out-of-scope for Django, we see complaints if sensitive values get used in URLs at all, so I'm half-inclined towards thinking the issue here lies elsewhere.)
I hope that makes sense.