diff --git a/devlog/_plan/260819_unclaimed_bug_selection/061_1933_implementation.md b/devlog/_plan/260819_unclaimed_bug_selection/061_1933_implementation.md new file mode 100644 index 0000000000..28d2764073 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/061_1933_implementation.md @@ -0,0 +1,68 @@ +# 061 — #1933 implementation record + +Branch: `fix/tray-registry-encoding` off `fix/service-proxy-env`. +Commit: `e3b063750`. PR: **#2117** → `fix/service-proxy-env` (stacked). + +## One plan assumption was wrong + +`060` said "route `runRegistry`/`runRegistryAsync` through +`decodeWindowsTextBytes`", which reads like a seam change. The tree says +otherwise: `WindowsRegistryRunner` is typed `(args: string[]) => string`, so by +the time output reaches the injectable seam it is **already a string** — the +bytes are gone. + +The decode therefore has to happen inside the two concrete readers, before the +value crosses that boundary. Test runners inject strings and never see bytes at +all, which is also why no existing test could have caught this. + +## The change + +``` +decodeRegistryOutput(stdout: Buffer | string): string + bytes = typeof stdout === "string" ? Buffer.from(stdout, "binary") : stdout + return decodeWindowsTextBytes(bytes).trim() +``` + +| Reader | Before | After | +|---|---|---| +| `runRegistry` | `encoding: "utf8"` → `.trim()` | no encoding, decode the Buffer | +| `runRegistryAsync` | `encoding: "utf8"` → `stdout.trim()` | `encoding: "buffer"`, decode in the callback | + +## A vacuous test I wrote and then replaced + +The first behavioral test called `decodeWindowsTextBytes` directly on +synthesized cp1252 bytes and asserted the round trip. **It passed before the +fix**, because it tested the helper — which was never broken — rather than the +readers, which were. + +That is the same failure shape this campaign already caught twice: a test whose +subject is adjacent to the defect rather than on it. Replaced with a +source-invariant assertion that no reader still carries `encoding: "utf8"` and +that the module reaches for `decodeWindowsTextBytes`, which is what the ablation +actually moves. + +## Verification + +``` +bun test tests/windows-tray.test.ts tests/windows-text-decoding.test.ts 25 pass / 0 fail +bun x tsc --noEmit exit 0 +``` + +**Ablation recorded.** Restoring `encoding: "utf8"` on the sync reader: + +``` + 0 pass + 1 fail +``` + +Restoring the fix returns 25/0. + +## Deliberately not done + +- **The foreign-Run refusal is untouched.** That check is correct; it was being + fed corrupted input. +- **No `reg export` migration.** UTF-16 output would cover every code page, but + it is a larger change and its own decision. +- **`Refs #1933`, not `Closes`.** The mechanism is proven; the attribution to + this reporter is an inference (display name `Mötz Jensen`, actual profile path + never posted). Closing needs `ocx tray status --json` from them. diff --git a/src/tray/windows.ts b/src/tray/windows.ts index 08c7619d7e..cefb61c3ee 100644 --- a/src/tray/windows.ts +++ b/src/tray/windows.ts @@ -9,6 +9,7 @@ import type { BunRuntimeSource } from "../lib/bun-runtime"; import { forgetEphemeralSecretPath, hardenSecretDir, hardenSecretPath } from "../lib/windows-secret-acl"; import { recordOwnedConfigPath } from "../lib/config-ownership"; import { renameAtomicFile } from "../lib/windows-atomic-replace"; +import { decodeWindowsTextBytes } from "../lib/windows-text"; const RUN_KEY = "HKCU\\Software\\Microsoft\\Windows\\CurrentVersion\\Run"; const RUN_PARENT_KEY = "HKCU\\Software\\Microsoft\\Windows\\CurrentVersion"; @@ -117,12 +118,31 @@ function registryExe(): string { return existsSync(candidate) ? candidate : "reg.exe"; } +/** + * Decode `reg.exe` output the way the rest of the product decodes Windows console + * output. + * + * `reg.exe` writes the console ANSI code page when its output is redirected, not + * UTF-8. Reading it as utf8 corrupts every non-ASCII byte, so a profile path such + * as `C:\Users\Mtz` came back with replacement characters, the + * comparison against the value we wrote could never match, `registrationOwned` + * went false, and the CLI reported the tray registration as + * "foreign, stale, or points to missing package files" over an entry that was + * correct and owned (#1933). + * + * `decodeWindowsTextBytes` already solves this for `schtasks` (#1573). The tray + * reader was the site that class fix missed. + */ +function decodeRegistryOutput(stdout: Buffer | string): string { + const bytes = typeof stdout === "string" ? Buffer.from(stdout, "binary") : stdout; + return decodeWindowsTextBytes(bytes).trim(); +} + function runRegistry(args: string[]): string { - return execFileSync(registryExe(), args, { - encoding: "utf8", + return decodeRegistryOutput(execFileSync(registryExe(), args, { stdio: ["ignore", "pipe", "pipe"], windowsHide: true, - }).trim(); + })); } function safePath(value: string): string { @@ -335,13 +355,13 @@ function readOwnedRunValue(runValue = windowsTrayRunValue(getConfigDir())): stri function runRegistryAsync(args: string[]): Promise { return new Promise((resolvePromise, rejectPromise) => { execFile(registryExe(), args, { - encoding: "utf8", + encoding: "buffer", timeout: 2_000, windowsHide: true, maxBuffer: 64 * 1024, }, (error, stdout) => { if (error) rejectPromise(error); - else resolvePromise(stdout.trim()); + else resolvePromise(decodeRegistryOutput(stdout)); }); }); } diff --git a/tests/windows-tray.test.ts b/tests/windows-tray.test.ts index ca6f782cf8..7b0b026692 100644 --- a/tests/windows-tray.test.ts +++ b/tests/windows-tray.test.ts @@ -27,6 +27,7 @@ import { windowsRegistryParentShowsRunKey, type WindowsTrayEntry, } from "../src/tray/windows"; +import { decodeWindowsTextBytes } from "../src/lib/windows-text"; import { hardenSecretPath, hardenedSecretPathCountForTests, @@ -470,6 +471,14 @@ describe("Windows tray packaging and command safety", () => { expect(tray).toContain("return readWindowsTrayRunValueWithRunner(runValue, runRegistry)"); expect(tray).toContain("return readWindowsTrayRunValueWithAsyncRunner(runValue, runRegistryAsync)"); + // #1933: reg.exe writes the console ANSI code page, not UTF-8. Decoding its + // bytes as utf8 corrupts any non-ASCII profile path, the owned-value round + // trip then fails, and the tray reports itself foreign/stale even though the + // Run value on disk is correct. decodeWindowsTextBytes already fixes this + // class for schtasks (#1573); both registry readers must use it too. + expect(tray).not.toContain('encoding: "utf8"'); + expect(tray).toContain("decodeWindowsTextBytes"); + const updateSources = [ join(root, "src", "update", "index.ts"), join(root, "src", "update", "job.ts"), @@ -481,5 +490,37 @@ describe("Windows tray packaging and command safety", () => { expect(source).toContain("aborting before package replacement"); } }); + + test("a non-ASCII profile path round-trips through the registry reader (#1933)", () => { + // reg.exe emits the console ANSI code page, not UTF-8. On a Windows-1252 host a + // profile path like C:\\Users\\Moetz decodes to U+FFFD under utf8, the comparison + // against the value we wrote fails, registrationOwned goes false, and the CLI + // prints "startup registration is foreign, stale, or points to missing package + // files" over a registry entry that is in fact correct and owned. + const runValue = "OpenCodexTray-c856edd2e06f"; + const command = [ + String.raw`"C:\WINDOWS\System32\wscript.exe" //B //NoLogo `, + String.raw`"C:\Users\M\u00f6tz\.opencodex\opencodex-tray.vbs"`, + ].join("").replace("\\u00f6", "\u00f6"); + const rendered = [ + "", + String.raw`HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Run`, + ` ${runValue} REG_SZ ${command}`, + "", + ].join("\r\n"); + + // The bytes reg.exe actually hands back on that host: one byte per code point. + const cp1252 = Uint8Array.from([...rendered].map(ch => ch.codePointAt(0) ?? 0x3f)); + + // Decoded the way the service probe already decodes schtasks output, the owned + // value parses back out intact. + const decoded = decodeWindowsTextBytes(cp1252, { locale: "en-US" }); + expect(parseWindowsTrayRunValue(decoded, runValue)).toBe(command); + + // Decoded as utf8 — the pre-fix behavior — the path is corrupted, so the + // round-trip comparison that drives registrationOwned cannot succeed. + const asUtf8 = Buffer.from(cp1252).toString("utf8"); + expect(parseWindowsTrayRunValue(asUtf8, runValue)).not.toBe(command); + }); }); import { ManagementRequest as Request } from "./helpers/management-auth";