mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
fix(openviking): sanitize embedded newlines when writing .env secrets
`_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 f29dd2df84)
This commit is contained in:
parent
46c7a4076f
commit
9291b786b4
2 changed files with 52 additions and 2 deletions
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue