Skip to content

Commit 5f4fd10

Browse files
sumitmsftCopilotbewithgaurav
authored
FIX: forked-pr-coverage.yml env-injection via untrusted artifact (#714)
### Work Item / Issue Reference [AB#46466](https://sqlclientdrivers.visualstudio.com/mssql-python/_workitems/edit/46466) ### 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. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Gaurav Sharma <sharmag@microsoft.com>
1 parent acdb9be commit 5f4fd10

4 files changed

Lines changed: 518 additions & 78 deletions

File tree

Lines changed: 241 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,241 @@
1+
#!/usr/bin/env python3
2+
3+
import argparse
4+
import html
5+
import json
6+
import re
7+
import unicodedata
8+
from decimal import Decimal, InvalidOperation
9+
from pathlib import Path
10+
from urllib.parse import parse_qs, urlsplit
11+
12+
COMMENT_MARKER = "<!-- mssql-python-code-coverage -->"
13+
EXPECTED_FIELDS = {
14+
"coverage_percentage",
15+
"covered_lines",
16+
"total_lines",
17+
"patch_coverage_pct",
18+
"low_coverage_files",
19+
"ado_url",
20+
}
21+
MAX_JSON_BYTES = 64 * 1024
22+
MAX_LOW_COVERAGE_BYTES = 8 * 1024
23+
PERCENTAGE_PATTERN = re.compile(r"^(?:0|[1-9][0-9]{0,2})(?:\.[0-9]+)?%$")
24+
COUNT_PATTERN = re.compile(r"^(?:0|[1-9][0-9]{0,9}|N/A)$")
25+
PATCH_COVERAGE_STATUSES = {"Could not parse", "Report not generated", "N/A%"}
26+
ADO_PROJECT_ID = "904996cc-6198-4d39-8540-eca72bdf0b7b"
27+
ADO_BUILD_PATHS = {
28+
"/sqlclientdrivers/public/_build/results",
29+
f"/sqlclientdrivers/{ADO_PROJECT_ID}/_build/results",
30+
}
31+
32+
33+
class ValidationError(ValueError):
34+
pass
35+
36+
37+
def _load_json(path: Path, maximum_size: int = MAX_JSON_BYTES):
38+
if not path.is_file() or path.is_symlink():
39+
raise ValidationError(f"{path.name} must be a regular file")
40+
if path.stat().st_size > maximum_size:
41+
raise ValidationError(f"{path.name} exceeds the {maximum_size}-byte limit")
42+
try:
43+
return json.loads(path.read_text(encoding="utf-8"))
44+
except (UnicodeDecodeError, json.JSONDecodeError) as exc:
45+
raise ValidationError(f"{path.name} is not valid UTF-8 JSON") from exc
46+
47+
48+
def _validate_single_line(name: str, value, maximum_length: int) -> str:
49+
if not isinstance(value, str):
50+
raise ValidationError(f"{name} must be a string")
51+
if not value or len(value) > maximum_length:
52+
raise ValidationError(f"{name} has an invalid length")
53+
if any(unicodedata.category(character).startswith("C") for character in value):
54+
raise ValidationError(f"{name} contains control characters")
55+
return value
56+
57+
58+
def _validate_percentage(name: str, value, allowed_statuses=frozenset()) -> str:
59+
value = _validate_single_line(name, value, 32)
60+
if value in allowed_statuses:
61+
return value
62+
if not PERCENTAGE_PATTERN.fullmatch(value):
63+
raise ValidationError(f"{name} must be a percentage")
64+
try:
65+
percentage = Decimal(value[:-1])
66+
except InvalidOperation as exc:
67+
raise ValidationError(f"{name} must be a percentage") from exc
68+
if percentage > 100:
69+
raise ValidationError(f"{name} cannot exceed 100%")
70+
return value
71+
72+
73+
def _validate_count(name: str, value) -> str:
74+
value = _validate_single_line(name, value, 16)
75+
if not COUNT_PATTERN.fullmatch(value):
76+
raise ValidationError(f"{name} must be a non-negative integer or N/A")
77+
return value
78+
79+
80+
def _validate_low_coverage_files(value) -> str:
81+
if not isinstance(value, str):
82+
raise ValidationError("low_coverage_files must be a string")
83+
if not value or len(value.encode("utf-8")) > MAX_LOW_COVERAGE_BYTES:
84+
raise ValidationError("low_coverage_files has an invalid length")
85+
if "\r" in value:
86+
raise ValidationError("low_coverage_files contains carriage returns")
87+
if len(value.splitlines()) > 10:
88+
raise ValidationError("low_coverage_files contains more than 10 lines")
89+
if any(
90+
character != "\n" and unicodedata.category(character).startswith("C") for character in value
91+
):
92+
raise ValidationError("low_coverage_files contains control characters")
93+
return value
94+
95+
96+
def _validate_ado_url(value) -> str:
97+
value = _validate_single_line("ado_url", value, 500)
98+
parsed = urlsplit(value)
99+
if (
100+
parsed.scheme != "https"
101+
or parsed.hostname is None
102+
or parsed.hostname.lower() != "dev.azure.com"
103+
or parsed.username is not None
104+
or parsed.password is not None
105+
or parsed.port is not None
106+
or parsed.path.lower() not in ADO_BUILD_PATHS
107+
or parsed.fragment
108+
):
109+
raise ValidationError("ado_url must reference the public SqlClientDrivers build")
110+
111+
query = parse_qs(parsed.query, keep_blank_values=True)
112+
if set(query) != {"buildId"}:
113+
raise ValidationError("ado_url contains unexpected query parameters")
114+
build_ids = query.get("buildId", [])
115+
if len(build_ids) != 1 or not build_ids[0].isdigit():
116+
raise ValidationError("ado_url must contain one numeric buildId")
117+
return (
118+
f"https://dev.azure.com/SqlClientDrivers/{ADO_PROJECT_ID}/_build/results"
119+
f"?buildId={build_ids[0]}"
120+
)
121+
122+
123+
def validate_artifact(artifact_directory: Path) -> dict:
124+
if not artifact_directory.is_dir() or artifact_directory.is_symlink():
125+
raise ValidationError("artifact path must be a directory")
126+
127+
entries = list(artifact_directory.iterdir())
128+
if (
129+
len(entries) != 1
130+
or entries[0].name != "pr-info.json"
131+
or not entries[0].is_file()
132+
or entries[0].is_symlink()
133+
):
134+
raise ValidationError("artifact must contain only pr-info.json")
135+
136+
data = _load_json(artifact_directory / "pr-info.json")
137+
if not isinstance(data, dict) or not EXPECTED_FIELDS.issubset(data):
138+
raise ValidationError("pr-info.json does not match the expected schema")
139+
140+
return {
141+
"coverage_percentage": _validate_percentage(
142+
"coverage_percentage", data["coverage_percentage"]
143+
),
144+
"covered_lines": _validate_count("covered_lines", data["covered_lines"]),
145+
"total_lines": _validate_count("total_lines", data["total_lines"]),
146+
"patch_coverage_pct": _validate_percentage(
147+
"patch_coverage_pct",
148+
data["patch_coverage_pct"],
149+
PATCH_COVERAGE_STATUSES,
150+
),
151+
"low_coverage_files": _validate_low_coverage_files(data["low_coverage_files"]),
152+
"ado_url": _validate_ado_url(data["ado_url"]),
153+
}
154+
155+
156+
def resolve_pr_number(event: dict, associated_pulls: list) -> int:
157+
repository = event.get("repository", {})
158+
workflow_run = event.get("workflow_run", {})
159+
repository_name = repository.get("full_name")
160+
default_branch = repository.get("default_branch")
161+
head_sha = workflow_run.get("head_sha")
162+
head_repository = workflow_run.get("head_repository") or {}
163+
head_repository_name = head_repository.get("full_name")
164+
165+
if not isinstance(repository_name, str) or not repository_name:
166+
raise ValidationError("event is missing the repository name")
167+
if not isinstance(default_branch, str) or not default_branch:
168+
raise ValidationError("event is missing the default branch")
169+
if not isinstance(head_sha, str) or not re.fullmatch(r"[0-9a-f]{40}", head_sha):
170+
raise ValidationError("workflow run has an invalid head SHA")
171+
if not isinstance(head_repository_name, str) or not head_repository_name:
172+
raise ValidationError("workflow run is missing the head repository")
173+
if head_repository_name == repository_name:
174+
raise ValidationError("workflow run did not originate from a fork")
175+
176+
if not isinstance(associated_pulls, list):
177+
raise ValidationError("associated pull request response must be a list")
178+
matching_pulls = [
179+
pull
180+
for pull in associated_pulls
181+
if pull.get("head", {}).get("sha") == head_sha
182+
and pull.get("head", {}).get("repo", {}).get("full_name") == head_repository_name
183+
and pull.get("base", {}).get("ref") == default_branch
184+
and pull.get("base", {}).get("repo", {}).get("full_name") == repository_name
185+
and isinstance(pull.get("number"), int)
186+
and pull["number"] > 0
187+
]
188+
if len(matching_pulls) != 1:
189+
raise ValidationError("workflow run must resolve to exactly one pull request")
190+
return matching_pulls[0]["number"]
191+
192+
193+
def build_comment(data: dict) -> str:
194+
low_coverage_files = html.escape(data["low_coverage_files"])
195+
ado_url = data["ado_url"]
196+
return f"""\
197+
{COMMENT_MARKER}
198+
# Code Coverage Report
199+
200+
| Diff coverage | Overall coverage | Lines covered |
201+
| --- | --- | --- |
202+
| **{data["patch_coverage_pct"]}** | **{data["coverage_percentage"]}** | **{data["covered_lines"]}** of **{data["total_lines"]}** |
203+
204+
### Files needing attention
205+
206+
<pre>{low_coverage_files}</pre>
207+
208+
[View Azure DevOps build]({ado_url})
209+
"""
210+
211+
212+
def prepare_comment(artifact_directory: Path, event_path: Path, pulls_path: Path):
213+
data = validate_artifact(artifact_directory)
214+
event = _load_json(event_path, 1024 * 1024)
215+
associated_pulls = _load_json(pulls_path, 1024 * 1024)
216+
pr_number = resolve_pr_number(event, associated_pulls)
217+
return pr_number, build_comment(data)
218+
219+
220+
def main():
221+
parser = argparse.ArgumentParser()
222+
parser.add_argument("--artifact-directory", required=True, type=Path)
223+
parser.add_argument("--event", required=True, type=Path)
224+
parser.add_argument("--associated-pulls", required=True, type=Path)
225+
parser.add_argument("--comment-output", required=True, type=Path)
226+
parser.add_argument("--pr-number-output", required=True, type=Path)
227+
args = parser.parse_args()
228+
229+
try:
230+
pr_number, comment = prepare_comment(
231+
args.artifact_directory, args.event, args.associated_pulls
232+
)
233+
except ValidationError as exc:
234+
parser.error(str(exc))
235+
236+
args.comment_output.write_text(json.dumps({"body": comment}), encoding="utf-8")
237+
args.pr_number_output.write_text(str(pr_number), encoding="ascii")
238+
239+
240+
if __name__ == "__main__":
241+
main()

‎.github/workflows/forked-pr-coverage.yml‎

Lines changed: 46 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,10 @@ jobs:
3030
runs-on: ubuntu-latest
3131
if: >
3232
github.event.workflow_run.event == 'pull_request' &&
33-
github.event.workflow_run.conclusion == 'success'
33+
github.event.workflow_run.conclusion == 'success' &&
34+
github.event.workflow_run.head_repository.full_name != github.repository
3435
permissions:
36+
actions: read
3537
pull-requests: write
3638
contents: read
3739

@@ -41,76 +43,52 @@ jobs:
4143
with:
4244
persist-credentials: false
4345

44-
- name: Download coverage data
46+
- name: Validate coverage data and post comment
4547
env:
4648
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
49+
RUN_ID: ${{ github.event.workflow_run.id }}
4750
run: |
48-
# Download artifact with error handling for non-existent artifacts
49-
if ! gh run download ${{ github.event.workflow_run.id }} \
51+
set -euo pipefail
52+
53+
ARTIFACT_DIR="$(mktemp -d "${RUNNER_TEMP}/coverage-comment-data.XXXXXX")"
54+
PULLS_FILE="${RUNNER_TEMP}/associated-pulls.json"
55+
COMMENT_FILE="${RUNNER_TEMP}/coverage-comment.json"
56+
PR_NUMBER_FILE="${RUNNER_TEMP}/coverage-pr-number"
57+
58+
gh run download "$RUN_ID" \
5059
--repo ${{ github.repository }} \
51-
--name coverage-comment-data 2>&1; then
52-
echo "⚠️ No coverage-comment-data artifact found"
53-
echo "This is expected for same-repo PRs (they post comments directly)"
54-
echo "Exiting gracefully..."
55-
exit 0
56-
fi
57-
58-
# Verify artifact was downloaded
59-
if [[ ! -f pr-info.json ]]; then
60-
echo "⚠️ Artifact downloaded but pr-info.json not found"
61-
echo "This may indicate an issue with artifact upload"
62-
exit 1
63-
fi
60+
--name coverage-comment-data \
61+
--dir "$ARTIFACT_DIR"
6462
65-
- name: Read coverage data
66-
id: coverage
67-
run: |
68-
if [[ ! -f pr-info.json ]]; then
69-
echo "❌ pr-info.json not found"
70-
exit 1
71-
fi
72-
73-
cat pr-info.json
74-
75-
# Extract values from JSON with proper quoting
76-
PR_NUMBER="$(jq -r '.pr_number' pr-info.json)"
77-
COVERAGE_PCT="$(jq -r '.coverage_percentage' pr-info.json)"
78-
COVERED_LINES="$(jq -r '.covered_lines' pr-info.json)"
79-
TOTAL_LINES="$(jq -r '.total_lines' pr-info.json)"
80-
PATCH_PCT="$(jq -r '.patch_coverage_pct' pr-info.json)"
81-
LOW_COV_FILES="$(jq -r '.low_coverage_files' pr-info.json)"
82-
PATCH_SUMMARY="$(jq -r '.patch_coverage_summary' pr-info.json)"
83-
ADO_URL="$(jq -r '.ado_url' pr-info.json)"
84-
85-
# Export to env for next step (single-line values)
86-
echo "PR_NUMBER=${PR_NUMBER}" >> $GITHUB_ENV
87-
echo "COVERAGE_PERCENTAGE=${COVERAGE_PCT}" >> $GITHUB_ENV
88-
echo "COVERED_LINES=${COVERED_LINES}" >> $GITHUB_ENV
89-
echo "TOTAL_LINES=${TOTAL_LINES}" >> $GITHUB_ENV
90-
echo "PATCH_COVERAGE_PCT=${PATCH_PCT}" >> $GITHUB_ENV
91-
echo "ADO_URL=${ADO_URL}" >> $GITHUB_ENV
92-
93-
# Handle multiline values with proper quoting
94-
{
95-
echo "LOW_COVERAGE_FILES<<EOF"
96-
echo "$LOW_COV_FILES"
97-
echo "EOF"
98-
} >> $GITHUB_ENV
99-
100-
{
101-
echo "PATCH_COVERAGE_SUMMARY<<EOF"
102-
echo "$PATCH_SUMMARY"
103-
echo "EOF"
104-
} >> $GITHUB_ENV
63+
gh api --paginate --slurp \
64+
-H "Accept: application/vnd.github+json" \
65+
"repos/${GITHUB_REPOSITORY}/pulls?state=open&per_page=100" \
66+
| jq 'add' > "$PULLS_FILE"
10567
106-
- name: Comment coverage summary on PR
107-
uses: ./.github/actions/post-coverage-comment
108-
with:
109-
pr_number: ${{ env.PR_NUMBER }}
110-
coverage_percentage: ${{ env.COVERAGE_PERCENTAGE }}
111-
covered_lines: ${{ env.COVERED_LINES }}
112-
total_lines: ${{ env.TOTAL_LINES }}
113-
patch_coverage_pct: ${{ env.PATCH_COVERAGE_PCT }}
114-
low_coverage_files: ${{ env.LOW_COVERAGE_FILES }}
115-
patch_coverage_summary: ${{ env.PATCH_COVERAGE_SUMMARY }}
116-
ado_url: ${{ env.ADO_URL }}
68+
python .github/scripts/prepare_fork_coverage_comment.py \
69+
--artifact-directory "$ARTIFACT_DIR" \
70+
--event "$GITHUB_EVENT_PATH" \
71+
--associated-pulls "$PULLS_FILE" \
72+
--comment-output "$COMMENT_FILE" \
73+
--pr-number-output "$PR_NUMBER_FILE"
74+
75+
PR_NUMBER="$(cat "$PR_NUMBER_FILE")"
76+
77+
COMMENT_ID="$(
78+
gh api --paginate --slurp \
79+
"repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \
80+
| jq -r 'add | map(select(
81+
.user.login == "github-actions[bot]" and
82+
(.body | contains("<!-- mssql-python-code-coverage -->"))
83+
)) | .[0].id // empty'
84+
)"
85+
86+
if [[ -n "$COMMENT_ID" ]]; then
87+
gh api --method PATCH \
88+
"repos/${GITHUB_REPOSITORY}/issues/comments/${COMMENT_ID}" \
89+
--input "$COMMENT_FILE" > /dev/null
90+
else
91+
gh api --method POST \
92+
"repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \
93+
--input "$COMMENT_FILE" > /dev/null
94+
fi

0 commit comments

Comments
 (0)