fix(cli): recognize whitespace around '=' in .env save/remove - #67488
fix(cli): recognize whitespace around '=' in .env save/remove#67488Frowtek wants to merge 1 commit into
Conversation
_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 NousResearch#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.
Related to merged #67213: that patch recognizes |
|
Thanks — the reported mismatch is present on current main. The added endpoint tests complement the existing export-prefix lifecycle coverage in Automated hermes-sweeper review. |
Summary
_env_line_defines_key()decides which.envlines the writers are allowed torewrite or drop. It matched on the
KEY=prefix:But
load_env()splits on the first=and strips the name:So
OPENAI_API_KEY = sk-...is a live assignment — the key resolves, theprovider authenticates, and every UI shows it as set. The writers never saw it.
This is the same resurrection hole #40041 just fixed for
export KEY=, stillopen for the whitespace form.
Impact
A credential the user revokes through the UI is not actually revoked:
.envcontainsOPENAI_API_KEY = sk-COMPROMISEDDELETE /api/envPUT /api/env(rotate)DELETEReproduced end-to-end against the real endpoint handlers:
Fix
Mirror
load_env()'s parse instead of prefix-matching, so the writers acceptprecisely what the reader accepts — skip blank/comment/no-
=lines, strip anexportprefix, then compare the stripped name:Both writers (
save_env_value,remove_env_value) go through this one helper,so the single change covers save and remove together.
Parity after the fix — writer matches a line iff
load_env()accepts it:load_env()KEY=vexport KEY=vKEY = vKEY\t=\tvexport KEY = v# KEY=vKEY_EXTRA=vMY_KEY=vCommented-out lines are still left untouched, and no prefix/substring key
collides.
Testing
Three tests added to
tests/hermes_cli/test_env_export_line_lifecycle.py,driving the real dashboard endpoints against a temp
HERMES_HOME(fake tokensconstructed at runtime, matching the file's existing convention):
test_remove_token_written_with_spaces_around_equalstest_spaced_token_rotate_then_delete_does_not_resurrecttest_writer_matches_exactly_what_load_env_acceptsAll three fail on
mainwith the resurrection above and pass with thischange. The 5 existing
export-line tests are unaffected.Config and managed-scope suites compared against a clean
origin/mainworktree (
09109fec9): identical pre-existing failure set(
TestGetHermesHome::test_default_path, twoSaveEnvValueSecurequotingtests), no new failures.
Checklist
mainand covered by a failing-before/passing-after testorigin/main)