Uh oh!
There was an error while loading. Please reload this page.
FIX: forked-pr-coverage.yml env-injection via untrusted artifact - #714
FIX: forked-pr-coverage.yml env-injection via untrusted artifact#714Sumit Sarabhai (sumitmsft) wants to merge 4 commits into
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>
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
📊 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
|
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)
left a comment
There was a problem hiding this comment.
requesting changes - some issues wrt forked pr comments and refactoring suggestions to cut the PR short
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- 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)
left a comment
There was a problem hiding this comment.
requesting changes - for a different blocker this time, added a couple of suggestions as well
| echo "$PATCH_SUMMARY" | ||
| echo "EOF" | ||
| } >> $GITHUB_ENV | ||
| gh api --paginate --slurp \ |
There was a problem hiding this comment.
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
| matching_pulls = [ | ||
| pull | ||
| for pull in associated_pulls | ||
| if pull.get("head", {}).get("sha") == head_sha |
There was a problem hiding this comment.
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
| PR_NUMBER="$(cat "$PR_NUMBER_FILE")" | ||
| COMMENT_ID="$( |
There was a problem hiding this comment.
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.
Work Item / Issue Reference
AB#46466
Summary
Validation