Skip to content

fix(desktop): stop decoding the HERMES_HOME registry read as utf-8 - #90427

Open
yoggydev wants to merge 1 commit into
NousResearch:mainfrom
yoggydev:fix/windows-user-env-codepage
Open

yoggydev wants to merge 1 commit into
NousResearch:mainfrom
yoggydev:fix/windows-user-env-codepage

Conversation

@yoggydev

Copy link
Copy Markdown

What does this PR do?

readWindowsUserEnvVar() reads HERMES_HOME out of HKCU\Environment and decodes reg.exe's stdout as UTF-8:

stdout = exec('reg', ['query', 'HKCU\\Environment', '/v', name], {
  encoding: 'utf8',
  windowsHide: true,
  timeout: 5000
})

reg.exe is a native console program; its stdout carries the machine code page. The value being read is a filesystem path.

Measured on ja-JP Windows 11 (ACP=932), Node v24.17.0. setx HERMES_PROBE "C:\十能予\hermes", then the same reg query captured as bytes:

raw bytes  83
value hex  43 3a 5c  8f 5c  94 5c  97 5c  5c  6865726d6573
             C  :  \    十     能     予    \      hermes

parsed, utf8   "C:\\\ufffd\\\ufffd\\\ufffd\\\\hermes"
parsed, sjis   "C:\\\u5341\u80fd\u4e88\\hermes"

backslashes    utf8 5    sjis 2
values equal   false

parent exists, utf8   false
parent exists, sjis   true

The three characters each have 0x5C as their CP932 trail byte, and 0x5C is the path separator. So the UTF-8 decode does not shorten the path, it lengthens it by three segments. There are 52 such characters in CPython's CP932 double-byte table, and they are ordinary ones: 表 十 能 予 構 貼.

What it costs. parseRegQueryValue matches on \S+ for the name and takes the rest of the line as the value, so a mangled value parses cleanly. The caller in main.ts is:

const fromRegistry = readWindowsUserEnvVar('HERMES_HOME')

if (fromRegistry) {
  return normalizeHermesHomeRoot(fromRegistry)
}
// LOCALAPPDATA fallback below is never reached

A corrupted string is still truthy, so the desktop resolves HERMES_HOME to a directory that does not exist rather than falling through to %LOCALAPPDATA%\hermes. That is worse than the stale-snapshot gap this function was added to close (#45471): the fallback that would have produced a working home is skipped by a value that only looks valid. parent exists utf8 = false above is that outcome on a real host.

The fix. Node exposes no API for the host ANSI or OEM code page, so rather than guess at one:

  • Take reg's stdout as bytes (drop encoding: 'utf8').
  • If every byte is < 0x80, the code page and UTF-8 agree by definition — decode and return, unchanged behaviour and no extra spawn. This is the path essentially every host takes.
  • If any byte is >= 0x80, the value is in a code page we cannot name. Re-read it through PowerShell base64-encoded from UTF-8 inside the child, so the bytes on the wire are ASCII and no code page is involved:
[Convert]::ToBase64String([Text.Encoding]::UTF8.GetBytes(
  [Environment]::GetEnvironmentVariable('HERMES_HOME','User')))

That is the same trick ui-tui/src/lib/clipboard.ts and hermes_cli/clipboard.py already use for clipboard text, for the same reason.

The trigger is a fact about the bytes, not a judgement about damage. I deliberately did not detect this by looking for U+FFFD: I measured on #89468 that the replacement output is not stable across runtimes (29 replacements on .NET Framework 4.8, 30 on .NET 8 and CPython 3.11 for identical input), so replacement characters are not a sound signal.

The name is checked against /^[A-Za-z_][A-Za-z0-9_]*$/ before it is interpolated into the PowerShell string. Today the only caller passes a literal, but the function is exported.

Note on the linter. scripts/check-windows-footguns.py cannot see this file: should_scan_file() returns True only for .py, .pyw, .pyi. Every .ts under apps/desktop/electron/ is outside the gate that exists to catch exactly this class of bug.

Related Issue

No separate issue — reporting it here with the measurement. Same class as #89878 / #89907 / #90017 / #90046, on the Electron side of the same seam. The Python half of bounded_probe_run is #90046; this is the TypeScript half, and it is independent of that PR.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • apps/desktop/electron/windows-user-env.ts — added isAsciiBytes(); readWindowsUserEnvVar() now captures reg stdout as bytes and only decodes it as UTF-8 when it is pure ASCII.
  • apps/desktop/electron/windows-user-env.ts — added readUserEnvVarAsBase64(), a PowerShell re-read that base64-encodes the UTF-8 bytes inside the child, used only on the non-ASCII path. Env-var names are validated before interpolation.
  • apps/desktop/electron/windows-user-env.test.ts — four tests added: the ASCII fast path spawns only reg; a CP932 value is re-read as base64 and returns the correct path, with the old five-backslash result pinned as an assertion; a failed re-read returns null; a name that is not a plain identifier never reaches PowerShell.

How to Test

  1. On a CP932 host:
mkdir C:\十能予 -Force
setx HERMES_PROBE "C:\十能予\hermes"

Then, in a new shell:

const { execFileSync } = require('node:child_process')
const raw = execFileSync('reg', ['query','HKCU\\Environment','/v','HERMES_PROBE'], { windowsHide: true })
console.log(raw.toString('hex'))
console.log(JSON.stringify(raw.toString('utf8')))
console.log(JSON.stringify(new TextDecoder('shift_jis').decode(raw)))

The backslash count goes from 2 to 5, and fs.statSync on the parent of the UTF-8 result throws. Output above is from that run.

  1. The test file, run directly under node --test with the type annotations stripped — 14 pass (10 pre-existing, unchanged). I did not run it through the repo's vitest runner: the desktop workspace's dependencies are not installed on my host, so npm test -w apps/desktop is not something I can honestly claim to have run.

  2. Against the pre-patch source, keeping the new tests: 2 failed / 12 passed. The other two pass either way on purpose — one pins that the ASCII path still spawns only reg so the common case cannot regress, the other pins the name-validation guard, which is new but not what the bug was.

Note on the checklist below: I left the full-suite box unchecked for the reason in 2.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11, ja-JP (ACP=932), Node v24.17.0

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

I have a ja-JP (ACP=932) Windows host and can measure anything else on that locale.

(Measured on my host. Drafted with Claude.)

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 20, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

  1. apps/desktop/electron/windows-user-env.ts:readUserEnvVarAsBase64 — Nit: on a machine whose HERMES_HOME contains non-ASCII, every read now pays a cold PowerShell spawn (~150-400ms) inside startup paths. Fine as a correctness fallback, but worth remembering if boot-time profiling ever points here — the value rarely changes, so an in-process memo per name would make the second read free.

  2. Same file + tests — Positive: this is the right fix shape for an unfixable decoding problem. Rather than guessing code pages (chcp, kernel32 APIs), it detects the ASCII-safe fast path byte-wise and routes everything else through a channel that carries its own encoding (PowerShell → UTF-8 → base64). The CP932 test case is chosen with real malice — trail bytes of 0x5C surviving a UTF-8 decode as literal backslashes is exactly the failure that makes the old bug insidious (a plausible path with extra separators instead of mojibake) — and pinning the old mangled output as an assertion documents the failure mode permanently. The SAFE_ENV_NAME guard with an injection-attempt test closes the obvious PowerShell interpolation hole, and failure degrades to null so callers keep their existing fallback behavior.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants