From 9e774a8bb39950e436f1d1d99b9a6256868cff66 Mon Sep 17 00:00:00 2001 From: Theodor Mihalache Date: Fri, 20 Sep 2024 12:22:12 -0400 Subject: [PATCH 1/2] Test workflow changes: on from pull_request_target to pull_request Signed-off-by: Theodor Mihalache --- .../fork_pr_integration_tests_aws.yml | 7 +++-- .../fork_pr_integration_tests_gcp.yml | 7 +++-- .../fork_pr_integration_tests_snowflake.yml | 7 +++-- .github/workflows/java_pr.yml | 26 +++++++++---------- .github/workflows/lint_pr.yml | 4 +-- .github/workflows/pr_integration_tests.yml | 11 ++++---- .../workflows/pr_local_integration_tests.yml | 11 ++++---- 7 files changed, 33 insertions(+), 40 deletions(-) diff --git a/.github/fork_workflows/fork_pr_integration_tests_aws.yml b/.github/fork_workflows/fork_pr_integration_tests_aws.yml index 6eb8b8feff0..b0ff3b5b63d 100644 --- a/.github/fork_workflows/fork_pr_integration_tests_aws.yml +++ b/.github/fork_workflows/fork_pr_integration_tests_aws.yml @@ -27,10 +27,9 @@ jobs: steps: - uses: actions/checkout@v4 with: - # pull_request_target runs the workflow in the context of the base repo - # as such actions/checkout needs to be explicit configured to retrieve - # code from the PR. - ref: refs/pull/${{ github.event.pull_request.number }}/merge + repository: ${{ github.event.repository.full_name }} # Uses the full repository name + ref: ${{ github.ref }} # Uses the ref from the event + token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token submodules: recursive - name: Setup Python uses: actions/setup-python@v5 diff --git a/.github/fork_workflows/fork_pr_integration_tests_gcp.yml b/.github/fork_workflows/fork_pr_integration_tests_gcp.yml index be9844a7e93..009fb8ce157 100644 --- a/.github/fork_workflows/fork_pr_integration_tests_gcp.yml +++ b/.github/fork_workflows/fork_pr_integration_tests_gcp.yml @@ -27,10 +27,9 @@ jobs: steps: - uses: actions/checkout@v4 with: - # pull_request_target runs the workflow in the context of the base repo - # as such actions/checkout needs to be explicit configured to retrieve - # code from the PR. - ref: refs/pull/${{ github.event.pull_request.number }}/merge + repository: ${{ github.event.repository.full_name }} # Uses the full repository name + ref: ${{ github.ref }} # Uses the ref from the event + token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token submodules: recursive - name: Setup Python uses: actions/setup-python@v5 diff --git a/.github/fork_workflows/fork_pr_integration_tests_snowflake.yml b/.github/fork_workflows/fork_pr_integration_tests_snowflake.yml index a136b47b9e7..d8626fb2450 100644 --- a/.github/fork_workflows/fork_pr_integration_tests_snowflake.yml +++ b/.github/fork_workflows/fork_pr_integration_tests_snowflake.yml @@ -27,10 +27,9 @@ jobs: steps: - uses: actions/checkout@v4 with: - # pull_request_target runs the workflow in the context of the base repo - # as such actions/checkout needs to be explicit configured to retrieve - # code from the PR. - ref: refs/pull/${{ github.event.pull_request.number }}/merge + repository: ${{ github.event.repository.full_name }} # Uses the full repository name + ref: ${{ github.ref }} # Uses the ref from the event + token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token submodules: recursive - name: Setup Python uses: actions/setup-python@v5 diff --git a/.github/workflows/java_pr.yml b/.github/workflows/java_pr.yml index fa373fea23c..08c69036e24 100644 --- a/.github/workflows/java_pr.yml +++ b/.github/workflows/java_pr.yml @@ -1,7 +1,7 @@ name: java_pr on: - pull_request_target: + pull_request: types: - opened - synchronize @@ -9,7 +9,7 @@ on: jobs: lint-java: - # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -18,16 +18,15 @@ jobs: steps: - uses: actions/checkout@v4 with: - # pull_request_target runs the workflow in the context of the base repo - # as such actions/checkout needs to be explicit configured to retrieve - # code from the PR. - ref: refs/pull/${{ github.event.pull_request.number }}/merge + repository: ${{ github.event.repository.full_name }} # Uses the full repository name + ref: ${{ github.ref }} # Uses the ref from the event + token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token submodules: recursive - name: Lint java run: make lint-java unit-test-java: - # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -37,10 +36,9 @@ jobs: steps: - uses: actions/checkout@v4 with: - # pull_request_target runs the workflow in the context of the base repo - # as such actions/checkout needs to be explicit configured to retrieve - # code from the PR. - ref: refs/pull/${{ github.event.pull_request.number }}/merge + repository: ${{ github.event.repository.full_name }} # Uses the full repository name + ref: ${{ github.ref }} # Uses the ref from the event + token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token submodules: recursive - name: Set up JDK 11 uses: actions/setup-java@v1 @@ -68,7 +66,7 @@ jobs: path: ${{ github.workspace }}/docs/coverage/java/target/site/jacoco-aggregate/ build-docker-image-java: - # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -103,7 +101,7 @@ jobs: run: make build-${{ matrix.component }}-docker REGISTRY=${REGISTRY} VERSION=${GITHUB_SHA} integration-test-java-pr: - # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -115,7 +113,7 @@ jobs: steps: - uses: actions/checkout@v4 with: - # pull_request_target runs the workflow in the context of the base repo + # pull_request runs the workflow in the context of the base repo # as such actions/checkout needs to be explicit configured to retrieve # code from the PR. ref: refs/pull/${{ github.event.pull_request.number }}/merge diff --git a/.github/workflows/lint_pr.yml b/.github/workflows/lint_pr.yml index d1aa7d16a3e..f64497a5ec3 100644 --- a/.github/workflows/lint_pr.yml +++ b/.github/workflows/lint_pr.yml @@ -1,14 +1,14 @@ name: lint-pr on: - pull_request_target: + pull_request: types: - opened - edited - synchronize permissions: - # read-only perms specified due to use of pull_request_target in lieu of security label check + # read-only perms specified due to use of pull_request in lieu of security label check pull-requests: read jobs: diff --git a/.github/workflows/pr_integration_tests.yml b/.github/workflows/pr_integration_tests.yml index f4a9132d292..62febb26f6b 100644 --- a/.github/workflows/pr_integration_tests.yml +++ b/.github/workflows/pr_integration_tests.yml @@ -1,7 +1,7 @@ name: pr-integration-tests on: - pull_request_target: + pull_request: types: - opened - synchronize @@ -14,7 +14,7 @@ on: jobs: integration-test-python: - # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -41,10 +41,9 @@ jobs: steps: - uses: actions/checkout@v4 with: - # pull_request_target runs the workflow in the context of the base repo - # as such actions/checkout needs to be explicit configured to retrieve - # code from the PR. - ref: refs/pull/${{ github.event.pull_request.number }}/merge + repository: ${{ github.event.repository.full_name }} # Uses the full repository name + ref: ${{ github.ref }} # Uses the ref from the event + token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token submodules: recursive - name: Setup Python uses: actions/setup-python@v5 diff --git a/.github/workflows/pr_local_integration_tests.yml b/.github/workflows/pr_local_integration_tests.yml index 3de72621931..abf9e3ced86 100644 --- a/.github/workflows/pr_local_integration_tests.yml +++ b/.github/workflows/pr_local_integration_tests.yml @@ -2,7 +2,7 @@ name: pr-local-integration-tests # This runs local tests with containerized stubs of online stores. This is the main dev workflow on: - pull_request_target: + pull_request: types: - opened - synchronize @@ -10,7 +10,7 @@ on: jobs: integration-test-python-local: - # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -27,10 +27,9 @@ jobs: steps: - uses: actions/checkout@v4 with: - # pull_request_target runs the workflow in the context of the base repo - # as such actions/checkout needs to be explicit configured to retrieve - # code from the PR. - ref: refs/pull/${{ github.event.pull_request.number }}/merge + repository: ${{ github.event.repository.full_name }} # Uses the full repository name + ref: ${{ github.ref }} # Uses the ref from the event + token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token submodules: recursive - name: Setup Python uses: actions/setup-python@v5 From ced4c783353cc284893ebce34271490a34b8486e Mon Sep 17 00:00:00 2001 From: Theodor Mihalache Date: Tue, 24 Sep 2024 14:24:36 -0400 Subject: [PATCH 2/2] fix: Removed usage of pull_request_target as much as possible to prevent security concerns Signed-off-by: Theodor Mihalache --- .../fork_pr_integration_tests_aws.yml | 7 ++-- .../fork_pr_integration_tests_gcp.yml | 7 ++-- .../fork_pr_integration_tests_snowflake.yml | 7 ++-- .github/workflows/java_pr.yml | 33 ++++++++++++------- .github/workflows/lint_pr.yml | 6 +--- .github/workflows/pr_integration_tests.yml | 15 ++++++--- .../workflows/pr_local_integration_tests.yml | 3 +- 7 files changed, 45 insertions(+), 33 deletions(-) diff --git a/.github/fork_workflows/fork_pr_integration_tests_aws.yml b/.github/fork_workflows/fork_pr_integration_tests_aws.yml index b0ff3b5b63d..6eb8b8feff0 100644 --- a/.github/fork_workflows/fork_pr_integration_tests_aws.yml +++ b/.github/fork_workflows/fork_pr_integration_tests_aws.yml @@ -27,9 +27,10 @@ jobs: steps: - uses: actions/checkout@v4 with: - repository: ${{ github.event.repository.full_name }} # Uses the full repository name - ref: ${{ github.ref }} # Uses the ref from the event - token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token + # pull_request_target runs the workflow in the context of the base repo + # as such actions/checkout needs to be explicit configured to retrieve + # code from the PR. + ref: refs/pull/${{ github.event.pull_request.number }}/merge submodules: recursive - name: Setup Python uses: actions/setup-python@v5 diff --git a/.github/fork_workflows/fork_pr_integration_tests_gcp.yml b/.github/fork_workflows/fork_pr_integration_tests_gcp.yml index 009fb8ce157..be9844a7e93 100644 --- a/.github/fork_workflows/fork_pr_integration_tests_gcp.yml +++ b/.github/fork_workflows/fork_pr_integration_tests_gcp.yml @@ -27,9 +27,10 @@ jobs: steps: - uses: actions/checkout@v4 with: - repository: ${{ github.event.repository.full_name }} # Uses the full repository name - ref: ${{ github.ref }} # Uses the ref from the event - token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token + # pull_request_target runs the workflow in the context of the base repo + # as such actions/checkout needs to be explicit configured to retrieve + # code from the PR. + ref: refs/pull/${{ github.event.pull_request.number }}/merge submodules: recursive - name: Setup Python uses: actions/setup-python@v5 diff --git a/.github/fork_workflows/fork_pr_integration_tests_snowflake.yml b/.github/fork_workflows/fork_pr_integration_tests_snowflake.yml index d8626fb2450..a136b47b9e7 100644 --- a/.github/fork_workflows/fork_pr_integration_tests_snowflake.yml +++ b/.github/fork_workflows/fork_pr_integration_tests_snowflake.yml @@ -27,9 +27,10 @@ jobs: steps: - uses: actions/checkout@v4 with: - repository: ${{ github.event.repository.full_name }} # Uses the full repository name - ref: ${{ github.ref }} # Uses the ref from the event - token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token + # pull_request_target runs the workflow in the context of the base repo + # as such actions/checkout needs to be explicit configured to retrieve + # code from the PR. + ref: refs/pull/${{ github.event.pull_request.number }}/merge submodules: recursive - name: Setup Python uses: actions/setup-python@v5 diff --git a/.github/workflows/java_pr.yml b/.github/workflows/java_pr.yml index 08c69036e24..caf31ab47fc 100644 --- a/.github/workflows/java_pr.yml +++ b/.github/workflows/java_pr.yml @@ -1,15 +1,18 @@ name: java_pr on: - pull_request: + pull_request_target: types: - opened - synchronize - labeled +permissions: + pull-requests: read + jobs: lint-java: - # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -18,15 +21,17 @@ jobs: steps: - uses: actions/checkout@v4 with: - repository: ${{ github.event.repository.full_name }} # Uses the full repository name - ref: ${{ github.ref }} # Uses the ref from the event - token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token + # pull_request_target runs the workflow in the context of the base repo + # as such actions/checkout needs to be explicit configured to retrieve + # code from the PR. + ref: refs/pull/${{ github.event.pull_request.number }}/merge submodules: recursive + persist-credentials: false - name: Lint java run: make lint-java unit-test-java: - # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -36,10 +41,12 @@ jobs: steps: - uses: actions/checkout@v4 with: - repository: ${{ github.event.repository.full_name }} # Uses the full repository name - ref: ${{ github.ref }} # Uses the ref from the event - token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token + # pull_request_target runs the workflow in the context of the base repo + # as such actions/checkout needs to be explicit configured to retrieve + # code from the PR. + ref: refs/pull/${{ github.event.pull_request.number }}/merge submodules: recursive + persist-credentials: false - name: Set up JDK 11 uses: actions/setup-java@v1 with: @@ -66,7 +73,7 @@ jobs: path: ${{ github.workspace }}/docs/coverage/java/target/site/jacoco-aggregate/ build-docker-image-java: - # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -82,6 +89,7 @@ jobs: - uses: actions/checkout@v4 with: submodules: 'true' + persist-credentials: false - name: Setup Python uses: actions/setup-python@v5 id: setup-python @@ -101,7 +109,7 @@ jobs: run: make build-${{ matrix.component }}-docker REGISTRY=${REGISTRY} VERSION=${GITHUB_SHA} integration-test-java-pr: - # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -113,11 +121,12 @@ jobs: steps: - uses: actions/checkout@v4 with: - # pull_request runs the workflow in the context of the base repo + # pull_request_target runs the workflow in the context of the base repo # as such actions/checkout needs to be explicit configured to retrieve # code from the PR. ref: refs/pull/${{ github.event.pull_request.number }}/merge submodules: recursive + persist-credentials: false - name: Set up JDK 11 uses: actions/setup-java@v1 with: diff --git a/.github/workflows/lint_pr.yml b/.github/workflows/lint_pr.yml index f64497a5ec3..81732258455 100644 --- a/.github/workflows/lint_pr.yml +++ b/.github/workflows/lint_pr.yml @@ -7,14 +7,10 @@ on: - edited - synchronize -permissions: - # read-only perms specified due to use of pull_request in lieu of security label check - pull-requests: read - jobs: validate-title: if: - github.repository == 'feast-dev/feast' + github.event.pull_request.base.repo.full_name == 'feast-dev/feast' name: Validate PR title runs-on: ubuntu-latest steps: diff --git a/.github/workflows/pr_integration_tests.yml b/.github/workflows/pr_integration_tests.yml index 62febb26f6b..59de3ce9585 100644 --- a/.github/workflows/pr_integration_tests.yml +++ b/.github/workflows/pr_integration_tests.yml @@ -1,7 +1,7 @@ name: pr-integration-tests on: - pull_request: + pull_request_target: types: - opened - synchronize @@ -11,10 +11,13 @@ on: #concurrency: # group: pr-integration-tests-${{ github.event.pull_request.number }} # cancel-in-progress: true +permissions: + actions: write + pull-requests: read jobs: integration-test-python: - # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. + # when using pull_request_target, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && @@ -41,10 +44,12 @@ jobs: steps: - uses: actions/checkout@v4 with: - repository: ${{ github.event.repository.full_name }} # Uses the full repository name - ref: ${{ github.ref }} # Uses the ref from the event - token: ${{ secrets.GITHUB_TOKEN }} # Automatically provided token + # pull_request_target runs the workflow in the context of the base repo + # as such actions/checkout needs to be explicit configured to retrieve + # code from the PR. + ref: refs/pull/${{ github.event.pull_request.number }}/merge submodules: recursive + persist-credentials: false - name: Setup Python uses: actions/setup-python@v5 id: setup-python diff --git a/.github/workflows/pr_local_integration_tests.yml b/.github/workflows/pr_local_integration_tests.yml index abf9e3ced86..6515d411f01 100644 --- a/.github/workflows/pr_local_integration_tests.yml +++ b/.github/workflows/pr_local_integration_tests.yml @@ -10,11 +10,10 @@ on: jobs: integration-test-python-local: - # when using pull_request, all jobs MUST have this if check for 'ok-to-test' or 'approved' for security purposes. if: ((github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'lgtm' || github.event.label.name == 'ok-to-test')) || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved') || contains(github.event.pull_request.labels.*.name, 'lgtm')))) && - github.repository == 'feast-dev/feast' + github.event.pull_request.base.repo.full_name == 'feast-dev/feast' runs-on: ${{ matrix.os }} strategy: fail-fast: false