fix: validate target input before YAML/CLI interpolation#34
Merged
Conversation
The `target` action input flowed verbatim into the generated apm.yml
(isolated mode) and into `apm pack --target <value>`. A value
containing newlines, `#`, `:`, quotes, or stray whitespace could
break YAML parsing, inject extra top-level keys into the manifest,
or smuggle CLI flags into the pack invocation.
Add a `parseTargetInput` helper applied at both call sites that:
- trims and rejects empty input (returns undefined as before),
- splits on `,` to support APM's CSV multi-target form,
- requires every token to match `^[a-z][a-z0-9-]{0,31}$` -- the
shape of every shipped APM harness name (agent-skills, claude,
codex, copilot, cursor, gemini, opencode, windsurf) and any
plausible future addition,
- throws a descriptive error on the first invalid token so the
action fails fast before writing a malformed manifest.
10 new unit tests cover the rejection matrix (newline, CR, `#`,
`:`, quoting, leading dash, empty CSV token, uppercase, stray
whitespace) and the normalisation of well-formed CSV input. All
109 existing + new tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the target action input by validating and normalizing it before it’s interpolated into the generated apm.yml (isolated mode) and before it’s passed to apm pack --target, preventing YAML breakage/injection and rejecting malformed values early.
Changes:
- Add
parseTargetInput(raw)to trim, normalize CSV targets, and enforce a strict allowlist regex. - Apply
parseTargetInputat bothtargetcall sites (isolated manifest generation and pack invocation). - Add unit tests covering unsafe
targetinputs and valid CSV normalization; regeneratedist/index.js.
Show a summary per file
| File | Description |
|---|---|
| src/runner.ts | Introduces parseTargetInput and uses it to validate/normalize target before manifest/pack usage. |
| src/tests/runner.test.ts | Adds tests for rejecting unsafe target values and accepting normalized CSV targets in isolated mode. |
| dist/index.js | Rebuilds compiled output to include the new validation logic. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/3 changed files
- Comments generated: 2
Comment on lines
349
to
352
| // 8. Pack mode: produce bundle after install | ||
| if (packInput) { | ||
| const target = core.getInput('target').trim() || undefined; | ||
| const target = parseTargetInput(core.getInput('target')); | ||
| const archive = core.getInput('archive') !== 'false'; |
Collaborator
Author
There was a problem hiding this comment.
Good catch — adopted in 713355d. Hoisted parseTargetInput to the top of run() (alongside packInput/isolated parsing), so an invalid target now fails before install/audit/compile run. Both downstream call sites consume the same validatedTarget constant.
Comment on lines
+239
to
+243
| it.each([ | ||
| ['newline injection', 'copilot\ninjected: true'], | ||
| ['carriage return', 'copilot\rfoo'], | ||
| ['comment char', 'copilot # nope'], | ||
| ['colon adds key', 'copilot: extra'], |
Collaborator
Author
There was a problem hiding this comment.
Adopted in 713355d. Added a pack-mode test (pack: true, isolated: false, target: "copilot --evil-flag") that asserts setFailed fires, mockRunPackStep is never called, and mockEnsureApmInstalled is never reached either — locking in both the CLI-side guard and the early-fail guarantee.
Address Copilot review on #34: - Move `parseTargetInput` to the top of `run()` (alongside packInput and isolated parsing), so an invalid `target` fails before any install/audit/compile/script side-effects. Both call sites now consume the validated `validatedTarget` constant rather than re-parsing core.getInput('target'). - Add regression test asserting that in pack mode (`pack: true, isolated: false`) an unsafe target rejects up front: setFailed fires, runPackStep is never called, and ensureApmInstalled is never even reached. This locks in the CLI-side guard against flag smuggling via `apm pack --target`. 110/110 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced May 7, 2026
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.
Why
Follow-up to #33. The
targetaction input flowed verbatim into two unsafe surfaces:apm.ymlscalar (isolated mode) —target: ${value}\napm pack --target <value>CLI callA value containing
\n,\r,#,:, quotes, or stray whitespace could:copilot\nname: hijacked),No template engine, no escaping, no allowlist — raw interpolation. Worth fixing even though the input is set by trusted workflow authors, because action inputs in
workflow_call/ reusable workflow chains can come from less-trusted callers, and defence-in-depth on the YAML boundary is cheap.What
Add a
parseTargetInput(raw)helper applied at both call sites:undefinedfor empty input (preserves current behaviour),,to support APM's CSV multi-target form,^[a-z][a-z0-9-]{0,31}$— the shape of every shipped APM harness name (agent-skills,claude,codex,copilot,cursor,gemini,opencode,windsurf) and any plausible future addition,Why a regex allowlist over a hardcoded list of names: APM ships new harnesses periodically. The pattern is tight enough to block every injection vector (no
:, no\n/\r, no#, no whitespace, no quotes, no..) while not pinning the action to a specific APM version's harness inventory.Tests
10 new unit tests in
runner.test.ts:"copilot\ninjected: true""copilot\rfoo""copilot # nope""copilot: extra"'"copilot"'"-copilot""copilot,,claude""Copilot""copilot, "" copilot , claude "→ normalises to"copilot,claude"All assert
setFailedis called withInvalid 'target' inputand that no apm.yml is written for rejected inputs. The valid CSV case asserts the normalised value lands in the manifest.109/109 unit tests pass locally.
Compat
copilot) or CSV (copilot,claude) keep working.