Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: error invalid arg type on WSL systems on url open #118

Closed
106 changes: 62 additions & 44 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
})

// Create error here so we have a more useful stack trace when rejecting
const closeError = new Error('command failed')

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - Linux - 16.14.0

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - Linux - 16.x

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - Linux - 18.x

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - Linux - 20.x

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - macOS - 16.14.0

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - macOS - 16.x

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - Linux - 18.0.0

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - Linux - 22.x

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - macOS - 22.x

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - macOS - 18.0.0

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - macOS - 20.x

command failed

Check failure on line 22 in lib/index.js

View workflow job for this annotation

GitHub Actions / Test - macOS - 18.x

command failed

const stdout = []
const stderr = []
Expand Down Expand Up @@ -65,87 +65,105 @@

const spawnWithShell = (cmd, args, opts, extra) => {
let command = opts.shell
let realArgs = []
let script = cmd
// if shell is set to true, we use a platform default. we can't let the core
// spawn method decide this for us because we need to know what shell is in use
// ahead of time so that we can escape arguments properly. we don't need coverage here.
if (command === true) {
if (opts.isWSL) {
command = 'powershell.exe'
realArgs = ['-Command']
const escapedArgs = args.map(arg => `'${arg.replace(/'/g, "''")}'`)
script = `Start-Process -FilePath '$[cmd]' -ArgumentList ${escapedArgs.join(', ')}`
realArgs.push('-Command', script)
Fixed Show fixed Hide fixed
} else if (command === true) {
// istanbul ignore next
command = process.platform === 'win32' ? process.env.ComSpec : 'sh'
command = process.platform === 'win32' ? 'cmd.exe' : 'sh'
script = `$[cmd] ${args.join(' ')}`
Fixed Show fixed Hide fixed
realArgs = ['/c', script]
Fixed Show fixed Hide fixed
}

const options = { ...opts, shell: false }
const realArgs = []
let script = cmd

// first, determine if we're in windows because if we are we need to know if we're
// running an .exe or a .cmd/.bat since the latter requires extra escaping
const isCmd = /(?:^|\\)cmd(?:\.exe)?$/i.test(command)
if (isCmd) {
let doubleEscape = false

// find the actual command we're running
let initialCmd = ''
let insideQuotes = false
for (let i = 0; i < cmd.length; ++i) {
const char = cmd.charAt(i)
if (char === ' ' && !insideQuotes) {
break
if (!opts.isWSL) {
if (isCmd) {
let doubleEscape = false

// find the actual command we're running
let initialCmd = ''
let insideQuotes = false
for (let i = 0; i < cmd.length; ++i) {
const char = cmd.charAt(i)
if (char === ' ' && !insideQuotes) {
break
}

initialCmd += char
if (char === '"' || char === "'") {
insideQuotes = !insideQuotes
}
}

initialCmd += char
if (char === '"' || char === "'") {
insideQuotes = !insideQuotes
let pathToInitial
try {
pathToInitial = which.sync(initialCmd, {
path: (options.env && findInObject(options.env, 'PATH')) || process.env.PATH,
pathext: (options.env && findInObject(options.env, 'PATHEXT')) || process.env.PATHEXT,
}).toLowerCase()
} catch (err) {
pathToInitial = initialCmd.toLowerCase()
}
}

let pathToInitial
try {
pathToInitial = which.sync(initialCmd, {
path: (options.env && findInObject(options.env, 'PATH')) || process.env.PATH,
pathext: (options.env && findInObject(options.env, 'PATHEXT')) || process.env.PATHEXT,
}).toLowerCase()
} catch (err) {
pathToInitial = initialCmd.toLowerCase()
}

doubleEscape = pathToInitial.endsWith('.cmd') || pathToInitial.endsWith('.bat')
for (const arg of args) {
script += ` ${escape.cmd(arg, doubleEscape)}`
}
realArgs.push('/d', '/s', '/c', script)
options.windowsVerbatimArguments = true
} else {
for (const arg of args) {
script += ` ${escape.sh(arg)}`
doubleEscape = pathToInitial.endsWith('.cmd') || pathToInitial.endsWith('.bat')
for (const arg of args) {
script += ` ${escape.cmd(arg, doubleEscape)}`
}
realArgs.push('/d', '/s', '/c', script)
Fixed Show fixed Hide fixed
options.windowsVerbatimArguments = true
} else {
for (const arg of args) {
script += ` ${escape.sh(arg)}`
}
realArgs.push('-c', script)
Fixed Show fixed Hide fixed
}
realArgs.push('-c', script)
}

return promiseSpawn(command, realArgs, options, extra)
}

// open a file with the default application as defined by the user's OS
const open = (_args, opts = {}, extra = {}) => {
const options = { ...opts, shell: true }
const args = [].concat(_args)

let platform = process.platform
let isWSL = false
// process.platform === 'linux' may actually indicate WSL, if that's the case
// we want to treat things as win32 anyway so the host can open the argument
if (platform === 'linux' && os.release().toLowerCase().includes('microsoft')) {
platform = 'win32'

if (platform === 'linux') {
const osRelease = os.release().toLowerCase()
if (osRelease.includes('microsoft') || osRelease.includes('wsl')) {
platform = 'win32'
isWSL = true
}
}

const options = { ...opts, shell: true, isWSL }
let command = options.command
if (!command) {
if (platform === 'win32') {
if (isWSL) {
command = 'powershell.exe'
} else if (platform === 'win32') {
// spawnWithShell does not do the additional os.release() check, so we
// have to force the shell here to make sure we treat WSL as windows.
options.shell = process.env.ComSpec
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting options.shell was load bearing here, spawnWithShell needs that behavior.

// options.shell = process.env.ComSpec
// also, the start command accepts a title so to make sure that we don't
// accidentally interpret the first arg as the title, we stick an empty
// string immediately after the start command
command = 'start ""'
command = process.env.ComSpec || 'cmd.exe'
} else if (platform === 'darwin') {
command = 'open'
} else {
Expand Down
Loading