shell: reject multi-line LLM output so bash -c can't execute hidden commands#365
Merged
Merged
Conversation
…ommands SanitizeCommand explicitly preserved '\n', and the sanitized string was passed straight to `bash -c`. Because `bash -c` treats newlines as command separators, a prompt-injected multi-line LLM response like `ls\nrm -rf ~` renders as two apparently-harmless lines in the confirmation prompt but executes both commands once the user types Y (tbckr#360). Change SanitizeCommand to return (string, error) and fail with ErrMultilineCommand whenever the cleaned payload contains '\n' or '\r'. Tabs are still preserved (they're legitimately used in printf and friends). ExecuteCommandWithConfirmation now aborts before showing the confirmation prompt when the LLM response is multi-line, so the hidden trailing commands never reach bash. Test coverage: new TestSanitizeCommand_RejectsMultilineInjection exercises the \n, \r, \r\n and `ls\nrm -rf ~` paths and asserts ErrMultilineCommand. Existing tests are updated for the new signature. The replaced TestSanitizeCommand_PreservesNewlineAndTab becomes TestSanitizeCommand_PreservesTab (tab-only). Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com> Fixes tbckr#360
tbckr
approved these changes
Apr 23, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #365 +/- ##
==========================================
+ Coverage 75.80% 75.92% +0.11%
==========================================
Files 20 21 +1
Lines 1298 1429 +131
==========================================
+ Hits 984 1085 +101
- Misses 226 256 +30
Partials 88 88 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tbckr
pushed a commit
that referenced
this pull request
Apr 26, 2026
…dden commands (#365) SanitizeCommand explicitly preserved '\n', and the sanitized string was passed straight to `bash -c`. Because `bash -c` treats newlines as command separators, a prompt-injected multi-line LLM response like `ls\nrm -rf ~` renders as two apparently-harmless lines in the confirmation prompt but executes both commands once the user types Y (#360). Change SanitizeCommand to return (string, error) and fail with ErrMultilineCommand whenever the cleaned payload contains '\n' or '\r'. Tabs are still preserved (they're legitimately used in printf and friends). ExecuteCommandWithConfirmation now aborts before showing the confirmation prompt when the LLM response is multi-line, so the hidden trailing commands never reach bash. Test coverage: new TestSanitizeCommand_RejectsMultilineInjection exercises the \n, \r, \r\n and `ls\nrm -rf ~` paths and asserts ErrMultilineCommand. Existing tests are updated for the new signature. The replaced TestSanitizeCommand_PreservesNewlineAndTab becomes TestSanitizeCommand_PreservesTab (tab-only). Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com> Fixes #360 Co-authored-by: SAY-5 <SAY-5@users.noreply.github.com>
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
2 participants
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.
Summary
Fixes #360.
SanitizeCommandexplicitly preserved\n, and the sanitized string was passed straight tobash -c. Becausebash -ctreats newlines as command separators, a prompt-injected multi-line LLM response likels\nrm -rf ~rendered as two apparently-harmless lines in the confirmation prompt but executed both commands once the user typedY.The sanitizer's own comment scoped it to "display manipulation attacks". The fix needs to go a step further: for anything that is handed to
bash -c, multi-line payloads have to be refused.Fix
SanitizeCommand(string)toSanitizeCommand(string) (string, error).ErrMultilineCommandwhenever the cleaned payload contains\nor\r.ExecuteCommandWithConfirmationnow aborts (and surfaces the error onoutput) before the confirmation prompt, so the user never sees just the harmless-looking first line.printf/awkinvocations.Pipes / redirects /
&&/||are intentionally still allowed - legitimate use cases likedu -a | sort -rh | head -3must continue to work - and are documented risks the user opts in to by choosing the shell-execution feature. Newlines are the specific class the confirmation prompt can't render honestly, which is why this layer is tightened.Test plan
go build ./...go test ./pkg/shell/... -count=1(all existing tests updated for the new signature; newTestSanitizeCommand_RejectsMultilineInjectionexercises\n,\r,\r\n, and thels\nrm -rf ~PoC)TestSanitizeCommand_PreservesTabreplaces the oldPreservesNewlineAndTabto lock in the tab-preservation contract