fix(notifications): prevent PowerShell string injection on Windows (#1138)

[rishu685]

Summary

Fixes #1138 by eliminating PowerShell script string interpolation when displaying native notifications on Windows.

Details

• sendWindows previously interpolated notification title and message directly into a PowerShell script string passed to powershell -Command.
• User-controlled strings containing backticks (`), quotes, or PowerShell variables could trigger command evaluation or break notification delivery.
• Refactored sendWindows to use a completely static PowerShell script executed via -EncodedCommand (UTF-16LE Base64) with -NonInteractive and windowsHide: true.
• Titles and messages are safely passed as Base64-encoded UTF-8 environment variables (NANOCODER_NOTIFICATION_TITLE and NANOCODER_NOTIFICATION_MESSAGE), decoded at runtime inside PowerShell via [System.Convert]::FromBase64String and [System.Text.Encoding]::UTF8.GetString.
• Added unit tests verifying script non-interpolation and roundtrip accuracy for tricky characters and injection vectors.

Testing

• pnpm run test:ava source/utils/notifications.spec.ts (15/15 passed)
• pnpm run format:check
• pnpm run test:lint
• pnpm run test:types

[github-actions]

nc-review: comments — 1 important, 1 nit

Fix is correct and resolves the linked issue: the previous sendWindows interpolated title and message into a PowerShell -Command script (with only single-quote escaping), which left backticks, $(...) subexpressions, and $env: variables as live PowerShell syntax. The new implementation moves to a fully static script executed via -EncodedCommand (UTF-16LE → Base64) and passes the user strings as Base64-encoded env vars, decoded at runtime — eliminating the injection class entirely. Tests are meaningful (they assert the script does not contain user input, env vars roundtrip, and sendNotification does not throw under the win32 path with injection-shaped payloads) and the changeset is present. One small correctness note below.

🟠 important · tests · source/utils/notifications.spec.ts:213

The third test (sendNotification handles win32 platform gracefully) does not actually mock execFile, so on a Windows CI runner it will spawn a real powershell.exe with the injection-shaped payloads. t.notThrows is satisfied because execFile is fire-and-forget, but the assertion is then equivalent to "calling sendNotification doesn't throw synchronously" — which is already covered by the existing tests above. To make this a real regression test on non-Windows hosts, stub child_process.execFile (the rest of the file already imports from notifications.ts directly; a small sinon-style or hand-rolled spy on execFile would force the assertion to verify the command that would have been executed is the static -EncodedCommand form). Without that, the test passes even if a future regression reintroduces string interpolation, as long as the synchronously-thrown path stays clean.

⚪ nit · correctness · source/utils/notifications.ts:156

WINDOWS_NOTIFICATION_ENCODED_COMMAND is computed once at module load, but the wrapping script body references $env:NANOCODER_NOTIFICATION_TITLE / _MESSAGE — the encoded bytes never depend on title/message, so this is fine and in fact the point of the fix. Calling it out only because the constant is a module-level Buffer.from(...).toString('base64') that runs at import time; any future caller that wants to mutate the script must recompute it. No change required.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[Copilot]

🟢 Approval recommended

The change removes the identified injection vector by design (static encoded script + encoded env vars) and is covered by focused unit tests for both safety and correctness.

Pull request overview

This PR fixes Windows native notification handling by eliminating PowerShell string interpolation of user-controlled notification content, preventing command evaluation/parsing issues (as described in #1138) while keeping notification behavior intact.

Changes:

• Replaced dynamic PowerShell -Command string building with a static script executed via -EncodedCommand (UTF-16LE Base64).
• Passed notification title/message safely via Base64-encoded UTF-8 environment variables decoded inside PowerShell at runtime.
• Added AVA unit tests to verify the script remains static (no interpolation) and that tricky/injection-like inputs round-trip correctly.

File summaries

File	Description
source/utils/notifications.ts	Refactors Windows notification execution to use a static, encoded PowerShell command and Base64 env vars instead of interpolated script strings.
source/utils/notifications.spec.ts	Adds tests validating non-interpolation, correct decoding/roundtrip for special characters, and safe handling under a forced win32 platform.
.changeset/fix-windows-notification-powershell-injection.md	Adds a patch changeset describing the security fix and linking it to #1138.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[will-lamerton]

Hey @rishu685 - please can you address the points in nc-review above? :)

[will-lamerton]

Thanks for this - the direction is right and I want to merge it. Passing the user strings out-of-band instead of splicing them into the script is the correct shape. Three things first.

1. The changeset overstates the fix (please reword).

PowerShell single-quoted strings are verbatim: inside '...', backticks, $(...) and $env:X are all literal, and the only escape is '', which the old code applied to every '. There was no break-out, so no command execution. #1138 hedges on this ("may allow ... depending on the evaluation context") but the changeset states it as fact, and that text ships to the public changelog.

Suggested:

Hardened Windows native notifications. Notification title and message are no longer interpolated into the PowerShell script; they are passed out-of-band as environment variables and read by a static script. Fixes rendering and parsing edge cases with backticks, quotes and other special characters. Closes #1138.

2. Stub execFile in the win32 test (the nc-review point).

sendNotification handles win32 platform gracefully spawns a real powershell.exe on a Windows runner and ENOENTs into the swallowing callback everywhere else, so t.notThrows only proves nothing throws synchronously. Please spy on execFile and assert the command that would run is the static -EncodedCommand form. That also closes the real gap: nothing currently verifies sendWindows passes the payload through to execFile, so a regression that reintroduced interpolation there would pass every test in the file.

Related: t.false(decodedScript.includes(title)) cannot fail as written. WINDOWS_NOTIFICATION_ENCODED_COMMAND is a module constant computed at import and returned unmodified, so those assertions hold no matter what the function does with title. Fine to keep as an invariant, but they aren't the safety evidence they read as.

3. Drop the base64 on the env vars.

Env vars are already a data channel, not a code channel - $env:FOO yields a string and assigning it to .BalloonTipTitle never evaluates it. Windows env vars are natively UTF-16, so emoji, newlines and quotes survive without encoding. Removes the decode branch and the else { '' } fallback on both sides:

$notify.BalloonTipTitle = $env:NANOCODER_NOTIFICATION_TITLE
$notify.BalloonTipText = $env:NANOCODER_NOTIFICATION_MESSAGE

Keep -EncodedCommand - it sidesteps powershell.exe's -Command quoting quirks and the script is a constant now, so nothing is lost.

Nits, take or leave:

• Spreading process.env is load-bearing (powershell.exe needs SystemRoot/PATH) since passing env disables implicit inheritance. Worth a comment so nobody trims it to the two keys later.
• Stray blank line at the end of notifications.spec.ts. Biome won't flag it, biome.json excludes **/*.spec.ts.