From dcf9a1e06f2b412ce16da7f5c70195d96040b366 Mon Sep 17 00:00:00 2001 From: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com> Date: Sun, 19 Apr 2026 00:40:20 -0700 Subject: [PATCH] fix(cli/env): preserve values that contain quotes, newlines, or backslashes The env file writer wrapped values containing whitespace in double quotes but never escaped embedded quotes or backslashes, and the reader stripped outer quote characters even when they were legitimate parts of the value. This caused silent data corruption on round-trip: - "value with leading quote -> read back as value with leading quote - value with trailing quote" -> read back as value with trailing quote - "quoted" -> read back as quoted (idempotency broken) - line1\nline2 -> read back as line1 (second line dropped) Any wizard re-run would then persist the corrupted value. Switches to quote-only-when-needed with proper escaping of quotes, backslashes, and control characters (\\n, \\r, \\t), and a matching reader that decodes the escape sequences in a single pass. Values outside double quotes are returned verbatim so legacy single-quoted values and unquoted values continue to round-trip unchanged. Adds tests/test_env.py covering the four corruption cases, idempotency across repeated writes, legacy single-quoted reads, key preservation, and empty-value deletion. --- src/sre_agent/cli/env.py | 82 ++++++++++++++++++++++++++++++++++++++-- tests/test_env.py | 70 ++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 tests/test_env.py diff --git a/src/sre_agent/cli/env.py b/src/sre_agent/cli/env.py index 73fe115d..e91b722e 100644 --- a/src/sre_agent/cli/env.py +++ b/src/sre_agent/cli/env.py @@ -6,6 +6,9 @@ from sre_agent.config.paths import env_path +# Characters that require a value to be quoted when serialised. +_SHELL_SPECIAL = re.compile(r"[\s\"'\\#]") + def load_env_values() -> dict[str, str]: """Load env file values and overlay environment variables. @@ -38,7 +41,7 @@ def read_env_file(path: Path) -> dict[str, str]: if not line or line.startswith("#") or "=" not in line: continue key, value = line.split("=", 1) - values[key.strip()] = value.strip().strip("\"'") + values[key.strip()] = _unescape_env_value(value.strip()) return values @@ -68,12 +71,85 @@ def write_env_file(path: Path, updates: dict[str, str]) -> None: def _escape_env_value(value: str) -> str: """Escape a value for env output. + Wraps the value in double quotes when it contains whitespace, quote + characters, a leading ``#``, or backslashes, and escapes embedded double + quotes and backslashes so the value survives a round-trip through + :func:`read_env_file`. + Args: value: Value to escape. Returns: The escaped value. """ - if re.search(r"\s", value): - return f'"{value}"' + if not _SHELL_SPECIAL.search(value): + return value + escaped = ( + value.replace("\\", "\\\\") + .replace('"', '\\"') + .replace("\n", "\\n") + .replace("\r", "\\r") + .replace("\t", "\\t") + ) + return f'"{escaped}"' + + +def _unescape_env_value(value: str) -> str: + """Reverse of :func:`_escape_env_value`. + + Handles values that were written either by the current escaping scheme + or by the legacy scheme that only stripped outer quotes. Returns the + value as-is when it is not wrapped in matching quotes, so unquoted + values that happen to contain a leading or trailing quote character + are preserved. + + Args: + value: Raw value read from an env file (already stripped of + surrounding whitespace). + + Returns: + The decoded value. + """ + if len(value) >= 2 and value[0] == value[-1] and value[0] in ('"', "'"): + inner = value[1:-1] + if value[0] == '"': + return _decode_double_quoted(inner) + return inner return value + + +_ESCAPE_MAP = { + '"': '"', + "\\": "\\", + "n": "\n", + "r": "\r", + "t": "\t", +} + + +def _decode_double_quoted(inner: str) -> str: + r"""Decode the body of a double-quoted env value in a single pass. + + Recognises ``\\``, ``\"``, ``\n``, ``\r``, and ``\t`` escape sequences. + An unknown escape (e.g. ``\z``) is preserved verbatim, matching the + behaviour of common ``.env`` parsers. + + Args: + inner: The characters between the surrounding double quotes. + + Returns: + The decoded string. + """ + out: list[str] = [] + i = 0 + length = len(inner) + while i < length: + ch = inner[i] + if ch == "\\" and i + 1 < length: + nxt = inner[i + 1] + out.append(_ESCAPE_MAP.get(nxt, ch + nxt)) + i += 2 + continue + out.append(ch) + i += 1 + return "".join(out) diff --git a/tests/test_env.py b/tests/test_env.py new file mode 100644 index 00000000..51c8c52f --- /dev/null +++ b/tests/test_env.py @@ -0,0 +1,70 @@ +"""Tests for ``sre_agent.cli.env`` round-tripping.""" + +from pathlib import Path + +import pytest + +from sre_agent.cli.env import read_env_file, write_env_file + + +@pytest.mark.parametrize( + "value", + [ + "plain", + "with spaces", + '"starts with quote', + 'ends with quote"', + '"quoted"', + "a=b=c", + "line1\nline2", + " ", + "back\\slash", + "# starts with hash", + 'has "inner" quotes', + "mix \"of\\weird' chars", + ], +) +def test_env_roundtrip(tmp_path: Path, value: str) -> None: + """Writing a value and reading it back returns the same value.""" + env_file = tmp_path / ".env" + write_env_file(env_file, {"MY_KEY": value}) + parsed = read_env_file(env_file) + assert parsed["MY_KEY"] == value + + +def test_env_roundtrip_is_idempotent(tmp_path: Path) -> None: + """Re-writing an already-stored value must not mutate it.""" + env_file = tmp_path / ".env" + value = '"quoted"' + write_env_file(env_file, {"API_KEY": value}) + first = read_env_file(env_file)["API_KEY"] + # Simulate the wizard running again — re-persist the previously read value. + write_env_file(env_file, {"API_KEY": first}) + second = read_env_file(env_file)["API_KEY"] + assert first == value + assert second == value + + +def test_env_preserves_other_keys(tmp_path: Path) -> None: + """Updating one key must not disturb others.""" + env_file = tmp_path / ".env" + write_env_file(env_file, {"KEEP": "keep-value", "UPDATE": "v1"}) + write_env_file(env_file, {"UPDATE": "v2"}) + parsed = read_env_file(env_file) + assert parsed == {"KEEP": "keep-value", "UPDATE": "v2"} + + +def test_env_removes_empty_values(tmp_path: Path) -> None: + """Empty updates clear the corresponding key.""" + env_file = tmp_path / ".env" + write_env_file(env_file, {"DELETE_ME": "value"}) + write_env_file(env_file, {"DELETE_ME": ""}) + parsed = read_env_file(env_file) + assert "DELETE_ME" not in parsed + + +def test_env_reads_legacy_single_quoted(tmp_path: Path) -> None: + """Legacy files written with single-quoted values still parse cleanly.""" + env_file = tmp_path / ".env" + env_file.write_text("TOKEN='value with space'\n") + assert read_env_file(env_file) == {"TOKEN": "value with space"}