fix: do not use electron argv parsing with ELECTRON_RUN_AS_NODE - #2604
fix: do not use electron argv parsing with ELECTRON_RUN_AS_NODE#2604nirmal-shaji wants to merge 1 commit into
Conversation
When ELECTRON_RUN_AS_NODE is set, Electron runs as plain Node: the script path is argv[1], exactly as with `node script.js [...args]`. However process.versions.electron is still truthy, so commander auto-selected the 'electron' argument convention and, because process.defaultApp is undefined, treated the script path as a user argument. Guard the auto-detection so 'electron' is only chosen when the process is actually running as an Electron app (ELECTRON_RUN_AS_NODE not set). Fixes tj#2603
nrps9909
left a comment
There was a problem hiding this comment.
Reviewed 3ac5c5d01fa91090218b8c4ed10663616cd03825.
Confirmed the reported script-path leak before the change and checked nine parsing cases on this head: unset/empty ELECTRON_RUN_AS_NODE, non-empty 1/0/false, explicit from: 'electron'/'node'/'user', and a supplied argv array. All matched the expected arguments. Using truthiness here also matches Electron's non-empty environment-value check in its macOS entry point; restricting the value to '1' would miss other enabled values.
npm test: 1,373 passed, one skipped. npm run check: types, lint and formatting passed (Node 26.8.1, macOS). I cleared inherited color variables for the full test run: the existing configureOutput tests otherwise fail with my NO_COLOR=1; that also reproduces independently on develop and is outside this change.
No blocking findings in this auto-detection change. The additional parsing probe mocks Electron process metadata, consistent with the existing test file; it is not a real Electron process launch.
|
To be clear: is this Pull Request entirely AI/LLM generated? |
|
This account opened multiple PR on the same day, the PR shows heavy use of AI, and does not follow the contributing guidelines. And in addition, giving the person who reported the problem first choice for fixing the problem! |
Summary
Command.parse()mis-parsesargvwhenELECTRON_RUN_AS_NODEis set (#2603).When an Electron main process spawns a JS child with
ELECTRON_RUN_AS_NODE=1, the child runs as plain Node — the argv layout isElectron script.js [...args], exactly likenode script.js [...args], soscript.jsis the script path, not a user argument. Butprocess.versions.electronis still truthy, so auto-detection pickedfrom: 'electron'; and becauseprocess.defaultAppisundefined, the script path was kept as a user argument (argv.slice(1)).Fix
Only auto-select the
electronconvention when the process is actually running as an Electron app, i.e. whenELECTRON_RUN_AS_NODEis not set:This is intentionally minimal and only touches auto-detection; passing
{ from: 'electron' }explicitly is unchanged.On the fragility
I saw @shadowspawn's note that this heuristic is inherently fragile — we can't know at parse time whether the executable was launched with the env var or it was set afterwards. Agreed. This guard doesn't try to solve that general case; it aligns auto-detection with the documented meaning of
ELECTRON_RUN_AS_NODE("run as a normal Node.js process"), which covers the common case in the report (env var set as part of thespawn). It's a small, spec-consistent improvement over unconditionally trustingprocess.versions.electron, and explicitfromremains available for anything more exotic.Test
Added a case to
tests/command.parse.test.jsalongside the existing electron auto-detection test: withprocess.versions.electronandELECTRON_RUN_AS_NODEset andargv = 'node script.js user', the script path is excluded andprogram.argsis['user']. Confirmed it fails before the fix and passes after.npm test(node --test) green — 1373 passing.Fixes #2603