fix: reject paths containing % on windows (#155)

fixes GHSA-698v-4r3x-3vcv

Author: sapphi-red

packages/launch-editor/construct-windows-launch-command.js

@@ -19,7 +19,9 @@
 
 // According to https://ss64.com/nt/syntax-esc.html,
 // we can use `^` to escape `&`, `<`, `>`, `|`, `%`, and `^`
-// I'm not sure if we have to escape all of these, but let's do it anyway
+//
+// `"` cannot appear in Windows paths, so it is outside the untrusted path input
+// this sanitization protects.
 function escapeCmdArgs(cmdArgs) {
   return cmdArgs.replace(/([&|<>,;=^])/g, '^$1')
 }
@@ -41,6 +43,18 @@ function doubleQuoteIfNeeded(str) {
 }
 
 function constructWindowsLaunchCommand(editor, args) {
+  // CMD treats line breaks as command separators. They cannot appear in regular Windows
+  // file names, but NTFS alternate data stream names may contain control characters.
+  if (args.some((arg) => /[\r\n]/.test(arg))) {
+    throw new Error('Cannot launch an editor with an argument containing a line break')
+  }
+
+  // CMD expands expressions such as `%VAR:old=new%` before consuming caret escapes, so
+  // percent signs in arguments cannot be preserved safely and are rejected.
+  if (args.some((arg) => arg.includes('%'))) {
+    throw new Error('Cannot launch an editor with an argument containing "%"')
+  }
+
   return [editor, ...args.map(escapeCmdArgs)].map(doubleQuoteIfNeeded).join(' ')
 }
 

packages/launch-editor/construct-windows-launch-command.test.js

@@ -40,6 +40,32 @@ describe('constructWindowsLaunchCommand', () => {
     )
   })
 
+  for (const [name, lineBreak] of [
+    ['carriage returns', '\r'],
+    ['line feeds', '\n'],
+  ]) {
+    test(`rejects ${name} in arguments`, () => {
+      assert.throws(
+        () => constructWindowsLaunchCommand('code', [`C:\\project\\file.js${lineBreak}calc.exe`]),
+        /argument containing a line break/,
+      )
+    })
+  }
+
+  test('rejects percent signs in arguments', () => {
+    assert.throws(
+      () => constructWindowsLaunchCommand('code', ['C:\\project\\100%\\file.js']),
+      /argument containing "%"/,
+    )
+  })
+
+  test('rejects CMD variable modifiers', () => {
+    assert.throws(
+      () => constructWindowsLaunchCommand('code', ['C:\\project\\%VAR:old=new%']),
+      /argument containing "%"/,
+    )
+  })
+
   test('quotes arguments containing both spaces and escaped CMD metacharacters', () => {
     assert.equal(
       constructWindowsLaunchCommand('code', ['C:\\my project\\&file.js']),

packages/launch-editor/index.js

@@ -93,6 +93,16 @@ function launchEditor(file, specifiedEditor, onErrorCallback) {
   const extraArgs = getArgumentsForFile(editor, fileName, lineNumber, columnNumber)
   args.push.apply(args, extraArgs)
 
+  let launchCommand
+  if (process.platform === 'win32') {
+    try {
+      launchCommand = constructWindowsLaunchCommand(editor, args)
+    } catch (error) {
+      onErrorCallback(fileName, error.message)
+      return
+    }
+  }
+
   if (currentChildProcess && isTerminalEditor(editor)) {
     // There's an existing editor process already and it's attached
     // to the terminal, so go kill it. Otherwise two separate editor
@@ -101,7 +111,6 @@ function launchEditor(file, specifiedEditor, onErrorCallback) {
   }
 
   if (process.platform === 'win32') {
-    const launchCommand = constructWindowsLaunchCommand(editor, args)
     currentChildProcess = childProcess.exec(launchCommand, {
       stdio: 'inherit',
       shell: true,