Skip to content

Attempt to sanitize some configurations before we output them#988

Merged
motus merged 14 commits intomicrosoft:mainfrom
bpkroth:feature/avoid-logging-passwords
Jun 12, 2025
Merged

Attempt to sanitize some configurations before we output them#988
motus merged 14 commits intomicrosoft:mainfrom
bpkroth:feature/avoid-logging-passwords

Conversation

@bpkroth
Copy link
Copy Markdown
Contributor

@bpkroth bpkroth commented Jun 2, 2025

Pull Request

Title

Attempt to sanitize some configurations before we output them


Description

Address some code scanning complaints and try to sanitize some configs before outputing them.

Note: this is not meant as a complete fix for those warnings, but only a start. We can fix more locations using sanitize_config in the future prior to printing.


Type of Change

  • 🛠️ Bug fix

Testing

  • Security scans and other usual CI

Additional Notes

Also includes link fixes from #989


Copilot AI review requested due to automatic review settings June 2, 2025 21:41
@bpkroth bpkroth requested a review from a team as a code owner June 2, 2025 21:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a simple redaction step to configuration logging to avoid exposing sensitive values.

  • Introduces sanitize_conf to obfuscate the "password" key in any config dict.
  • Updates two debug‐level logging calls in config_persistence.py to use the sanitized config.
  • Addresses scanner complaints by preventing raw secrets from appearing in logs.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
mlos_bench/mlos_bench/util.py New sanitize_conf function to redact passwords.
mlos_bench/mlos_bench/services/config_persistence.py Import and apply sanitize_conf in debug logs.
Comments suppressed due to low confidence (2)

mlos_bench/mlos_bench/util.py:467

  • [nitpick] Consider renaming sanitize_conf to sanitize_config for clarity and consistency with the config parameter.
def sanitize_conf(config: dict[str, Any]) -> dict[str, Any]:

mlos_bench/mlos_bench/util.py:467

  • Add unit tests for sanitize_conf to verify that sensitive keys are redacted correctly and that other fields remain unchanged.
def sanitize_conf(config: dict[str, Any]) -> dict[str, Any]:

Comment thread mlos_bench/mlos_bench/util.py Outdated
Comment thread mlos_bench/mlos_bench/util.py Outdated
@bpkroth bpkroth added bug Something isn't working ready for review Ready for review ci labels Jun 5, 2025
@bpkroth bpkroth mentioned this pull request Jun 5, 2025
motus pushed a commit that referenced this pull request Jun 12, 2025
# Pull Request

## Title

Fixup some broken links

______________________________________________________________________

## Description

Fixes some broken links preventing the Markdown Link Check CI job from
passing.

______________________________________________________________________

## Type of Change

- 🛠️ Bug fix

______________________________________________________________________

## Testing

Existing CI jobs

> Note: the currently failing CI job about code scanning is addressed in
#988

______________________________________________________________________
@motus motus enabled auto-merge (squash) June 12, 2025 01:11
@motus motus merged commit 3111f9b into microsoft:main Jun 12, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ci ready for review Ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants