Opened 2 years ago
Closed 2 years ago
#9544 defect closed fixed (fixed)
adbapi.ConnectionPool.connect logs password
Reported by: | Nolan Prescott | Owned by: | Tom Most |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | Nolan Prescott | Branch: | |
Author: | 9544-adbapi-logging |
Description
When noisy
is set to True, connections are logged with connection arguments and keyword arguments, resulting in a log line similar to:
2018-10-02T17:08:02+0000 [-] adbapi connecting: psycopg2 {'database': 'database-name', 'host': 'database-host', 'user': 'my-username', 'password': 'leaked-password', 'port': ''}
This stems from adbapi.py:426 --
if self.noisy: log.msg('adbapi connecting: %s %s%s' % (self.dbapiName, self.connargs or '', self.connkw or ''))
This was touched on in ticket #1806, where the noisy
default was changed as a security risk. This was also fixed/removed entirely from the _close
method as part of #871.
Change History (3)
comment:1 Changed 2 years ago by
Cc: | Nolan Prescott added |
---|---|
Keywords: | review added |
comment:2 Changed 2 years ago by
Author: | → 9544-adbapi-logging |
---|---|
Keywords: | review removed |
Owner: | set to Tom Most |
Status: | new → assigned |
I posted a review on GitHub: https://github.com/twisted/twisted/pull/1074#pullrequestreview-164541672 +1
If the None comparison change merited all the overhead of its own ticket/newsfile/review then I'd ask for that, but I think it is fine to make such a trivial stylistic change when working on adjacent code. The tests still pass, after all. This is in line with our general approach of "fix linting issues where you find them, and add tests where there were none."
Thanks for contributing to Twisted!
https://github.com/twisted/twisted/pull/1074
The proposed fix duplicates the approach for #871 - removing the connection arguments and keyword arguments from the log message entirely.
I've included a second commit in the PR to fix an issue with the linter around a comparison to
None
, please let me know if this is out of band as the line in question was unchanged as part of my "fix" for this ticket.