fix(cli): recognize whitespace around '=' in .env save/remove
_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.
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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}"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user