chore: secure PUBLISH_PREVIEW_NPM_TOKEN with GitHub environment#8011
chore: secure PUBLISH_PREVIEW_NPM_TOKEN with GitHub environment#8011cryptodev-2s merged 4 commits intomainfrom
Conversation
| run: | | ||
| bad=0 | ||
| for f in packages/*/package.json; do | ||
| if jq -e '.scripts // {} | keys[] | select(test("^(pre|post)(pack|publish)"))' "$f" > /dev/null 2>&1; then |
There was a problem hiding this comment.
Rejects any package.json that contains prepack, postpack, prepublish, or postpublish scripts.
| echo "::error::Forbidden lifecycle script in $f" | ||
| bad=1 | ||
| fi | ||
| reg=$(jq -r '.publishConfig.registry // ""' "$f") |
There was a problem hiding this comment.
Rejects any package.json where publishConfig.registry isn't https://registry.npmjs.org/.
Split the publish-preview workflow into 3 jobs to isolate the NPM token behind a `publish-preview` GitHub environment: - build-preview: checks out PR code, builds, uploads artifacts (no secrets) - publish-preview: checks out main, validates and publishes artifacts (token via environment) The publish job validates artifact package.json files before publishing to block lifecycle script injection (prepack/postpack) and registry overrides, then uses a hardcoded `yarn npm publish` loop instead of delegating to package scripts. Also removes the now-unused `publish:preview` script from all packages, `publish-previews` from root package.json, and the corresponding constraint from yarn.config.cjs.
cryptodev-2s
force-pushed
the
chore/secure-publish-preview-token
branch
from
9bf8536 to
5a3033e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| bad=1 | ||
| fi | ||
| done | ||
| exit "$bad" |
There was a problem hiding this comment.
Manifest validation silently passes if glob matches nothing
Low Severity
The "Validate artifact manifests" step can silently succeed without actually checking any files. Bash's default behavior (no nullglob or failglob) means for f in packages/**/package.json iterates once with the literal string if nothing matches. Since jq errors are suppressed via 2>&1, both the lifecycle-script check and registry check silently pass, bad stays at 0, and exit 0 succeeds. This undermines the security guarantee this step is designed to provide — a structural change in the artifact layout could bypass the validation entirely without any error or warning.
There was a problem hiding this comment.
Realistically, it's very unlikely in this workflow. The previous step (actions/download-artifact@v7) downloads artifacts into packages/, and if that step fails, the job already fails before reaching validation. The upload step explicitly includes ./packages/**/package.json.
| bad=1 | ||
| fi | ||
| done | ||
| exit "$bad" |
There was a problem hiding this comment.
Validation misses .yarnrc.yml registry override attack vector
Medium Severity
The manifest validation checks publishConfig.registry in package.json but doesn't check for .yarnrc.yml files that a malicious PR could place in a package directory to set npmPublishRegistry or npmRegistryServer to an attacker-controlled server. Since the upload step uses include-hidden-files: true, such a .yarnrc.yml would be included in the artifact. Yarn Berry resolves .yarnrc.yml by walking up from the current working directory, so a per-workspace file would be found first when yarn npm publish runs via exec, causing YARN_NPM_AUTH_TOKEN to be sent to the attacker's registry.
Additional Locations (1)
## Explanation The `publish-preview` job fails because `yarn workspaces foreach` can't resolve the renamed `@metamask-previews/` packages. The root cause is that `prepare-preview-builds.sh` updates the lockfile via `yarn install --no-immutable`, but `yarn.lock` is not included in the uploaded artifacts. The publish job gets a stale lockfile from main that doesn't match the renamed package names. This adds `yarn.lock` to the artifact upload and removes the `path` override on download so it extracts to the workspace root alongside `packages/`. ## References * Fixes failure introduced in #8011 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > CI workflow-only change that affects artifact packaging/restoration; low risk outside of potentially impacting preview publish reliability. > > **Overview** > Fixes the preview publish workflow by **including the updated `yarn.lock`** in the uploaded build artifact set, ensuring the publish job uses the same lockfile produced by `yarn prepare-preview-builds`. > > Also removes the `actions/download-artifact` `path` override so artifacts (including `yarn.lock`) restore to the workspace root instead of being extracted under `packages/`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 0a45543. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
## Explanation The `publish-preview` job fails because `yarn workspaces foreach` requires a consistent workspace state. The restored artifacts include an updated `yarn.lock` (from `prepare-preview-builds.sh`), but `node_modules` is from the main branch checkout. This adds `yarn install --no-immutable` after restoring artifacts to reconcile the two. ## References * Fixes publish failure introduced in #8011 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > CI-only workflow change that adds a dependency install step; low risk beyond potential impact to publish job timing/behavior. > > **Overview** > Fixes the `publish-preview` GitHub Actions workflow by running `yarn install --no-immutable` after restoring build artifacts, ensuring the checked-out workspace state matches the restored `yarn.lock` before `yarn workspaces foreach ... npm publish` executes. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 0f25648. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
2 participants
Explanation
Scopes PUBLISH_PREVIEW_NPM_TOKEN to the default-branch GitHub environment, matching the pattern used by publish-release.yml.
Splits the workflow into two jobs: build (no secrets) and publish (environment-scoped secret, manifest validation). Removes the unused publish:preview script from all packages and yarn.config.cjs.
Required before merge
References
publish-release.yml(environment: npm-publish)Checklist
Note
Medium Risk
Changes the CI release/publish pipeline and secret handling; mistakes could block or mis-publish preview releases, but the added environment scoping and manifest validation reduce security exposure.
Overview
Preview publishing is refactored to separate build and publish:
build-previewbuilds the PR and uploads artifacts, andpublish-preview(running in thedefault-branchGitHub environment) downloads artifacts and publishes withPUBLISH_PREVIEW_NPM_TOKEN.The publish job adds a safety gate that rejects artifacts whose
package.jsoncontainspre/postpack/publishlifecycle scripts or a non-npmjspublishConfig.registry, reducing token-exfiltration risk. Repository scripts/constraints are updated by removing the rootpublish-previewsscript and deletingpublish:previewscripts from workspace packages (and the related enforcement inyarn.config.cjs).Written by Cursor Bugbot for commit 17bd1c8. This will update automatically on new commits. Configure here.