From 1152d4d3ce0bd25aaea78cdc132f13ff084d0bc8 Mon Sep 17 00:00:00 2001 From: Frowtek Date: Sun, 19 Jul 2026 15:31:57 +0300 Subject: [PATCH] fix(cli): recognize whitespace around '=' in .env save/remove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _env_line_defines_key() decides which .env lines the writers may rewrite or drop. It matched on the `KEY=` prefix, but load_env() splits on the first `=` and strips the name: key, _, value = line.partition('=') env_vars[key.strip()] = _parse_env_value(value) so `OPENAI_API_KEY = sk-...` is a live assignment — the key resolves, the provider works, and every UI shows it as set. The writers did not see it. This is the same resurrection hole #40041 fixed for `export KEY=`, still open for the whitespace form: - DELETE /api/env 404s ("not found in .env") while the credential stays active — a key the user revoked through the UI is never actually revoked - PUT /api/env appends a SECOND line instead of replacing; a later delete removes the appended line and the original value silently comes back Rotate-then-delete on a spaced line therefore restores exactly the key the user rotated away from. Match load_env()'s parse instead of prefix-matching, so the writers accept precisely what the reader accepts: skip blank/comment/no-'=' lines, strip an `export ` prefix, then compare the stripped name. Commented-out lines stay untouched and `KEY_EXTRA=`/`MY_KEY=` still do not match `KEY`. Verified against the real dashboard endpoints on a temp HERMES_HOME: the spaced line is now removed, rotation replaces it in place with no duplicate, and a parity check asserts the writer matches a line iff load_env() does. --- hermes_cli/config.py | 6 +- .../test_env_export_line_lifecycle.py | 122 ++++++++++++++++++ 2 files changed, 127 insertions(+), 1 deletion(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 051472ad90..4d8863267f 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -4387,8 +4387,12 @@ def _env_line_defines_key( assigned_key, separator, _value = stripped.partition("=") if not separator: return False + # load_env() strips whitespace around the parsed name, so `KEY = value` + # IS a live assignment. The writers must match the same shape, or a + # hand-edited spaced line is invisible to save (duplicate appended) and + # remove (line survives -> value resurrects on next load). #67488. return _env_var_policy_name( - assigned_key, + assigned_key.strip(), is_windows=is_windows, ) == _env_var_policy_name(key, is_windows=is_windows) diff --git a/tests/hermes_cli/test_env_export_line_lifecycle.py b/tests/hermes_cli/test_env_export_line_lifecycle.py index b1333a7f47..5ea6247a64 100644 --- a/tests/hermes_cli/test_env_export_line_lifecycle.py +++ b/tests/hermes_cli/test_env_export_line_lifecycle.py @@ -55,8 +55,45 @@ def test_classic_pat_save_via_endpoint_succeeds(hermes_home): assert load_env()["GITHUB_TOKEN"] == NEW_PAT +def test_remove_export_prefixed_token(hermes_home): + """DELETE must clear an ``export KEY=...`` line, not 404 on it.""" + _write_env_raw(hermes_home, f"export GITHUB_TOKEN={OLD_PAT}\n") + + resp = client.request( + "DELETE", "/api/env", json={"key": "GITHUB_TOKEN"}, headers=HEADERS + ) + assert resp.status_code == 200, ( + "export-prefixed lines are parsed by load_env (UI shows the token as " + "set) so the delete path must recognise them too (#40041)" + ) + + env_text = hermes_home.joinpath(".env").read_text(encoding="utf-8") + assert OLD_PAT not in env_text + + from hermes_cli.config import load_env + + assert "GITHUB_TOKEN" not in load_env() +def test_update_export_prefixed_token_does_not_duplicate(hermes_home): + """Saving over an ``export KEY=`` line must replace it in place.""" + _write_env_raw(hermes_home, f"export GITHUB_TOKEN={OLD_PAT}\n") + + resp = client.put( + "/api/env", json={"key": "GITHUB_TOKEN", "value": NEW_PAT}, headers=HEADERS + ) + assert resp.status_code == 200 + + env_text = hermes_home.joinpath(".env").read_text(encoding="utf-8") + assert OLD_PAT not in env_text, "old exported token line must be replaced" + assert env_text.count("GITHUB_TOKEN") == 1, ( + "save must not append a duplicate GITHUB_TOKEN line alongside the " + "export-prefixed one" + ) + + from hermes_cli.config import load_env + + assert load_env()["GITHUB_TOKEN"] == NEW_PAT def test_plain_line_save_and_remove_still_work(hermes_home): @@ -72,3 +109,88 @@ def test_plain_line_save_and_remove_still_work(hermes_home): assert "GITHUB_TOKEN" not in load_env() +def test_export_line_with_comment_untouched(hermes_home): + """Commented-out export lines are not live assignments — leave them.""" + _write_env_raw( + hermes_home, + f"# export GITHUB_TOKEN={OLD_PAT}\nOTHER_KEY=value\n", + ) + + resp = client.request( + "DELETE", "/api/env", json={"key": "GITHUB_TOKEN"}, headers=HEADERS + ) + assert resp.status_code == 404 + env_text = hermes_home.joinpath(".env").read_text(encoding="utf-8") + assert "# export GITHUB_TOKEN=" in env_text + assert "OTHER_KEY=value" in env_text + + +# -- whitespace around '=' (same resurrection hole, other line form) ---------- + + +def test_remove_token_written_with_spaces_around_equals(hermes_home): + """DELETE must clear a ``KEY = value`` line, not 404 on it. + + ``load_env()`` splits on the first ``=`` and strips the name, so this is a + live assignment and every UI shows the token as set — the writers have to + recognise it for the same reason they recognise ``export`` (#40041). + """ + _write_env_raw(hermes_home, f"GITHUB_TOKEN = {OLD_PAT}\n") + + from hermes_cli.config import load_env + + assert load_env()["GITHUB_TOKEN"] == OLD_PAT, "precondition: token is live" + + resp = client.request( + "DELETE", "/api/env", json={"key": "GITHUB_TOKEN"}, headers=HEADERS + ) + assert resp.status_code == 200, resp.text + + env_text = hermes_home.joinpath(".env").read_text(encoding="utf-8") + assert OLD_PAT not in env_text + assert "GITHUB_TOKEN" not in load_env() + + +def test_spaced_token_rotate_then_delete_does_not_resurrect(hermes_home): + """Rotating over a spaced line must replace it, not shadow it. + + Otherwise the save appends a second line and a later delete removes only + that one — silently restoring the key the user rotated away from. + """ + _write_env_raw(hermes_home, f"GITHUB_TOKEN = {OLD_PAT}\n") + + resp = client.put( + "/api/env", json={"key": "GITHUB_TOKEN", "value": NEW_PAT}, headers=HEADERS + ) + assert resp.status_code == 200 + env_text = hermes_home.joinpath(".env").read_text(encoding="utf-8") + assert OLD_PAT not in env_text, "the rotated-away token must not survive" + assert env_text.count("GITHUB_TOKEN") == 1 + + from hermes_cli.config import load_env + + assert load_env()["GITHUB_TOKEN"] == NEW_PAT + + client.request("DELETE", "/api/env", json={"key": "GITHUB_TOKEN"}, headers=HEADERS) + assert "GITHUB_TOKEN" not in load_env(), "the old token must not resurrect" + + +def test_writer_matches_exactly_what_load_env_accepts(hermes_home): + """Parity: the writers must recognise a line iff ``load_env()`` does.""" + from hermes_cli.config import _env_line_defines_key, load_env + + for line, expected in [ + (f"GITHUB_TOKEN={OLD_PAT}", True), + (f"export GITHUB_TOKEN={OLD_PAT}", True), + (f"GITHUB_TOKEN = {OLD_PAT}", True), + (f"GITHUB_TOKEN\t=\t{OLD_PAT}", True), + (f"export GITHUB_TOKEN = {OLD_PAT}", True), + (f"# GITHUB_TOKEN={OLD_PAT}", False), + (f"GITHUB_TOKEN_EXTRA={OLD_PAT}", False), + (f"MY_GITHUB_TOKEN={OLD_PAT}", False), + ]: + _write_env_raw(hermes_home, line + "\n") + assert _env_line_defines_key(line, "GITHUB_TOKEN") is expected, line + assert ("GITHUB_TOKEN" in load_env()) is expected, ( + f"writer and load_env disagree on: {line!r}" + )