Skip to content
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

Add option to disable logging of request/response headers at DEBUG level #532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erlendvollset
Copy link

Currently access tokens are exposed in logs when running with DEBUG log level. In many cases it's desirable to run a process with log level DEBUG without exposing such secrets. If there is some other way you think this should be configured, please advise.

Currently access tokens are exposed in logs when running with DEBUG log level. In many cases it's desirable to run a process with log level DEBUG without exposing such secrets. If there is some other way you think this should be configured, please advise.
@jtroussard
Copy link
Contributor

I can take a closer look this weekend at this PR. At first blush though it will need an accompanying test or updates to existing tests.

Copy link
Contributor

@jtroussard jtroussard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall concept of controlling the level of logging in DEBUG mode is interesting and I can think of a few use cases that would benefit from this sort of control.

Considering implementation, it might be more fitting to integrate these controls directly with the logger to maintain separation of concerns.

For example, introducing a filter to the logger that adjusts based on DEBUG level could be a cleaner, less hardcoded approach. Additionally, offering options like NONE, MASKED, FULL for log visibility could add flexibility and keep a certain level of logging without causing security concerns.

While this PR has merit, incorporating these adjustments could enhance functionality and align with coding best practices, all with a fairly low LOE.

@jtroussard
Copy link
Contributor

First, I want to genuinely thank you @erlendvollset for your contribution and for spotlighting this security concern within the project. Your initiative is greatly appreciated. At the risk of stepping on toes, and after much consideration, I would like to propose and directly offer a different approach, which may address the issue more comprehensively. The nature of the changes I'm contemplating significantly diverges from the current proposal, extending beyond what might be practical to suggest via this PR alone or a simple PR code suggestion. To properly ilustrate this alt implementation I will open another PR inspired by this one (the commits will name you as a co-author). This is done with the utmost respect for your initial effort and is aimed solely at following a certain coding principles the community is trying to enforce. I hope this is taken in the collaborative spirit intended, and I'm more than open to discussing this further, potentially on even a different implementation all together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants