fix(core): harden plain text pager spawning#358
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Greptile SummaryThis PR hardens plain-text pager spawning by replacing
Confidence Score: 4/5Safe to merge — the shell-injection hardening is correct and well-tested; the only issues are a redundant second parse and an untested async error path The core security change (replacing shell: true with argv-parsed direct spawn) is sound and backed by focused tests. The double-parse in pagePlainText — where resolvePagerSpec is called on the already-validated output of resolveTextPagerCommand — is wasteful and leaves a dead fallback. The new async spawnError capture logic also has no test exercising the error-event path, so a regression there would be invisible. Neither issue affects the primary security or correctness goal of this PR. src/core/pager.ts lines 147–153 (double-parse and unreachable guard) and lines 169–178 (untested async error path) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["pagePlainText(text, env)"] --> B{isTTY?}
B -- No --> C["stdout.write(text)"]
B -- Yes --> D["resolveTextPagerCommand(env)"]
D --> E["resolvePagerSpec(candidate)"]
E --> F{pagerSpec null\nor command == 'hunk'?}
F -- Yes --> G["return 'less -R'"]
F -- No --> H["return displayCommand\n(original string)"]
G --> I["resolvePagerSpec(pagerCommand)\n⚠️ second parse of same input"]
H --> I
I --> J["spawnImpl(command, args,\n{ shell: false, env: {...env, ...pagerSpec.env} })"]
J --> K{spawn throws?}
K -- Yes --> L["throw Error(cause: error)"]
K -- No --> M["pager.once('error', capture)"]
M --> N["pager.stdin.end(text)"]
N --> O["await once(pager, 'close')"]
O --> P{spawnError or\ncode != 0?}
P -- Yes --> Q["throw Error(cause: spawnError)"]
P -- No --> R["return (done)"]
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/core/pager.ts:147-148
Double-parse and unreachable fallback: `resolveTextPagerCommand` already calls `resolvePagerSpec` internally and returns `pagerSpec.displayCommand` — the original command string. Line 148 then calls `resolvePagerSpec` again on that identical string, discarding the result of the first parse. Because `resolveTextPagerCommand` only ever returns either the original (valid, parseable) candidate or the literal `"less -R"`, `resolvePagerSpec(pagerCommand)` can never return `null` here, making the `?? resolvePagerSpec("less -R")` branch permanently dead code.
```suggestion
const pagerCommand = resolveTextPagerCommand(env);
// resolvePagerSpec is guaranteed to succeed here: resolveTextPagerCommand only
// returns the original (already-validated) candidate string or the literal
// "less -R", both of which parse without error.
const pagerSpec = resolvePagerSpec(pagerCommand);
```
### Issue 2 of 3
src/core/pager.ts:147-153
**Residual null-guard on `pagerSpec` is now unreachable**
After the `??` fallback is removed (see sibling comment), the `if (!pagerSpec)` guard on line 150 is also unreachable — a consequence of the same double-parse design. Keeping it silently swallows the case where `resolvePagerSpec` could theoretically return `null` (e.g., if `resolveTextPagerCommand` is refactored to return a non-parseable string), which would make debugging harder. Consider asserting or throwing instead so failures are loud rather than silently falling back to stdout.
### Issue 3 of 3
src/core/pager.ts:169-178
**Async spawn error path is untested**
The `spawnError` capture (lines 169–172) is new in this PR, but no test exercises it. The existing "throws when the pager exits with a non-zero status" test only exercises the `code !== 0` branch; a case where `spawnImpl` returns a process that immediately emits an `error` event (e.g., simulating `ENOENT` or `EACCES` from `spawn`) is not covered. If the error event fires but `close` fires with `code: null`, `spawnError` would be truthy and the thrown `Error` would carry the right `cause` — but this path has no test to catch a regression.
Reviews (1): Last reviewed commit: "fix(core): harden plain text pager spawn..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const pagerCommand = resolveTextPagerCommand(env); | ||
| const pager = deps.spawnImpl(pagerCommand, { | ||
| shell: true, | ||
| stdio: ["pipe", "inherit", "inherit"], | ||
| env, | ||
| const pagerSpec = resolvePagerSpec(pagerCommand) ?? resolvePagerSpec("less -R"); |
There was a problem hiding this comment.
resolveTextPagerCommand already calls resolvePagerSpec internally and returns pagerSpec.displayCommand — the original command string. Line 148 then calls resolvePagerSpec again on that identical string, discarding the result of the first parse. Because resolveTextPagerCommand only ever returns either the original (valid, parseable) candidate or the literal "less -R", resolvePagerSpec(pagerCommand) can never return null here, making the ?? resolvePagerSpec("less -R") branch permanently dead code.
| const pagerCommand = resolveTextPagerCommand(env); | |
| const pager = deps.spawnImpl(pagerCommand, { | |
| shell: true, | |
| stdio: ["pipe", "inherit", "inherit"], | |
| env, | |
| const pagerSpec = resolvePagerSpec(pagerCommand) ?? resolvePagerSpec("less -R"); | |
| const pagerCommand = resolveTextPagerCommand(env); | |
| // resolvePagerSpec is guaranteed to succeed here: resolveTextPagerCommand only | |
| // returns the original (already-validated) candidate string or the literal | |
| // "less -R", both of which parse without error. | |
| const pagerSpec = resolvePagerSpec(pagerCommand); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/pager.ts
Line: 147-148
Comment:
Double-parse and unreachable fallback: `resolveTextPagerCommand` already calls `resolvePagerSpec` internally and returns `pagerSpec.displayCommand` — the original command string. Line 148 then calls `resolvePagerSpec` again on that identical string, discarding the result of the first parse. Because `resolveTextPagerCommand` only ever returns either the original (valid, parseable) candidate or the literal `"less -R"`, `resolvePagerSpec(pagerCommand)` can never return `null` here, making the `?? resolvePagerSpec("less -R")` branch permanently dead code.
```suggestion
const pagerCommand = resolveTextPagerCommand(env);
// resolvePagerSpec is guaranteed to succeed here: resolveTextPagerCommand only
// returns the original (already-validated) candidate string or the literal
// "less -R", both of which parse without error.
const pagerSpec = resolvePagerSpec(pagerCommand);
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Refactored pagePlainText to call a new resolveTextPagerSpec() helper directly, so we keep the parsed pager spec and no longer parse the validated command a second time.
Responded by Pi using OpenAI GPT-5.
| const pagerCommand = resolveTextPagerCommand(env); | ||
| const pager = deps.spawnImpl(pagerCommand, { | ||
| shell: true, | ||
| stdio: ["pipe", "inherit", "inherit"], | ||
| env, | ||
| const pagerSpec = resolvePagerSpec(pagerCommand) ?? resolvePagerSpec("less -R"); | ||
|
|
||
| if (!pagerSpec) { | ||
| deps.stdout.write(text); | ||
| return; | ||
| } |
There was a problem hiding this comment.
pagerSpec is now unreachable
After the ?? fallback is removed (see sibling comment), the if (!pagerSpec) guard on line 150 is also unreachable — a consequence of the same double-parse design. Keeping it silently swallows the case where resolvePagerSpec could theoretically return null (e.g., if resolveTextPagerCommand is refactored to return a non-parseable string), which would make debugging harder. Consider asserting or throwing instead so failures are loud rather than silently falling back to stdout.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/pager.ts
Line: 147-153
Comment:
**Residual null-guard on `pagerSpec` is now unreachable**
After the `??` fallback is removed (see sibling comment), the `if (!pagerSpec)` guard on line 150 is also unreachable — a consequence of the same double-parse design. Keeping it silently swallows the case where `resolvePagerSpec` could theoretically return `null` (e.g., if `resolveTextPagerCommand` is refactored to return a non-parseable string), which would make debugging harder. Consider asserting or throwing instead so failures are loud rather than silently falling back to stdout.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Removed the unreachable null fallback/guard from pagePlainText. The fallback is now handled inside resolveTextPagerSpec(), and an invalid default pager throws loudly.
Responded by Pi using OpenAI GPT-5.
| let spawnError: unknown; | ||
| pager.once("error", (error) => { | ||
| spawnError = error; | ||
| }); | ||
|
|
||
| pager.stdin?.end(text); | ||
| const [code] = await once(pager, "close"); | ||
|
|
||
| if (typeof code === "number" && code !== 0) { | ||
| throw new Error(`Pager command failed: ${pagerCommand}`); | ||
| if (spawnError || (typeof code === "number" && code !== 0)) { | ||
| throw new Error(`Pager command failed: ${pagerCommand}`, { cause: spawnError }); |
There was a problem hiding this comment.
The spawnError capture (lines 169–172) is new in this PR, but no test exercises it. The existing "throws when the pager exits with a non-zero status" test only exercises the code !== 0 branch; a case where spawnImpl returns a process that immediately emits an error event (e.g., simulating ENOENT or EACCES from spawn) is not covered. If the error event fires but close fires with code: null, spawnError would be truthy and the thrown Error would carry the right cause — but this path has no test to catch a regression.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/pager.ts
Line: 169-178
Comment:
**Async spawn error path is untested**
The `spawnError` capture (lines 169–172) is new in this PR, but no test exercises it. The existing "throws when the pager exits with a non-zero status" test only exercises the `code !== 0` branch; a case where `spawnImpl` returns a process that immediately emits an `error` event (e.g., simulating `ENOENT` or `EACCES` from `spawn`) is not covered. If the error event fires but `close` fires with `code: null`, `spawnError` would be truthy and the thrown `Error` would carry the right `cause` — but this path has no test to catch a regression.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Added a regression test for an async spawn error event followed by close, and adjusted the close wait path so the wrapped pager failure preserves the original error as cause.
Responded by Pi using OpenAI GPT-5.
1 participant
Summary
PAGER/HUNK_TEXT_PAGERinto argv and spawning without an implicit shell.NAME=valueprefixes, andenv NAME=value pager ...wrappers.hunkfallback, quoted Windows paths, and inline env assignments.Refs #355.
Verification
bun run typecheckbun testbun run format:checkbun run lint; touch markerno longer executes; explicitsh -c ...still works as intentional pager config.This PR description was generated by Pi using OpenAI GPT-5