fix(core): pass release publish commands as argv instead of shell strings - #36874
Open
AgentEnder wants to merge 7 commits into
Open
fix(core): pass release publish commands as argv instead of shell strings#36874AgentEnder wants to merge 7 commits into
AgentEnder wants to merge 7 commits into
Conversation
Use pnpm's config-prefixed scoped registry option so explicit release registries override workspace .npmrc settings.\n\nclosed #36860
A line break ends a cmd.exe command line whatever the quoting sits around it, so a quoted run cannot carry one and everything after it is read as a fresh command. `quoteShellArg` caught a literal double quote but not this. - move `LINE_BREAK` into shell-quoting and export it, so the rule has one definition; `safe-spawn` now imports it instead of declaring its own - throw from `quoteShellArg` on Windows only, since a POSIX single-quoted run does hold a line break safely `safeSpawn`'s own `quoteForCmd` check stays as the first guard so its contextual messages are unchanged.
`publish` returned a shell string into which the registry and tag were
interpolated. Both come back from `npm config get`, i.e. verbatim from a
workspace .npmrc, which an install script can write - so shell syntax in
either one reached the shell that ran the command.
- add `publishArgv`, returning `{ command, args }`, for all four package
managers
- derive `publish` from it through `quoteShellArg` so the two renderings
cannot drift, and deprecate it for Nx 24
Off Windows the argv form needs no shell at all. Windows still quotes,
and now fails closed on the arguments quoting cannot make safe.
Every child process the publish executor started went through `execSync` with its arguments interpolated into the command string. Besides publish itself, `npm view` and `npm dist-tag add` carried the same registry and tag, and the latter interpolated the tag with no quoting at all. - move all four sinks onto `safeExecFileSync`, passing argv - pass `otp`, `access` and `--dry-run` as argv elements too - read npm config through `safeSpawn` in `parseRegistryOptions`, so the scope taken from the package manifest is an argument rather than part of the command string `safeExecFileSync` fixes the encoding to utf-8, so the error streams are now strings; the specs track that instead of leaving the mocks wrong.
`readCurrentVersionFromRegistry` interpolated the same registry and tag into an `exec` command string, so `nx release version` reached a shell with them on every platform when `currentVersionResolver` is `registry`. Run it through `safeSpawn` with argv, matching the publish path, and cover the method - it had no test before. `cwd` stays unset so npm still reads the same .npmrc `exec` gave it.
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
View your CI Pipeline Execution ↗ for commit d3cbc23
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
🎓 Learn more about Self-Healing CI on nx.dev
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Stacked on #36867 — needs a rebase once that merges.
The bottom two commits here are @wangxpych's from #36867, rebased onto current master. They are
included only because this PR rewrites the same
pnpmpublish block and would otherwise conflictwith it. Once #36867 lands, rebase this branch onto master and those two commits drop out; the
diff is then just the four
NXC-4918commits. Please don't merge this before #36867.Current Behavior
nx releasebuilds its npm invocations as shell strings and runs them throughexecSync/exec,interpolating the registry and tag directly into the command. Both values default to
npm config get registry/npm config get tag, which return the workspace.npmrcvalueverbatim, and the tag was interpolated with no quoting at all.
That makes
.npmrcan execution vector rather than configuration. It matters because.npmrciswritable by a dependency's install lifecycle script, and these commands run in CI with publish
credentials present.
The same pair reaches a shell from more than the one place:
nx release publish<pm> publish,npm view/bun info,npm dist-tag addnx release versionnpm view … version, whencurrentVersionResolverisregistrynpm config get <scope>:registry, where the scope comes from the package manifest--otpand--accesswere interpolated unquoted too.Expected Behavior
Every one of those commands is passed as argv, so shell syntax in any of the values is data
rather than syntax. Off Windows no shell is involved at all.
PackageManagerCommandsgainspublishArgv, returning{ command, args }, for all four packagemanagers. The existing
publishis kept for external callers, but is now derived frompublishArgvthroughquoteShellArg— so the two renderings cannot drift — and is deprecated forNx 24.
parseRegistryOptionsandreadCurrentVersionFromRegistryall move ontothe existing
safeExecFileSync/safeSpawnhelpers.quoteShellArgnow refuses a line break on Windows, matching the guardsafeSpawnalreadyapplied. A line break ends a cmd.exe command line whatever quoting surrounds it, so this is the
one input quoting cannot make safe. POSIX is unaffected, since a single-quoted run holds a newline
fine.
Coverage
Regression tests assert the untrusted values arrive as single argv elements rather than
asserting an emitted string, so they fail if the escaping is removed rather than merely changing.
The string form additionally round-trips through a real shell and asserts the tokens come back equal
to the argv, with NUL delimiting so a newline cannot forge a token boundary. Each new guard was
mutation-tested — removing it fails the corresponding test.
readCurrentVersionFromRegistryhad no test at all before this; it has one now.Not closed by this PR
On Windows
npmis a bare name, sosafeExecFileSyncstill goes through cmd.exe, which expands%VAR%inside quotes. That is the pre-existing, documented gap tracked as NXC-4798, and it nowhas
.npmrcas one of its inputs. Worth knowing before treating this as a complete fix on Windows.Related Issue(s)
Internal ticket: NXC-4918. No public issue is linked, so there is nothing for this to auto-close.