Skip to content

ci: prevent command injection in release workflow - #99

Merged
dinwwwh merged 1 commit into
mainfrom
claude/npm-publishing-ci-injection-1499ce
Sep 25, 2026
Merged

dinwwwh merged 1 commit into
mainfrom
claude/npm-publishing-ci-injection-1499ce

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Sep 25, 2026

Copy link
Copy Markdown
Member

The release workflow substituted its workflow_dispatch inputs (version, tag) directly into shell scripts, so a crafted value could run arbitrary commands in the job that publishes to npm with provenance (id-token: write). The version check didn't help, because it ran after the substituted line had already executed. Inputs now reach the scripts only as environment variables, and both the version and the dist-tag are validated in the first step, before anything is installed, bumped or published.

Fixes

  • Crafted version or tag inputs no longer execute: no ${{ }} expression remains inside any run: script
  • Malformed or option-like dist-tags (e.g. x --registry=…) now fail the run immediately instead of reaching pnpm publish
  • Dist-tags must now start with a lowercase letter and contain only a-z, 0-9, ., _ or -

Testing

  • Ran the original and patched workflows side by side under act, with bump/changelog/publish commands replaced by argument printers. Valid inputs (stable, prerelease, explicit tag, every skip flag on/off) produce identical commands; payloads that executed on the original are rejected in the first step
  • zizmor: 3 template-injection findings → 0. actionlint and ESLint pass; the only remaining note is an intentionally unquoted $NO_GIT_CHECKS, so an empty value adds no argument

Pass workflow_dispatch inputs to shell scripts through env instead of
${{ }} substitution, and validate the npm dist-tag alongside the version
in the first step so a malformed tag fails before install or publish.
@pkg-pr-new

pkg-pr-new Bot commented Sep 25, 2026

Copy link
Copy Markdown
@standard-server/aws-lambda

npm i https://pkg.pr.new/@standard-server/aws-lambda@99

@standard-server/core

npm i https://pkg.pr.new/@standard-server/core@99

@standard-server/fastify

npm i https://pkg.pr.new/@standard-server/fastify@99

@standard-server/fetch

npm i https://pkg.pr.new/@standard-server/fetch@99

@standard-server/node

npm i https://pkg.pr.new/@standard-server/node@99

@standard-server/peer

npm i https://pkg.pr.new/@standard-server/peer@99

@standard-server/shared

npm i https://pkg.pr.new/@standard-server/shared@99

commit: 5a02ebc

@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@codspeed

codspeed Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 108 skipped benchmarks1


Comparing claude/npm-publishing-ci-injection-1499ce (5a02ebc) with main (e92aea4)2

Open in CodSpeed

Footnotes

  1. 108 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

  2. No successful run was found on main (469d94b) during the generation of this report, so e92aea4 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

Reviewed the single-file change to .github/workflows/release.yaml (initial review) — a CI security hardening fix.

  • Env-var migration for dispatch inputs — version and tag now reach scripts only through env: (VERSION, TAG_INPUT); no ${{ }} interpolation remains in any run: body, which removes the injection vector that reached the npm-publishing job.
  • Up-front version and tag validation — the new first step validates version against the semver regex, rejects a latest preid, resolves the dist-tag, and validates it against ^[a-z][a-z0-9._-]*$ before checkout, install, bump, or publish.
  • Boolean skip flags — skip_bump/skip_changelog conditions changed to !inputs.skip_bump / !inputs.skip_changelog, which preserves behavior for typed boolean inputs including when omitted.
  • Publish step — uses the validated --tag="$TAG" and $NO_GIT_CHECKS, both supplied via env.

Verified that the validated tag cannot inject extra $GITHUB_OUTPUT lines (the regex excludes = and newlines), that steps.version.outputs.tag can only carry a validated value, and that the dist-tag fallback ${TAG_INPUT:-${PREID:-latest}} is equivalent to the prior || chain and always non-empty. The one remaining raw interpolation of inputs.version is run-name, which is display-only and not an execution context.

Pullfrog  | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

@dinwwwh
dinwwwh merged commit a6c90ca into main Sep 25, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant