From a6eabc8b00c16c1685df9b291f798c73ec1189ee Mon Sep 17 00:00:00 2001 From: shejnowicz Date: Wed, 19 Aug 2026 10:27:19 +0200 Subject: [PATCH 1/5] docs(sdd): spec + plan for #19 shlex.quote in sandcat.env Replace the hand-rolled _shell_escape with stdlib shlex.quote when generating sandcat.env. Fixes silent newline corruption (\n is not an escape inside shell double quotes), closes the interactive-shell `!` history-expansion edge, and removes a hand-maintained escaping table. Values flow from settings.json AND secret vaults, so robust quoting is defense-in-depth. Two tasks: code+unit tests, hands-on container verification of bit-perfect hostile-value delivery. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01DFCiFbv1Cpr7yzCU8ftvzZ --- .../plans/2026-08-19-shlex-quote.md | 60 +++++++++++++++++++ .../specs/2026-08-19-shlex-quote-design.md | 54 +++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-19-shlex-quote.md create mode 100644 docs/superpowers/specs/2026-08-19-shlex-quote-design.md diff --git a/docs/superpowers/plans/2026-08-19-shlex-quote.md b/docs/superpowers/plans/2026-08-19-shlex-quote.md new file mode 100644 index 00000000..dc169df4 --- /dev/null +++ b/docs/superpowers/plans/2026-08-19-shlex-quote.md @@ -0,0 +1,60 @@ +# shlex.quote for sandcat.env Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Issue #19 — replace hand-rolled `_shell_escape` with stdlib `shlex.quote` in `sandcat.env` generation; fix newline value-corruption; update the unit tests that lock the old format. + +**Architecture:** One function change in `mitmproxy_addon_common.py` (`_write_placeholders_env` uses `shlex.quote`, `_shell_escape` deleted, `import shlex` added), test updates in `test_mitmproxy_addon.py`, plus a hands-on container verification that a hostile env value arrives bit-perfect. + +**Tech Stack:** Python (mitmproxy addon), pytest, Docker for integration. + +## Global Constraints + +- Line format becomes `export NAME=` — no double-quote wrapper. +- `_shell_escape` deleted entirely (verified: only callers are the two lines in `_write_placeholders_env` and its own tests). +- `_validate_env_name` untouched. +- `import shlex` at module level, alphabetically ordered with the existing stdlib imports. +- Tests updated, not weakened: the hostile-input test (`$(rm -rf /)` + backtick) must still exist, asserting the shlex-quoted form; add a round-trip property assertion (`shlex.split` on the emitted line recovers the original value); newline test asserts PRESERVATION (not the old corruption). +- Local pytest cannot run on this host (system Python 3.9 vs `str | None` syntax in the addon) — the implementer verifies via `python3 -m py_compile` + running pytest INSIDE a container if convenient, or defers pytest to CI with the syntax check done. Bats suites are runnable locally and must stay green. + +--- + +### Task 1: Code + unit tests + +**Files:** +- Modify: `cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py` +- Modify: `cli/test/mitmproxy/test_mitmproxy_addon.py` + +**Steps:** + +- [ ] **Step 1**: Grep all `_shell_escape` references (`grep -rn "_shell_escape" cli/`) and all format-locking assertions (`grep -n 'export ' cli/test/mitmproxy/test_mitmproxy_addon.py`). List them in the report with dispositions. +- [ ] **Step 2**: In `mitmproxy_addon_common.py`: add `import shlex`; rewrite the two `lines.append` calls in `_write_placeholders_env` to `f"export {name}={shlex.quote(...)}"`; delete `_shell_escape` and its docstring. +- [ ] **Step 3**: Update tests: + - Assertions like `'export A="SANDCAT_PLACEHOLDER_A"'` → shlex form. NOTE: placeholders match shlex's safe charset, so they emit BARE: `export A=SANDCAT_PLACEHOLDER_A`. Values with spaces/quotes emit single-quoted. + - The hostile-input test asserts the new quoted form AND adds a round-trip check: parse the emitted line with `shlex.split`, assert the token equals `X=`. + - Replace `TestShellEscapingStaticHelpers` with `TestShlexQuoting` (or similar): safe-value-bare, spaces-quoted, newline-preserved-literally (round-trip), `!`-quoted, embedded-single-quote round-trip. + - Update `test_helpers_inherited_by_variants` (references `_shell_escape`) — delete or re-point. +- [ ] **Step 4**: Verify: `python3 -m py_compile` both files. If a Python ≥3.10 with mitmproxy+pytest is reachable (check `docker run --rm mitmproxy/mitmproxy:12.2.3 python3 -c "import pytest"` — mitmproxy image may lack pytest; alternatively `pip install` inside a throwaway container), run the pytest file; otherwise document CI-deferral. Run bats regression: `cd cli && ./run-tests.bash test/init/` (green — bats doesn't assert env format... verify with grep first; if any bats test asserts `export X="`, update it too). +- [ ] **Step 5**: Commit: `security(mitmproxy): use shlex.quote for sandcat.env generation (#19)` + +--- + +### Task 2: Hands-on integration verification + +**Files:** none (evidence for PR body). + +**Steps:** + +- [ ] **Step 1**: Scratch project (`sandcat init --agent claude --stacks "" --secret-provider none --features "no-rtk,no-gitignore" --proxy web`). Back up `~/.config/sandcat/settings.json`; add a hostile env var: + ```bash + yq -i -o json '.env.SANDCAT_E2E_NASTY = "sp ace \"dq\" '\''sq'\'' $(reboot) `tick` $HOME ! end"' ~/.config/sandcat/settings.json + ``` + (Skip literal newline in settings.json if yq injection is fiddly — cover newline at unit level; note the decision.) +- [ ] **Step 2**: `docker compose up -d --build`; inside agent (login shell): compare `"$SANDCAT_E2E_NASTY"` against the expected literal, byte-for-byte (e.g. `python3 -c 'import os,sys; sys.exit(0 if os.environ["SANDCAT_E2E_NASTY"] == sys.argv[1] else 1)' ''` or `od -c` diff). Assert `$(reboot)`, backtick, and `$HOME` arrive UNEXPANDED. +- [ ] **Step 3**: Regression: placeholder still exported (`echo $ANTHROPIC_API_KEY` shows `SANDCAT_PLACEHOLDER_ANTHROPIC_API_KEY` in login shell) and networking through the proxy works (`curl https://github.com` → 200). +- [ ] **Step 4**: Restore settings backup; teardown `down -v`; write `.superpowers/sdd/2026-08-19-shlex-quote/task-2-report.md`. + +## Out of scope + +- Escaping in bash templates/heredocs elsewhere in the CLI (different surface, no vault-value flow). +- Any change to `_validate_env_name` or placeholder naming. diff --git a/docs/superpowers/specs/2026-08-19-shlex-quote-design.md b/docs/superpowers/specs/2026-08-19-shlex-quote-design.md new file mode 100644 index 00000000..5f469149 --- /dev/null +++ b/docs/superpowers/specs/2026-08-19-shlex-quote-design.md @@ -0,0 +1,54 @@ +# Use shlex.quote for sandcat.env generation (issue #19) — Design + +## Goal + +Replace the hand-rolled `_shell_escape` in `mitmproxy_addon_common.py` with stdlib `shlex.quote` when generating `sandcat.env`, fixing a real value-corruption bug (embedded newlines), closing an interactive-shell edge (`!` history expansion), and eliminating a hand-maintained escaping table. + +## Motivation + +`_write_placeholders_env` emits `export NAME=""` lines consumed by shell `source` in `app-init.sh` (and via the `/etc/profile.d/sandcat-env.sh` copy). Values flow from the user's settings.json AND from secret vaults (1Password `op read`, ProtonPass) — semi-external input, so robust quoting is defense-in-depth. + +Defects in the current `_shell_escape`: + +1. **Newline corruption**: `\n` → `\\n`, but inside double quotes the shell does NOT interpret `\n` — a value containing a real newline is silently corrupted into a literal backslash-n. The unit test `test_newlines_escaped` locks in this wrong behavior. +2. **`!` unhandled**: harmless in non-interactive sourcing, but a user manually sourcing `sandcat.env` in an interactive bash gets history expansion on values containing `!`. +3. **Maintenance anti-pattern**: hand-rolled escape tables are what `shlex.quote` exists to replace. + +No live injection hole today — `\`, `"`, `$`, `` ` `` are covered — this is robustness hardening, not an active-exploit fix. + +## Design + +In `_write_placeholders_env`: + +```python +import shlex # module-level import + +lines.append(f"export {name}={shlex.quote(value)}") # env vars +lines.append(f"export {name}={shlex.quote(entry['placeholder'])}") # placeholders +``` + +- Delete `_shell_escape` entirely (no other callers — verified). +- Keep `_validate_env_name` unchanged (names still regex-validated; quoting applies to values only). +- `shlex.quote` semantics: safe charset (`[\w@%+=:,./-]`) → returned bare (e.g. `export X=SANDCAT_PLACEHOLDER_X`); anything else → single-quoted with the `'"'"'` dance for embedded single quotes. Both forms source identically. +- Multi-line values now produce multi-line quoted exports — valid shell, value preserved bit-perfect. + +## Consumers audit (verified) + +- `app-init.sh`: `cp` to profile.d + `. sandcat.env` (shell source) — quoting-agnostic. ✓ +- `su - vscode -c '. /mitmproxy-config/sandcat.env; ...'` — shell source. ✓ +- No non-shell parser of sandcat.env exists (compose does not env_file it; no grep-based consumers in scripts). +- Only shell-construction site in the addons — no `shell=True` subprocess anywhere. + +## Testing + +- **Unit (pytest)**: update ~10 assertions locking the `export X="..."` format to the shlex format; replace `TestShellEscapingStaticHelpers` with tests asserting: safe values bare, spaces single-quoted, `$(cmd)`/backtick/quote/backslash values round-trip through `shlex.split` back to the original, newline preserved literally, `!` quoted. The existing injection test (`$(rm -rf /)` + backtick) is updated to assert the single-quoted form. +- **Round-trip property**: the strongest unit assertion is `shlex.split(line)` recovering `NAME=` — tests the actual contract (what a shell sees), not the escape spelling. +- **Integration (real container)**: settings.json `env` with a hostile value — spaces, `"`, `'`, `$(reboot)`, backtick, `$HOME`, `!`, literal newline — then inside the agent container `printf '%s' "$X" | od -c` (or compare via `python3 -c`) proves bit-perfect delivery. Also confirm placeholders still substitute end-to-end (curl through proxy with a secret). + +## Global Constraints + +- `shlex.quote` from stdlib; no new dependencies. +- `_shell_escape` removed, not deprecated. +- `sandcat.env` line format: `export NAME=` — no double-quote wrapper. +- `_validate_env_name` untouched. +- All existing pytest + bats suites green; format-locking assertions updated, not weakened (injection test keeps asserting hostile input is neutralized). From 6d98a4e030090d54480b48492b5f0848c473604d Mon Sep 17 00:00:00 2001 From: shejnowicz Date: Wed, 19 Aug 2026 10:38:09 +0200 Subject: [PATCH 2/5] security(mitmproxy): use shlex.quote for sandcat.env generation (#19) --- .../sandcat/scripts/mitmproxy_addon_common.py | 16 +--- cli/test/mitmproxy/test_mitmproxy_addon.py | 82 ++++++++++++++----- 2 files changed, 63 insertions(+), 35 deletions(-) diff --git a/cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py b/cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py index de54e8fe..31b7f423 100644 --- a/cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py +++ b/cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py @@ -45,6 +45,7 @@ import logging import os import re +import shlex import subprocess import sys from fnmatch import fnmatch @@ -651,17 +652,6 @@ def _is_request_allowed(self, method: str | None, host: str) -> bool: # ----------------------------------------------------------- env writer - @staticmethod - def _shell_escape(value: str) -> str: - """Escape a string for safe inclusion inside double quotes in shell.""" - return ( - value.replace("\\", "\\\\") - .replace('"', '\\"') - .replace("$", "\\$") - .replace("`", "\\`") - .replace("\n", "\\n") - ) - @staticmethod def _validate_env_name(name: str): """Raise ValueError if name is not a valid shell variable name.""" @@ -673,10 +663,10 @@ def _write_placeholders_env(self): # Non-secret env vars (e.g. git identity) — passed through as-is. for name, value in self.env.items(): self._validate_env_name(name) - lines.append(f'export {name}="{self._shell_escape(value)}"') + lines.append(f"export {name}={shlex.quote(value)}") for name, entry in self.secrets.items(): self._validate_env_name(name) - lines.append(f'export {name}="{self._shell_escape(entry["placeholder"])}"') + lines.append(f"export {name}={shlex.quote(entry['placeholder'])}") self._atomic_write_text(SANDCAT_ENV_PATH, "\n".join(lines) + "\n") def _write_cursor_cli_config(self, merged: dict): diff --git a/cli/test/mitmproxy/test_mitmproxy_addon.py b/cli/test/mitmproxy/test_mitmproxy_addon.py index 086c1dc1..70922645 100644 --- a/cli/test/mitmproxy/test_mitmproxy_addon.py +++ b/cli/test/mitmproxy/test_mitmproxy_addon.py @@ -15,6 +15,7 @@ import json import os import re +import shlex import sys import types from pathlib import Path @@ -896,8 +897,8 @@ def test_placeholders_env_written_correctly(self, addon_cls, tmp_path): patch(f"{_COMMON}.SANDCAT_ENV_PATH", str(env_path)): addon.load(MagicMock()) content = env_path.read_text() - assert 'export A="SANDCAT_PLACEHOLDER_A"' in content - assert 'export B="SANDCAT_PLACEHOLDER_B"' in content + assert "export A=SANDCAT_PLACEHOLDER_A" in content + assert "export B=SANDCAT_PLACEHOLDER_B" in content def test_env_vars_written_to_placeholders_env(self, addon_cls, tmp_path): settings = { @@ -912,9 +913,9 @@ def test_env_vars_written_to_placeholders_env(self, addon_cls, tmp_path): patch(f"{_COMMON}.SANDCAT_ENV_PATH", str(env_path)): addon.load(MagicMock()) content = env_path.read_text() - assert 'export GIT_USER_NAME="Alice"' in content - assert 'export GIT_USER_EMAIL="alice@example.com"' in content - assert 'export K="SANDCAT_PLACEHOLDER_K"' in content + assert "export GIT_USER_NAME=Alice" in content + assert "export GIT_USER_EMAIL=alice@example.com" in content + assert "export K=SANDCAT_PLACEHOLDER_K" in content def test_env_vars_partial(self, addon_cls, tmp_path): settings = {"env": {"EDITOR": "vim"}} @@ -926,7 +927,7 @@ def test_env_vars_partial(self, addon_cls, tmp_path): patch(f"{_COMMON}.SANDCAT_ENV_PATH", str(env_path)): addon.load(MagicMock()) content = env_path.read_text() - assert 'export EDITOR="vim"' in content + assert "export EDITOR=vim" in content def test_missing_env_section_omits_vars(self, addon_cls, tmp_path): settings = {"secrets": {"K": {"value": "v", "hosts": []}}} @@ -957,7 +958,7 @@ def test_double_quotes_escaped(self, addon_cls, tmp_path): patch(f"{_COMMON}.SANDCAT_ENV_PATH", str(env_path)): addon.load(MagicMock()) content = env_path.read_text() - assert 'export X="val\\"ue"' in content + assert "export X='val\"ue'" in content def test_backslashes_escaped(self, addon_cls, tmp_path): settings = {"env": {"X": "a\\b"}} @@ -969,7 +970,7 @@ def test_backslashes_escaped(self, addon_cls, tmp_path): patch(f"{_COMMON}.SANDCAT_ENV_PATH", str(env_path)): addon.load(MagicMock()) content = env_path.read_text() - assert 'export X="a\\\\b"' in content + assert "export X='a\\b'" in content def test_dollar_and_backtick_escaped(self, addon_cls, tmp_path): settings = {"env": {"X": "$(rm -rf /)`cmd`"}} @@ -981,23 +982,60 @@ def test_dollar_and_backtick_escaped(self, addon_cls, tmp_path): patch(f"{_COMMON}.SANDCAT_ENV_PATH", str(env_path)): addon.load(MagicMock()) content = env_path.read_text() - assert 'export X="\\$(rm -rf /)\\`cmd\\`"' in content + assert "export X='$(rm -rf /)`cmd`'" in content + # Round-trip is the real contract: what a shell would actually see + # when it sources sandcat.env. The hostile value must come back + # byte-for-byte, not just "look escaped" in the raw file text. + line = next(l for l in content.splitlines() if l.startswith("export X=")) + assert shlex.split(line) == ["export", "X=$(rm -rf /)`cmd`"] -class TestShellEscapingStaticHelpers: - """Static helpers live in the shared library; both variants reuse them.""" +class TestShlexEnvQuoting: + """`_write_placeholders_env` quotes via ``shlex.quote``; lock its properties + directly (shared by both addon variants — inherited, not overridden).""" - def test_newlines_escaped(self): - assert BaseAddon._shell_escape("line1\nline2") == "line1\\nline2" - - def test_plain_values_unchanged(self): - assert BaseAddon._shell_escape("hello world") == "hello world" - assert BaseAddon._shell_escape("sk-ant-abc123") == "sk-ant-abc123" + @staticmethod + def _write(tmp_path, value): + addon = BaseAddon() + addon.env = {"X": value} + env_path = tmp_path / "sandcat.env" + with patch(f"{_COMMON}.SANDCAT_ENV_PATH", str(env_path)): + addon._write_placeholders_env() + return env_path.read_text() + + def test_safe_value_emitted_bare(self, tmp_path): + content = self._write(tmp_path, "sk-ant-abc123") + assert content == "export X=sk-ant-abc123\n" + + def test_value_with_spaces_single_quoted(self, tmp_path): + content = self._write(tmp_path, "hello world") + line = content.splitlines()[0] + assert line == "export X='hello world'" + assert shlex.split(line) == ["export", "X=hello world"] + + def test_embedded_single_quote_round_trips(self, tmp_path): + value = "it's a test" + content = self._write(tmp_path, value) + assert shlex.split(content) == ["export", f"X={value}"] + + def test_literal_newline_preserved(self, tmp_path): + # Regression: the old hand-rolled escaper turned a real newline into + # the two-character sequence "\n", corrupting the value. shlex.quote + # single-quotes it instead, keeping the newline byte-for-byte. + value = "line1\nline2" + content = self._write(tmp_path, value) + assert shlex.split(content) == ["export", f"X={value}"] + + def test_exclamation_quoted(self, tmp_path): + content = self._write(tmp_path, "hello!") + line = content.splitlines()[0] + assert line == "export X='hello!'" + assert shlex.split(line) == ["export", "X=hello!"] def test_helpers_inherited_by_variants(self): - # Sanity: subclasses inherit the same helper from the base. - assert ClaudeAddon._shell_escape == BaseAddon._shell_escape - assert CursorAddon._shell_escape == BaseAddon._shell_escape + # Sanity: subclasses inherit the shared env writer from the base. + assert ClaudeAddon._write_placeholders_env == BaseAddon._write_placeholders_env + assert CursorAddon._write_placeholders_env == BaseAddon._write_placeholders_env # --------------------------------------------------------------------------- @@ -1141,7 +1179,7 @@ def test_op_reference_in_full_load(self, addon_cls, tmp_path): addon.load(MagicMock()) assert addon.secrets["API_KEY"]["value"] == "resolved-secret" content = env_path.read_text() - assert 'export API_KEY="SANDCAT_PLACEHOLDER_API_KEY"' in content + assert "export API_KEY=SANDCAT_PLACEHOLDER_API_KEY" in content def test_op_failure_logs_warning_and_continues(self, addon_cls, tmp_path): settings = {"secrets": { @@ -2221,7 +2259,7 @@ def test_debug_not_exported_to_sandcat_env(self, tmp_path, monkeypatch): addon.load(MagicMock()) written = env_path.read_text() assert "SANDCAT_MITM_DEBUG" not in written - assert 'export GIT_USER_NAME="dev"' in written + assert "export GIT_USER_NAME=dev" in written def test_debug_logs_to_stderr_on_request(self, tmp_path, monkeypatch, capsys): monkeypatch.delenv("SANDCAT_MITM_DEBUG", raising=False) From 7f115843a9ad358b0274ec09861099e6dfbe9a62 Mon Sep 17 00:00:00 2001 From: shejnowicz Date: Wed, 19 Aug 2026 12:04:04 +0200 Subject: [PATCH 3/5] fix(mitmproxy): names-only header in sandcat.env; app-init reads it (#19 review) shlex.quote preserves literal newlines in values, so a multi-line value's continuation line could itself match app-init.sh's `^export ` grep, producing a phantom var count and printing a fragment of the value to the startup log. The addon now writes an authoritative `# names: ...` header (built only from validated, whitespace-free names) as the first line of sandcat.env, and app-init.sh parses that header instead of grepping export lines. Falls back to a count-only message (no grep) if the header is absent, for old-addon/new-init-script transitions. --- .../devcontainer/sandcat/scripts/app-init.sh | 38 +++++++++++++++++-- .../sandcat/scripts/mitmproxy_addon_common.py | 21 ++++++++-- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/cli/templates/devcontainer/sandcat/scripts/app-init.sh b/cli/templates/devcontainer/sandcat/scripts/app-init.sh index 6a4c2a84..9dc67026 100644 --- a/cli/templates/devcontainer/sandcat/scripts/app-init.sh +++ b/cli/templates/devcontainer/sandcat/scripts/app-init.sh @@ -77,6 +77,40 @@ export GIT_CONFIG_KEY_0="commit.gpgsign" export GIT_CONFIG_VALUE_0="false" GITEOF +# Print a "Loaded N env var(s): NAME1, NAME2, ..." summary for the startup +# log from the "# names: ..." header the addon writes as the first line of +# sandcat.env (see mitmproxy_addon_common.py::_write_placeholders_env). +# +# We parse that header instead of grepping `export` lines: shlex.quote +# preserves literal newlines in values, so a multi-line value's continuation +# line can itself start with "export " — grepping for that pattern would +# both miscount vars and print a fragment of the value to the startup log. +# +# Defined as a function (not inlined) so `set --` below only rebinds this +# function's own positional parameters — bash gives each function its own +# "$@"/"$#" scope, restored on return — leaving the script's own "$@" +# (needed later for `exec gosu vscode "$@"`) untouched. +_sandcat_env_summary() { + local env_file="$1" header names name + header=$(head -n 1 "$env_file") + case "$header" in + "# names: "*) + names=$(printf '%s' "$header" | sed 's/^# names: //') + set -- $names + echo "Loaded $# env var(s) from $env_file" + for name in "$@"; do + echo " $name" + done + ;; + *) + # Old addon / transition: no header line yet. Report loading + # without enumerating names rather than falling back to a + # value-leaking grep. + echo "Loaded env var(s) from $env_file" + ;; + esac +} + # Source env vars and secret placeholders (if available) SANDCAT_ENV="/mitmproxy-config/sandcat.env" if [ -f "$SANDCAT_ENV" ]; then @@ -84,9 +118,7 @@ if [ -f "$SANDCAT_ENV" ]; then # Make vars available to new shells (e.g. VS Code terminals in dev # containers) that won't inherit the entrypoint's environment. cp "$SANDCAT_ENV" /etc/profile.d/sandcat-env.sh - count=$(grep -c '^export ' "$SANDCAT_ENV" 2>/dev/null || echo 0) - echo "Loaded $count env var(s) from $SANDCAT_ENV" - grep '^export ' "$SANDCAT_ENV" | sed 's/=.*//' | sed 's/^export / /' + _sandcat_env_summary "$SANDCAT_ENV" else echo "No $SANDCAT_ENV found — env vars and secret substitution disabled" fi diff --git a/cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py b/cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py index 31b7f423..5b630afe 100644 --- a/cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py +++ b/cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py @@ -659,13 +659,28 @@ def _validate_env_name(name: str): raise ValueError(f"Invalid env var name: {name!r}") def _write_placeholders_env(self): - lines = [] + # Validate every name up front so the header below is built only + # from names that are guaranteed to match _VALID_ENV_NAME (no + # whitespace, no shell metacharacters) — safe-by-construction, so no + # value content can ever reach it. + for name in self.env: + self._validate_env_name(name) + for name in self.secrets: + self._validate_env_name(name) + + # Authoritative names-only header consumed by app-init.sh to report + # "Loaded N env var(s)" + names without grepping `export` lines. + # shlex.quote below preserves literal newlines in values, so a + # multi-line value's continuation line can itself start with + # "export " — grepping for that pattern would both miscount and + # print a fragment of the value to the startup log. This header is + # always a single line: names contain no whitespace. + names = list(self.env) + list(self.secrets) + lines = [f"# names: {' '.join(names)}"] # Non-secret env vars (e.g. git identity) — passed through as-is. for name, value in self.env.items(): - self._validate_env_name(name) lines.append(f"export {name}={shlex.quote(value)}") for name, entry in self.secrets.items(): - self._validate_env_name(name) lines.append(f"export {name}={shlex.quote(entry['placeholder'])}") self._atomic_write_text(SANDCAT_ENV_PATH, "\n".join(lines) + "\n") From 0c0c747a167f0f311030c8b2a9596c23fa4d2367 Mon Sep 17 00:00:00 2001 From: shejnowicz Date: Wed, 19 Aug 2026 12:04:23 +0200 Subject: [PATCH 4/5] test+docs: quoting-semantics test names, empty-string case, README env format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename TestShellEscaping (and its test_*_escaped methods) to TestEnvValueQuoting / test_*_preserved_via_quoting — shlex.quote quotes values, it doesn't escape them, and the old names described behavior that no longer exists. Add coverage for the new sandcat.env header (names-only, no value fragments — including from a hostile multi-line value) and for the empty-string value case. Update README's sandcat.env format example from the old escaped-double-quote style to the shlex.quote reality. --- README.md | 4 +- cli/test/mitmproxy/test_mitmproxy_addon.py | 81 ++++++++++++++++++---- 2 files changed, 71 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index d9fe3ff7..374e430b 100644 --- a/README.md +++ b/README.md @@ -950,8 +950,8 @@ restart-proxy` after changing 1Password items. local), merges them according to the precedence rules above, and writes `sandcat.env` to the `mitmproxy-config` shared volume (`/home/mitmproxy/.mitmproxy/sandcat.env`). This file contains plain env vars - (e.g. `export GIT_USER_NAME="Your Name"`) and secret placeholders (e.g. - `export ANTHROPIC_API_KEY="SANDCAT_PLACEHOLDER_ANTHROPIC_API_KEY"`). + (e.g. `export GIT_USER_NAME='Your Name'`) and secret placeholders (e.g. + `export ANTHROPIC_API_KEY=SANDCAT_PLACEHOLDER_ANTHROPIC_API_KEY`). 3. App containers mount `mitmproxy-config` read-only at `/mitmproxy-config/`. The shared entrypoint (`app-init.sh`) sources `sandcat.env` after installing the CA cert, so every process gets the env vars and placeholder values. diff --git a/cli/test/mitmproxy/test_mitmproxy_addon.py b/cli/test/mitmproxy/test_mitmproxy_addon.py index 70922645..1cf85f63 100644 --- a/cli/test/mitmproxy/test_mitmproxy_addon.py +++ b/cli/test/mitmproxy/test_mitmproxy_addon.py @@ -939,16 +939,17 @@ def test_missing_env_section_omits_vars(self, addon_cls, tmp_path): patch(f"{_COMMON}.SANDCAT_ENV_PATH", str(env_path)): addon.load(MagicMock()) content = env_path.read_text() - assert content.startswith('export K=') + assert "# names: K" in content + assert "export K=SANDCAT_PLACEHOLDER_K" in content # --------------------------------------------------------------------------- -# Shell escaping — applies regardless of variant. +# Env value quoting — applies regardless of variant. # --------------------------------------------------------------------------- @pytest.mark.parametrize("addon_cls", ADDONS) -class TestShellEscaping: - def test_double_quotes_escaped(self, addon_cls, tmp_path): +class TestEnvValueQuoting: + def test_double_quotes_preserved_via_quoting(self, addon_cls, tmp_path): settings = {"env": {"X": 'val"ue'}} p = tmp_path / "settings.json" p.write_text(json.dumps(settings)) @@ -960,7 +961,7 @@ def test_double_quotes_escaped(self, addon_cls, tmp_path): content = env_path.read_text() assert "export X='val\"ue'" in content - def test_backslashes_escaped(self, addon_cls, tmp_path): + def test_backslashes_preserved_via_quoting(self, addon_cls, tmp_path): settings = {"env": {"X": "a\\b"}} p = tmp_path / "settings.json" p.write_text(json.dumps(settings)) @@ -972,7 +973,7 @@ def test_backslashes_escaped(self, addon_cls, tmp_path): content = env_path.read_text() assert "export X='a\\b'" in content - def test_dollar_and_backtick_escaped(self, addon_cls, tmp_path): + def test_dollar_and_backtick_preserved_via_quoting(self, addon_cls, tmp_path): settings = {"env": {"X": "$(rm -rf /)`cmd`"}} p = tmp_path / "settings.json" p.write_text(json.dumps(settings)) @@ -985,7 +986,7 @@ def test_dollar_and_backtick_escaped(self, addon_cls, tmp_path): assert "export X='$(rm -rf /)`cmd`'" in content # Round-trip is the real contract: what a shell would actually see # when it sources sandcat.env. The hostile value must come back - # byte-for-byte, not just "look escaped" in the raw file text. + # byte-for-byte, not just "look quoted" in the raw file text. line = next(l for l in content.splitlines() if l.startswith("export X=")) assert shlex.split(line) == ["export", "X=$(rm -rf /)`cmd`"] @@ -1003,20 +1004,28 @@ def _write(tmp_path, value): addon._write_placeholders_env() return env_path.read_text() + @staticmethod + def _strip_header(content): + """Drop the leading `# names: ...` header, returning the export + line(s) verbatim — including any embedded literal newlines, so + multi-line values still round-trip through shlex.split correctly.""" + _, _, rest = content.partition("\n") + return rest + def test_safe_value_emitted_bare(self, tmp_path): content = self._write(tmp_path, "sk-ant-abc123") - assert content == "export X=sk-ant-abc123\n" + assert self._strip_header(content) == "export X=sk-ant-abc123\n" def test_value_with_spaces_single_quoted(self, tmp_path): content = self._write(tmp_path, "hello world") - line = content.splitlines()[0] + line = self._strip_header(content).rstrip("\n") assert line == "export X='hello world'" assert shlex.split(line) == ["export", "X=hello world"] def test_embedded_single_quote_round_trips(self, tmp_path): value = "it's a test" content = self._write(tmp_path, value) - assert shlex.split(content) == ["export", f"X={value}"] + assert shlex.split(self._strip_header(content)) == ["export", f"X={value}"] def test_literal_newline_preserved(self, tmp_path): # Regression: the old hand-rolled escaper turned a real newline into @@ -1024,20 +1033,68 @@ def test_literal_newline_preserved(self, tmp_path): # single-quotes it instead, keeping the newline byte-for-byte. value = "line1\nline2" content = self._write(tmp_path, value) - assert shlex.split(content) == ["export", f"X={value}"] + assert shlex.split(self._strip_header(content)) == ["export", f"X={value}"] def test_exclamation_quoted(self, tmp_path): content = self._write(tmp_path, "hello!") - line = content.splitlines()[0] + line = self._strip_header(content).rstrip("\n") assert line == "export X='hello!'" assert shlex.split(line) == ["export", "X=hello!"] + def test_empty_value_quoted(self, tmp_path): + content = self._write(tmp_path, "") + line = self._strip_header(content).rstrip("\n") + assert line == "export X=''" + assert shlex.split(line) == ["export", "X="] + def test_helpers_inherited_by_variants(self): # Sanity: subclasses inherit the shared env writer from the base. assert ClaudeAddon._write_placeholders_env == BaseAddon._write_placeholders_env assert CursorAddon._write_placeholders_env == BaseAddon._write_placeholders_env +# --------------------------------------------------------------------------- +# names-only header — app-init.sh parses this instead of grepping `export` +# lines, so a multi-line value's continuation line (which can itself start +# with "export ", since shlex.quote preserves literal newlines) can never +# miscount vars or leak a value fragment into the startup log (#19 review). +# --------------------------------------------------------------------------- + +class TestSandcatEnvNamesHeader: + def test_header_lists_names_only_no_values(self, tmp_path): + addon = BaseAddon() + # A hostile multi-line value whose continuation line itself looks + # like an `export` statement — the exact shape that broke the old + # grep-based app-init.sh parsing. + addon.env = {"GIT_USER_NAME": "line1\nexport EVIL=leaked\nline3"} + addon.secrets = { + "API_KEY": { + "value": "irrelevant", + "hosts": [], + "placeholder": "SANDCAT_PLACEHOLDER_API_KEY", + } + } + env_path = tmp_path / "sandcat.env" + with patch(f"{_COMMON}.SANDCAT_ENV_PATH", str(env_path)): + addon._write_placeholders_env() + content = env_path.read_text() + lines = content.splitlines() + + header = lines[0] + assert header == "# names: GIT_USER_NAME API_KEY" + + # No fragment of the hostile value leaked into the header. + for fragment in ("EVIL", "leaked", "line1", "line3"): + assert fragment not in header + + # The header is the only comment line in the file — app-init.sh's + # `head -n 1` + prefix match must not be fooled by a later line that + # happens to start with "#" (none should exist here, but this locks + # the invariant the parser depends on). + comment_lines = [l for l in lines if l.startswith("#")] + assert comment_lines == [header] + + # --------------------------------------------------------------------------- # Env var name validation (shared). # --------------------------------------------------------------------------- From 175785bf33a161ab534cf23488b1e6cbe7e5452c Mon Sep 17 00:00:00 2001 From: shejnowicz Date: Wed, 19 Aug 2026 12:04:27 +0200 Subject: [PATCH 5/5] chore: remove SDD spec + plan artifacts from branch Per repo convention, SDD spec/plan docs live in the working session, not the PR. --- .../plans/2026-08-19-shlex-quote.md | 60 ------------------- .../specs/2026-08-19-shlex-quote-design.md | 54 ----------------- 2 files changed, 114 deletions(-) delete mode 100644 docs/superpowers/plans/2026-08-19-shlex-quote.md delete mode 100644 docs/superpowers/specs/2026-08-19-shlex-quote-design.md diff --git a/docs/superpowers/plans/2026-08-19-shlex-quote.md b/docs/superpowers/plans/2026-08-19-shlex-quote.md deleted file mode 100644 index dc169df4..00000000 --- a/docs/superpowers/plans/2026-08-19-shlex-quote.md +++ /dev/null @@ -1,60 +0,0 @@ -# shlex.quote for sandcat.env Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. - -**Goal:** Issue #19 — replace hand-rolled `_shell_escape` with stdlib `shlex.quote` in `sandcat.env` generation; fix newline value-corruption; update the unit tests that lock the old format. - -**Architecture:** One function change in `mitmproxy_addon_common.py` (`_write_placeholders_env` uses `shlex.quote`, `_shell_escape` deleted, `import shlex` added), test updates in `test_mitmproxy_addon.py`, plus a hands-on container verification that a hostile env value arrives bit-perfect. - -**Tech Stack:** Python (mitmproxy addon), pytest, Docker for integration. - -## Global Constraints - -- Line format becomes `export NAME=` — no double-quote wrapper. -- `_shell_escape` deleted entirely (verified: only callers are the two lines in `_write_placeholders_env` and its own tests). -- `_validate_env_name` untouched. -- `import shlex` at module level, alphabetically ordered with the existing stdlib imports. -- Tests updated, not weakened: the hostile-input test (`$(rm -rf /)` + backtick) must still exist, asserting the shlex-quoted form; add a round-trip property assertion (`shlex.split` on the emitted line recovers the original value); newline test asserts PRESERVATION (not the old corruption). -- Local pytest cannot run on this host (system Python 3.9 vs `str | None` syntax in the addon) — the implementer verifies via `python3 -m py_compile` + running pytest INSIDE a container if convenient, or defers pytest to CI with the syntax check done. Bats suites are runnable locally and must stay green. - ---- - -### Task 1: Code + unit tests - -**Files:** -- Modify: `cli/templates/devcontainer/sandcat/scripts/mitmproxy_addon_common.py` -- Modify: `cli/test/mitmproxy/test_mitmproxy_addon.py` - -**Steps:** - -- [ ] **Step 1**: Grep all `_shell_escape` references (`grep -rn "_shell_escape" cli/`) and all format-locking assertions (`grep -n 'export ' cli/test/mitmproxy/test_mitmproxy_addon.py`). List them in the report with dispositions. -- [ ] **Step 2**: In `mitmproxy_addon_common.py`: add `import shlex`; rewrite the two `lines.append` calls in `_write_placeholders_env` to `f"export {name}={shlex.quote(...)}"`; delete `_shell_escape` and its docstring. -- [ ] **Step 3**: Update tests: - - Assertions like `'export A="SANDCAT_PLACEHOLDER_A"'` → shlex form. NOTE: placeholders match shlex's safe charset, so they emit BARE: `export A=SANDCAT_PLACEHOLDER_A`. Values with spaces/quotes emit single-quoted. - - The hostile-input test asserts the new quoted form AND adds a round-trip check: parse the emitted line with `shlex.split`, assert the token equals `X=`. - - Replace `TestShellEscapingStaticHelpers` with `TestShlexQuoting` (or similar): safe-value-bare, spaces-quoted, newline-preserved-literally (round-trip), `!`-quoted, embedded-single-quote round-trip. - - Update `test_helpers_inherited_by_variants` (references `_shell_escape`) — delete or re-point. -- [ ] **Step 4**: Verify: `python3 -m py_compile` both files. If a Python ≥3.10 with mitmproxy+pytest is reachable (check `docker run --rm mitmproxy/mitmproxy:12.2.3 python3 -c "import pytest"` — mitmproxy image may lack pytest; alternatively `pip install` inside a throwaway container), run the pytest file; otherwise document CI-deferral. Run bats regression: `cd cli && ./run-tests.bash test/init/` (green — bats doesn't assert env format... verify with grep first; if any bats test asserts `export X="`, update it too). -- [ ] **Step 5**: Commit: `security(mitmproxy): use shlex.quote for sandcat.env generation (#19)` - ---- - -### Task 2: Hands-on integration verification - -**Files:** none (evidence for PR body). - -**Steps:** - -- [ ] **Step 1**: Scratch project (`sandcat init --agent claude --stacks "" --secret-provider none --features "no-rtk,no-gitignore" --proxy web`). Back up `~/.config/sandcat/settings.json`; add a hostile env var: - ```bash - yq -i -o json '.env.SANDCAT_E2E_NASTY = "sp ace \"dq\" '\''sq'\'' $(reboot) `tick` $HOME ! end"' ~/.config/sandcat/settings.json - ``` - (Skip literal newline in settings.json if yq injection is fiddly — cover newline at unit level; note the decision.) -- [ ] **Step 2**: `docker compose up -d --build`; inside agent (login shell): compare `"$SANDCAT_E2E_NASTY"` against the expected literal, byte-for-byte (e.g. `python3 -c 'import os,sys; sys.exit(0 if os.environ["SANDCAT_E2E_NASTY"] == sys.argv[1] else 1)' ''` or `od -c` diff). Assert `$(reboot)`, backtick, and `$HOME` arrive UNEXPANDED. -- [ ] **Step 3**: Regression: placeholder still exported (`echo $ANTHROPIC_API_KEY` shows `SANDCAT_PLACEHOLDER_ANTHROPIC_API_KEY` in login shell) and networking through the proxy works (`curl https://github.com` → 200). -- [ ] **Step 4**: Restore settings backup; teardown `down -v`; write `.superpowers/sdd/2026-08-19-shlex-quote/task-2-report.md`. - -## Out of scope - -- Escaping in bash templates/heredocs elsewhere in the CLI (different surface, no vault-value flow). -- Any change to `_validate_env_name` or placeholder naming. diff --git a/docs/superpowers/specs/2026-08-19-shlex-quote-design.md b/docs/superpowers/specs/2026-08-19-shlex-quote-design.md deleted file mode 100644 index 5f469149..00000000 --- a/docs/superpowers/specs/2026-08-19-shlex-quote-design.md +++ /dev/null @@ -1,54 +0,0 @@ -# Use shlex.quote for sandcat.env generation (issue #19) — Design - -## Goal - -Replace the hand-rolled `_shell_escape` in `mitmproxy_addon_common.py` with stdlib `shlex.quote` when generating `sandcat.env`, fixing a real value-corruption bug (embedded newlines), closing an interactive-shell edge (`!` history expansion), and eliminating a hand-maintained escaping table. - -## Motivation - -`_write_placeholders_env` emits `export NAME=""` lines consumed by shell `source` in `app-init.sh` (and via the `/etc/profile.d/sandcat-env.sh` copy). Values flow from the user's settings.json AND from secret vaults (1Password `op read`, ProtonPass) — semi-external input, so robust quoting is defense-in-depth. - -Defects in the current `_shell_escape`: - -1. **Newline corruption**: `\n` → `\\n`, but inside double quotes the shell does NOT interpret `\n` — a value containing a real newline is silently corrupted into a literal backslash-n. The unit test `test_newlines_escaped` locks in this wrong behavior. -2. **`!` unhandled**: harmless in non-interactive sourcing, but a user manually sourcing `sandcat.env` in an interactive bash gets history expansion on values containing `!`. -3. **Maintenance anti-pattern**: hand-rolled escape tables are what `shlex.quote` exists to replace. - -No live injection hole today — `\`, `"`, `$`, `` ` `` are covered — this is robustness hardening, not an active-exploit fix. - -## Design - -In `_write_placeholders_env`: - -```python -import shlex # module-level import - -lines.append(f"export {name}={shlex.quote(value)}") # env vars -lines.append(f"export {name}={shlex.quote(entry['placeholder'])}") # placeholders -``` - -- Delete `_shell_escape` entirely (no other callers — verified). -- Keep `_validate_env_name` unchanged (names still regex-validated; quoting applies to values only). -- `shlex.quote` semantics: safe charset (`[\w@%+=:,./-]`) → returned bare (e.g. `export X=SANDCAT_PLACEHOLDER_X`); anything else → single-quoted with the `'"'"'` dance for embedded single quotes. Both forms source identically. -- Multi-line values now produce multi-line quoted exports — valid shell, value preserved bit-perfect. - -## Consumers audit (verified) - -- `app-init.sh`: `cp` to profile.d + `. sandcat.env` (shell source) — quoting-agnostic. ✓ -- `su - vscode -c '. /mitmproxy-config/sandcat.env; ...'` — shell source. ✓ -- No non-shell parser of sandcat.env exists (compose does not env_file it; no grep-based consumers in scripts). -- Only shell-construction site in the addons — no `shell=True` subprocess anywhere. - -## Testing - -- **Unit (pytest)**: update ~10 assertions locking the `export X="..."` format to the shlex format; replace `TestShellEscapingStaticHelpers` with tests asserting: safe values bare, spaces single-quoted, `$(cmd)`/backtick/quote/backslash values round-trip through `shlex.split` back to the original, newline preserved literally, `!` quoted. The existing injection test (`$(rm -rf /)` + backtick) is updated to assert the single-quoted form. -- **Round-trip property**: the strongest unit assertion is `shlex.split(line)` recovering `NAME=` — tests the actual contract (what a shell sees), not the escape spelling. -- **Integration (real container)**: settings.json `env` with a hostile value — spaces, `"`, `'`, `$(reboot)`, backtick, `$HOME`, `!`, literal newline — then inside the agent container `printf '%s' "$X" | od -c` (or compare via `python3 -c`) proves bit-perfect delivery. Also confirm placeholders still substitute end-to-end (curl through proxy with a secret). - -## Global Constraints - -- `shlex.quote` from stdlib; no new dependencies. -- `_shell_escape` removed, not deprecated. -- `sandcat.env` line format: `export NAME=` — no double-quote wrapper. -- `_validate_env_name` untouched. -- All existing pytest + bats suites green; format-locking assertions updated, not weakened (injection test keeps asserting hostile input is neutralized).