FIX: security issue SEC101/037 : SQLLegacyCredentials fix #369
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
subrata-ms
requested review from
bewithgaurav,
gargsaumya,
jahnvi480 and
sumitmsft
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.pybind.ddbc_bindings.cpp: 66.3%
mssql_python.helpers.py: 67.5%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.connection.py: 83.7%
mssql_python.cursor.py: 84.3%
mssql_python.logging.py: 85.3%🔗 Quick Links
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a security issue (SEC101/037: SQLLegacyCredentials) by removing hardcoded credentials from test connection strings. The fix replaces UID and PWD parameters with Trusted_Connection=yes across multiple test files and introduces environment variable usage for server and database names in select tests.
Key changes:
- Removed all hardcoded username/password credentials from test connection strings
- Replaced credential parameters with
Trusted_Connection=yesfor Windows authentication - Added environment variable support (
TEST_SERVER,TEST_DATABASE) in one test function to enable configurable test environments
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_002_types.py | Updated three connection strings in test_invalid_surrogate_handling() to use Trusted_Connection=yes and added environment variable support for server/database names with os.getenv() |
| tests/test_013_sqlwchar_conversions.py | Systematically replaced all hardcoded UID=user;PWD=pass with Trusted_Connection=yes across 21 connection strings in various test methods |
| tests/test_014_ddbc_bindings_coverage.py | Replaced all hardcoded UID=u;PWD=p with Trusted_Connection=yes across 12 connection strings in various test methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
This pull request updates several test files to improve security and consistency in database connection strings. The main change is replacing hardcoded usernames and passwords with
Trusted_Connection=yesin all connection strings, and in some cases, using environment variables for server and database names. This prevents the exposure of credentials and avoids related security warnings.Test connection string improvements:
Replaced all hardcoded
UIDandPWDparameters in connection strings withTrusted_Connection=yesintests/test_002_types.py,tests/test_013_sqlwchar_conversions.py, andtests/test_014_ddbc_bindings_coverage.py, enhancing security and aligning with best practices. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22] [23] [24] [25] [26] [27] [28] [29] [30]Updated some tests in
tests/test_002_types.pyto useos.getenvforTEST_SERVERandTEST_DATABASE, allowing for configurable test environments and reducing hardcoded values. [1] [2] [3]These changes make the test suite safer to run in different environments and prevent accidental credential leaks.