Skip to content

fix(core): handle heredoc in command substitution guard - #1701

Merged
tanzhenxin merged 3 commits into
QwenLM:mainfrom
liqiongyu:fix/1698-heredoc-command-substitution
Feb 4, 2026
Merged

fix(core): handle heredoc in command substitution guard#1701
tanzhenxin merged 3 commits into
QwenLM:mainfrom
liqiongyu:fix/1698-heredoc-command-substitution

Conversation

@liqiongyu

Copy link
Copy Markdown
Contributor

Fixes #1698

What

  • Teach detectCommandSubstitution() to understand heredocs.
  • Allow substitution-like text in heredoc bodies when the delimiter is quoted (e.g. <<'EOF').
  • Block $() / backticks in heredoc bodies when the delimiter is unquoted (because bash would execute them).

Why

Users saving sessions/logs to Markdown often include ``` and $()/backticks as plain text. The previous detector could false-positive on these, blocking safe file writes.

Test

  • npm test -w packages/core -- src/utils/shell-utils.test.ts

@tanzhenxin

Copy link
Copy Markdown
Collaborator

@liqiongyu Thank you for this valuable PR! The heredoc support is a great addition to the shell command parser. We've reviewed the changes and identified a couple of issues that need to be addressed before we can merge:


Issue 1: Security vulnerability - Heredoc syntax in comments suppresses substitution detection [P1]

Problem:
The heredoc detection logic processes << operators whenever they appear outside quotes, without checking if the line is a comment. A commented heredoc like # <<'EOF' creates a pending quoted heredoc state. This causes subsequent lines to be treated as heredoc content (with substitution checks disabled), allowing malicious $(...) to bypass detection.

Attack Example:

# Fake heredoc <<'EOF' (this is just a comment)
$(rm -rf /)  # This executes but isn't flagged!
EOF

Issue 2: False positives for command substitution inside comments [P2 - Enhancement]

Problem:
The function reports command substitution patterns (e.g., $(date), `cmd`) even when they appear inside shell comments. Since bash does not execute substitutions in comments, these are false positives that incorrectly flag safe commands. This is a code quality improvement rather than a security fix.

Example:

# Note: $(rm -rf /) is dangerous
echo hello

This should be allowed, but currently may trigger detection.


Suggested Fix

Both issues can be addressed by adding inComment state tracking:

  • Skip heredoc operator parsing inside comments (fixes Issue 1 - Security)
  • Skip command substitution detection inside comments (fixes Issue 2 - False positives)

@tanzhenxin tanzhenxin self-assigned this Feb 4, 2026
@liqiongyu

liqiongyu commented Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

@tanzhenxin
Thanks for the detailed review!

I addressed both items you pointed out:

  1. Comments handling (P1 + P2):
  • Added an inComment state so heredoc operator parsing and substitution detection are skipped inside shell comments.
  • This prevents a commented fake heredoc like # <<'EOF' from creating a pending heredoc state, and avoids false positives for $(...) / backticks inside comments.
  • Added regression tests for these cases.
  1. Extra hardening:
  • In unquoted heredoc bodies, bash ignores \\<newline> (line continuation), so $\\\\\n( becomes $( and can bypass a line-based scan.
  • Added detection for this cross-line pattern (and tests), while still allowing the escaped form (\\$\\\\\n().

CI is green on all platforms now. Could you please take another look when you have time? Thanks!

@tanzhenxin tanzhenxin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tanzhenxin
tanzhenxin merged commit 4235681 into QwenLM:main Feb 4, 2026
14 checks passed
xaelistic pushed a commit to xaelistic/qwen-code that referenced this pull request Jun 7, 2026
…d-substitution

fix(core): handle heredoc in command substitution guard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Since 0.9.0 Qwen companion can't create/save text file

2 participants