#59805 closed task (blessed) (fixed)
GitHub Actions updates and improvements for 6.5
Reported by: | jorbin | Owned by: | |
---|---|---|---|
Milestone: | 6.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | has-patch |
Focuses: | Cc: |
Change History (49)
This ticket was mentioned in PR #5619 on WordPress/wordpress-develop by @ayeshrajans.
11 months ago
#1
- Keywords has-patch added
#2
@
11 months ago
Suggesting a small improve to add permissions: {}
to all GHA yml files. Most of the CI workflows already have it, but I found four that did not.
This ticket was mentioned in PR #5620 on WordPress/wordpress-develop by @ayeshrajans.
11 months ago
#3
Trac ticket: https://core.trac.wordpress.org/ticket/59805
This ticket was mentioned in PR #5625 on WordPress/wordpress-develop by @swissspidy.
11 months ago
#4
This is a follow-up to [56972] from core-58867
Turns out github.event.before
is also "empty" containing all 00000
for the first commit in a new branch when opening a PR. That means when opening a PR performance tests don’t currently run until you add a second commit.
To avoid this scenario but still prevent an error when a new permanent branch like 6.4
is created, this changes the performance test workflow to simply skip the target comparison if there is no "before".
Trac ticket: https://core.trac.wordpress.org/ticket/59805
@swissspidy commented on PR #5625:
11 months ago
#5
Proof that the performance test ran for the very first commit in this PR, before I accidentally cancelled it by pushing another commit: https://github.com/WordPress/wordpress-develop/actions/runs/6771717029/job/18402712469?pr=5625
11 months ago
#6
Thanks for this, @ayesh!
I did a little bit of digging, and this is explicitly noted in the Reusing Workflows documentation:
- If
jobs.<job_id>.permissions
is not specified in the calling job, the called workflow will have the default permissions for theGITHUB_TOKEN
.- The
GITHUB_TOKEN
permissions passed from the caller workflow can be only downgraded (not elevated) by the called workflow.
We are currently passing permissions
to the callable workflows with contents: read
in the calling workflows with a few exceptions in the upgrade-testing.yml
file. It seems the first testing job only has permissions
defined. We should add that.
I'm trying to think through scenarios where not having permissions
in the callable workflow would be problematic if we're always explicitly passing permissions
. Additionally, since the workflow is hard coded to use the version found within trunk
(which can only be committed to through SVN), I'm not sure if we need to. I guess the scenario would need to be:
- Create a PR changing the target branch for the called workflow.
- Remove
permissions
within that branch in the calling workflow. - Change the called workflow to misuse/escalate
$GITHUB_TOKEN
.
However, I don't think that this would have any affect. The maximum access for pull requests from public forked repositories is `read` for all scopes. The attacker would need to use pull_request_target
, and that does not work unless the workflow exists within the base branch with pull_request_target
as well.
@swissspidy commented on PR #5625:
11 months ago
#9
Looks like https://core.trac.wordpress.org/changeset/57085 fixed this
11 months ago
#10
@swissspidy Went to reopen this but looks like the branch was deleted. Do you want to undelete and reopen, or open a fresh PR?
#16
follow-up:
↓ 17
@
9 months ago
@desrosj actions/cache
was just updated to v4 last week which updates the action to use Node 20. Would be nice to make that bump to get rid of the dozens of "Node.js 16 actions are deprecated" warnings in the logs.
#17
in reply to:
↑ 16
@
8 months ago
Replying to swissspidy:
@desrosj
actions/cache
was just updated to v4 last week which updates the action to use Node 20. Would be nice to make that bump to get rid of the dozens of "Node.js 16 actions are deprecated" warnings in the logs.
Looks like the latest Dependabot PR should take care of these.
I wish that GitHub surfaced these in some sort of notifications screen. They do announce these things ahead of time on their (WordPress 🎉) blog, but unless you're looking at the workflow encountering the notices you'd never know when they actually implement the changes.
#19
@
8 months ago
Looks like we're still seeing some of those Node.js notices, most seem due to the setup-php
action needing to be updated. The change has been made upstream, but a release has not yet been published.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
7 months ago
#24
@
7 months ago
- Resolution set to fixed
- Status changed from new to closed
Closing for now since 6.5 has been branched, so any new updates like this can happen in trunk for 6.6.
@swissspidy commented on PR #5625:
6 months ago
#25
No longer needed I think
Trac ticket: https://core.trac.wordpress.org/ticket/59805