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

Gaurav Sharma (bewithgaurav) · 2026-08-13T06:32:05Z

+        and isinstance(pull.get("number"), int)
+        and pull["number"] > 0
+    ]
+    if len(matching_pulls) != 1:


forked PRs never get a comment. this line always fails for them.

GET /commits/{sha}/pulls only sees commits that live in the base repo, and a fork's head commit doesn't, so matching_pulls is empty every time. checked all open fork PRs I could find across mssql-python, every one returns 0. the same call returns 1 for a same-repo PR like this one, which is why it looks fine from here.

ran a real artifact plus the live api responses for 3 forked PRs through the script, all three exit 2 on workflow run must resolve to exactly one pull request.

GET /pulls?state=open matched on head.sha resolves all three. the exactly-one-match, base repo and base branch checks all carry over, so no protection is given up

+1

/commits/{sha}/pulls can't see a fork's head commit, so it's empty every time. /pulls?state=open + head.sha match fixes it with no loss of protection; head_sha still comes from the trusted workflow_run event. Blocking.

Gaurav Sharma (bewithgaurav) · 2026-08-13T06:32:39Z

+        raise ValidationError("workflow run did not originate from a fork")
+
+    event_pulls = workflow_run.get("pull_requests") or []
+    if len(event_pulls) == 1:


this is dead code actually

line 170 already bails out when the run isn't from a fork, and github always sends an empty pull_requests on fork runs, 8/8 real fork runs I sampled, so this never fires

the cost is that test_prepares_safe_comment_for_trusted_event_pr only exercises this branch. it goes green while the path that actually runs is untested and broken

Agree- fork workflow_run events always send empty pull_requests, so this never fires, and it's the only branch the green happy-path test covers.

Gaurav Sharma (bewithgaurav) · 2026-08-13T06:34:44Z

    steps:
      - name: Checkout repo
-        uses: actions/checkout@v4
+        uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4


#716 that was raised yesterday pins these same SHAs, plus setup-python, github-script, devskim and codeql, and carries the exact patch version in the comment

worth dropping from here so the two don't collide

same for persist-credentials: false, that landed with #713

Gaurav Sharma (bewithgaurav) · 2026-08-13T06:37:36Z

+        raise ValidationError("artifact must contain only pr-info.json")
+
+    data = _load_json(artifact_directory / "pr-info.json")
+    if not isinstance(data, dict) or set(data) != EXPECTED_FIELDS:


suggestion: check the six fields are there rather than requiring an exact match.

as it stands, anyone who adds a field to the artifact later turns fork comments off without knowing, and nothing on the PR says why - the extra field is harmless anyway, nothing here reads past the six it wants

Gaurav Sharma (bewithgaurav) · 2026-08-13T06:39:50Z

-            echo "This may indicate an issue with artifact upload"
+          set -euo pipefail
+
+          [[ "$RUN_ID" =~ ^[0-9]+$ ]] || {


suggestion: drop the RUN_ID, HEAD_SHA and PR_NUMBER guards, the script already covers all three.

the first two come straight from github, and the script re-checks the sha anyway
the PR_NUMBER one further down is checking this script's own output - same rule is written twice

Agree - all three are re-validated in the script, safe to remove.

gargsaumya · 2026-08-13T09:19:27Z

+    return value
+
+
+def _validate_ado_url(value) -> str:


ado_url comes from ADO's _links.web.href, but this only accepts dev.azure.com, an exact path, and a lone buildId param. If ADO returns extra query params or the *.visualstudio.com host, the comment is silently dropped. Please confirm a real build's href passes these rules

gargsaumya · 2026-08-13T09:20:57Z

+
+          gh api \
+            -H "Accept: application/vnd.github+json" \
+            "repos/${GITHUB_REPOSITORY}/commits/${HEAD_SHA}/pulls" > "$PULLS_FILE"


When this moves to /pulls?state=open (per the resolve fix), add --paginate — the default 30-per-page cap can miss the match on a repo with many open PRs.

gargsaumya · 2026-08-13T09:22:55Z

+        and isinstance(pull.get("number"), int)
+        and pull["number"] > 0
+    ]
+    if len(matching_pulls) != 1:


+1

/commits/{sha}/pulls can't see a fork's head commit, so it's empty every time. /pulls?state=open + head.sha match fixes it with no loss of protection; head_sha still comes from the trusted workflow_run event. Blocking.

gargsaumya · 2026-08-13T09:23:24Z

+        raise ValidationError("workflow run did not originate from a fork")
+
+    event_pulls = workflow_run.get("pull_requests") or []
+    if len(event_pulls) == 1:


Agree- fork workflow_run events always send empty pull_requests, so this never fires, and it's the only branch the green happy-path test covers.

gargsaumya · 2026-08-13T09:24:17Z

-            echo "This may indicate an issue with artifact upload"
+          set -euo pipefail
+
+          [[ "$RUN_ID" =~ ^[0-9]+$ ]] || {


Agree - all three are re-validated in the script, safe to remove.

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

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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