Skip to content

fix(config): strip ASCII control characters from env values before writing to .env - #40887

Open
yubingz wants to merge 2 commits into
NousResearch:mainfrom
yubingz:fix/strip-control-chars-env-values
Open

fix(config): strip ASCII control characters from env values before writing to .env#40887
yubingz wants to merge 2 commits into
NousResearch:mainfrom
yubingz:fix/strip-control-chars-env-values

Conversation

@yubingz

@yubingz yubingz commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #40840

Problem

On Windows, pressing ESC or arrow keys during CLI prompts (hermes setup) inserts raw ANSI control characters (e.g. \x1b, \x1b[A) into input. These characters pass through prompt()save_env_value() and get written to .env, silently corrupting configuration.

For example, a corrupted SEARXNG_URL like http://\x1blocalhost:8888 causes web_search to permanently fail with no clear error message — the user has no idea their config is broken.

Root Cause

  1. Windows input() returns raw escape sequences on ESC/arrow key presses
  2. prompt() only calls .strip() which does not filter control characters
  3. save_env_value() strips non-ASCII but ESC (0x1b) is valid ASCII — it passes through
  4. No URL prefix validation in _configure_provider() to catch malformed values

Fix: Three-Layer Defense

Layer 1 — Input Sanitization (config.py):

  • Added _strip_control_chars(): strips C0 (0x00–0x1F except TAB 0x09) and C1 (0x80–0x9F) control characters
  • TAB is preserved because some tools use it as a delimiter
  • Called in save_env_value() before writing to .env — single point of defense for all env writes

Layer 2 — Business Validation (tools_config.py):

  • Added http(s):// prefix validation in _configure_provider() for env vars ending in _URL, _HOST, or _ENDPOINT
  • Catches malformed URLs that pass character sanitization but lack a valid scheme

Layer 3 — Regression Safety:

  • 14 new tests in test_control_char_stripping.py covering ESC, arrow keys, mixed control chars, TAB preservation, etc.
  • Updated comments in test_non_ascii_credential.py to clarify scope

Test Results

151 passed in 8.77s — 0 regressions

Files Changed

File Change
hermes_cli/config.py +78 lines: _strip_control_chars() + integration in save_env_value()
hermes_cli/tools_config.py +18 lines: URL prefix validation in _configure_provider()
tests/hermes_cli/test_control_char_stripping.py New file: 14 tests
tests/hermes_cli/test_non_ascii_credential.py Updated test comments

@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 tool/web Web search and extraction labels Jun 7, 2026
…iting to .env

Fixes NousResearch#40840

Problem:
On Windows, pressing ESC/arrow keys during CLI prompts inserts ANSI
control characters (e.g. \x1b, \x1b[A) into input. These characters
pass through prompt() → save_env_value() and get written to .env,
silently corrupting configuration. A corrupted SEARXNG_URL (for example)
causes web_search to permanently fail with no clear error message.

Root cause:
- Windows input() returns raw escape sequences on ESC/arrow key presses
- prompt() only calls .strip() which does not filter control characters
- save_env_value() strips non-ASCII but ESC (0x1b) is valid ASCII
- No URL prefix validation in _configure_provider()

Fix (three-layer defense):
1. Add _strip_control_chars() in config.py — strips C0/C1 control chars
   while preserving TAB (0x09) used as delimiter by some tools
2. Call _strip_control_chars() in save_env_value() before writing to .env
3. Add http(s):// prefix validation in _configure_provider() for
   _URL/_HOST/_ENDPOINT environment variables

Tests:
- 14 new tests in test_control_char_stripping.py
- All 151 tests pass, 0 regressions

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing a real Windows configuration-corruption path. Current main still writes all ASCII controls other than LF/CR through save_env_value() (hermes_cli/config.py:7554-7618), while the shared non-secret prompt uses raw input() plus .strip() (hermes_cli/cli_output.py:63-65).

Problems

  • tests/hermes_cli/test_control_char_stripping.py:125, :143, and :160 only inspect the result under if env_path.exists(). Those tests pass if the writer produces no .env file, so they do not prove the persistence behavior.
  • The new URL-validation/retry branch in hermes_cli/tools_config.py:3084-3101 has no regression test. The current SearXNG plugin exposes SEARXNG_URL through this exact provider flow (plugins/web/searxng/provider.py:141-152).

Suggested changes

  • Assert the file exists unconditionally, verify its saved value, and verify the process environment is sanitized too.
  • Add a provider-flow test proving bare ESC is not saved and a valid http(s) retry is accepted.

Automated hermes-sweeper review.

save_env_value("SEARXNG_URL", "\x1b")

# The value written to .env should be empty (ESC stripped)
if env_path.exists():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional makes the integration test pass if save_env_value() returns without creating .env. Assert env_path.exists() unconditionally before reading it, then verify the persisted value (and ideally os.environ); the identical guards below should be changed too.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 14, 2026
…gression tests

- Assert env_path.exists() unconditionally in all integration tests
  (previous `if env_path.exists()` guards let tests pass without proving
  persistence behavior)
- Verify os.environ is sanitized after save_env_value
- Add TestProviderFlowUrlValidation class covering URL validation in
  _configure_provider: bare ESC rejection, valid retry acceptance,
  and scheme-less URL passthrough

Addresses review feedback on PR NousResearch#40887.
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 P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/web Web search and extraction type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Hermes tools wizard writes ESC character (\x1b) to .env as SEARXNG_URL on Windows, causing all web_search calls to fail permanently

3 participants