fix(cli): sanitize NODE_OPTIONS to prevent --localstorage-file warning #1078
Conversation
Fixes #1077 When NODE_OPTIONS contains --localstorage-file without a valid path, Node.js emits a warning before any JavaScript code runs. This commonly happens when IDEs like VSCode set NODE_OPTIONS in the environment. This change sanitizes NODE_OPTIONS at multiple levels: - Shell wrapper script (sanitize-node-options.sh) for npm start/debug - Shell launcher (llxprt.sh) for the published CLI binary - relaunchAppInChildProcess() for relaunched processes The sanitization removes --localstorage-file flags (with or without values) while preserving other NODE_OPTIONS.
WalkthroughThe PR adds NODE_OPTIONS sanitization across startup layers: a shell wrapper used by npm scripts, a Bash wrapper for the CLI, an exported TypeScript sanitizer used when relaunching a child Node process, and test coverage for both shell and TypeScript implementations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NPM_Script as "npm / package.json script"
participant Shell_Wrapper as "./scripts/sanitize-node-options.sh"
participant CLI_Wrapper as "packages/cli/llxprt.sh"
participant Node_Process as "node (parent)"
participant Relaunch_Module as "relaunch.ts"
participant Child_Node as "node (child)"
User->>NPM_Script: run `npm start` / `npm run debug`
NPM_Script->>Shell_Wrapper: exec with original args
Shell_Wrapper->>Shell_Wrapper: sanitize NODE_OPTIONS (remove --localstorage-file)
Shell_Wrapper->>Node_Process: exec node with sanitized env
Node_Process->>Relaunch_Module: detect relaunch needed -> sanitizeNodeOptions(process.env.NODE_OPTIONS)
Relaunch_Module->>Child_Node: spawn child with sanitized NODE_OPTIONS + LLXPRT_CODE_NO_RELAUNCH
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
the
maintainer:e2e:ok
|
> <function_parameters> <function_results> Todo Progress<function_results> |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/cli/llxprt.sh (2)
5-10: Address ShellCheck warnings from pipeline.The pipeline reports ShellCheck warnings that should be fixed:
- SC2292: Use
[[ ]]instead of[ ]for tests in Bash- SC2250: Use braces around variable references
♻️ Proposed fix
-if [ -n "$NODE_OPTIONS" ]; then +if [[ -n "${NODE_OPTIONS}" ]]; then # Remove --localstorage-file with optional value (but don't consume following flags starting with -) # Handles: --localstorage-file, --localstorage-file=value, --localstorage-file value - NODE_OPTIONS=$(echo "$NODE_OPTIONS" | sed -E 's/(^|[[:space:]])--localstorage-file(=[^[:space:]]*|[[:space:]]+[^-][^[:space:]]*)?//g' | sed -E 's/[[:space:]]+/ /g' | sed -E 's/^[[:space:]]+|[[:space:]]+$//g') + NODE_OPTIONS=$(echo "${NODE_OPTIONS}" | sed -E 's/(^|[[:space:]])--localstorage-file(=[^[:space:]]*|[[:space:]]+[^-][^[:space:]]*)?//g' | sed -E 's/[[:space:]]+/ /g' | sed -E 's/^[[:space:]]+|[[:space:]]+$//g') export NODE_OPTIONS fi
16-16: Add braces around variable reference.ShellCheck SC2250 also applies here.
♻️ Proposed fix
-exec node --no-deprecation "$SCRIPT_DIR/dist/index.js" "$@" +exec node --no-deprecation "${SCRIPT_DIR}/dist/index.js" "$@"scripts/sanitize-node-options.sh (2)
5-10: Address ShellCheck warnings from pipeline.Same ShellCheck warnings as in
llxprt.shapply here (SC2292, SC2250).♻️ Proposed fix
-if [ -n "$NODE_OPTIONS" ]; then +if [[ -n "${NODE_OPTIONS}" ]]; then # Remove --localstorage-file with optional value (but don't consume following flags starting with -) # Handles: --localstorage-file, --localstorage-file=value, --localstorage-file value - SANITIZED=$(echo "$NODE_OPTIONS" | sed -E 's/(^|[[:space:]])--localstorage-file(=[^[:space:]]*|[[:space:]]+[^-][^[:space:]]*)?//g' | sed -E 's/[[:space:]]+/ /g' | sed -E 's/^[[:space:]]+|[[:space:]]+$//g') - export NODE_OPTIONS="$SANITIZED" + SANITIZED=$(echo "${NODE_OPTIONS}" | sed -E 's/(^|[[:space:]])--localstorage-file(=[^[:space:]]*|[[:space:]]+[^-][^[:space:]]*)?//g' | sed -E 's/[[:space:]]+/ /g' | sed -E 's/^[[:space:]]+|[[:space:]]+$//g') + export NODE_OPTIONS="${SANITIZED}" fi
1-13: Consider extracting shared sanitization logic.The sed pipeline for sanitizing
NODE_OPTIONSis duplicated between this script andpackages/cli/llxprt.sh. While the duplication is minor, if the sanitization logic needs updating, both files must be changed.You could source a shared function or have
llxprt.shinvoke this script, but given the simplicity, this is optional.scripts/tests/sanitize-node-options.test.js (1)
79-86: Consider capturing stderr to verify no warning is emitted.This test verifies successful execution but doesn't actually assert that stderr is empty. Warnings would go to stderr and the test would still pass.
♻️ Suggested improvement to capture and verify stderr
it('should allow node to run without warning when NODE_OPTIONS has --localstorage-file', () => { - // This test verifies that node runs successfully without the warning - const result = runWithNodeOptions( - '--localstorage-file', - 'node -e "console.log(\\"success\\")"' - ); - expect(result).toBe('success'); + // Verify node runs without emitting warnings to stderr + const env = { ...process.env, NODE_OPTIONS: '--localstorage-file' }; + const result = execSync(`${scriptPath} node -e "console.log('success')"`, { + env, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'] + }); + expect(result.trim()).toBe('success'); });Note: You'd also need to capture and assert on stderr separately if using
spawnSyncinstead, or use the{ stdio: 'pipe' }option with error handling.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.jsonpackages/cli/llxprt.shpackages/cli/package.jsonpackages/cli/src/utils/relaunch.test.tspackages/cli/src/utils/relaunch.tsscripts/sanitize-node-options.shscripts/tests/sanitize-node-options.test.js
🧰 Additional context used
🧬 Code graph analysis (2)
packages/cli/src/utils/relaunch.test.ts (1)
packages/cli/src/utils/relaunch.ts (2)
relaunchAppInChildProcess(43-64)sanitizeNodeOptions(22-31)
packages/cli/src/utils/relaunch.ts (1)
scripts/start.js (1)
sanitizedNodeOptions(107-107)
🪛 GitHub Actions: LLxprt Code CI
packages/cli/llxprt.sh
[warning] 5-5: ShellCheck warning: Prefer [[ ]] over [ ] for tests in Bash/Ksh/Busybox. [SC2292]
[warning] 5-5: ShellCheck warning: Prefer putting braces around variable references even when not strictly required. [SC2250]
[warning] 16-16: ShellCheck warning: Prefer putting braces around variable references even when not strictly required. [SC2250]
scripts/sanitize-node-options.sh
[warning] 5-5: ShellCheck warning: Prefer [[ ]] over [ ] for tests in Bash/Ksh/Busybox. [SC2292]
[warning] 5-5: ShellCheck warning: Prefer putting braces around variable references even when not strictly required. [SC2250]
[warning] 8-8: ShellCheck warning: Prefer putting braces around variable references even when not strictly required. [SC2250]
[warning] 9-9: ShellCheck warning: Prefer putting braces around variable references even when not strictly required. [SC2250]
🪛 GitHub Check: CodeQL
scripts/tests/sanitize-node-options.test.js
[warning] 24-24: Shell command built from environment values
This shell command depends on an uncontrolled absolute path.
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
🔇 Additional comments (9)
packages/cli/package.json (2)
12-14: Verify Windows compatibility for shell wrapper bin entry.Pointing the
binentry to a.shfile works on Unix systems but may not work natively on Windows (cmd.exe, PowerShell). npm typically handles this by creating a.cmdwrapper, but the shell-specific sanitization logic won't execute.Consider whether Windows users need the same
--localstorage-filesanitization, or if this is primarily an issue on Unix environments where IDEs set NODE_OPTIONS.
32-35: LGTM!The
filesarray correctly includes bothdist(the compiled JavaScript) andllxprt.sh(the new wrapper script) for publishing.packages/cli/src/utils/relaunch.test.ts (2)
177-228: LGTM! Comprehensive test coverage for sanitizeNodeOptions.The tests cover the key scenarios:
- Undefined/empty input
- All three
--localstorage-fileforms (bare,=value, space-separated value)- Preservation of surrounding options
- Protection against consuming following flags as values
- Whitespace normalization
139-174: LGTM!Good integration tests verifying that
relaunchAppInChildProcessproperly sanitizesNODE_OPTIONSbefore spawning the child process.package.json (1)
24-26: LGTM!Wrapping the
startanddebugscripts with the sanitization script ensuresNODE_OPTIONSis cleaned before Node.js parses it at startup. The order (sanitize → cross-env → node) is correct. The script is executable and properly tracked in git.packages/cli/src/utils/relaunch.ts (2)
14-31: LGTM! Well-designed sanitization logic.The regex correctly handles all
--localstorage-filevariants:
- Bare flag (
--localstorage-file)- Equals-style value (
--localstorage-file=/path)- Space-separated value (
--localstorage-file /path)The negative lookahead
(?!-)ensures subsequent flags aren't mistakenly consumed as values. The whitespace normalization andundefinedreturn for empty results are appropriate.
47-52: LGTM! Clean integration with sanitization.The sanitization is correctly applied before spawning the child process. Setting
NODE_OPTIONS: sanitizedNodeOptions(which can beundefined) properly handles removal of the variable when the sanitized result is empty.scripts/tests/sanitize-node-options.test.js (2)
15-31: CodeQL warning is a false positive; test helper is acceptable.The static analysis flags
scriptPathas "uncontrolled," but it's derived fromimport.meta.urlwhich is the trusted path of this module file—not external input.The string interpolation of
commandinto the shell string is a minor code smell, but acceptable here since all test inputs are hardcoded literals controlled by the test author.
33-77: Good test coverage for sanitization scenarios.The tests comprehensively cover all
--localstorage-filevariants and preservation of surrounding options.
| } | ||
|
|
||
| try { | ||
| return execSync(`${scriptPath} bash -c '${command}'`, { |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, to fix this kind of issue you should avoid building a single shell command string that mixes paths and dynamic values. Instead, either (1) call the program directly with execFileSync or spawn and pass arguments as an array, or (2) if you must use a shell, provide the command and its arguments via the execFileSync/spawn interface rather than interpolating them into one string.
For this specific code, the best fix with minimal functional change is to replace execSync’s single-string command with a call that passes the script path and its arguments as an array. The shell semantics we need are: run sanitize-node-options.sh with arguments bash, -c, and the provided command string. That is equivalent to invoking execSync with scriptPath as the file to execute and ['bash', '-c', command] as the arguments. This preserves behavior (including use of bash -c and the current command string) while preventing scriptPath from being embedded inside a larger shell command, and it stops the shell from re-parsing scriptPath. Concretely, in scripts/tests/sanitize-node-options.test.js, change line 24 to call execSync(scriptPath, ['bash', '-c', command], { ... }) instead of using a template string. No new imports or helper functions are needed.
| @@ -21,7 +21,7 @@ | ||
| } | ||
|
|
||
| try { | ||
| return execSync(`${scriptPath} bash -c '${command}'`, { | ||
| return execSync(scriptPath, ['bash', '-c', command], { | ||
| env, | ||
| encoding: 'utf-8', | ||
| }).trim(); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/sanitize-node-options.sh (1)
1-13: LGTM - Clean shell wrapper with correct sanitization logic.The sed pipeline correctly handles all
--localstorage-filevariants (no value,=value, and space-separated value) while preserving other NODE_OPTIONS flags. The[^-]guard prevents accidentally consuming the next flag when no value follows.💡 Optional: Split sed pipeline for readability
if [[ -n "${NODE_OPTIONS}" ]]; then # Remove --localstorage-file with optional value (but don't consume following flags starting with -) # Handles: --localstorage-file, --localstorage-file=value, --localstorage-file value - SANITIZED=$(echo "${NODE_OPTIONS}" | sed -E 's/(^|[[:space:]])--localstorage-file(=[^[:space:]]*|[[:space:]]+[^-][^[:space:]]*)?//g' | sed -E 's/[[:space:]]+/ /g' | sed -E 's/^[[:space:]]+|[[:space:]]+$//g') + SANITIZED="${NODE_OPTIONS}" + # Remove --localstorage-file with optional value + SANITIZED=$(echo "${SANITIZED}" | sed -E 's/(^|[[:space:]])--localstorage-file(=[^[:space:]]*|[[:space:]]+[^-][^[:space:]]*)?//g') + # Collapse multiple spaces and trim + SANITIZED=$(echo "${SANITIZED}" | sed -E 's/[[:space:]]+/ /g; s/^[[:space:]]+|[[:space:]]+$//g') export NODE_OPTIONS="${SANITIZED}" fiscripts/tests/sanitize-node-options.test.js (1)
33-96: Consider adding test coverage for multiple occurrences.The shell script's
gflag handles multiple--localstorage-fileentries, but this isn't explicitly tested. Also, the warning verification test could be more robust by checking stderr.💡 Suggested additional tests
it('should remove multiple --localstorage-file occurrences', () => { const result = runWithNodeOptions( '--localstorage-file --max-old-space-size=4096 --localstorage-file=/path', ); expect(result).toBe('--max-old-space-size=4096'); }); it('should produce no stderr warning when running node', () => { const env = { ...process.env, NODE_OPTIONS: '--localstorage-file' }; const result = execSync( `${scriptPath} node -e "console.log('success')"`, { env, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] } ); // If this throws or includes warning text, the sanitization failed expect(result.trim()).toBe('success'); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/cli/llxprt.shpackages/cli/src/utils/relaunch.test.tsscripts/sanitize-node-options.shscripts/tests/sanitize-node-options.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/utils/relaunch.test.ts
- packages/cli/llxprt.sh
🧰 Additional context used
🪛 GitHub Check: CodeQL
scripts/tests/sanitize-node-options.test.js
[warning] 24-24: Shell command built from environment values
This shell command depends on an uncontrolled absolute path.
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
🔇 Additional comments (1)
scripts/tests/sanitize-node-options.test.js (1)
23-31: CodeQL warning is a false positive in this test context.The
scriptPathis derived from the module's own location with hardcoded path segments, and allcommandarguments are hardcoded test literals - there's no user input injection risk here.
Summary
Fixes #1077
When NODE_OPTIONS contains
--localstorage-filewithout a valid path, Node.js emits a warning before any JavaScript code runs. This commonly happens when IDEs like VSCode set NODE_OPTIONS in the environment.Changes
This change sanitizes NODE_OPTIONS at multiple levels:
scripts/sanitize-node-options.sh) - used bynpm startandnpm run debugto strip--localstorage-filebefore invoking nodepackages/cli/llxprt.sh) - the published CLI binary now uses a bash wrapper that sanitizes NODE_OPTIONS before invoking the node scriptThe sanitization removes
--localstorage-fileflags (with or without values) while preserving other NODE_OPTIONS.Testing
sanitizeNodeOptions()function inrelaunch.test.tsscripts/tests/sanitize-node-options.test.jsNODE_OPTIONS=--localstorage-file ./packages/cli/llxprt.sh --helpconfirms no warning is emittedSummary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.