Terminate Ninja options before build target operands

[leynos]

Summary

Netsuke appends build targets directly after its trusted Ninja options without inserting Ninja's -- option terminator.

On main at ac10b783f33661a0af27bd8f5d19065f095f98db, configure_ninja_build_command() constructs the base command with -j and -f <generated-build-file>, then calls:

cmd.args(targets.as_slice());

Ninja continues parsing options in that suffix. A target string beginning with - can therefore alter Ninja's control plane instead of naming a target. This affects both explicit CLI targets and project-configured default_targets; the two Codex findings are the same argv-boundary defect.

Reproduction

A bounded reproduction against Ninja 1.12.1 used a trusted build file and an attacker-controlled sibling directory containing another build.ninja.

Without a terminator:

ninja safe -C ../evil default

Ninja entered ../evil, loaded its build file, executed its recipe, and returned success.

With a terminator:

ninja -- safe -C ../evil default

Ninja treated -C and its following strings as target operands; the attacker-controlled recipe did not run.

The same primitive permits later -f, -t, -j, and other recognized Ninja options. For example, project configuration can supply:

default_targets = ["-f", "evil.ninja"]

When the user invokes netsuke or netsuke build without explicit targets, the resulting argv selects the attacker's Ninja file after Netsuke has already supplied its generated one.

std::process::Command correctly prevents shell metacharacter injection, but it does not distinguish child-program options from operands.

Impact

An attacker who can influence a target name forwarded by a developer/CI wrapper, or who controls automatically discovered project configuration, can redirect Ninja to an attacker-selected directory, build file, tool mode, or option set. That can execute attacker-controlled build recipes with the invoking developer or CI account's ambient filesystem and environment privileges.

Required change

Insert a literal -- between all Netsuke-owned Ninja options and every build-target operand:

ninja ... -f <generated-build-file> -- <targets...>

Use the same command-shaping path for explicit and configured default targets. If supported Ninja versions behave differently for an empty operand list, insert the terminator whenever the selected target list is non-empty; otherwise prefer one uniform argv shape.

Do not solve this solely by rejecting leading hyphens. Ninja can represent unusual literal target names, and the child program already provides the correct operand boundary.

Acceptance criteria

• Every Ninja build invocation places -- immediately before target operands.
• Explicit CLI targets and default_targets from configuration use the same protected path.
• A configured target list ['-f', 'evil.ninja'] cannot replace the generated build file.
• An explicit target suffix equivalent to -- -C evil default cannot change Ninja's working directory.
• Unit tests inspect the exact Command argv ordering, including no-target and multi-target cases.
• An integration test with a real Ninja executable proves the attacker-controlled build file or directory is not selected.
• Ordinary target selection, default-target behaviour, clean/graph tool modes, and supported platforms remain unchanged.
• make check-fmt, make lint, and make test pass.

References

• Codex finding: configured default targets: https://chatgpt.com/codex/cloud/security/findings/d57f2937e1e481919d2ecc25f19bc81e?repo=https%3A%2F%2Fgithub.com%2Fleynos%2Fnetsuke&sev=medium
• Codex finding: explicit target forwarding: https://chatgpt.com/codex/cloud/security/findings/52ffb4aa67f88191b655cb737ecdb9f8?repo=https%3A%2F%2Fgithub.com%2Fleynos%2Fnetsuke&sev=medium
• Current command construction: https://github.com/leynos/netsuke/blob/ac10b783f33661a0af27bd8f5d19065f095f98db/src/runner/process/configure.rs
• Current configuration model: https://github.com/leynos/netsuke/blob/ac10b783f33661a0af27bd8f5d19065f095f98db/src/cli/config.rs

[leynos]

@CodeRabbit plan

[coderabbitai]

Coding Plan

Summary

Insert a literal -- in configure_ninja_build_command() immediately before the forwarded targets, but only when the target list is non-empty.

Rely on the existing single convergence point so explicit CLI targets and configured default_targets share the same protected path; leave the clean tool path and in-process graph path unchanged.

Add unit tests for exact argv ordering (no-target, single-target, multi-target), fake-Ninja argv-log tests for the malicious default_targets and explicit-suffix cases, and a real-Ninja integration test that proves the attacker directory is not selected.

Record the decision in a new ADR and reference it from design §6.1, following the repository's documentation conventions.

Follow-up agent prompts

Apply the Phase 1 fix in src/runner/process/configure.rs. In configure_ninja_build_command(), after configure_ninja_base() returns, check whether request.targets.as_slice() is non-empty. When non-empty, push a literal "--" argument before the existing cmd.args(targets.as_slice()) call. When empty, append nothing extra. Do not modify configure_ninja_tool_command() or configure_ninja_base().

Add the Phase 2 tests. Extend src/runner/process/configure.rs::tests to assert the exact argv Vec<OsString> for no-target, single-target, and multi-target cases, using the existing command_arguments() and expected_base_arguments() helpers. Extend tests/runner_cases/default_targets.rs with FakeNinjaFixture cases for default_targets = ["-f", "evil.ninja"] and an explicit "-C evil default" suffix, asserting "--" precedes the operands in the recorded argv log. Add a real-Ninja integration test using the test_support ninja harness that proves the attacker sibling build.ninja is not selected.

Add the Phase 3 documentation. Create a new ADR in docs/ numbered after adr-017, following docs/documentation-style-guide.md section order, recording the decision to insert "--" before Ninja build-target operands and rejecting the leading-hyphen approach. Cross-reference adr-015. Update docs/netsuke-design.md §6.1 to describe the terminator and reference the new ADR.

Design Choices

Design Choice 1: Insert the `--` terminator unconditionally, or only when the target list is non-empty?

Options Considered:

1. Insert -- only when the resolved target list is non-empty. An empty target list keeps the current argv (... -f <build_file>), so Ninja builds its defaults exactly as before.
2. Insert -- unconditionally, even for an empty target list, producing ... -f <build_file> --.

Chosen Option: 1

Rationale: It removes any risk from an empty-operand edge case, preserves the existing empty-target behaviour and its current test, and still satisfies every attacker-facing acceptance criterion, because those all involve non-empty target lists. The ticket permits either shape: insert the terminator whenever the selected target list is non-empty if supported Ninja versions behave differently for an empty operand list; otherwise prefer one uniform argv shape.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Insert the argv option terminator

This phase applies the security fix at the single vulnerable command-shaping site. It changes only the build path, so clean and graph modes stay unchanged. Both explicit CLI targets and configured default_targets already converge through this site, so one change protects both.

Terminate options before build targets

Add the -- terminator immediately before the forwarded target operands in the build command builder.

• Update configure_ninja_build_command() in src/runner/process/configure.rs.
• After configure_ninja_base(...) returns, test whether request.targets.as_slice() is non-empty.
• When the target list is non-empty, append a literal -- argument, then append the targets with the existing cmd.args(...) call.
• When the target list is empty, append nothing extra, so the argv stays ... -f <build_file>.
• Do not change configure_ninja_tool_command(); it appends only the fixed -t <tool> operand and takes no user-controlled list.
• Do not change configure_ninja_base(); the flag order (-j, then -f) is unchanged.

🤖 Prompt for AI agents

Implement the security fix that stops attacker-controlled target names from
being read as Ninja options.

- Edit `configure_ninja_build_command()` in `src/runner/process/configure.rs`.
- After `configure_ninja_base(...)` runs, check `request.targets.as_slice()`.
- If the target list is non-empty, push a literal `--` argument onto the command
before the existing `cmd.args(targets.as_slice())` call, so the terminator sits
immediately before the target operands.
- If the target list is empty, append nothing extra; the argv must stay as `...
-f <build_file>` with no trailing `--`.
- Leave `configure_ninja_tool_command()` untouched; it only appends a fixed `-t
<tool>` operand and never handles a user-controlled list.
- Leave `configure_ninja_base()` untouched; keep the existing flag order of `-j`
then `-f`.
- This is the single change point: explicit CLI targets and configured
`default_targets` both flow through `BuildTargets<'a>` into this function via
`execute_build` in `src/runner/mod.rs`, so one edit protects both sources. Clean
mode and graph mode do not spawn Ninja through this path and must remain
unaffected.

Phase 2: Prove the terminator with tests

This phase adds unit coverage for the exact argv shape and an integration test with a real Ninja binary. The tests use the established patterns so they slot into the existing suites.

Task 1: Assert exact argv ordering in unit tests

Extend the argv-ordering unit tests to prove the terminator position for the no-target, single-target, and multi-target cases.

• Extend the #[cfg(test)] mod tests block in src/runner/process/configure.rs.
• Reuse the existing command_arguments() helper and the expected_base_arguments() builder.
• Update or extend build_configuration_preserves_argument_order so the expected Vec<OsString> contains a literal "--" entry immediately before the target "default".
• Add a multi-target case that asserts "--" appears once, immediately before the first target, followed by every target in order.
• Add a no-target case that asserts no "--" entry is present and the argv ends at -f <build_file>; align this with the existing build_configuration_uses_uncanonicalized_missing_build_file expectation.
• Keep tool_configuration_preserves_argument_order unchanged to confirm the tool path is unaffected.

Task 2: Cover the configured-default-targets path with the fake Ninja fixture

Prove that a malicious default_targets list is passed as operands after --, not interpreted as Ninja options.

• Extend tests/runner_cases/default_targets.rs, reusing FakeNinjaFixture and its args_log.
• Add a case that sets default_targets to ["-f", "evil.ninja"] and invokes the build path with no explicit CLI targets.
• Read the recorded argv log, collect the lines, and assert that "--" appears before "-f" and "evil.ninja" in the operand suffix.
• Add a case with an explicit target suffix equivalent to -C evil default and assert the same ---before-operands ordering in the recorded argv.

Task 3: Prove protection against a real Ninja executable

Add an integration test that shows the attacker-controlled build file or directory is not selected.

• Add the test using the real-Ninja harness in test_support/src/ninja.rs / ninja_gen.rs (ninja_integration_setup), following tests/ninja_gen_integration_tests.rs.
• Create a trusted build.ninja in the workspace and a sibling directory that contains an attacker-controlled build.ninja with a distinct recipe or output.
• Drive the build through Netsuke's build path with a target list equivalent to safe -C ../evil default.
• Assert that the attacker recipe did not run and the attacker directory was not selected; for example, assert the attacker's output artefact is absent.
• Gate the test on the existing Unix and Ninja-availability conditions, so it skips when Ninja is absent unless NETSUKE_REQUIRE_NINJA=1.

🤖 Prompt for AI agents

Add test coverage across three levels to prove the terminator works and blocks
the attack.

Unit tests (`src/runner/process/configure.rs::tests`):
- Reuse `command_arguments()` and `expected_base_arguments()`.
- Update or extend `build_configuration_preserves_argument_order` so the
expected `Vec<OsString>` has `"--"` immediately before the target `"default"`.
- Add a multi-target case: assert `"--"` appears exactly once, immediately
before the first target, and all targets follow in order.
- Add a no-target case: assert there is no `"--"` entry, and the argv ends at
`-f <build_file>`; keep this consistent with
`build_configuration_uses_uncanonicalized_missing_build_file`.
- Leave `tool_configuration_preserves_argument_order` unchanged.

Fake-Ninja fixture tests (`tests/runner_cases/default_targets.rs`):
- Reuse `FakeNinjaFixture` and its `args_log`.
- Add a case where `default_targets = ["-f", "evil.ninja"]` with no explicit CLI
targets; read the argv log and assert `"--"` precedes `"-f"` and `"evil.ninja"`.
- Add a case with an explicit target suffix equivalent to `-C evil default`;
assert the same `--`-before-operands ordering.

Real-Ninja integration test:
- Use the harness in `test_support/src/ninja.rs` / `ninja_gen.rs`
(`ninja_integration_setup`), following the pattern in
`tests/ninja_gen_integration_tests.rs`.
- Set up a trusted `build.ninja` in the workspace and a sibling directory
holding an attacker-controlled `build.ninja` with a distinct recipe or output.
- Run the build through Netsuke's build path with a target list equivalent to
`safe -C ../evil default`.
- Assert the attacker recipe did not run and the attacker directory was not
selected, for example by asserting the attacker's output artefact is absent.
- Gate the test on the existing Unix and Ninja-availability checks so it skips
cleanly when Ninja is absent, unless `NETSUKE_REQUIRE_NINJA=1` is set.

Phase 3: Record the decision

This phase documents the trust-boundary decision. The repository convention records substantive security decisions in an ADR and references the design document.

Add an ADR and update the design document

Record the argv option-terminator decision and cross-reference it from the design document.

• Create a new ADR in docs/, numbered after adr-017, using the naming pattern adr-NNN-short-description.md.
• Follow the required section order from docs/documentation-style-guide.md: Status, Date, Context and Problem Statement, then Decision, Rationale, Consequences, Alternatives considered, Implementation references.
• State the decision to insert -- between Netsuke-owned Ninja options and build-target operands on the build path.
• In Alternatives considered, record and reject the "reject leading hyphens" approach, because Ninja can represent unusual literal target names and the child program already provides the correct operand boundary.
• Cross-reference docs/adr-015-use-bounded-git-cli-for-change-detection.md as the existing option-separation precedent.
• Update docs/netsuke-design.md §6.1 ("Invoking Ninja") to describe the terminator and reference the new ADR.

🤖 Prompt for AI agents

Document the security decision so future maintainers understand the trust
boundary.

- Create a new ADR file in `docs/`, numbered after `adr-017`, using the naming
pattern `adr-NNN-short-description.md`.
- Follow the section order required by `docs/documentation-style-guide.md`:
Status, Date, Context and Problem Statement, Decision, Rationale, Consequences,
Alternatives considered, Implementation references.
- State the decision clearly: insert `--` between Netsuke-owned Ninja options
and the forwarded build-target operands on the build path.
- In the Alternatives considered section, record and reject the "reject leading
hyphens" approach. Give the reason: Ninja can represent unusual literal target
names, and the child program already provides the correct operand boundary.
- Cross-reference `docs/adr-015-use-bounded-git-cli-for-change-detection.md` as
the existing precedent for option separation.
- Update `docs/netsuke-design.md` §6.1 ("Invoking Ninja") to describe the
terminator and to reference the new ADR.

Research

Netsuke builds the Ninja invocation in src/runner/process/configure.rs. The private helper configure_ninja_base() appends the Netsuke-owned flags (-j <N> when present, then -f <build_file>). The public configure_ninja_build_command() then calls cmd.args(targets.as_slice()), which appends the target strings as the final argv entries with no separator. The sibling configure_ninja_tool_command() appends only a fixed -t <tool> operand and takes no user-controlled list. Explicit CLI targets and default_targets converge into a single decision point in src/runner/mod.rs::execute_build, which wraps them in BuildTargets<'a> before they reach configure_ninja_build_command(). Graph mode never spawns Ninja. Existing argv-ordering unit tests live in src/runner/process/configure.rs::tests, and a real-Ninja integration harness plus a fake-Ninja argv-logging fixture already exist in test_support/ and tests/runner_cases/default_targets.rs.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement the security fix that stops attacker-controlled target names from
being read as Ninja options.

- Edit `configure_ninja_build_command()` in `src/runner/process/configure.rs`.
- After `configure_ninja_base(...)` runs, check `request.targets.as_slice()`.
- If the target list is non-empty, push a literal `--` argument onto the command
before the existing `cmd.args(targets.as_slice())` call, so the terminator sits
immediately before the target operands.
- If the target list is empty, append nothing extra; the argv must stay as `...
-f <build_file>` with no trailing `--`.
- Leave `configure_ninja_tool_command()` untouched; it only appends a fixed `-t
<tool>` operand and never handles a user-controlled list.
- Leave `configure_ninja_base()` untouched; keep the existing flag order of `-j`
then `-f`.
- This is the single change point: explicit CLI targets and configured
`default_targets` both flow through `BuildTargets<'a>` into this function via
`execute_build` in `src/runner/mod.rs`, so one edit protects both sources. Clean
mode and graph mode do not spawn Ninja through this path and must remain
unaffected.
===============================================================================

Task: 2

Add test coverage across three levels to prove the terminator works and blocks
the attack.

Unit tests (`src/runner/process/configure.rs::tests`):
- Reuse `command_arguments()` and `expected_base_arguments()`.
- Update or extend `build_configuration_preserves_argument_order` so the
expected `Vec<OsString>` has `"--"` immediately before the target `"default"`.
- Add a multi-target case: assert `"--"` appears exactly once, immediately
before the first target, and all targets follow in order.
- Add a no-target case: assert there is no `"--"` entry, and the argv ends at
`-f <build_file>`; keep this consistent with
`build_configuration_uses_uncanonicalized_missing_build_file`.
- Leave `tool_configuration_preserves_argument_order` unchanged.

Fake-Ninja fixture tests (`tests/runner_cases/default_targets.rs`):
- Reuse `FakeNinjaFixture` and its `args_log`.
- Add a case where `default_targets = ["-f", "evil.ninja"]` with no explicit CLI
targets; read the argv log and assert `"--"` precedes `"-f"` and `"evil.ninja"`.
- Add a case with an explicit target suffix equivalent to `-C evil default`;
assert the same `--`-before-operands ordering.

Real-Ninja integration test:
- Use the harness in `test_support/src/ninja.rs` / `ninja_gen.rs`
(`ninja_integration_setup`), following the pattern in
`tests/ninja_gen_integration_tests.rs`.
- Set up a trusted `build.ninja` in the workspace and a sibling directory
holding an attacker-controlled `build.ninja` with a distinct recipe or output.
- Run the build through Netsuke's build path with a target list equivalent to
`safe -C ../evil default`.
- Assert the attacker recipe did not run and the attacker directory was not
selected, for example by asserting the attacker's output artefact is absent.
- Gate the test on the existing Unix and Ninja-availability checks so it skips
cleanly when Ninja is absent, unless `NETSUKE_REQUIRE_NINJA=1` is set.
===============================================================================

Task: 3

Document the security decision so future maintainers understand the trust
boundary.

- Create a new ADR file in `docs/`, numbered after `adr-017`, using the naming
pattern `adr-NNN-short-description.md`.
- Follow the section order required by `docs/documentation-style-guide.md`:
Status, Date, Context and Problem Statement, Decision, Rationale, Consequences,
Alternatives considered, Implementation references.
- State the decision clearly: insert `--` between Netsuke-owned Ninja options
and the forwarded build-target operands on the build path.
- In the Alternatives considered section, record and reject the "reject leading
hyphens" approach. Give the reason: Ninja can represent unusual literal target
names, and the child program already provides the correct operand boundary.
- Cross-reference `docs/adr-015-use-bounded-git-cli-for-change-detection.md` as
the existing precedent for option separation.
- Update `docs/netsuke-design.md` §6.1 ("Invoking Ninja") to describe the
terminator and to reference the new ADR.

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---