diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2b790de26f..47a5d46591 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -8,10 +8,13 @@ on: workflow_dispatch: inputs: branch: - description: 'Branch to release from' + description: 'Branch to release from (must be a protected release branch)' required: true default: 'next' - type: string + type: choice + options: + - next + - master version_type: description: 'Version bump type' required: true @@ -55,6 +58,25 @@ jobs: id-token: write steps: + # The `branch` choice is not enforced for API-triggered dispatches, so + # re-validate it here and require the workflow to be dispatched from the + # very branch it releases (dispatch ref == release branch). This is the + # load-bearing guard against releasing an arbitrary ref. + - name: Guard release branch + env: + INPUT_BRANCH: ${{ github.event.inputs.branch }} + DISPATCH_REF: ${{ github.ref_name }} + run: | + set -euo pipefail + case "$INPUT_BRANCH" in + next|master) ;; + *) echo "::error::Disallowed release branch '$INPUT_BRANCH' (allowed: next, master)"; exit 1 ;; + esac + if [ "$DISPATCH_REF" != "$INPUT_BRANCH" ]; then + echo "::error::Dispatched from '$DISPATCH_REF' but branch input is '$INPUT_BRANCH'. Re-run 'Use workflow from' set to '$INPUT_BRANCH'." + exit 1 + fi + - name: Checkout uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6 with: diff --git a/scripts/publish.js b/scripts/publish.js index 3e915c2a48..07916b15fb 100755 --- a/scripts/publish.js +++ b/scripts/publish.js @@ -26,6 +26,7 @@ import path from 'node:path'; import { applyPublishConfigOverrides, + assertValidNpmPackageName, getCatalogs, getPublishablePackages, getWorkspaceVersionMap, @@ -48,6 +49,28 @@ const versionMap = getWorkspaceVersionMap(baseDir); const catalogs = getCatalogs(baseDir); const npmBin = process.platform === 'win32' ? 'npm.cmd' : 'npm'; +// Reject any malformed package name before it reaches `npm publish` (a typo'd +// name would otherwise publish a brand-new bogus package). +for (const pkg of packages) { + assertValidNpmPackageName(pkg.name); +} + +// Never let a prerelease version land on the `latest` dist-tag (that would make +// it the default install for every consumer). The release workflow derives the +// tag, but this is the last line of defence regardless of how it was invoked. +if (npmTag === 'latest') { + const prereleases = packages.filter((pkg) => pkg.version.includes('-')); + if (prereleases.length > 0) { + const sample = prereleases + .slice(0, 5) + .map((pkg) => `${pkg.name}@${pkg.version}`) + .join(', '); + const more = prereleases.length > 5 ? ` (+${prereleases.length - 5} more)` : ''; + console.error(`❌ Refusing to publish prerelease version(s) to the "latest" tag: ${sample}${more}`); + process.exit(1); + } +} + console.log( `πŸ“¦ Publishing ${packages.length} packages (tag: ${npmTag}${isDryRun ? ', dry-run' : ''}${useProvenance ? ', provenance' : ''})`, ); @@ -63,9 +86,16 @@ function isPublished(name, version) { timeout: 15000, }).trim(); return result === version; - } catch { - // Could be 404 (not published) or network error. - // Either way, we should attempt to publish. + } catch (err) { + const stderr = String(err?.stderr ?? ''); + // A genuine 404 means the version is not published yet β€” expected, stay quiet. + // Anything else (network, 5xx, auth) is indeterminate: we cannot confirm, so + // warn loudly. We still return false, but npm's version immutability prevents + // an accidental overwrite if a publish is then attempted. + if (!/E404|404 Not Found/i.test(stderr)) { + const detail = stderr.split('\n')[0] || (err instanceof Error ? err.message : String(err)); + console.warn(` ⚠️ could not verify ${name}@${version} on npm: ${detail}`); + } return false; } } @@ -98,7 +128,10 @@ function publishOne(pkg) { execFileSync(npmBin, publishArgs, { cwd: packageDir, stdio: 'inherit', - env: { ...process.env, NPM_CONFIG_LOGLEVEL: 'verbose' }, + // Verbose logging only on dry-run. On a real publish, force a non-verbose + // level so an inherited NPM_CONFIG_LOGLEVEL=verbose can't surface auth + // headers in CI logs. + env: { ...process.env, NPM_CONFIG_LOGLEVEL: isDryRun ? 'verbose' : 'notice' }, timeout: 120000, }); } finally { diff --git a/scripts/utils.js b/scripts/utils.js index 1e81f28014..349f60e62a 100644 --- a/scripts/utils.js +++ b/scripts/utils.js @@ -29,6 +29,33 @@ const PUBLISH_CONFIG_OVERRIDE_FIELDS = [ 'os', ]; +// Valid npm package name (scoped or unscoped). Names that fail this are +// rejected before they reach a git command (release commit message) or an +// `npm publish` invocation β€” defence in depth against a malicious or +// malformed `name` field smuggling shell metacharacters or a typo'd package. +const NPM_NAME_RE = /^(?:@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; + +/** + * Whether `name` is a syntactically valid npm package name (scoped or unscoped, + * 1-214 chars, lowercase, URL-safe). + * @param {unknown} name + * @returns {boolean} + */ +export function isValidNpmPackageName(name) { + return typeof name === 'string' && name.length > 0 && name.length <= 214 && NPM_NAME_RE.test(name); +} + +/** + * Throw if `name` is not a valid npm package name. Used to reject a malicious or + * typo'd `name` before it reaches a git command or `npm publish`. + * @param {unknown} name + */ +export function assertValidNpmPackageName(name) { + if (!isValidNpmPackageName(name)) { + throw new Error(`Invalid npm package name: ${JSON.stringify(name)}`); + } +} + function readWorkspaceConfig(baseDir) { const workspaceFile = path.join(baseDir, 'pnpm-workspace.yaml'); diff --git a/scripts/version.js b/scripts/version.js index 082249a160..c93b1df415 100644 --- a/scripts/version.js +++ b/scripts/version.js @@ -1,13 +1,13 @@ #!/usr/bin/env node -import { execSync } from 'node:child_process'; +import { execFileSync, execSync } from 'node:child_process'; import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import semver from 'semver'; -import { getPublishablePackages } from './utils.js'; +import { assertValidNpmPackageName, getPublishablePackages } from './utils.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); @@ -67,6 +67,7 @@ packageFolders.forEach(({ folder, directory }) => { if (fs.existsSync(packageJsonPath)) { const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8')); + assertValidNpmPackageName(packageJson.name); const originalContent = JSON.stringify(packageJson, null, 2) + '\n'; if (!isDryRun) { @@ -135,14 +136,15 @@ try { ${updatedVersions.map((pkg) => `- ${pkg.name}@${pkg.newVersion}`).join('\n')}`; - // Commit changes + // Commit changes. Pass the message as an argv entry (execFileSync), never as + // an interpolated shell string β€” the message embeds package names. console.log('\nπŸ’Ύ Creating version commit...'); - execSync(`git commit -m "${commitMessage}"`, { stdio: 'inherit' }); + execFileSync('git', ['commit', '-m', commitMessage], { stdio: 'inherit' }); // Create tag using the main egg version const tagName = `v${eggVersion}`; console.log(`\n🏷️ Creating tag ${tagName}...`); - execSync(`git tag ${tagName}`, { stdio: 'inherit' }); + execFileSync('git', ['tag', tagName], { stdio: 'inherit' }); console.log('\nβœ… Version bump complete!'); console.log(`\nTo publish, push the changes and tag:`); diff --git a/wiki/decisions/secure-release-pipeline.md b/wiki/decisions/secure-release-pipeline.md new file mode 100644 index 0000000000..9951ccb39d --- /dev/null +++ b/wiki/decisions/secure-release-pipeline.md @@ -0,0 +1,198 @@ +--- +title: Secure release pipeline +type: decision +summary: Target design for hardening the npm release pipeline β€” GitHub-Release-triggered publish, Environment-gated with a second-person approval, OIDC/provenance pinned to the workflow+environment, build/publish privilege split, and a GitHub App token replacing the long-lived PAT. P0 script/workflow hardening landed in #6017. +source_files: + - .github/workflows/release.yml + - scripts/version.js + - scripts/publish.js + - scripts/utils.js + - scripts/sync-cnpm.js +updated_at: 2026-06-28 +status: active +--- + +# Secure release pipeline + +Decision record for hardening the eggjs/egg npm release pipeline. Produced from a +multi-agent design pass (three candidate architectures, each red-teamed, then +synthesized). The maintainer chose a **GitHub-Release-triggered** target model. + +## Context + +### Current pipeline (as-is, before this work) + +`.github/workflows/release.yml` is a single `workflow_dispatch` job that, with +`permissions: contents:write packages:write id-token:write`, checks out +`inputs.branch` (a free-form string) using a `GIT_TOKEN` PAT, then runs version +bump β†’ `git push origin --tags` β†’ `ut run build` β†’ +`node scripts/publish.js --provenance` (npm OIDC trusted publishing) β†’ draft +GitHub Release β†’ `sync-cnpm.js`. All steps run in one job with every secret +present throughout. + +### Threat model (anchor) + +- **Not externally exploitable.** `workflow_dispatch` requires repo _write_ + access; fork PRs get no secrets. +- **Real threats:** (a) compromised maintainer account or leaked `GIT_TOKEN` + PAT; (b) social engineering ("test-release my branch"); (c) accidental + wrong-branch / wrong-version publish. +- **Amplifier:** provenance/OIDC means a malicious artifact gets a _genuine_ + provenance attestation β€” publishing arbitrary branch code as "official, signed" + egg packages is the worst case. + +### Weaknesses + +- **W1** `inputs.branch` is unconstrained β†’ any ref can be checked out, pushed + to, and published. +- **W2** The dispatch ref (workflow definition) and `inputs.branch` (code) can + differ β†’ reviewing `release.yml` does not tell you what code ships. +- **W3** One job runs arbitrary install/build/publish code _with_ the npm OIDC + credential and the PAT present β†’ no privilege separation. +- **W4** `GIT_TOKEN` is a long-lived broad PAT (vs an ephemeral token). +- **W5** No second-person approval before npm publish. + +## Decision + +Move to a **two-workflow, GitHub-Release-triggered, Environment-gated** pipeline: + +1. **`release-prepare.yml`** (`workflow_dispatch`): bump versions, create a + **signed** commit + tag pushed to a protected branch via a short-lived GitHub + App token, and open a **draft** GitHub Release. No publish, no OIDC. +2. A maintainer reviews and **publishes** the draft Release. That deliberate, + audited action is the trigger. +3. **`release.yml`** (`on: release: [published]`): `guard` β†’ `build` (zero-secret) + β†’ `publish` (Environment-gated, OIDC-only, manifests re-derived from source) + β†’ `finalize`. + +Chosen over the dispatch-driven and release-PR alternatives because `on: release` +makes review-integrity automatic (see below) while keeping egg's prerelease/retry +UX and the existing `version.js` / `publish.js` scripts largely intact. + +## Verified GitHub-behavior facts (load-bearing) + +- For the **`release`** event, the workflow YAML is read from the **default + branch**, and `GITHUB_SHA` is the tagged release's commit, `GITHUB_REF` is + `refs/tags/`. Source: GitHub docs, "Events that trigger workflows". This + means the publish logic can **not** be swapped via a crafted tag/release β€” it + is always the reviewed default-branch version (closes W2 by construction). +- egg's **default branch is `next`** (`gh api repos/eggjs/egg --jq .default_branch`), + so `release.yml` must live on `next`, and the version bump/tag land on `next`. +- `workflow_dispatch` **`choice` inputs are NOT enforced for API-triggered + dispatches** (only the UI dropdown validates). So an in-workflow guard step + re-validating the value is load-bearing, not cosmetic. Source: GitHub + changelog "Input types for manual workflows" + community confirmation. +- A GitHub Release can be created pointing at **any** commit, so the `guard` must + verify `GITHUB_SHA` is reachable from a protected branch. + +## Architecture + +``` +release-prepare.yml (workflow_dispatch, from next/master) + bump install (NO token) β†’ version.js (manifests only) β†’ upload bumped tree + push download tree β†’ mint GitHub App token β†’ signed commit + signed tag + β†’ push to next β†’ create DRAFT GitHub Release [no 3rd-party code] + + ── maintainer reviews & PUBLISHES the draft Release ──► + +release.yml (on: release: [published]; workflow read from `next`) + guard contents:read no secrets β†’ GITHUB_SHA is ancestor of next/master? + tag == egg version? derive npm_tag from semver (prereleaseβ‡’beta/…, + else latest); fail-closed on mismatch + build contents:read NO secrets, NO id-token + ut install (frozen) β†’ ut run build β†’ emit dist digest + publish environment:npm-release id-token:write only ← REQUIRED-REVIEWER GATE + download artifact + verify digest == build output + checkout GITHUB_SHA source; re-derive manifests from SOURCE + publish.js --provenance (npm publish, --ignore-scripts in P1) + finalize contents:read (+ App token, no OIDC) β†’ undraft/notes + sync-cnpm +``` + +### Per-job permissions & secret exposure + +| Job | `permissions` | Secrets | Runs 3rd-party code? | +| ------------ | ----------------------------------- | ------------------- | ------------------------------ | +| prepare/bump | `contents: read` | none | yes (`ut install`) | +| prepare/push | `contents: read` | App key β†’ ≀1h token | no (git only) | +| guard | `contents: read` | none | no | +| build | `contents: read` | **none** | yes (install+build) | +| publish | `id-token: write`, `contents: read` | **OIDC only** | no (`npm publish`) | +| finalize | `contents: read` | App token | github-script + tokenless sync | + +The npm trusted publisher is pinned to `repository=eggjs/egg` **AND** +`workflow=.github/workflows/release.yml` **AND** `environment=npm-release`. The +environment claim is the keystone: a token can only be minted by a run that +passed the required-reviewer gate. + +## Threat-coverage matrix + +| ID | Weakness / threat | How the target design closes it | Residual risk | +| --- | ----------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| W1 | Any ref releasable | release can point at any commit β†’ `guard` requires `GITHUB_SHA` reachable from `next`/`master`; npm won't mint a token outside `release.yml`+environment | admin edits guard/allowlist (covered by CODEOWNERS) | +| W2 | Review β‰  what ships | `on: release` reads the workflow from the **default branch** (always the reviewed one); code checked out is the tagged `GITHUB_SHA` | `dist/` is built post-tag β€” see W3 | +| W3 | One job holds OIDC + arbitrary code | build (no secrets) β†’ publish (OIDC only, no `ut`/build, manifests re-derived from source) | poisoned build dependency can corrupt `dist/` β†’ signed; mitigated by `--ignore-scripts` + frozen lockfile + digest pin; full closure needs reproducible builds (P2) | +| W4 | Long-lived PAT | GitHub App token (≀1h, `contents:write` only), minted only in the push/finalize steps, never co-resident with 3rd-party code; delete `GIT_TOKEN` | leaked App key mints ≀1h `contents:write` but **cannot publish** | +| W5 | No second person | publishing the draft Release is one gate; `environment: npm-release` required reviewers (prevent-self-review, reviewer β‰  releaser) is the second | two colluding/compromised maintainers; admin Environment bypass | +| (c) | Accidental wrong version | npm dist-tag derived from the version's semver, fail-closed; prereleaseβ†’`latest` refused in `publish.js` | reviewer rubber-stamps | + +## Script-level hardening (independent of topology) + +Found by the red-team; landed as **P0** in +[#6017](https://github.com/eggjs/egg/pull/6017): + +- `version.js` built the release commit via `execSync(\`git commit -m "${msg}"\`)`with package names interpolated β†’ **shell-injection sink**. Fixed with`execFileSync` argv + npm-name validation. +- `publish.js` `isPublished()` swallowed _all_ errors (network/5xx β‡’ "not + published"). Now distinguishes a real 404 from indeterminate errors (warns). +- `publish.js` real-publish logging forced to `notice` (was `verbose`) to avoid + surfacing auth headers in CI logs. +- `publish.js` rejects malformed package names and refuses to publish a + prerelease version to the `latest` dist-tag. +- `release.yml` `branch` input constrained to `next`/`master` + a runtime guard + (dispatch ref == release branch). + +Still **deferred to P1**: `npm publish --ignore-scripts` β€” unsafe today because +`@eggjs/egg-bundler` has `prepublishOnly: npm run build`; belongs with the +build/publish split where `dist/` is pre-built and digest-verified. + +## Supporting config + +| Area | Setting | Where | +| --------------------------------- | -------------------------------------------------------------------------------------------------------------- | -------------- | +| npm trusted publisher (Γ—79) | repo + `release.yml` + environment `npm-release`; remove any static `NPM_TOKEN` fallback | npm | +| Environment `npm-release` | required reviewers; prevent-self-review ON; reviewer β‰  releaser; deployment branches `next`/`master` | repo settings | +| Branch protection `next`/`master` | required review; include administrators; signed commits; direct push only by the App | repo settings | +| CODEOWNERS | `/.github/`, `/scripts/`, `pnpm-workspace.yaml` β†’ release owners | repo file | +| Tag protection | `refs/tags/v*`: only the release App creates; no force-update/delete | repo settings | +| Token (W4) | GitHub App, `contents: write`; `actions/create-github-app-token`; delete `GIT_TOKEN` PAT | repo/org | +| Action pinning | SHA-pin `create-github-app-token`, `upload/download-artifact`; verify `setup-utoo`; checksum the `ut` download | workflow files | +| Runners | GitHub-hosted only; no self-hosted for release | org/repo | + +## Phased migration + +- **P0 (done β€” [#6017](https://github.com/eggjs/egg/pull/6017))** β€” code/workflow + hardening above; no infra changes. +- **P1 (structural)** β€” GitHub App + delete PAT; split buildβ†’publish with digest + verification + `--ignore-scripts`; `environment: npm-release` with required + reviewers; npm trusted-publisher pinned; switch to `on: release` trigger + + `release-prepare.yml`; CODEOWNERS + enforce-admins + signed commits/tags. +- **P2 (depth)** β€” split `release-prepare` so `ut install` never co-resides with + the App token; `actions/attest-build-provenance` + reproducible-build diff + (closes the W3 residual); pin/checksum the toolchain; periodic out-of-band + audit of the npm trusted-publisher config. + +## Decisions taken + +| Decision | Choice | Rationale | +| ------------------------- | -------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------- | +| Trigger model | **GitHub Release (`on: release`)** | Workflow read from default branch β‡’ review-integrity is automatic; publishing the Release is an auditable human gate | +| Token | **GitHub App** (replace `GIT_TOKEN` PAT) | Ephemeral per-run vs standing broad credential | +| Manifest rewrite location | **re-derive in `publish` from `GITHUB_SHA` source** | a poisoned build artifact must not be able to rewrite `dependencies`/`exports` post-review | +| `master` in allowlist | keep **only** if it has identical protection + reviewers | weakest allowlisted branch sets the floor | +| Reproducible builds | accept residual now, schedule P2 | high-effort for 79 packages; mitigate with `--ignore-scripts` + frozen lockfile + digest first | + +## Related + +- [[local-ci]] β€” release runs build then tests; this design keeps build in a + zero-secret job. +- npm publish protocol/`publishConfig` handling: see #6016 (the npm-switch this + builds on). diff --git a/wiki/index.md b/wiki/index.md index e2996b1c3a..4054896924 100644 --- a/wiki/index.md +++ b/wiki/index.md @@ -18,7 +18,7 @@ Read this file before exploring raw sources. ## Decisions -- No decision pages yet. +- [Secure release pipeline](./decisions/secure-release-pipeline.md) - Target design for hardening npm release: GitHub-Release-triggered, Environment-gated second-person approval, OIDC pinned to workflow+environment, build/publish privilege split, GitHub App token replacing the PAT; P0 script/workflow hardening landed in #6017. ## Packages diff --git a/wiki/log.md b/wiki/log.md index 97cc34bb55..0f98303d1f 100644 --- a/wiki/log.md +++ b/wiki/log.md @@ -2,6 +2,12 @@ Dates use the workspace-local Asia/Shanghai calendar date. +## [2026-06-28] decision | secure release pipeline design + P0 hardening + +- sources touched: `.github/workflows/release.yml`, `scripts/version.js`, `scripts/publish.js`, `scripts/utils.js`, `scripts/sync-cnpm.js` +- pages updated: `wiki/index.md`, `wiki/log.md`, `wiki/decisions/secure-release-pipeline.md` +- note: Recorded the target secure-release design from a multi-agent design pass (3 architectures red-teamed β†’ synthesized). Maintainer chose a GitHub-Release-triggered model: `release-prepare.yml` (dispatch) bumps + signs + drafts a Release; publishing the Release triggers `release.yml` (`on: release`, read from default branch `next`) β†’ guard β†’ zero-secret build β†’ Environment-gated OIDC publish β†’ finalize. Verified GitHub facts: release events read the workflow from the default branch with `GITHUB_SHA`=tag commit; `choice` inputs aren't API-enforced; egg default branch is `next`. P0 (code/workflow hardening: argv git ops, npm-name validation, prereleaseβ†’latest guard, isPublished 404-handling, notice loglevel, branch choice+guard) landed in #6017; `--ignore-scripts` deferred to P1 because `@eggjs/egg-bundler` has a `prepublishOnly` build. + ## [2026-06-28] workflow | record egg-bin Windows shell probe hotspot - sources touched: `tools/egg-bin/bin/run.js`, `tools/egg-bin/test/fixtures/my-egg-bin/bin/run.js`, PR #6014 CI logs