FIX: forked-pr-coverage.yml env-injection via untrusted artifact - #714
Merged
Conversation
Validate untrusted coverage artifacts before posting comments, bind comments to the triggering pull request, remove privileged environment propagation, and pin actions to immutable revisions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Sumit Sarabhai (sumitmsft)
August 11, 2026 11:17
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the forked PR coverage-comment pipeline by treating coverage artifacts as untrusted input, validating/normalizing the data used to generate PR comments, and reducing the privileged workflow’s exposure to artifact-driven environment injection.
Changes:
- Introduces a Python validator/comment builder (
prepare_fork_coverage_comment.py) and a security-focused regression test suite. - Refactors
forked-pr-coverage.ymlto validate artifacts + resolve the target PR from the triggering workflow context (not artifact-supplied data), and post/update a single sticky comment. - Tightens the producer workflow (
pr-code-coverage.yml) by removing artifact-supplied PR targeting fields, hardening multilineGITHUB_ENVusage, and pinning actions to SHAs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_fork_coverage_security.py |
Adds regression tests for artifact schema validation, URL/markup injection, PR resolution, and workflow env-injection checks. |
.github/workflows/pr-code-coverage.yml |
Pins actions, disables persisted credentials, hardens multiline env export, and removes untrusted fields from the uploaded artifact payload. |
.github/workflows/forked-pr-coverage.yml |
Moves privileged workflow to validate downloaded artifacts + resolve PR from the event/commit association before commenting. |
.github/scripts/prepare_fork_coverage_comment.py |
Implements schema/value validation and safe comment construction for fork coverage artifacts. |
.github/actions/post-coverage-comment/action.yml |
Pins the sticky-comment action to an immutable SHA. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
📊 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: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 75.5%
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
|
Enforce the one-file artifact schema without recursively traversing attacker-controlled directory trees. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Gaurav Sharma (bewithgaurav)
requested changes
Aug 13, 2026
Gaurav Sharma (bewithgaurav)
left a comment
Gaurav Sharma (bewithgaurav)
left a comment
Collaborator
There was a problem hiding this comment.
requesting changes - some issues wrt forked pr comments and refactoring suggestions to cut the PR short
gargsaumya
requested changes
Aug 13, 2026
- forked-pr-coverage.yml: query /pulls?state=open (paginated) instead of
/commits/{sha}/pulls, which never returns a fork's head commit, so fork
PRs never resolved and no coverage comment was ever posted.
- prepare_fork_coverage_comment.py: remove the workflow_run.pull_requests
fast-path; GitHub always sends it empty for fork runs, so it was dead
code that only the happy-path test exercised.
- tests: point the happy-path test at the real head-SHA resolution and add
a regression that an attacker-supplied event pull_requests entry is ignored.
- Revert action SHA pins (checkout, upload-artifact, sticky-comment) to tags
to avoid colliding with #716, which owns SHA pinning.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- prepare_fork_coverage_comment.py: require the six expected fields to be present rather than an exact set, so adding a field to the producer no longer silently disables fork coverage comments. All six values are still strictly validated and only those six are propagated. - tests: assert an artifact-supplied pr_number is tolerated but never propagated, and that a missing required field is still rejected. - forked-pr-coverage.yml: drop the RUN_ID, HEAD_SHA and PR_NUMBER shell guards. RUN_ID/HEAD_SHA come straight from the trusted workflow_run event (HEAD_SHA is re-validated in the script), and PR_NUMBER only re-checks the script's own validated output. HEAD_SHA env removed as it is now unused. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Gaurav Sharma (bewithgaurav)
requested changes
Aug 14, 2026
Gaurav Sharma (bewithgaurav)
left a comment
Gaurav Sharma (bewithgaurav)
left a comment
Collaborator
There was a problem hiding this comment.
requesting changes - for a different blocker this time, added a couple of suggestions as well
- forked-pr-coverage.yml: gh api rejects --slurp combined with --jq
("the --slurp option is not supported with --jq or --template"), which
failed the whole step. Pipe --slurp output to jq instead, for both the
open-PR fetch and the coverage-comment lookup. Verified against gh 2.96
and a real fork PR end to end.
- prepare_fork_coverage_comment.py: also match the triggering fork's head
repository (from the trusted workflow_run event), so two forks sitting on
the same commit resolve to the correct PR instead of colliding and
dropping the comment. Reject events missing the head repository.
- tests: add head-repo disambiguation and missing-head-repository cases;
factor pull fixtures into a shared helper.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Gaurav Sharma (bewithgaurav)
approved these changes
Aug 19, 2026
gargsaumya
approved these changes
Aug 19, 2026
Subrata (subrata-ms)
added a commit
that referenced
this pull request
Aug 28, 2026
### Work Item / Issue Reference <!-- IMPORTANT: Please follow the PR template guidelines below. For mssql-python maintainers: Insert your ADO Work Item ID below For external contributors: Insert Github Issue number below Only one reference is required - either GitHub issue OR ADO Work Item. --> <!-- mssql-python maintainers: ADO Work Item --> > [AB#47683](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/47683) <!-- External contributors: GitHub Issue --> ------------------------------------------------------------------- ### Summary <!-- Insert your summary of changes below. Minimum 10 characters required. --> **Symptom** Build-Release-Package-Pipeline failed on all four Linux variants during Step 8 (pytest) with a collection-time error, before any test ran: tests/test_fork_coverage_security.py:10: in <module> SPEC.loader.exec_module(coverage_comment) E FileNotFoundError: [Errno 2] No such file or directory: '/test_isolated_cp310/.github/scripts/prepare_fork_coverage_comment.py' !!! Interrupted: 1 error during collection !!! Combined with --maxfail=1, this aborted cp310–cp314 on: - Linux manylinux_2_28 x86_64 - Linux manylinux_2_28 aarch64 - Linux musllinux x86_64 - Linux musllinux aarch64 **Root cause** Two independent design assumptions collided: - Test-side assumption. [test_fork_coverage_security.py:7] resolves helpers at module top level (collection time) via [Path(__file__).parents[1] / ".github" / "scripts" / "prepare_fork_coverage_comment.py"], and also reads [forked-pr-coverage.yml] and [pr-code-coverage.yml]. It assumes [parents[1]] is a full repo checkout. Pipeline-side assumption. The Linux release lane in [build-linux-single-stage.yml] intentionally runs pytest from an empty isolated directory to prove the installed wheel is self-contained: TEST_DIR="/test_isolated_${PYBIN}" rm -rf $TEST_DIR; mkdir -p $TEST_DIR $PY -m pip install -q "$WHEEL" cp -r /workspace/tests $TEST_DIR/ cp /workspace/pytest.ini $TEST_DIR/ cp /workspace/requirements.txt $TEST_DIR/ Only [tests], [pytest.ini], [requirements.txt] are copied. [.github] is not — until now no test needed anything outside [tests]. [test_fork_coverage_security.py] (added by the fork-coverage hardening series culminating in PR #714) is the first test that reaches outside [tests] at import time. In the isolated layout [parents[1]]= /test_isolated_cp310/, so the .github/… lookup fell off the map. Only the Linux lane runs the empty-isolated-dir model; Windows and macOS run from $(Build.SourcesDirectory) (a real checkout), so they were unaffected — and repo-root GitHub CI was unaffected too. **Fix:** Make the pipeline meet the test's assumption, and drop an earlier stop-gap. Change added to Linux release pipeline Copy [.github] into $TEST_DIR alongside [tests], in both the manylinux branch (covers x86_64 + aarch64) and the musllinux branch (covers x86_64 + aarch64): [build-linux-single-stage.yml:322] — manylinux bash -lc block: [build-linux-single-stage.yml:392] — musllinux sh -lc block: same two lines. cp -r /workspace/tests $TEST_DIR/ || echo "WARNING: No tests directory"; Some tests read repo-side helper scripts/workflows (e.g. .github/scripts/prepare_fork_coverage_comment.py). cp -r /workspace/.github $TEST_DIR/ || echo "WARNING: No .github directory"; These are the only two code sites reached by the four failing Linux stages. <!-- ### PR Title Guide > For feature requests FEAT: (short-description) > For non-feature requests like test case updates, config updates , dependency updates etc CHORE: (short-description) > For Fix requests FIX: (short-description) > For doc update requests DOC: (short-description) > For Formatting, indentation, or styling update STYLE: (short-description) > For Refactor, without any feature changes REFACTOR: (short-description) > For performance improvements PERF: (short-description) > For release related changes, without any feature changes RELEASE: #<RELEASE_VERSION> (short-description) ### Contribution Guidelines External contributors: - Create a GitHub issue first: https://github.com/microsoft/mssql-python/issues/new - Link the GitHub issue in the "GitHub Issue" section above - Follow the PR title format and provide a meaningful summary mssql-python maintainers: - Create an ADO Work Item following internal processes - Link the ADO Work Item in the "ADO Work Item" section above - Follow the PR title format and provide a meaningful summary -->
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
AB#46466
Summary
Validation