Security: Unsafe shell execution with user-controlled pager command

[tomaioo]

Summary

Security: Unsafe shell execution with user-controlled pager command

Problem

Severity: High | File: src/core/pager.ts:L45

The pagePlainText function in src/core/pager.ts spawns a pager command with shell: true using the resolveTextPagerCommand result. While the function attempts to prevent recursive hunk launches via regex, the PAGER or HUNK_TEXT_PAGER environment variable is passed directly to the shell without proper sanitization. An attacker who can control these environment variables could inject arbitrary shell commands (e.g., PAGER='$(malicious)' or backtick injection). The regex check /(^|\s)hunk(\s|$)/ is insufficient to prevent command injection.

Solution

Avoid shell: true and instead parse the pager command into arguments using a safe argument parser, or use execFile with an explicit array of arguments. If shell is required, strictly validate the pager command against an allowlist of known safe pagers, or use shell: false with spawn and pass the command as an first argument with any flags as subsequent array elements.

Changes

• src/core/pager.ts (modified)

[greptile-apps]

Greptile Summary

This PR eliminates a shell-injection risk in pagePlainText by replacing spawn(pagerCommand, { shell: true }) with a new parsePagerCommand helper that splits the pager command string into an executable and an arguments array, then calls spawn(executable, args, { shell: false }). The change is effective — shell metacharacters in PAGER/HUNK_TEXT_PAGER can no longer trigger unintended command execution.

• parsePagerCommand tokenises on whitespace and handles single/double-quoted segments, but does not support backslash escape sequences; arguments like less --prompt=my\\ pager would be passed with a literal backslash rather than the intended space.
• pager.test.ts was not updated to match the new three-argument spawnImpl signature, leaving one test with stale assertions (command === \"less -R\" and options.shell === true) that will fail against the new implementation.

Confidence Score: 3/5

Safe to merge once the broken test is fixed; the core security change is correct but the test suite as-is will not pass.

The implementation change itself is sound — removing shell: true closes the injection path entirely. However, the test file was not updated alongside the new three-argument spawnImpl interface, so the "non-zero exit" test will fail on both of its assertions (command and options.shell). Merging with a known-failing test obscures CI signal and should be resolved before the PR lands.

src/core/pager.test.ts — the spawnImpl override in the "throws when the pager exits with a non-zero status" test needs to be updated to match the new three-argument signature.

Important Files Changed

Filename	Overview
src/core/pager.ts	Shell injection fixed by replacing shell: true + raw string with shell: false + parsePagerCommand; the new parser correctly handles quoted tokens but silently ignores backslash escape sequences.
src/core/pager.test.ts	Test at line 150 not updated for the new 3-argument spawnImpl signature; both expect(command).toBe("less -R") and expect(options.shell).toBe(true) will fail at runtime with the new implementation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["pagePlainText(text, env, deps)"] --> B{stdout.isTTY?}
    B -- No --> C["deps.stdout.write(text)"]
    B -- Yes --> D["resolveTextPagerCommand(env)"]
    D --> E{HUNK_TEXT_PAGER or PAGER set?}
    E -- No --> F["return 'less -R'"]
    E -- Yes --> G{matches /hunk/ regex?}
    G -- Yes --> F
    G -- No --> H["return candidate string"]
    F --> I["parsePagerCommand(command)"]
    H --> I
    I --> J["tokenise: split on whitespace,\nrespect quoted segments"]
    J --> K["[executable, ...args]"]
    K --> L["spawn(executable, args,\n{ shell: false })"]
    L --> M["pager.stdin.end(text)"]
    M --> N["await 'close' event"]
    N --> O{exit code === 0?}
    O -- Yes --> P["done"]
    O -- No --> Q["throw Error('Pager command failed')"]

Loading

_{Reviews (1): Last reviewed commit: "fix(core): unsafe shell execution with u..." | Re-trigger Greptile}

[benvinegar]

More exhaustive solution with tests in #358