Introduce the GitHub Actions Workflow Standards page#159
Conversation
dingo-d
left a comment
There was a problem hiding this comment.
I had just one comment, otherwise looks good 👍🏼
|
@johnbillion You seem to be mixing two different tasks in this single PR:
As those are different decision points, with different justifications, could you please move the workflow changes to their own PR ? |
ab12d32 to
9a907eb
Compare
Done: #161 |
desrosj
left a comment
There was a problem hiding this comment.
@johnbillion some small suggestions. Some may be a bit deeper than you're looking to go in this high-level guide, and some are stylistic preferences.
|
|
||
| ```yaml | ||
| - name: Print title | ||
| run: echo "Title: ${PR_TITLE}" |
There was a problem hiding this comment.
Small nit and admittedly a personal preference, but I prefer that run: comes last in the step definition because it ensures the reader notices all conditions, variables, environments, etc. before reading the actual command that is run.
| **Correct:** | ||
|
|
||
| ```yaml | ||
| - uses: actions/github-script@... |
There was a problem hiding this comment.
Same preference comment here about the uses: being last. However, I feel more strongly that any inputs passed should be immediately after the uses: line.
|
|
||
| Writing to `$GITHUB_ENV` or `$GITHUB_PATH` from a shell script is dangerous if the input is user-controlled, because an attacker can inject arbitrary environment variables or prepend to `PATH`. | ||
|
|
||
| - Only write to `$GITHUB_ENV` or `$GITHUB_OUTPUT` with values that are fully controlled by the workflow, not with values derived from pull request content, issue bodies, commit messages, or other user-controllable inputs. |
There was a problem hiding this comment.
Should we include an example about how to do this properly by wrapping $GITHUB_OUTPUT in double quotes?
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| ``` | ||
|
|
||
| Always include a version comment after the SHA to make the pinned version human-readable. When updating an action, update both the SHA and the version comment. |
There was a problem hiding this comment.
May be worth noting that the version specified within the online comment must match the actual version tag published by the action's repository.
|
|
||
| Using GitHub Actions caching in workflows that produce release artifacts is risky. A cache can be poisoned by an attacker in a separate workflow, allowing the poisoned cache to inject malicious content into a release. | ||
|
|
||
| Avoid using `actions/cache` or built-in caching features in workflows that build and publish packages or release artifacts. If caching is necessary in such workflows, ensure the cache key is scoped tightly and the cache contents are verified before use. |
There was a problem hiding this comment.
Cache poisoning can also occur when a workflow specifies restore-keys. We should explicitly discourage the use of partial cache key matching patterns in favor of explicit hash-based keys where the hash was produced using the relevant lock files.
rodrigoprimo
left a comment
There was a problem hiding this comment.
Looks good to me overall. I left a couple of inline comments with questions about small things that caught my attention.
|
|
||
| Writing to `$GITHUB_ENV` or `$GITHUB_PATH` from a shell script is dangerous if the input is user-controlled, because an attacker can inject arbitrary environment variables or prepend to `PATH`. | ||
|
|
||
| - Only write to `$GITHUB_ENV` or `$GITHUB_OUTPUT` with values that are fully controlled by the workflow, not with values derived from pull request content, issue bodies, commit messages, or other user-controllable inputs. |
There was a problem hiding this comment.
The paragraph above flags writing to $GITHUB_ENV or $GITHUB_PATH, but this line mentions writing to $GITHUB_ENV or $GITHUB_OUTPUT. Should it say $GITHUB_PATH instead of $GITHUB_OUTPUT? I might be missing something, as I'm not super familiar with how GitHub Actions uses those environment variables.
|
|
||
| **Incorrect:** | ||
|
|
||
| ```yaml |
There was a problem hiding this comment.
The YAML code blocks on the published page are not syntax-highlighted. They render as <code class=""> rather than <code class="language-yaml"> like the PHP and CSS blocks do on other pages. Do you know if ```yaml fences work with the importer? I don't know whether the published page was generated from a version of this Markdown file.
GaryJones
left a comment
There was a problem hiding this comment.
Also, the PR body still says "This PR also fixes some issues in the qa.yml workflow file in this repo so it adheres to these standards.", even though that was moved out.
| <h2>Language-specific Standards</h2> | ||
| <ul> | ||
| <li><a href="https://developer.wordpress.org/coding-standards/wordpress-coding-standards/css/">CSS Coding Standards</a></li> | ||
| <li><a href="https://developer.wordpress.org/coding-standards/wordpress-coding-standards/github-actions/">GitHub Actions Workflow Standards</a></li> |
There was a problem hiding this comment.
It’s slotted under “Language-specific Standards” alongside CSS/HTML/JS/PHP. GitHub Actions isn’t a language; it’s YAML config for a CI platform. This will read oddly to anyone scanning the index, and it implies a parity (“this is a WordPress coding standard”) that the content doesn’t quite live up to. A separate “Infrastructure / Tooling Standards” grouping would be more honest, and would leave room for similar future docs (Composer, npm, Docker).
|
|
||
| GitHub Actions workflows operate in a highly privileged software supply chain environment. Workflows can access repository secrets, push code, create releases, publish packages, and interact with external services. A security weakness in a workflow file can have severe consequences. | ||
|
|
||
| WordPress uses two complementary linting tools to help maintain the quality and security of workflow files in the `.github/workflows` directory: [Actionlint](https://github.com/rhysd/actionlint) and [Zizmor](https://github.com/zizmorcore/zizmor). This page documents the tools and how contributors should address errors or warnings that they report. |
There was a problem hiding this comment.
The scope is undefined. The doc says “WordPress uses…” then only names wordpress-develop as an enforcement point. Does this apply to Gutenberg? Plugin/theme contributors? The wpcs-docs repo itself? The PR body hints at all of them, but the page should state its scope explicitly — otherwise it’s a guideline dressed as a standard.
There was a problem hiding this comment.
The end goal is to add enforcement of these practices org-wide to ensure all repositories remain secure. But that will take time.
That said, we could maybe add a section linking to the repositories enforcing this to date. Gutenberg runs a workflow, but I don't believe it is configured as a check that is required to pass currently. I could be wrong on that and would need to check.
| @@ -0,0 +1,185 @@ | |||
| # GitHub Actions Workflow Standards | |||
There was a problem hiding this comment.
There's a title vs content imbalance here. It's called “GitHub Actions Workflow Standards”, but the substance is almost entirely Zizmor audit findings rephrased. Actionlint gets one paragraph and zero concrete examples. Either rename it to something like “Workflow Security Standards”, or beef up the Actionlint side (common findings, shellcheck guidance, expression typing pitfalls).
For a document presented as the standard, I’d expect at least a mention of:
- timeout-minutes (DoS / runaway jobs)
- Concurrency controls (
concurrency:block) - GITHUB_TOKEN scoping vs PATs
- etc.
| contents: read | ||
| ``` | ||
|
|
||
| ### Artipacked credentials |
There was a problem hiding this comment.
This is a Zizmor audit name. For readers who’ve never used Zizmor, it’s jargon. “Persistent checkout credentials (Zizmor: artipacked)” reads more naturally.
|
|
||
| Using GitHub Actions caching in workflows that produce release artifacts is risky. A cache can be poisoned by an attacker in a separate workflow, allowing the poisoned cache to inject malicious content into a release. | ||
|
|
||
| Avoid using `actions/cache` or built-in caching features in workflows that build and publish packages or release artifacts. If caching is necessary in such workflows, ensure the cache key is scoped tightly and the cache contents are verified before use. |
There was a problem hiding this comment.
“Scope the cache key tightly and verify the cache contents before use” — how? A concrete example or a hard rule (“don’t cache in release workflows, full stop”) would be more useful than the current half-advice.
Work has been ongoing recently to improve the quality and security of GitHub Actions workflow files in the
wordpress-developandgutenbergrepos. Examples:While the Actionlint and Zizmor tools that we now use are great for surfacing problems in workflow files, we also need good user-facing documentation that provides guidance on addressing common issues and what automation is in place.
Members of the security team have been working on this page and it's already live at https://developer.wordpress.org/coding-standards/wordpress-coding-standards/github-actions/. It now needs to be moved into the docs repo alongside the other pages.