Skip to content

Conversation

@bpkroth
Copy link
Contributor

@bpkroth bpkroth commented Jun 13, 2025

Pull Request

Title

Sanitize more config logging.


Description

Follow ups to #988


Type of Change

  • 🛠️ Bug fix

Testing

CI


Copilot AI review requested due to automatic review settings June 13, 2025 03:52
@bpkroth bpkroth requested a review from a team as a code owner June 13, 2025 03:52
Copy link
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

This PR enhances security by sanitizing configuration data before logging to prevent sensitive information exposure.

  • Wrapped raw JSON inputs with sanitize_config in logging calls.
  • Updated warning and info messages to use sanitized output.
  • Follow-up adjustments from PR #988 to cover additional logging points.
Comments suppressed due to low confidence (3)

mlos_bench/services/config_persistence.py:197

  • The exception message in the raise ValueError still uses the raw json and may expose sensitive data. Use sanitize_config(json) in the formatted message to ensure consistency and prevent leakage.
_LOG.error("Failed to parse config from JSON string: %s", sanitize_config(json))

mlos_bench/services/config_persistence.py:703

  • [nitpick] Logging the entire jsons list may generate very large logs and impact readability. Consider logging a summary (e.g., count of services) or truncating long entries after sanitization.
_LOG.info("Load services: %s parent: %s", sanitize_config(jsons), parent.__class__.__name__)

mlos_bench/services/config_persistence.py:738

  • [nitpick] The variable name jsons is ambiguous and could be confused with the JSON module. Consider renaming it to configs or tunable_payloads for clarity.
def load_tunables(...):

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