Skip to content

harden github actions#8650

Open
perminder-17 wants to merge 6 commits intodev-2.0from
harden-github-actions-for-dev-2.0
Open

harden github actions#8650
perminder-17 wants to merge 6 commits intodev-2.0from
harden-github-actions-for-dev-2.0

Conversation

@perminder-17
Copy link
Collaborator

Hardening github actions for dev-2.0

run: bash <(curl -s https://codecov.io/bash) -f coverage/coverage-final.json
env:
CI: true
uses: codecov/codecov-action@671740ac38dd9b0130fbe1cec585b89eea48d3de # v5.5.2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://about.codecov.io/apr-2021-post-mortem/

This actually happened in real life in 2021, Codecov's bash uploader was compromised and attackers stole secrets/tokens from thousands of CI pipelines. @ksen0 @davepagurek @limzykenneth

Copy link
Member

Choose a reason for hiding this comment

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

We're not really utilizing it at the moment so it probably can be skipped entirely. We'll do code coverage in Vitest for 2.x at some point and reporting can either use a service like this or even our own bot.

env:
CI: true
- name: Run test
run: npm test -- --project=unit-tests
Copy link
Member

Choose a reason for hiding this comment

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

@perminder-17 needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I think I was directly using the same diff from the main branch which gets me to some unnecessary changes, Fixing that now.

files: release/*
generate_release_notes: true
token: ${{ secrets.ACCESS_TOKEN }}
token: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

@ksen0 ksen0 Mar 16, 2026

Choose a reason for hiding this comment

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

@perminder-17 I don't think the token should change (or, why should it?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In Step 3 (Release p5.js), the release is created on the same repository, so GITHUB_TOKEN is sufficient. Unlike ACCESS_TOKEN, which is a long-lived Personal Access Token, GITHUB_TOKEN is automatically generated and scoped to the current repository for each workflow run, and expires once the workflow completes. ACCESS_TOKEN is only required in Step 4, where cross-repository access is needed to push changes to the p5.js-website repository. What you think?

Copy link
Member

@ksen0 ksen0 Mar 16, 2026

Choose a reason for hiding this comment

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

Ah sure! Then GITHUB_TOKEN seems alright, thank you for the explanation

env:
CI: true
- name: Run test
- name: Run build
Copy link
Member

@ksen0 ksen0 Mar 16, 2026

Choose a reason for hiding this comment

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

Typo (?)

env:
CI: true
- name: Run build
run: npm run build
Copy link
Member

Choose a reason for hiding this comment

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

also typo?

with:
node-version: 22.x
persist-credentials: false
- name: Use Node.js 20.x
Copy link
Member

Choose a reason for hiding this comment

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

why 20?

Copy link
Member

@ksen0 ksen0 left a comment

Choose a reason for hiding this comment

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

I left a few comments, thanks for working on this!

@perminder-17
Copy link
Collaborator Author

I left a few comments, thanks for working on this!

I thought the diff for dev-2.0 would be the same as main, so I directly pushed the same changes there too. But it looks like a few unnecessary changes and maybe some typos got included in the 2.0 branch. Fixing that, :)

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.

3 participants