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/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 de54e8fe..5b630afe 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.""" @@ -669,14 +659,29 @@ 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}="{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..1cf85f63 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": []}}} @@ -938,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)) @@ -957,9 +959,9 @@ 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): + 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)) @@ -969,9 +971,9 @@ 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): + 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)) @@ -981,23 +983,116 @@ 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 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`"] -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" + @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_plain_values_unchanged(self): - assert BaseAddon._shell_escape("hello world") == "hello world" - assert BaseAddon._shell_escape("sk-ant-abc123") == "sk-ant-abc123" + @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 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 = 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(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 + # 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(self._strip_header(content)) == ["export", f"X={value}"] + + def test_exclamation_quoted(self, tmp_path): + content = self._write(tmp_path, "hello!") + 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 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 + + +# --------------------------------------------------------------------------- +# 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] # --------------------------------------------------------------------------- @@ -1141,7 +1236,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 +2316,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)