Skip to content

fix(cli): strip ASCII control characters in save_env_value - #55767

Open
GauravPatil2515 wants to merge 1 commit into
NousResearch:mainfrom
GauravPatil2515:fix/save-env-value-ascii-controls-55335
Open

fix(cli): strip ASCII control characters in save_env_value#55767
GauravPatil2515 wants to merge 1 commit into
NousResearch:mainfrom
GauravPatil2515:fix/save-env-value-ascii-controls-55335

Conversation

@GauravPatil2515

Copy link
Copy Markdown
Contributor

Summary

save_env_value() only removed \\n and \\r before writing credential values to ~/.hermes/.env. Other ASCII control characters (C0 range, 0x00-0x1F, and DEL, 0x7F) could still be persisted.

  • On Windows, saving a copied key containing a NUL byte causes os.environ[key] = value to crash with ValueError: embedded null character, leaving a partially written and dirty .env file.
  • On other systems, control bytes such as TAB or DEL survive, resulting in confusing request authorization/header failures at request time.

Fix

Strip all ASCII control characters (0x00-0x1F and 0x7F) from values in save_env_value() before performing other validations or writes. This also handles the previous \\n and \\r removal.

Tests

Adds test_save_env_value_strips_ascii_control_characters to verify NUL, TAB, CR, LF, DEL, etc. are stripped cleanly from both process env and loaded env.

Fixes #55335

save_env_value() only removed '\n' and '\r' from credentials written to
~/.hermes/.env. Other ASCII control characters (C0 range, 0x00-0x1F, and DEL,
0x7F) could still be persisted in API keys and tokens.

On Windows, saving a copied key containing a NUL byte causes os.environ[key] = value
to crash with ValueError: embedded null character, leaving a partially written
and dirty .env file. On other systems, control bytes such as TAB or DEL survive,
resulting in confusing request authorization failures.

Fix: strip all ASCII control characters (0x00-0x1F and 0x7F) from values in
save_env_value() before performing other validations or writes. This also
subsumes the previous '\n' and '\r' removal.

Adds test_save_env_value_strips_ascii_control_characters to verify NUL, TAB, CR,
LF, DEL, etc. are stripped cleanly from both process env and loaded env.

Fixes NousResearch#55335
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data duplicate This issue or pull request already exists labels Jun 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #55336 — same code site (save_env_value() in hermes_cli/config.py) and same mechanism (strip ASCII control chars before writing credentials to ~/.hermes/.env). #55336 (opened ~6h earlier) is the earliest-open canonical fix for #55335. Also related to the earlier competing fix #40887 (keeps TAB, adds *_URL/_HOST/_ENDPOINT validation). Flagging for the maintainer to consolidate.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. The premise is confirmed on current main: save_env_value() still performs only CR/LF normalization at hermes_cli/config.py:7554, writes .env at hermes_cli/config.py:7601, and assigns os.environ afterward at hermes_cli/config.py:7618. The proposed filter runs before both operations and correctly excludes C0 controls plus DEL.

The surrounding file moved since the PR base, but the target statement remains materially unchanged, so salvage is a small mechanical reapplication. The member triage comment and #55336 identify an earlier competing implementation of the same fix; that consolidation choice remains for maintainers.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

save_env_value can persist ASCII control characters in API keys

3 participants