From 9291b786b425532d41a1f2dfad5a9b2953cdc0d6 Mon Sep 17 00:00:00 2001 From: pprism13 <290877921+pprism13@users.noreply.github.com> Date: Sun, 21 Jun 2026 21:34:57 +0300 Subject: [PATCH] fix(openviking): sanitize embedded newlines when writing .env secrets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_write_env_vars` in the OpenViking memory provider interpolates each secret straight into a `KEY=VALUE` line, but the values only ever pass through `_clean_config_value`, whose `value.strip()` trims surrounding whitespace and leaves internal CR/LF intact. Because the file is strictly line-oriented and is re-read via `read_text().splitlines()`, a value that carries an embedded newline spills onto a second physical line, and the tail is re-parsed as an independent `KEY=VALUE` entry on the next round trip. A secret pasted with a trailing record (e.g. an `OPENVIKING_API_KEY` copied with an extra line) therefore injects an arbitrary additional variable into the persisted credentials file and silently corrupts it. The fix neutralizes the line terminators at the single chokepoint where values reach the file. A small `_env_line_safe` helper strips `\r`, `\n`, and the NUL byte from each value, and both write sites in `_write_env_vars` (the existing-key update branch and the appended-key branch) route through it, so a value can only ever occupy the single line it is written on. ## What does this PR do? Hardens the OpenViking memory provider's `.env` writer so a malformed or pasted secret value can no longer break out of its `KEY=VALUE` line and inject a rogue variable into the profile-scoped credentials file. ## Related Issue N/A ## Type of Change - [x] 🐛 Bug fix (non-breaking change that fixes an issue) ## Changes Made - `plugins/memory/openviking/__init__.py`: add `_env_line_safe()` which removes `\r`, `\n`, and `\x00` from a value, and apply it to both the updated-key and appended-key write branches in `_write_env_vars()`. - `tests/plugins/memory/test_openviking_provider.py`: add two regression tests covering a fresh write and an in-place key update with embedded CR/LF, asserting no injected line survives the read-back. ## How to Test 1. Run the targeted tests: `pytest tests/plugins/memory/test_openviking_provider.py -k env_writer -q` 2. Reverting the `_env_line_safe` sanitization makes `test_openviking_env_writer_strips_embedded_newlines_in_values` and `test_openviking_env_writer_strips_newlines_when_updating_existing_key` fail with a rogue `INJECTED_KEY=`/`ROGUE=1` line appearing in the file, confirming the tests pin the bug. 3. `ruff check plugins/memory/openviking/__init__.py` and `python scripts/check-windows-footguns.py plugins/memory/openviking/__init__.py` both pass. ## Checklist ### Code - [x] I've read the Contributing Guide - [x] My commit messages follow Conventional Commits - [x] I searched for existing PRs to make sure this isn't a duplicate - [x] My PR contains only changes related to this fix - [x] I've run the relevant tests and they pass - [x] I've added tests for my changes (required for bug fixes) - [x] I've tested on my platform: macOS 15 (Darwin 25.5) ### Documentation & Housekeeping - [x] I've updated relevant documentation (docstrings) — or N/A - [x] I've updated `cli-config.yaml.example` if I added/changed config keys — N/A - [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — N/A - [x] I've considered cross-platform impact (strips CR as well as LF) — done - [x] I've updated tool descriptions/schemas if I changed tool behavior — N/A (cherry picked from commit f29dd2df845c3f049820797b473437b577b75362) --- plugins/memory/openviking/__init__.py | 21 ++++++++++-- .../memory/test_openviking_provider.py | 33 +++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py index 5553a4b63169..48589492e351 100644 --- a/plugins/memory/openviking/__init__.py +++ b/plugins/memory/openviking/__init__.py @@ -962,6 +962,23 @@ def _precreate_secret_file(path: Path) -> None: logger.debug("Could not pre-create secret file %s: %s", path, e) +def _env_line_safe(value: Any) -> str: + """Neutralize characters that would break ``.env`` line structure. + + A ``.env`` file is strictly line-oriented (one ``KEY=VALUE`` per line), + and ``_write_env_vars`` interpolates each value straight into that line. + A value carrying an embedded CR/LF would therefore spill onto a new line + and be re-parsed as a *separate* ``KEY=VALUE`` entry on the next + ``read_text().splitlines()`` round-trip — letting a malformed or pasted + secret (e.g. an api_key copied with a trailing record) inject an + arbitrary additional variable into the persisted credentials file. Strip + the line terminators (``\r``, ``\n``) and the NUL byte so a value can + only ever occupy the single line it is written on. + """ + text = value if isinstance(value, str) else str(value) + return text.replace("\r", "").replace("\n", "").replace("\x00", "") + + def _write_env_vars(env_path: Path, env_writes: dict, remove_keys: tuple[str, ...] = ()) -> None: env_path.parent.mkdir(parents=True, exist_ok=True) remove_set = set(remove_keys) - set(env_writes) @@ -973,13 +990,13 @@ def _write_env_vars(env_path: Path, env_writes: dict, remove_keys: tuple[str, .. if key_match in remove_set: continue if key_match in env_writes: - new_lines.append(f"{key_match}={env_writes[key_match]}") + new_lines.append(f"{key_match}={_env_line_safe(env_writes[key_match])}") updated_keys.add(key_match) else: new_lines.append(line) for key, val in env_writes.items(): if key not in updated_keys: - new_lines.append(f"{key}={val}") + new_lines.append(f"{key}={_env_line_safe(val)}") # Pre-create with 0600 so secrets are never briefly world-readable. _precreate_secret_file(env_path) env_path.write_text("\n".join(new_lines) + ("\n" if new_lines else ""), encoding="utf-8") diff --git a/tests/plugins/memory/test_openviking_provider.py b/tests/plugins/memory/test_openviking_provider.py index df2e5632be87..87246fec2eb9 100644 --- a/tests/plugins/memory/test_openviking_provider.py +++ b/tests/plugins/memory/test_openviking_provider.py @@ -91,6 +91,39 @@ def test_openviking_env_writer_restricts_file_permissions(tmp_path): assert stat.S_IMODE(env_path.stat().st_mode) == 0o600 +def test_openviking_env_writer_strips_embedded_newlines_in_values(tmp_path): + # A secret pasted with an embedded CR/LF must not spill onto a new line, + # or the round-trip re-parses the tail as a separate KEY=VALUE entry and + # injects an arbitrary variable into the persisted credentials file. + env_path = tmp_path / ".env" + + openviking_module._write_env_vars( + env_path, + {"OPENVIKING_API_KEY": "good\nINJECTED_KEY=attacker"}, + ) + + lines = env_path.read_text(encoding="utf-8").splitlines() + assert lines == ["OPENVIKING_API_KEY=goodINJECTED_KEY=attacker"] + # No injected line means a follow-up read sees no rogue key. + parsed = dict(line.split("=", 1) for line in lines if "=" in line) + assert set(parsed) == {"OPENVIKING_API_KEY"} + assert "INJECTED_KEY" not in parsed + + +def test_openviking_env_writer_strips_newlines_when_updating_existing_key(tmp_path): + env_path = tmp_path / ".env" + env_path.write_text("OPENVIKING_API_KEY=old\n", encoding="utf-8") + + openviking_module._write_env_vars( + env_path, + {"OPENVIKING_API_KEY": "new\r\nROGUE=1"}, + ) + + lines = env_path.read_text(encoding="utf-8").splitlines() + assert lines == ["OPENVIKING_API_KEY=newROGUE=1"] + assert all(not line.startswith("ROGUE=") for line in lines) + + @pytest.mark.skipif(os.name == "nt", reason="POSIX file modes") def test_ovcli_config_writer_restricts_file_permissions(tmp_path): config_path = tmp_path / "ovcli.conf"