Skip to content

Commit

Permalink
Python: Secure Python test coverage workflow (#9961)
Browse files Browse the repository at this point in the history
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->
The Python test coverage workflow needs to post comments of coverage
results on PRs, which requires the proper permissions. By default, PRs
from forks don't have the required permissions if the workflow is
triggered by the `pull_request` event.

However, one can use the `pull_request_target` event as the trigger and
check out the code from the PR for test coverage. This approach posts
various security risks as it gives the code from the PR full access to
the repository secrets.

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
It's recommended to use a two-stage approach for this kind of scenarios:
https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/

In this PR, 
1. a new workflow is created named `python-test-coverage-report`. This
workflow is given the proper permissions to post the coverage report in
the PR comments by using the `workflow_run` trigger. This trigger
ensures the workflow is run on the context of the default branch.
2. the existing workflow `python-test-coverage` is updated such that it
only runs the unit tests and doesn't have the permissions to post the
coverage report in the PR comments.

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
  • Loading branch information
TaoChenOSU authored Dec 12, 2024
1 parent b1f4769 commit ed58607
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 19 deletions.
38 changes: 38 additions & 0 deletions .github/workflows/python-test-coverage-report.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
name: Python Test Coverage Report

on:
workflow_run:
workflows: ["Python Test Coverage"]
types:
- completed

jobs:
python-test-coverage-report:
runs-on: ubuntu-latest
if: github.event.workflow_run.conclusion == 'success'
continue-on-error: false
defaults:
run:
working-directory: python
steps:
- uses: actions/checkout@v4
- name: Download coverage report
uses: actions/download-artifact@v4
with:
github-token: ${{ secrets.GH_ACTIONS_PR_WRITE }}
run-id: ${{ github.event.workflow_run.id }}
path: ./python
merge-multiple: true
- name: Display structure of downloaded files
run: ls
- name: Pytest coverage comment
id: coverageComment
uses: MishaKav/pytest-coverage-comment@main
with:
pytest-coverage-path: python/python-coverage.txt
title: "Python Test Coverage Report"
badge-title: "Python Test Coverage"
junitxml-title: "Python Unit Test Overview"
junitxml-path: python/pytest.xml
default-branch: "main"
report-only-changed-files: true
27 changes: 8 additions & 19 deletions .github/workflows/python-test-coverage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ env:
# Configure a constant location for the uv cache
UV_CACHE_DIR: /tmp/.uv-cache

permissions:
contents: write
checks: write
pull-requests: write

jobs:
python-tests-coverage:
runs-on: ubuntu-latest
Expand All @@ -26,8 +21,6 @@ jobs:
UV_PYTHON: "3.10"
steps:
- uses: actions/checkout@v4
- name: Setup filename variables
run: echo "FILE_ID=${{ github.event.number }}" >> $GITHUB_ENV
- name: Set up uv
uses: astral-sh/setup-uv@v4
with:
Expand All @@ -38,16 +31,12 @@ jobs:
run: uv sync --all-extras --dev
- name: Test with pytest
run: uv run --frozen pytest -q --junitxml=pytest.xml --cov=semantic_kernel --cov-report=term-missing:skip-covered ./tests/unit | tee python-coverage.txt
- name: Pytest coverage comment
id: coverageComment
uses: MishaKav/pytest-coverage-comment@main
continue-on-error: false
- name: Upload coverage report
uses: actions/upload-artifact@v4
with:
pytest-coverage-path: python/python-coverage.txt
coverage-path-prefix: "python/"
title: "Python Test Coverage Report"
badge-title: "Python Test Coverage"
junitxml-title: "Python Unit Test Overview"
junitxml-path: python/pytest.xml
default-branch: "main"
report-only-changed-files: true
path: |
python/python-coverage.txt
python/pytest.xml
overwrite: true
retention-days: 1
if-no-files-found: error

0 comments on commit ed58607

Please sign in to comment.