Skip to content

fix: generated pre-commit diagnostic executes shell substitutions in project paths #782

Description

@0xGaspar

Problem

The generated pre-commit hook can execute shell command substitutions embedded in the project directory name when specsync check fails. The issue was reported during PR #774 review and remains in current main after that PR merged.

Confirmed source: src/hooks.rs, main inspected at ba0df69360dadd20062bcb6d2ed9557ea8964ed5:

  • PRE_COMMIT_HOOK failure diagnostic around line 173: echo " Run 'specsync check' to see details."
  • pre_commit_block, around lines 267–283, globally replaces specsync check with a command containing shell_single_quote(&project_root).

That substitution changes both the executable command and the diagnostic. Single quotes protect the directory in the command, but have no quoting effect once inserted inside the diagnostic's existing double quotes. $() and backticks in the directory name are therefore evaluated by the shell on the failure path.

Reproduction

I reproduced this at the template/substitution level using the exact PR template, the production replacement, a fake specsync executable that exits 1, and a harmless marker-file command in the root string. The hook exited 1 and created the marker. No credentials or network access were involved.

The resulting vulnerable diagnostic has this shape:

echo "  Run 'specsync --root '/tmp/project-$(touch SPECSYNC_HOOK_MARKER)' check' to see details."

Run that line only inside a disposable temporary directory: it creates SPECSYNC_HOOK_MARKER instead of merely printing the project path. A directory name is data and must never become executable shell text.

Impact and conditions

This requires a project-root path containing shell metacharacters and a failing check. It is not execution from ordinary spec contents or every checkout. Under those conditions, directory-name content executes with the hook user's permissions. The executable check itself being single-quoted does not protect the later diagnostic.

Suggested fix

Generate the executable command and diagnostic independently, or restrict substitution to the exact executable template line. Keep paths out of the diagnostic, or render them as safely quoted arguments to a fixed-format printf.

Add an executable generated-hook regression covering:

  • $() and backticks in an actual project directory name;
  • a deliberately failing check;
  • no marker-command execution;
  • ordinary paths and paths containing spaces/single quotes;
  • correct project selection and propagation of the failed check status.

Validation must exercise the installed/generated hook, not only assert that its text contains quote characters.

Original scoped reproduction/review: #774 (review)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:product-bugRuntime or CLI behavior is incorrectarea:securityIntegrity, authentication, supply-chain, or secret-handling riskarea:uxMessages, output, or next actions mislead or frustrateimpact:user-facingCan affect an end user or repository operator directlypriority:p1Highest-impact or release-blocking riskscope:specsync-6Applies to current SpecSync 6 behavior or release contract

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions