-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: add injectable LoggerSanitizer to mask sensitive data in logs #16771
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
base: master
Are you sure you want to change the base?
Conversation
e91eee0 to
c50f963
Compare
c50f963 to
f61b65c
Compare
Introduce LoggerSanitizer service that automatically masks credentials in log messages to prevent sensitive data leakage (e.g., proxy URLs with username:password, api keys). - Add LoggerSanitizer interface and DefaultLoggerSanitizer implementation - Integrate sanitizer into Logger.format() - Provide a base set of sanitization rules to mask any URL protocol with credentials, api keys and authtokens - Make sanitizer injectable and optional - Add unit test cases Contributed on behalf of STMicroelectronics
f61b65c to
803d552
Compare
| const sanitized = this.sanitize(stringified); | ||
| return JSON.parse(sanitized); | ||
| } catch { | ||
| return value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have two distinct try/catch here, to return sanitize if we successfully parsed value, but failed to parse the sanitized value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks! I added an inner try/catch to return the sanitized string if parsing fails, along with a test case to cover this scenario too.
CamilleLetavernier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good to me!
| * Capture groups: $1=protocol, $2=username, $3=password, $4=host (with optional port) | ||
| */ | ||
| pattern: /([a-z][a-z0-9+.-]*:\/\/)([^:/@]+):([^:/@]+)@([^/:@\s]+(?::\d+)?)/gi, | ||
| pattern: /([a-z][a-z0-9+.-]+:\/\/)([^:/@]+):([^:/@]+)@([^/:@\s]+(?::\d+)?)/giu, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think '*' is better in this case, as we already check [a-z] first (so a:// would be a valid protocol)
[a-z][a-z0-9+.-]+: one letter followed by at least one alphanumeric character (so at least 2 characters) ❌
[a-z][a-z0-9+.-]*: one letter, optionally followed by alphanumeric characters (so at least 1 character) ✔️
143f286 to
d32d880
Compare
CamilleLetavernier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
What it does
Introduce LoggerSanitizer service that automatically masks credentials in log messages to prevent sensitive data leakage (e.g., proxy URLs with username:password, api keys).
How to test
"HTTP_PROXY": "http://myusername:[email protected]:8080""--log-file=example.log"to make log inspection easier"HTTP_PROXY": "http://****:****@proxy.example.com:8080"Follow-ups
Breaking changes
Attribution
Contributed on behalf of STMicroelectronics
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers