fix(config): strip control chars from saved env credentials - #55336
fix(config): strip control chars from saved env credentials#55336ooiuuii wants to merge 1 commit into
Conversation
|
tonydwb
left a comment
There was a problem hiding this comment.
LGTM. Strips ASCII control characters from saved credentials. Clean security fix with test.
Code Review SummaryVerdict: Approved Strips ASCII control characters (below space, DEL) from saved environment credentials. This prevents invisible characters from being stored in .env files which could cause authentication failures. ✅ Looks Good
Reviewed by Hermes Agent |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the save-time control-byte gap. The central premise is still present on current main: hermes_cli/config.py:7554 only removes LF/CR before the helper writes the value and assigns it to os.environ at hermes_cli/config.py:7618.
Problems
- The TUI save flow calls
save_env_value()attui_gateway/server.py:12958but then replaces the sanitized environment value with rawapi_keyattui_gateway/server.py:12962. The same raw post-save overwrite exists inhermes_cli/secrets_cli.py:176-177andhermes_cli/onepassword_secrets_cli.py:145-146; a NUL can still fail there after the clean.envwrite. - The new helper applies to all
save_env_value()inputs, whilehermes_cli/mcp_catalog.py:438documents that this writer stores non-secrets too. The unconditional TAB removal needs an explicit compatibility decision or narrower credential-only scope.
Suggested changes
- Eliminate or sanitize the post-save raw assignments and cover a TUI provider-key save regression.
- Specify and test the non-secret/TAB policy.
Automated hermes-sweeper review.
| @@ -6826,7 +6831,7 @@ def save_env_value(key: str, value: str): | |||
| if not _ENV_VAR_NAME_RE.match(key): | |||
There was a problem hiding this comment.
save_env_value() itself updates os.environ, but tui_gateway/server.py:12958-12962, hermes_cli/secrets_cli.py:176-177, and hermes_cli/onepassword_secrets_cli.py:145-146 overwrite that entry with the original raw credential afterward. Please remove or sanitize those redundant assignments and add a path-level regression; otherwise a NUL still raises after the clean .env write.
| @@ -6789,6 +6789,11 @@ def _check_non_ascii_credential(key: str, value: str) -> str: | |||
| return sanitized | |||
|
|
|||
|
|
|||
| def _strip_ascii_control_credential_chars(value: str) -> str: | |||
There was a problem hiding this comment.
This helper is invoked for every save_env_value() call, including non-secret MCP catalog variables (hermes_cli/mcp_catalog.py:438). Please establish whether stripping TAB is intended for that broader API, or constrain the normalization to credential values.
1566874 to
0d59186
Compare
Summary
Fixes #55335.
This strips ASCII control bytes (C0 plus DEL) from values passed through
save_env_value()before Hermes writes.envand before it updatesos.environ.Hermes already stripped
\nand\r, and already warns/strips non-ASCII credential artifacts. The missing gap was other ASCII controls such as NUL, TAB, ESC, and DEL:os.environ[key] = valueraiseValueError: embedded null characteron Windows after the dirty value has already been written to.env..envand process env, causing confusing provider auth/header failures later.Validation
ValueError: embedded null character.OPENAI_API_KEY=sk-livekeynextwith no control bytes.python -m pytest tests/hermes_cli/test_config.py::TestSaveEnvValueSecure -q --basetemp .pytest-tmp-envsave→ 9 passedpython -m pytest tests/hermes_cli/test_non_ascii_credential.py -q --basetemp .pytest-tmp-nonascii→ 12 passedruff check hermes_cli/config.py tests/hermes_cli/test_config.py→ passedgit diff --check→ passedLocal note: the full
tests/hermes_cli/test_config.pyfile is not a clean Windows/GBK gate in this checkout; it has unrelated existing failures around the Windows default home expectation and locale-decoding UTF-8 config files. The touched save-env class passes.Agent transcript
save_env_value()path..envpath..envthen crashes onos.environ; TAB/DEL persist without crashing.