diff --git a/.config/CredScanSuppressions.json b/.config/CredScanSuppressions.json deleted file mode 100644 index ad1314938..000000000 --- a/.config/CredScanSuppressions.json +++ /dev/null @@ -1,21 +0,0 @@ -{ - "tool": "Credential Scanner", - "suppressions": [ - { - "file": "tests/*", - "justification": "Test projects contain sample credentials and should be skipped" - }, - { - "file": "benchmarks/*", - "justification": "Benchmark code may include test connection strings" - }, - { - "file": "eng/*", - "justification": "Engineering and pipeline configuration files" - }, - { - "file": "OneBranchPipelines/*", - "justification": "OneBranch pipeline configuration files" - } - ] -} diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 870bd8477..b21082d18 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -70,7 +70,7 @@ python -m pytest -v # 'stress' marker excl ## Security and credentials -- **Committed connection strings that contain `UID`/`PWD` must use `SERVER=localhost` (or `127.0.0.1`) with dummy values.** Real remote or Azure credentials come only from secrets or the `DB_CONNECTION_STRING` env var, and are never committed. Automated credential scanning (see `.config/CredScanSuppressions.json`, `.gdn/`) can block unsafe patterns. +- **Committed connection strings that contain `UID`/`PWD` must use `SERVER=localhost` (or `127.0.0.1`) with dummy values.** Real remote or Azure credentials come only from secrets or the `DB_CONNECTION_STRING` env var, and are never committed. Automated credential scanning (see `.gdn/`) can block unsafe patterns. - Do **not** put `Driver=` in a connection string — the bundled driver is selected automatically. - `TrustServerCertificate=yes` is local-development only; never suggest it in remote or production examples. diff --git a/.github/workflows/devskim.yml b/.github/workflows/devskim.yml index d82e02e24..2ae1a3746 100644 --- a/.github/workflows/devskim.yml +++ b/.github/workflows/devskim.yml @@ -13,6 +13,9 @@ on: schedule: - cron: '44 7 * * 5' +permissions: + contents: read + jobs: lint: name: DevSkim @@ -24,6 +27,8 @@ jobs: steps: - name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: false - name: Run DevSkim scanner uses: microsoft/DevSkim-Action@v1 diff --git a/.github/workflows/forked-pr-coverage.yml b/.github/workflows/forked-pr-coverage.yml index e616e8848..6ff9a326c 100644 --- a/.github/workflows/forked-pr-coverage.yml +++ b/.github/workflows/forked-pr-coverage.yml @@ -22,6 +22,9 @@ on: types: - completed +permissions: + contents: read + jobs: post-comment: runs-on: ubuntu-latest @@ -35,6 +38,8 @@ jobs: steps: - name: Checkout repo uses: actions/checkout@v4 + with: + persist-credentials: false - name: Download coverage data env: diff --git a/.github/workflows/lint-check.yml b/.github/workflows/lint-check.yml index 761620d10..35e36f7b2 100644 --- a/.github/workflows/lint-check.yml +++ b/.github/workflows/lint-check.yml @@ -19,7 +19,7 @@ on: - main permissions: - pull-requests: write + contents: read jobs: python-lint: @@ -29,6 +29,8 @@ jobs: steps: - name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: false - name: Set up Python uses: actions/setup-python@v5 @@ -85,6 +87,8 @@ jobs: steps: - name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: false - name: Set up Python (for cpplint) uses: actions/setup-python@v5 diff --git a/.github/workflows/pr-code-coverage.yml b/.github/workflows/pr-code-coverage.yml index c07204f3e..337615d46 100644 --- a/.github/workflows/pr-code-coverage.yml +++ b/.github/workflows/pr-code-coverage.yml @@ -5,6 +5,9 @@ on: branches: - main +permissions: + contents: read + jobs: coverage-report: runs-on: ubuntu-latest @@ -17,6 +20,7 @@ jobs: uses: actions/checkout@v4 with: fetch-depth: 0 + persist-credentials: false - name: Setup git for diff-cover run: | diff --git a/tests/conftest.py b/tests/conftest.py index 3440e598e..95d7cc3e7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,7 +2,8 @@ This file contains fixtures for the tests in the mssql_python package. Functions: - pytest_configure: Add any necessary configuration. -- conn_str: Fixture to get the connection string from environment variables. +- conn_str: Fixture to get the connection string from environment variables, + wrapped so its password is not printed in pytest failure output. - db_connection: Fixture to create and yield a database connection. - cursor: Fixture to create and yield a cursor from the database connection. - is_azure_sql_connection: Helper function to detect Azure SQL Database connections. @@ -12,9 +13,33 @@ import os import re from mssql_python import connect +from mssql_python.connection_string_parser import sanitize_connection_string import time +class _MaskedConnectionString(str): + """A str that behaves like the connection string but never reveals its + password when repr()'d. + + pytest prints every test argument in the failure header (``conn_str = + '...'``) and uses repr() to do it, so a plain str fixture puts the live + credential into CI logs on any failure in any test that takes conn_str, + not just the ones asserting on connection strings. Masking repr() keeps + the value fully usable while keeping the password out of that output. + + This has to be a subclass rather than a call to + sanitize_connection_string() at the point of use: pytest reads repr() off + the object it holds, str.__repr__ cannot be reassigned on the builtin, and + sanitizing the fixture value itself would leave the tests unable to + connect. + """ + + __slots__ = () + + def __repr__(self): + return repr(sanitize_connection_string(str(self))) + + def is_qemu_emulated(): """Detect if running under QEMU user-mode emulation (e.g. ARM64 on x86_64 host). @@ -53,7 +78,7 @@ def pytest_configure(config): @pytest.fixture(scope="session") def conn_str(): conn_str = os.getenv("DB_CONNECTION_STRING") - return conn_str + return _MaskedConnectionString(conn_str) if conn_str else conn_str @pytest.fixture(scope="module") diff --git a/tests/test_012_connection_string_integration.py b/tests/test_012_connection_string_integration.py index dc843ec8c..d03c9308b 100644 --- a/tests/test_012_connection_string_integration.py +++ b/tests/test_012_connection_string_integration.py @@ -13,6 +13,7 @@ from mssql_python.connection_string_parser import ( _ConnectionStringParser, ConnectionStringParseError, + sanitize_connection_string, ) from mssql_python.connection_string_builder import _ConnectionStringBuilder from mssql_python import connect @@ -493,12 +494,13 @@ def test_connect_with_real_database(self, conn_str): conn = connect(conn_str) assert conn is not None + # Assert on the sanitized string so a failure here cannot print the + # live credential through pytest assertion introspection. + sanitized = sanitize_connection_string(conn.connection_str) + # Verify connection string has required parameters - assert "Driver=" in conn.connection_str or "driver=" in conn.connection_str - assert ( - "APP=MSSQL-Python" in conn.connection_str - or "app=mssql-python" in conn.connection_str.lower() - ) + assert "Driver=" in sanitized or "driver=" in sanitized + assert "APP=MSSQL-Python" in sanitized or "app=mssql-python" in sanitized.lower() # Test basic query execution cursor = conn.cursor() @@ -521,12 +523,13 @@ def test_connect_kwargs_override_with_real_database(self, conn_str): # Verify connection works and autocommit is set assert conn.autocommit == True + # Assert on the sanitized string so a failure here cannot print the + # live credential through pytest assertion introspection. + sanitized = sanitize_connection_string(conn.connection_str) + # Verify connection string still has all required params - assert "Driver=" in conn.connection_str or "driver=" in conn.connection_str - assert ( - "APP=MSSQL-Python" in conn.connection_str - or "app=mssql-python" in conn.connection_str.lower() - ) + assert "Driver=" in sanitized or "driver=" in sanitized + assert "APP=MSSQL-Python" in sanitized or "app=mssql-python" in sanitized.lower() conn.close()