From 6ccee67457deebd97fc9d49cd7905eec3cf5f621 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 12:22:50 +0530 Subject: [PATCH 1/6] CHORE: keep db credentials out of pytest output and tighten workflow permissions The conn_str fixture returned a plain str, and pytest renders every test argument in the failure header with repr(). Any failing test that takes conn_str therefore printed the live connection string, password included, into CI logs. That is 205 tests across 16 files, and it fires on any assertion failure, not just the ones that look at connection strings. The fixture now returns a str subclass whose repr() runs the value through sanitize_connection_string(). The value itself is unchanged, so equality, str(), f-strings and concatenation all behave as before. test_012 separately asserted on conn.connection_str in the two tests that run against a real database. Those now assert on a sanitized local, so a failure cannot surface the credential through assertion introspection. Workflows: lint-check granted pull-requests: write at workflow level but neither job writes to pull requests, so it drops to contents: read. devskim, pr-code-coverage and forked-pr-coverage only declared permissions per job and now carry a contents: read default, with the existing job-level grants left intact. Every actions/checkout that does not push gets persist-credentials: false. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/devskim.yml | 5 ++ .github/workflows/forked-pr-coverage.yml | 5 ++ .github/workflows/lint-check.yml | 6 ++- .github/workflows/pr-code-coverage.yml | 4 ++ tests/conftest.py | 23 ++++++++- .../test_012_connection_string_integration.py | 50 +++++++++++++++---- 6 files changed, 80 insertions(+), 13 deletions(-) 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..93f7e2333 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,27 @@ 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. + """ + + __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 +72,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..92c0fb23c 100644 --- a/tests/test_012_connection_string_integration.py +++ b/tests/test_012_connection_string_integration.py @@ -13,9 +13,11 @@ 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 +from conftest import _MaskedConnectionString class TestConnectionStringIntegration: @@ -493,12 +495,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 +524,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() @@ -651,3 +655,29 @@ def test_connect_multiple_empty_values_raises_error(self, mock_ddbc_conn): assert len(errors) >= 2 assert any("Empty value for keyword 'server'" in err for err in errors) assert any("Empty value for keyword 'pwd'" in err for err in errors) + + +class TestConnStrFixtureRepr: + """The conn_str fixture is wrapped so pytest failure headers do not print + the password. These lock that behaviour in.""" + + def test_repr_masks_password(self): + raw = "Server=localhost,1433;Database=master;UID=sa;PWD=Sup3rSecret!;Encrypt=no" + masked = _MaskedConnectionString(raw) + assert "Sup3rSecret!" not in repr(masked) + assert "***" in repr(masked) + + def test_value_is_unchanged(self): + raw = "Server=localhost,1433;Database=master;UID=sa;PWD=Sup3rSecret!;Encrypt=no" + masked = _MaskedConnectionString(raw) + assert isinstance(masked, str) + assert masked == raw + assert str(masked) == raw + assert f"{masked}" == raw + + def test_repr_masks_braced_password(self): + # Braced values may contain semicolons; the sanitizer must not truncate + # and leak the tail. + raw = "Server=localhost;UID=sa;PWD={p@ss;w}}rd};Encrypt=no" + masked = _MaskedConnectionString(raw) + assert "p@ss" not in repr(masked) From 2552944986c56a25090a23823c25b25ea92f1da2 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 13:15:05 +0530 Subject: [PATCH 2/6] CHORE: drop the unwired credscan suppressions file `.config/CredScanSuppressions.json` has never been in effect. CredScan only reads a suppressions file when a pipeline passes `credscan.suppressionsFile`, and no pipeline here does, so the directory globs it lists have never suppressed anything. Guardian is what actually runs, wired in all three OneBranch pipelines through `sdl.suppression.suppressionFile` pointing at `.gdn/.gdnsuppress`, which excuses individual findings by signature rather than by directory. Leaving the file in place is worse than not having it. It sits at the conventional path so it reads as active, and the next person to notice it is as likely to wire it up as to remove it. Wiring it up would switch credential scanning off across `tests/`, `benchmarks/`, `eng/` and `OneBranchPipelines/`. copilot-instructions.md pointed at the file, so that reference now names `.gdn/` only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .config/CredScanSuppressions.json | 21 --------------------- .github/copilot-instructions.md | 2 +- 2 files changed, 1 insertion(+), 22 deletions(-) delete mode 100644 .config/CredScanSuppressions.json 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. From ed17bd958c66d4f656d80d3f7fab52ff7e7263f7 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 13:35:41 +0530 Subject: [PATCH 3/6] CHORE: use the repo's dummy password vocabulary in the new fixture tests The new repr tests used a realistic-looking password literal. Committed connection strings here stick to Server=localhost with a plain dummy value, and the value now matches what the rest of tests/ already uses, so credential scanning has nothing new to flag. A new finding would not be in .gdn/.gdnsuppress and would break the build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/test_012_connection_string_integration.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_012_connection_string_integration.py b/tests/test_012_connection_string_integration.py index 92c0fb23c..168a40532 100644 --- a/tests/test_012_connection_string_integration.py +++ b/tests/test_012_connection_string_integration.py @@ -662,13 +662,13 @@ class TestConnStrFixtureRepr: the password. These lock that behaviour in.""" def test_repr_masks_password(self): - raw = "Server=localhost,1433;Database=master;UID=sa;PWD=Sup3rSecret!;Encrypt=no" + raw = "Server=localhost,1433;Database=master;UID=sa;PWD=secret123;Encrypt=no" masked = _MaskedConnectionString(raw) - assert "Sup3rSecret!" not in repr(masked) + assert "secret123" not in repr(masked) assert "***" in repr(masked) def test_value_is_unchanged(self): - raw = "Server=localhost,1433;Database=master;UID=sa;PWD=Sup3rSecret!;Encrypt=no" + raw = "Server=localhost,1433;Database=master;UID=sa;PWD=secret123;Encrypt=no" masked = _MaskedConnectionString(raw) assert isinstance(masked, str) assert masked == raw From e9f2520cd18094241f1438121f45ba55cc213b75 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 13:40:55 +0530 Subject: [PATCH 4/6] CHORE: collapse the fixture repr tests into one test_007_logging already covers sanitize_connection_string thoroughly, braced values with semicolons and escaped braces included, so testing that surface again through the wrapper added nothing. What is actually new here is that repr() routes through the sanitizer at all and that the subclass still behaves like the string it wraps, which is one test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../test_012_connection_string_integration.py | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/tests/test_012_connection_string_integration.py b/tests/test_012_connection_string_integration.py index 168a40532..1f99bc79b 100644 --- a/tests/test_012_connection_string_integration.py +++ b/tests/test_012_connection_string_integration.py @@ -659,25 +659,14 @@ def test_connect_multiple_empty_values_raises_error(self, mock_ddbc_conn): class TestConnStrFixtureRepr: """The conn_str fixture is wrapped so pytest failure headers do not print - the password. These lock that behaviour in.""" + the password. Sanitizer behaviour itself is covered in test_007_logging.""" - def test_repr_masks_password(self): + def test_repr_is_masked_and_value_is_untouched(self): raw = "Server=localhost,1433;Database=master;UID=sa;PWD=secret123;Encrypt=no" masked = _MaskedConnectionString(raw) + # repr() is what pytest prints in the failure header assert "secret123" not in repr(masked) - assert "***" in repr(masked) - - def test_value_is_unchanged(self): - raw = "Server=localhost,1433;Database=master;UID=sa;PWD=secret123;Encrypt=no" - masked = _MaskedConnectionString(raw) + # the value still has to behave exactly like the string it wraps assert isinstance(masked, str) assert masked == raw assert str(masked) == raw - assert f"{masked}" == raw - - def test_repr_masks_braced_password(self): - # Braced values may contain semicolons; the sanitizer must not truncate - # and leak the tail. - raw = "Server=localhost;UID=sa;PWD={p@ss;w}}rd};Encrypt=no" - masked = _MaskedConnectionString(raw) - assert "p@ss" not in repr(masked) From d50d4efba06f062616464806bdfc2fb0d8d05155 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 13:43:00 +0530 Subject: [PATCH 5/6] CHORE: drop the fixture repr test Deleting the wrapper breaks the import at collection time and a sanitizer regression trips the nine tests in test_007_logging, so the only thing left for this test to catch was someone editing __repr__ to stop calling the sanitizer. Not worth the conftest private-name import into a test module. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/test_012_connection_string_integration.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/tests/test_012_connection_string_integration.py b/tests/test_012_connection_string_integration.py index 1f99bc79b..d03c9308b 100644 --- a/tests/test_012_connection_string_integration.py +++ b/tests/test_012_connection_string_integration.py @@ -17,7 +17,6 @@ ) from mssql_python.connection_string_builder import _ConnectionStringBuilder from mssql_python import connect -from conftest import _MaskedConnectionString class TestConnectionStringIntegration: @@ -655,18 +654,3 @@ def test_connect_multiple_empty_values_raises_error(self, mock_ddbc_conn): assert len(errors) >= 2 assert any("Empty value for keyword 'server'" in err for err in errors) assert any("Empty value for keyword 'pwd'" in err for err in errors) - - -class TestConnStrFixtureRepr: - """The conn_str fixture is wrapped so pytest failure headers do not print - the password. Sanitizer behaviour itself is covered in test_007_logging.""" - - def test_repr_is_masked_and_value_is_untouched(self): - raw = "Server=localhost,1433;Database=master;UID=sa;PWD=secret123;Encrypt=no" - masked = _MaskedConnectionString(raw) - # repr() is what pytest prints in the failure header - assert "secret123" not in repr(masked) - # the value still has to behave exactly like the string it wraps - assert isinstance(masked, str) - assert masked == raw - assert str(masked) == raw From 071dfe60f6b64f19bbcf497b8ec501c78ca95001 Mon Sep 17 00:00:00 2001 From: Gaurav Sharma <223556219+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:17:08 +0530 Subject: [PATCH 6/6] CHORE: say why the fixture wrapper has to be a subclass First question anyone asks is why this is not just a call to sanitize_connection_string() where the value is used. 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. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/conftest.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 93f7e2333..95d7cc3e7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -26,6 +26,12 @@ class _MaskedConnectionString(str): 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__ = ()