FIX: forked-pr-coverage.yml env-injection via untrusted artifact by sumitmsft · Pull Request #714 · microsoft/mssql-python

Sumit Sarabhai (sumitmsft) · 2026-08-11T11:16:23Z

Work Item / Issue Reference

Summary

Treat fork-produced coverage artifacts as untrusted input and validate their schema and values before use.
Bind coverage comments to the triggering pull request instead of accepting an artifact-supplied PR number.
Remove untrusted artifact propagation through GITHUB_ENV and reject unexpected files, unsafe URLs, control characters, and oversized values.
Pin GitHub Actions to immutable commit SHAs and disable persisted checkout credentials.
Add regression coverage for environment injection, cross-PR targeting, executable artifacts, and Markdown/URL injection.

Validation

12 targeted security regression tests passed.
Changed workflow YAML parsed successfully.
Black formatting checks passed.
Native extension build completed successfully.

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

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.yml to 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 multiline GITHUB_ENV usage, 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.

github-actions · 2026-08-11T11:22:33Z

📊 Code Coverage Report

🔥 Diff Coverage 100%	🎯 Overall Coverage 82%	📈 Total Lines Covered: `7368` out of `8963` 📁 Project: `mssql-python`

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

No 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

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

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)

requesting changes - some issues wrt forked pr comments and refactoring suggestions to cut the PR short

- 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)

requesting changes - for a different blocker this time, added a couple of suggestions as well

Gaurav Sharma (bewithgaurav) · 2026-08-14T05:38:10Z

-            echo "$PATCH_SUMMARY"
-            echo "EOF"
-          } >> $GITHUB_ENV
+          gh api --paginate --slurp \


blocker: gh won't run this. --slurp and --jq can't be used together:

$ gh api --paginate --slurp "repos/.../pulls?state=open&per_page=100" --jq 'add' the `--slurp` option is not supported with `--jq` or `--template` exit 1

fails on every gh version, and the whole step goes with it
same combination at line 75 (commented there as well - with a better suggestion)

piping instead works:

gh api --paginate --slurp "repos/${GITHUB_REPOSITORY}/pulls?state=open&per_page=100" \ | jq 'add' > "$PULLS_FILE"

checked against the live api and all three fork PRs here resolve correctly through it

Gaurav Sharma (bewithgaurav) · 2026-08-14T05:42:19Z

+    matching_pulls = [
+        pull
+        for pull in associated_pulls
+        if pull.get("head", {}).get("sha") == head_sha


optional: match the fork name too, not just the commit.

two forks sitting on the same commit would both match today, and then nobody gets a comment. harmless, just noticed it

Gaurav Sharma (bewithgaurav) · 2026-08-14T06:31:50Z

+
+          PR_NUMBER="$(cat "$PR_NUMBER_FILE")"
+
+          COMMENT_ID="$(


this has the same blocker as the line 60 one, I have a better suggestion which includes fixing that as well:

gh pr comment does find-or-create on its own, so this whole block collapses to one call.

gh pr comment "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" \ --body-file "$COMMENT_FILE" --edit-last --create-if-none

18 lines down to 2. the script writes the body as markdown instead of wrapping it in json, same line either way. the argus writeup named this one directly, a plain gh pr comment call, so it stays on the no-node-action path you took.

if you do take it, the line 75 call goes with the block, so that one needs no fix.

Copilot AI lite review requested due to automatic review settings August 11, 2026 11:16

github-actions Bot added the pr-size: large Substantial code update label Aug 11, 2026

Copilot started reviewing on behalf of Sumit Sarabhai (sumitmsft) August 11, 2026 11:17 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Comment thread .github/scripts/prepare_fork_coverage_comment.py

Comment thread tests/test_fork_coverage_security.py

FIX: Reject unexpected artifact directories

5aec312

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

View reviewed changes

gargsaumya requested changes Aug 13, 2026

View reviewed changes

Sumit Sarabhai (sumitmsft) and others added 2 commits August 13, 2026 15:26

Gaurav Sharma (bewithgaurav) requested changes Aug 14, 2026

View reviewed changes

Uh oh!

Conversation

Sumit Sarabhai (sumitmsft) commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work Item / Issue Reference

Summary

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Code Coverage Report

🔥 Diff Coverage

100%

🎯 Overall Coverage

82%

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

📋 Files Needing Attention

🔗 Quick Links

Uh oh!

Gaurav Sharma (bewithgaurav) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gaurav Sharma (bewithgaurav) left a comment

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sumit Sarabhai (sumitmsft) commented Aug 11, 2026 •

edited

Loading

github-actions Bot commented Aug 11, 2026 •

edited

Loading