Skip to content

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.

2 participants