CHORE: credential hygiene in test output, workflow permissions, and scanning config - #713
Merged
Gaurav Sharma (bewithgaurav) merged 8 commits intoAug 12, 2026
Conversation
…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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📊 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.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 76.6%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.4%
mssql_python.pybind.connection.connection.cpp: 84.3%
mssql_python.logging.py: 85.5%🔗 Quick Links
|
`.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>
github-actions
Bot
added
pr-size: medium
Gaurav Sharma (bewithgaurav)
changed the title
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>
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>
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>
github-actions
Bot
added
pr-size: small
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>
Gaurav Sharma (bewithgaurav)
marked this pull request as ready for review
August 11, 2026 08:49
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
August 11, 2026 08:50
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Improves credential hygiene across the test suite and CI configuration to reduce the chance of leaking live connection string secrets into logs, while also tightening default GitHub Actions permissions.
Changes:
- Mask
conn_strfixturerepr()output so pytest failure introspection can’t print live passwords. - Ensure real-DB connection string assertions in
test_012operate on a sanitized value. - Reduce GitHub Actions default permissions to
contents: read, disable checkout credential persistence, and remove a misleading/dead CredScan suppressions config reference/file.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_012_connection_string_integration.py | Asserts on sanitized connection strings to prevent leaking credentials via assertion introspection. |
| tests/conftest.py | Wraps conn_str in a str subclass overriding __repr__ to sanitize sensitive values in pytest output. |
| .github/workflows/pr-code-coverage.yml | Adds default contents: read and disables persisted checkout credentials. |
| .github/workflows/lint-check.yml | Drops unnecessary workflow-level PR write permissions and disables persisted checkout credentials. |
| .github/workflows/forked-pr-coverage.yml | Adds default contents: read and disables persisted checkout credentials while keeping job-level PR write permissions. |
| .github/workflows/devskim.yml | Adds default contents: read and disables persisted checkout credentials. |
| .github/copilot-instructions.md | Removes reference to the inactive CredScan suppressions file, pointing only to .gdn/. |
| .config/CredScanSuppressions.json | Removes an unused/misleading suppressions file to avoid accidental broad credential-scanning bypass. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sumit Sarabhai (sumitmsft)
approved these changes
Aug 11, 2026
gargsaumya
approved these changes
Aug 12, 2026
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
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 improves security and CI hygiene by tightening permissions in GitHub Actions workflows, preventing accidental credential leaks in test logs, and cleaning up outdated credential scanning configuration. The most significant changes are grouped below.
Security improvements in test logging:
conn_strpytest fixture is now wrapped in a custom_MaskedConnectionStringclass that hides passwords from pytest failure output, preventing accidental exposure of credentials in CI logs. All assertions in integration tests now use sanitized connection strings for extra safety. [1] [2] [3] [4] [5]GitHub Actions workflow hardening:
permissions: contents: readinstead of broader permissions, reducing the risk if a workflow is compromised. [1] [2] [3] [4]actions/checkoutsteps now setpersist-credentials: false, ensuring that the default GitHub token is not persisted to the workflow environment, further limiting the risk of credential leakage in forked PRs or CI jobs. [1] [2] [3] [4] [5]Credential scanning configuration cleanup:
.config/CredScanSuppressions.jsonfile was removed, and documentation was updated to reference only the current.gdn/scanning mechanism. [1] [2]