Adds logger filter for TOKENS when logging in DEBUG MODE #539
Conversation
Co-authored-by: Erlend vollset <erlendvollset@gmail.com>
|
@erlendvollset : Github wouldn't let me ping you in the reviewers windows so I'm doing it here @XMoose25X : Thought you might want to take a peek/observe/comment/correct/whatever-your-heart-desires xD |
jtroussard
force-pushed
the
536-access-tokens-leaked-in-logs-when-using-debug-level
branch
from
313fe40
to
a77b392
Compare
|
Converting to a draft until I have time to thoroughly test the regex, and provide evidence. |
requests_oauthlib/log_filters.py
Outdated
| """ | ||
| if record.levelno == logging.DEBUG: | ||
| if self.mode == "MASK": | ||
| record.msg = re.sub(r'Bearer\s+([A-Za-z0-9\-._~+\/]+)', '[MASKED]', record.getMessage()) |
There was a problem hiding this comment.
- I would suggest compiling the regex pattern once to avoid doing that for every log message.
- I would suggest putting this behind an LRU cache as we'll be logging a lot of the same statements repeatedly.
re.subis expensive enough that we would want to avoid doing it each time.
| if self.mode == "MASK": | ||
| record.msg = re.sub(r'Bearer\s+([A-Za-z0-9\-._~+\/]+)', '[MASKED]', record.getMessage()) | ||
| elif self.mode == "SUPPRESS": | ||
| return False |
There was a problem hiding this comment.
Maybe only suppress log messages which match the above regex? Otherwise this equivalent to disabling the logger entirely.
There was a problem hiding this comment.
I am hoping a single space would suffice as a replacement? Or maybe just pass None to the msg?
requests_oauthlib/log_filters.py
Outdated
| environment variable. | ||
| """ | ||
| super().__init__() | ||
| self.mode = os.getenv('DEBUG_MODE_TOKEN_FILTER', 'DEFAULT').upper() |
There was a problem hiding this comment.
I would suggest prefixing the envvar with the library name to reduce likelihood of collisions with other environment variables.
| self.mode = os.getenv('DEBUG_MODE_TOKEN_FILTER', 'DEFAULT').upper() | |
| self.mode = os.getenv('REQUESTS_OAUTHLIB_DEBUG_MODE_TOKEN_FILTER', 'DEFAULT').upper() |
There was a problem hiding this comment.
Love this comment. In fact I was running into this issue in another project. This project then variable name pattern is clean. I'm going to borrow and apply this pattern there as well. 🙏 Thank you!
|
@erlendvollset This is great feedback! I'll try and carve out some time this weekend to address these comments. Thanks again! |
|
Pending TODOs
|
This PR is a derivative of another #532
This PR addresses the same issue but aims to demonstrate a broader perspective on implementation. While the original PR highlighted a significant concern, the proposed solution here follows an existing patten of configuring the logger and leveraging the features within the logger itself. Following this pattern reinforces and encourages contributions in a more uniform and predictable way for future logger configurations.
This approach also enables a more comprehensive cleansing of sensitive information from logs, extending beyond headers to all areas of the code base.
The necessity to create a new PR stems from the considerable deviation in approach, which couldn't be effectively communicated within the framework of #532's PR discussion. To acknowledge the foundational work laid by the original proposal, this submission incorporates co-authoring commits, reinforcing a collaborative effort towards a scalable and secure logging strategy.
Relevant links
python logger docs
SO