From ef76f085a3821e544aa7aa1bee4e1472af88cc5a Mon Sep 17 00:00:00 2001 From: sxh Date: Mon, 5 Oct 2026 12:25:41 +0800 Subject: [PATCH] fix(skills): stop reporting an env path, shebang or flag as an env dump (#6297) * fix(skills): stop reporting an env path, shebang or flag as an env dump * fix(skills): report shell env dumps only at a command position `shell-env-dump` matched the word `env` anywhere it was not preceded by a path, dot, dash or word character, so ordinary argument text (`echo env`) and comment text (`# env`, `# export -p`) were still reported as environment dumps. Anchor the match to a shell command position -- the start of a line or right after a `;`, `&`, `|`, `(`, `)` or `{` separator, optionally behind `NAME=value` assignment prefixes -- and add regression cases for argument and comment text. * fix(skills): ignore an env dump inside a comment or heredoc body `shell-env-dump` matches `env`, `printenv` and `export -p` at a command position, but the text it sees is still raw shell. A `;` inside a comment (`# documentation; env is only an example`) and a bare `env` line inside heredoc data both sit at a command position on paper while the shell runs neither, so both were reported as environment dumps. Reduce the text to shell code before matching: a quote-aware pass strips comments (only a `#` at a word start opens one) and blanks heredoc bodies, covering `<<-` tab stripping and quoted delimiters, while keeping one output line per input line so reported line numbers stay correct. Real commands after a separator (`true; env`) and on the line that opens a heredoc (`env < --- .../deerflow/skills/skillscan/orchestrator.py | 91 +++++++- .../tests/test_skillscan_shell_env_dump.py | 208 ++++++++++++++++++ 2 files changed, 297 insertions(+), 2 deletions(-) create mode 100644 backend/tests/test_skillscan_shell_env_dump.py diff --git a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py index 4af38c254f..8c75c90dc9 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -145,6 +145,25 @@ _DESTRUCTIVE_RM_RE = ( r"(?:-\S+\s+|--no-preserve-root\s+)*" r"/(?:\*|\s|$|(?:bin|boot|dev|etc|home|lib|lib64|opt|proc|root|run|sbin|srv|sys|usr|var)(?:/\*?)?(?:\s|$))" ) +# `env`, `printenv` and `export -p` dump the environment only when they run as a +# command: at the start of a line, right after a `;`, `&`, `|`, `(`, `)` or +# backtick separator, after the `{` that opens a brace group (`{ env; }`, which +# bash requires to be followed by whitespace), or after a reserved word that must +# introduce a command (`then`, `do`, `exec`, ...). An `NAME=value` assignment +# prefix may come first. Anywhere else the word names something else -- a path in +# `#!/usr/bin/env bash`, a host in `https://env.example.com`, a flag in +# `--env FOO=1`, an argument in `echo env`, a variable in `${env}`, or comment +# text in `# export -p` -- and dumps nothing. The text this is matched against is +# first reduced to shell code by `_shell_code_only`, so a `;` inside a comment and +# a command-looking line inside a heredoc body do not count either. +_SHELL_ENV_DUMP_RE = re.compile( + r"(?m)(?:^|(?<=[;&|()`])|\{(?=[ \t])|(?env\b|printenv\b|export[ \t]+-p\b)" +) +# The head of a heredoc redirection: `<<` or `<<-`, an optional quoted delimiter, +# then the delimiter word. Requiring a leading letter/underscore keeps arithmetic +# shifts such as `$((1 << 2))` from being read as a heredoc opener. +_HEREDOC_HEAD_RE = re.compile(r"<<(-?)[ \t]*([\"']?)([A-Za-z_]\w*)\2") def skill_scan_enabled(app_config: Any | None = None) -> bool: @@ -692,6 +711,72 @@ def _scan_python(rel_path: str, text: str) -> list[SecurityFinding]: return findings +def _split_shell_line(line: str) -> tuple[str, list[tuple[str, bool]]]: + """Split one shell line into its code part and the heredocs it declares. + + Quote-aware: a `#` outside quotes starts a comment only at a word start, and + `<<` outside quotes opens a heredoc. Returns the code text (comment stripped) + together with the `(delimiter, strip_tabs)` pairs the line opens. + + A `{` or `}` immediately before the `#` is not a word start: bash reads + `${#HOME}` as the length operator and `}#` as part of a word, while a brace + group needs the space of `{ # ...`. Those two characters are therefore left + out of the set that admits a comment. + """ + heredocs: list[tuple[str, bool]] = [] + in_single = in_double = False + i = 0 + while i < len(line): + ch = line[i] + if in_single: + in_single = ch != "'" + elif in_double: + if ch == "\\": + i += 2 + continue + in_double = ch != '"' + elif ch == "\\": + i += 2 + continue + elif ch == "'": + in_single = True + elif ch == '"': + in_double = True + elif ch == "#" and (i == 0 or line[i - 1] in " \t;&|()<>"): + return line[:i], heredocs + elif ch == "<" and line.startswith("<<", i) and not line.startswith("<<<", i) and (i == 0 or not (line[i - 1].isalnum() or line[i - 1] == "_")): + if head := _HEREDOC_HEAD_RE.match(line, i): + heredocs.append((head.group(3), head.group(1) == "-")) + i = head.end() + continue + i += 1 + return line, heredocs + + +def _shell_code_only(text: str) -> str: + """Blank out comment text and heredoc bodies, preserving line structure. + + `_SHELL_ENV_DUMP_RE` matches at a command position, but a `;` inside a + comment (`# documentation; env is only an example`) and a bare `env` line + inside heredoc data are not commands. Replacing them with blanks -- one + output line per input line, so `_line_number` stays correct -- leaves the + matcher looking only at shell code. + """ + lines: list[str] = [] + pending: list[tuple[str, bool]] = [] + for raw in text.split("\n"): + if pending: + delimiter, strip_tabs = pending[0] + if (raw.lstrip("\t") if strip_tabs else raw) == delimiter: + pending.pop(0) + lines.append("") + continue + code, heredocs = _split_shell_line(raw) + pending.extend(heredocs) + lines.append(code) + return "\n".join(lines) + + def _scan_shell(rel_path: str, text: str) -> list[SecurityFinding]: findings: list[SecurityFinding] = [] # Unmistakable reverse-shell signals hard-block; weaker idioms (bash -i, @@ -706,8 +791,10 @@ def _scan_shell(rel_path: str, text: str) -> list[SecurityFinding]: findings.append(_finding_from_match("shell-curl-pipe-shell", rel_path, text, match)) if match := re.search(_DESTRUCTIVE_RM_RE + r"|:\(\)\{\s*:\|:&\s*\};:|dd\s+[^#\n]*\bof=/dev/", text): findings.append(_finding_from_match("shell-destructive-command", rel_path, text, match)) - if match := re.search(r"\b(env|printenv|export\s+-p)\b", text): - findings.append(_finding_from_match("shell-env-dump", rel_path, text, match)) + # Only a command position counts, and only in shell code: see `_shell_code_only`. + code = _shell_code_only(text) + if match := _SHELL_ENV_DUMP_RE.search(code): + findings.append(_finding("shell-env-dump", file=rel_path, line=_line_number(code, match.start("cmd")), evidence=match.group("cmd"))) return findings diff --git a/backend/tests/test_skillscan_shell_env_dump.py b/backend/tests/test_skillscan_shell_env_dump.py new file mode 100644 index 0000000000..c6fcb84f36 --- /dev/null +++ b/backend/tests/test_skillscan_shell_env_dump.py @@ -0,0 +1,208 @@ +"""`shell-env-dump` must report an environment dump, not a path or a shebang. + +The rule's own message is "Shell script dumps the environment", so only a command +that actually dumps it counts. The word `env` also appears as a path component in +the portable shebang (`#!/usr/bin/env bash`, used by this repository's own +`skills/public/claude-to-deerflow/scripts/*.sh`), in host labels +(`https://env.example.com`), in dotted file names and in option names +(`--env`), as a plain command argument (`echo env`), as a variable (`${env}`) and +inside comments (`# export -p`); none of them dumps anything, so the command word +must sit at the start of a command: a line start, a position after a `;`, `&`, +`|`, `(`, `)` or backtick separator, the `{` of a brace group, or the position +after a reserved word that introduces a command (`then`, `do`, `exec`, ...). +Comment text after a separator and heredoc bodies are data rather than commands +too, so they are stripped before that command-position match. +""" + +from __future__ import annotations + +from pathlib import Path + +from deerflow.skills.skillscan.orchestrator import scan_skill_dir + + +def _write_script(skill_dir: Path, body: str) -> None: + scripts = skill_dir / "scripts" + scripts.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text("---\nname: demo\ndescription: A demo skill\n---\n\nBody.\n", encoding="utf-8") + (scripts / "run.sh").write_text(body, encoding="utf-8") + + +def _rules(skill_dir: Path) -> set[str]: + return {finding["rule_id"] for finding in scan_skill_dir(skill_dir)["findings"]} + + +def test_env_shebang_is_not_an_environment_dump(tmp_path: Path) -> None: + """The portable shebang names a path, so it is not an `env` invocation.""" + _write_script(tmp_path, "#!/usr/bin/env bash\necho hello\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_env_in_a_url_host_is_not_an_environment_dump(tmp_path: Path) -> None: + """A host label that happens to read `env` is not an environment dump.""" + _write_script(tmp_path, "#!/bin/bash\ncurl https://env.example.com/x\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_env_file_name_is_not_an_environment_dump(tmp_path: Path) -> None: + """`source .env` loads one file; it does not dump the environment.""" + _write_script(tmp_path, "#!/bin/bash\nset -a\nsource .env\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_env_option_name_is_not_an_environment_dump(tmp_path: Path) -> None: + """`--env MODE=fast` sets one variable for one command; it is not a dump.""" + _write_script(tmp_path, "#!/bin/bash\ndocker run --env MODE=fast image\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_bare_env_command_still_reports(tmp_path: Path) -> None: + """A bare `env` is exactly the dump the rule exists for.""" + _write_script(tmp_path, "#!/bin/bash\nenv\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_printenv_command_still_reports(tmp_path: Path) -> None: + _write_script(tmp_path, "#!/bin/bash\nprintenv | sort\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_export_p_still_reports(tmp_path: Path) -> None: + _write_script(tmp_path, "#!/bin/bash\nexport -p\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_as_a_command_argument_is_not_an_environment_dump(tmp_path: Path) -> None: + """`echo env` prints the word, not the environment: `env` is an argument here.""" + _write_script(tmp_path, "#!/bin/bash\necho env\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_env_in_a_comment_is_not_an_environment_dump(tmp_path: Path) -> None: + """Comment text is not a command, so `# env` dumps nothing.""" + _write_script(tmp_path, "#!/bin/bash\n# env\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_export_p_in_a_comment_is_not_an_environment_dump(tmp_path: Path) -> None: + """A commented `export -p` is documentation, not an invocation.""" + _write_script(tmp_path, "#!/bin/bash\n# export -p prints the environment\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_env_after_a_command_separator_still_reports(tmp_path: Path) -> None: + """`true; env` runs `env` as a second command on the line.""" + _write_script(tmp_path, "#!/bin/bash\ntrue; env\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_after_a_pipe_still_reports(tmp_path: Path) -> None: + _write_script(tmp_path, "#!/bin/bash\ncat file | printenv\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_indented_env_still_reports(tmp_path: Path) -> None: + """Indentation is not a command argument, so a line-leading `env` still dumps.""" + _write_script(tmp_path, "#!/bin/bash\n env\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_after_an_assignment_prefix_still_reports(tmp_path: Path) -> None: + """`FOO=1 env` runs `env`; the assignment is a prefix, not an argument.""" + _write_script(tmp_path, "#!/bin/bash\nFOO=1 env\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_after_a_separator_inside_a_comment_is_not_an_environment_dump(tmp_path: Path) -> None: + """A `;` inside a comment does not turn its text into a second command.""" + _write_script(tmp_path, "#!/bin/bash\n# documentation; env is only an example\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_export_p_after_a_separator_inside_a_comment_is_not_an_environment_dump(tmp_path: Path) -> None: + """Comment text after a separator is documentation, not an invocation.""" + _write_script(tmp_path, "#!/bin/bash\necho hi # ; export -p\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_env_in_a_quoted_heredoc_body_is_not_an_environment_dump(tmp_path: Path) -> None: + """A line inside heredoc data is input, not a command.""" + _write_script(tmp_path, "#!/bin/bash\ncat <<'EOF'\nenv\nEOF\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_printenv_in_a_tab_stripped_heredoc_body_is_not_an_environment_dump(tmp_path: Path) -> None: + """`<<-` strips leading tabs from the body and its terminator alike.""" + _write_script(tmp_path, "#!/bin/bash\ncat <<-EOF\n\tprintenv\n\tEOF\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_env_on_the_line_that_opens_a_heredoc_still_reports(tmp_path: Path) -> None: + """Only the heredoc body is data; the command before it is still code.""" + _write_script(tmp_path, "#!/bin/bash\nenv < None: + """`echo hi; export -p` runs `export -p` as a second command on the line.""" + _write_script(tmp_path, "#!/bin/bash\necho hi; export -p\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_after_then_still_reports(tmp_path: Path) -> None: + """`then` introduces a command list, so an `env` there is a real invocation.""" + _write_script(tmp_path, "#!/bin/bash\nif true; then env; fi\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_printenv_after_do_still_reports(tmp_path: Path) -> None: + """A loop body is a command list too, so `do printenv` dumps the environment.""" + _write_script(tmp_path, "#!/bin/bash\nwhile true; do printenv; done\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_as_a_condition_still_reports(tmp_path: Path) -> None: + """`if env; then ...` runs `env` as the condition command.""" + _write_script(tmp_path, "#!/bin/bash\nif env; then :; fi\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_after_exec_still_reports(tmp_path: Path) -> None: + _write_script(tmp_path, "#!/bin/bash\nexec env\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_in_a_backtick_substitution_still_reports(tmp_path: Path) -> None: + """A backtick opens a command substitution, just as `$(` does.""" + _write_script(tmp_path, "#!/bin/bash\nV=`env`\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_in_a_brace_group_still_reports(tmp_path: Path) -> None: + """A brace group is written `{ env; }`, so the `{` is followed by whitespace.""" + _write_script(tmp_path, "#!/bin/bash\n{ env; }\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_env_in_a_parameter_expansion_is_not_an_environment_dump(tmp_path: Path) -> None: + """`${env}` reads one variable; that `{` does not open a command group.""" + _write_script(tmp_path, "#!/bin/bash\nx=${env}\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_keyword_like_path_component_is_not_a_command_position(tmp_path: Path) -> None: + """`./then env` runs a local program named `then`; `env` is its argument.""" + _write_script(tmp_path, "#!/bin/bash\n./then env\n") + assert "shell-env-dump" not in _rules(tmp_path) + + +def test_env_after_a_length_expansion_still_reports(tmp_path: Path) -> None: + """`${#HOME}` is not a comment, so the dump after the `;` is still real code.""" + _write_script(tmp_path, "#!/bin/bash\nn=${#HOME}; env\n") + assert "shell-env-dump" in _rules(tmp_path) + + +def test_printenv_after_an_array_length_expansion_still_reports(tmp_path: Path) -> None: + """`${#arr[@]}` keeps the line as code, so its `;` starts a real command.""" + _write_script(tmp_path, "#!/bin/bash\nn=${#arr[@]}; printenv\n") + assert "shell-env-dump" in _rules(tmp_path)