Skip to content

Conversation

himanshusinghs
Copy link
Collaborator

Proposed changes

This commit ensures that our examples promote use of environment variables for providing sensitive configuration options. Additionally we callout, where ever necessary, our recommendation of choosing env variables over command line arguments for the same.

Checklist

@himanshusinghs himanshusinghs requested a review from a team as a code owner September 11, 2025 12:49
@Copilot Copilot AI review requested due to automatic review settings September 11, 2025 12:49
Copy link
Contributor

@Copilot 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 updates the README to improve security by promoting the use of environment variables for sensitive configuration instead of command line arguments. The changes align with security recommendations to prevent sensitive data exposure through process lists and system logs.

  • Added security recommendations highlighting the use of environment variables over command line arguments
  • Updated all configuration examples to use environment variables for sensitive data (connection strings, API credentials)
  • Restructured sections to emphasize secure configuration practices

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Collaborator

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Seems reasonable, though note that export foo=bar works in bash, but not in pwsh or cmd. Would be great if we could show both ways of configuring things - e.g. in <details> or something similar.

Comment on lines +159 to +164
-e MDB_MCP_API_CLIENT_ID \
-e MDB_MCP_API_CLIENT_SECRET \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docker noob here, but does this work by passing through the environment vars?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup yup. We just need to make sure the env var is set and docker will pull the value from the referred env var name.

Copy link
Collaborator

@blva blva left a comment

Choose a reason for hiding this comment

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

LGTM

@himanshusinghs
Copy link
Collaborator Author

@blva @nirinchev I have added cmd and powershell examples as well and cross referenced them, if any of you want to take another look.

himanshusinghs and others added 4 commits September 11, 2025 17:14
This commit ensures that our examples promote use of environment
variables for providing sensitive configuration options. Additionally we
callout, whereever necessary, our recommendation of choosing env
variables over command line arguments for the same.
Co-authored-by: Copilot <[email protected]>
@himanshusinghs himanshusinghs force-pushed the chore/MCP-198-update-readme branch from e7e34bd to 86cfd03 Compare September 11, 2025 15:14
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 17649080824

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 81.204%

Files with Coverage Reduction New Missed Lines %
src/common/connectionManager.ts 6 87.78%
Totals Coverage Status
Change from base Build 17647912778: -0.1%
Covered Lines: 4774
Relevant Lines: 5784

💛 - Coveralls

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 17649033546

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 81.204%

Files with Coverage Reduction New Missed Lines %
src/common/connectionManager.ts 6 87.78%
Totals Coverage Status
Change from base Build 17647912778: -0.1%
Covered Lines: 4774
Relevant Lines: 5784

💛 - Coveralls

@himanshusinghs himanshusinghs merged commit ed8c19f into main Sep 11, 2025
17 of 18 checks passed
@himanshusinghs himanshusinghs deleted the chore/MCP-198-update-readme branch September 11, 2025 16:00
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.

4 participants