Skip to content

Disable caching when publishing built assets#79835

Open
desrosj wants to merge 10 commits into
trunkfrom
actions/disable-caching-for-production-builds
Open

Disable caching when publishing built assets#79835
desrosj wants to merge 10 commits into
trunkfrom
actions/disable-caching-for-production-builds

Conversation

@desrosj

@desrosj desrosj commented Jul 2, 2026

Copy link
Copy Markdown
Member

What?

This disables caching within workflows that build production assets to harden against potential cache poisoning attacks.

Why?

Though it's difficult to achieve, cache poisoning is an attack where release workflows leverage a build state cached from previous workflow executions.

How?

Popular third-party actions configure caching by default but support inputs that can disable the behavior.

Use of AI Tools

None

@desrosj desrosj requested a review from Copilot July 2, 2026 19:55
@desrosj desrosj self-assigned this Jul 2, 2026
@desrosj desrosj added the [Type] Build Tooling Issues or PRs related to build tooling label Jul 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens release/publishing workflows against cache-poisoning risks by disabling Node/package-manager caching in workflows that build and publish production assets, and by introducing a caching toggle in the repo’s composite Node setup action.

Changes:

  • Disable actions/setup-node package-manager caching in npm publishing and plugin zip build workflows.
  • Add an enable-caching input to the local .github/setup-node composite action.
  • Gate the composite action’s post-install-on-cache-hit behavior behind the new enable-caching input.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
.github/workflows/publish-npm-packages.yml Disables actions/setup-node package-manager caching during npm publishing.
.github/workflows/build-plugin-zip.yml Disables actions/setup-node package-manager caching during plugin zip build/publish steps.
.github/setup-node/action.yml Adds enable-caching input and uses it to influence setup-node caching and post-install behavior.

Comment thread .github/setup-node/action.yml
Caching can be used when building a zip file for a pull request but should always be disabled for other scenarios (when building the zip for the GitHub Container Registry or for final release).
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @Copilot.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: Copilot.

Co-authored-by: desrosj <desrosj@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown

Flaky tests detected in d111a36.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/29345238761
📝 Reported issues:

@desrosj desrosj requested a review from johnbillion July 6, 2026 14:53
desrosj added 4 commits July 14, 2026 10:50
It's not clear why this was the only workflow that contains the `cache` input, but removing this will allow `package-manager-cache` only to be used without conflict.
with:
node-version-file: ${{ inputs.node-version == '' && '.nvmrc' || '' }}
node-version: ${{ inputs.node-version }}
package-manager-cache: ${{ inputs.enable-caching == 'true' && 'true' || 'false' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Flagged during ai-assisted review

The new guards correctly make the node_modules cache opt-outable, but removing cache: npm also disables actions/setup-node's npm cache for every default caller.

package-manager-cache: true only enables automatic caching when the root package.json declares npm in devEngines.packageManager or the top-level packageManager field (upstream implementation, detection logic). Gutenberg declares neither, so the default enable-caching: true path no longer restores or saves the npm cache across the action's 13 call sites. That contradicts the new input's contract and creates a broad CI performance regression unrelated to the production hardening.

Could we either remove the currently unused composite-action input, or preserve the explicit cache conditionally?

cache: ${{ inputs.enable-caching == 'true' && 'npm' || '' }}
package-manager-cache: false

uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: '.nvmrc'
package-manager-cache: ${{ matrix.IS_GUTENBERG_PLUGIN == 'true' && github.event_name == 'pull_request' && 'true' || 'false' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Flagged during ai-assisted review

Keeping npm caching for Gutenberg-plugin PR builds makes sense, but this condition never enables it. matrix.IS_GUTENBERG_PLUGIN comes from the YAML booleans [true, false], while the expression compares it with the string 'true'; under GitHub's loose-equality rules, boolean true is coerced to 1 and the non-numeric string is NaN, so the comparison is false. Even without that mismatch, package-manager-cache: true cannot auto-detect npm because this repository has no packageManager declaration.

Could we make the intended cache input explicit instead, or remove the exception if caching should now stay disabled for all plugin builds?

cache: ${{ matrix.IS_GUTENBERG_PLUGIN && github.event_name == 'pull_request' && 'npm' || '' }}
package-manager-cache: false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I'd just drop the string comparison ( == 'true') and keep matrix.IS_GUTENBERG_PLUGIN

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread .github/workflows/build-plugin-zip.yml
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: '.nvmrc'
package-manager-cache: ${{ matrix.IS_GUTENBERG_PLUGIN == 'true' && github.event_name == 'pull_request' && 'true' || 'false' }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I'd just drop the string comparison ( == 'true') and keep matrix.IS_GUTENBERG_PLUGIN

Comment on lines +9 to +10
required: true
default: 'true'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we have a default for a required input?


- name: Install npm dependencies
if: ${{ steps.cache-node_modules.outputs.cache-hit != 'true' }}
if: ${{ inputs.enable-caching == 'false' || steps.cache-node_modules.outputs.cache-hit != 'true' }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If caching is disabled the cache step is skipped, so cache-hit != true is already true, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Build Tooling Issues or PRs related to build tooling

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

5 participants