Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Favor GITHUB_WORKFLOW_REF over using actions: read to calculate the workflow path #2126

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Feb 9, 2024

Introduced with GHES 3.9:
https://docs.github.com/en/[email protected]/actions/learn-github-actions/variables

GITHUB_WORKFLOW_REF means that actions don't need to use actions: read to determine the path to the running workflow.

This should address the problem in #2117 as long as the user is running on GHES 3.9+ (including GHEC).

It doesn't precisely fix the issue because someone running on GHES 3.8 will still need to add actions: read until #2121 makes that code path non fatal.

Note that these aren't mutually exclusive PRs (although as they both impact the changelog, they will conflict there...) -- both changes should be applied -- this simplifies the general logic for the average user (and once GHES 3.8 is sunset, the other code path should be removed entirely).

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@jsoref jsoref requested a review from a team as a code owner February 9, 2024 04:47
@SPodjasek
Copy link

I can confirm that this fixes my issue from #2117 without the need to declare actions: read permission:

diff --git a/.github/workflows/local-cd.yml b/.github/workflows/local-cd.yml
index c6bf452..ee11bac 100644
--- a/.github/workflows/local-cd.yml
+++ b/.github/workflows/local-cd.yml
@@ -236,7 +236,6 @@ jobs:
     runs-on: ubuntu-latest
     needs: build-and-publish
     permissions:
-      actions: read
       contents: read
       packages: read
       pull-requests: write
@@ -282,7 +281,8 @@ jobs:
         id: upload-sarif
         continue-on-error: true
         if: hashFiles('sarif.output.json') != '' && github.event_name != 'pull_request_target'
-        uses: github/codeql-action/upload-sarif@main
+        uses: jsoref/github-codeql-action/upload-sarif@favor-workflow-ref
         with:
           sarif_file: sarif.output.json

Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me ✨ just a few documentation suggestions, and the changelog merge conflict will need to be addressed of course!

CHANGELOG.md Outdated Show resolved Hide resolved
src/api-client.ts Outdated Show resolved Hide resolved
src/api-client.ts Fixed Show resolved Hide resolved
@jsoref jsoref force-pushed the favor-workflow-ref branch 2 times, most recently from f8bd51a to b66391a Compare February 13, 2024 14:39
@jsoref jsoref requested a review from angelapwen February 13, 2024 14:39
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had been planning to test something off of your forkmyself, but I haven't had time to in the last few days so I thought I'd ask if you were willing to test it and link the runs after testing 😄

We'd like to make sure that for reusable workflows, the workflow path we originally got from the API is definitely the same as the one we're getting from GITHUB_WORKFLOW_REF. Basically, if they're not the same, alert categories will change after this change. They should be the same, but we'd like to be certain 😸

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -119,6 +119,18 @@
* Get the path of the currently executing workflow relative to the repository root.
*/
export async function getWorkflowRelativePath(): Promise<string> {
const workflow_ref = process.env["GITHUB_WORKFLOW_REF"];

Check warning

Code scanning / CodeQL

Some environment variables may not exist in default setup workflows

The environment variable GITHUB_WORKFLOW_REF may not exist in default setup workflows. If all uses are safe, add it to the list of environment variables that are known to be safe in 'queries/default-setup-environment-variables.ql'. If this use is safe but others are not, dismiss this alert as a false positive.
@jsoref jsoref force-pushed the favor-workflow-ref branch from 1bd245e to 4280b0e Compare February 16, 2024 03:16
@jsoref
Copy link
Contributor Author

jsoref commented Feb 16, 2024

@angelapwen: um, if someone gave me a workflow, I'm open to running it.

I haven't managed to get reusable workflows to do anything particularly useful for me, so I don't generally use them myself (I think there are some reusable workflows somewhere in one of the codeql repositories) -- mostly they've made me dizzy (I tried to figure out a way to set up a reusable workflow for check-spelling, and it didn't seem to add any value).

I have very little time tomorrow and won't have time until Tuesday (Monday's a holiday in Canada). I expect to be moderately busy next week, but I can generally run commands if people provide them.

(Sorry, I have no idea how/why I missed your event this morning...)

Introduced with GHES 3.9:
https://docs.github.com/en/[email protected]/actions/learn-github-actions/variables

GITHUB_WORKFLOW_REF means that actions don't need to use `actions: read`
to determine the path to the running workflow.
@jsoref jsoref force-pushed the favor-workflow-ref branch from 4280b0e to abe17b5 Compare May 22, 2024 02:23
lschiedel02 added a commit to hs-heilbronn-devsecops-fireballs/note-api that referenced this pull request Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants