Ensure escaped string are parsed in NODE_OPTIONS (vercel#65046)

timneutkens · wyattjoh · Ethan-Arrowood · web-flow · commit ae1fe5690b67 · 2024-04-26T10:56:33.000+02:00
## What? Ensures paths that have spaces in them in `NODE_OPTIONS` are handled. An example of that is VS Code's debugger which adds: ``` --require "/Applications/Visual Studio Code.app/Contents/Resources/app/extensions/ms-vscode.js-debug/src/bootloader.js" ``` Currently the output is cut off and causes: `invalid value for NODE_OPTIONS (unterminated string)`. Related issue: vercel#63740  Closes NEXT-3226 --------- Co-authored-by: Wyatt Johnson <accounts+github@wyattjoh.ca> Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
diff --git a/packages/next/src/server/lib/utils.test.ts b/packages/next/src/server/lib/utils.test.ts
@@ -1,6 +1,8 @@
 import {
   getFormattedNodeOptionsWithoutInspect,
   getParsedDebugAddress,
+  formatNodeOptions,
+  tokenizeArgs,
 } from './utils'
 
 const originalNodeOptions = process.env.NODE_OPTIONS
@@ -9,6 +11,48 @@ afterAll(() => {
   process.env.NODE_OPTIONS = originalNodeOptions
 })
 
+describe('tokenizeArgs', () => {
+  it('splits arguments by spaces', () => {
+    const result = tokenizeArgs('--spaces "thing with spaces" --normal 1234')
+
+    expect(result).toEqual([
+      '--spaces',
+      'thing with spaces',
+      '--normal',
+      '1234',
+    ])
+  })
+
+  it('supports quoted values', () => {
+    const result = tokenizeArgs(
+      '--spaces "thing with spaces" --spacesAndQuotes "thing with \\"spaces\\"" --normal 1234'
+    )
+
+    expect(result).toEqual([
+      '--spaces',
+      'thing with spaces',
+      '--spacesAndQuotes',
+      'thing with "spaces"',
+      '--normal',
+      '1234',
+    ])
+  })
+})
+
+describe('formatNodeOptions', () => {
+  it('wraps values with spaces in quotes', () => {
+    const result = formatNodeOptions({
+      spaces: 'thing with spaces',
+      spacesAndQuotes: 'thing with "spaces"',
+      normal: '1234',
+    })
+
+    expect(result).toBe(
+      '--spaces="thing with spaces" --spacesAndQuotes="thing with \\"spaces\\"" --normal=1234'
+    )
+  })
+})
+
 describe('getParsedDebugAddress', () => {
   it('supports the flag with an equal sign', () => {
     process.env.NODE_OPTIONS = '--inspect=1234'
@@ -38,6 +82,26 @@ describe('getFormattedNodeOptionsWithoutInspect', () => {
     expect(result).toBe('--other')
   })
 
+  it('handles options with spaces', () => {
+    process.env.NODE_OPTIONS =
+      '--other --inspect --additional --spaces "/some/path with spaces"'
+    const result = getFormattedNodeOptionsWithoutInspect()
+
+    expect(result).toBe(
+      '--other --additional --spaces="/some/path with spaces"'
+    )
+  })
+
+  it('handles options with quotes', () => {
+    process.env.NODE_OPTIONS =
+      '--require "./file with spaces to-require-with-node-require-option.js"'
+    const result = getFormattedNodeOptionsWithoutInspect()
+
+    expect(result).toBe(
+      '--require="./file with spaces to-require-with-node-require-option.js"'
+    )
+  })
+
   it('removes --inspect option with parameters', () => {
     process.env.NODE_OPTIONS = '--other --inspect=0.0.0.0:1234 --additional'
     const result = getFormattedNodeOptionsWithoutInspect()
diff --git a/packages/next/src/server/lib/utils.ts b/packages/next/src/server/lib/utils.ts
@@ -16,45 +16,113 @@ const parseNodeArgs = (args: string[]) => {
 
   // For the `NODE_OPTIONS`, we support arguments with values without the `=`
   // sign. We need to parse them manually.
-  let found = null
+  let orphan = null
   for (let i = 0; i < tokens.length; i++) {
     const token = tokens[i]
 
     if (token.kind === 'option-terminator') {
       break
     }
 
-    // If we haven't found a possibly orphaned option, we need to look for one.
-    if (!found) {
-      if (token.kind === 'option' && typeof token.value === 'undefined') {
-        found = token
-      }
+    // When we encounter an option, if it's value is undefined, we should check
+    // to see if the following tokens are positional parameters. If they are,
+    // then the option is orphaned, and we can assign it.
+    if (token.kind === 'option') {
+      orphan = typeof token.value === 'undefined' ? token : null
+      continue
+    }
 
+    // If the token isn't a positional one, then we can't assign it to the found
+    // orphaned option.
+    if (token.kind !== 'positional') {
+      orphan = null
       continue
     }
 
-    // If the next token isn't a positional value, then it's truly orphaned.
-    if (token.kind !== 'positional' || !token.value) {
-      found = null
+    // If we don't have an orphan, then we can skip this token.
+    if (!orphan) {
       continue
     }
 
-    // We found an orphaned option. Let's add it to the values.
-    values[found.name] = token.value
-    found = null
+    // If the token is a positional one, and it has a value, so add it to the
+    // values object. If it already exists, append it with a space.
+    if (orphan.name in values && typeof values[orphan.name] === 'string') {
+      values[orphan.name] += ` ${token.value}`
+    } else {
+      values[orphan.name] = token.value
+    }
   }
 
   return values
 }
 
+/**
+ * Tokenizes the arguments string into an array of strings, supporting quoted
+ * values and escaped characters.
+ * Converted from: https://github.com/nodejs/node/blob/c29d53c5cfc63c5a876084e788d70c9e87bed880/src/node_options.cc#L1401
+ *
+ * @param input The arguments string to be tokenized.
+ * @returns An array of strings with the tokenized arguments.
+ */
+export const tokenizeArgs = (input: string): string[] => {
+  let args: string[] = []
+  let isInString = false
+  let willStartNewArg = true
+
+  for (let i = 0; i < input.length; i++) {
+    let char = input[i]
+
+    // Skip any escaped characters in strings.
+    if (char === '\\' && isInString) {
+      // Ensure we don't have an escape character at the end.
+      if (input.length === i + 1) {
+        throw new Error('Invalid escape character at the end.')
+      }
+
+      // Skip the next character.
+      char = input[++i]
+    }
+    // If we find a space outside of a string, we should start a new argument.
+    else if (char === ' ' && !isInString) {
+      willStartNewArg = true
+      continue
+    }
+
+    // If we find a quote, we should toggle the string flag.
+    else if (char === '"') {
+      isInString = !isInString
+      continue
+    }
+
+    // If we're starting a new argument, we should add it to the array.
+    if (willStartNewArg) {
+      args.push(char)
+      willStartNewArg = false
+    }
+    // Otherwise, add it to the last argument.
+    else {
+      args[args.length - 1] += char
+    }
+  }
+
+  if (isInString) {
+    throw new Error('Unterminated string')
+  }
+
+  return args
+}
+
 /**
  * Get the node options from the environment variable `NODE_OPTIONS` and returns
  * them as an array of strings.
  *
  * @returns An array of strings with the node options.
  */
-const getNodeOptionsArgs = () =>
-  process.env.NODE_OPTIONS?.split(' ').map((arg) => arg.trim()) ?? []
+const getNodeOptionsArgs = () => {
+  if (!process.env.NODE_OPTIONS) return []
+
+  return tokenizeArgs(process.env.NODE_OPTIONS)
+}
 
 /**
  * The debug address is in the form of `[host:]port`. The host is optional.
@@ -129,7 +197,13 @@ export function formatNodeOptions(
       }
 
       if (value) {
-        return `--${key}=${value}`
+        return `--${key}=${
+          // Values with spaces need to be quoted. We use JSON.stringify to
+          // also escape any nested quotes.
+          value.includes(' ') && !value.startsWith('"')
+            ? JSON.stringify(value)
+            : value
+        }`
       }
 
       return null
diff --git a/test/integration/cli/basic/file with spaces to --require.js b/test/integration/cli/basic/file with spaces to --require.js
@@ -0,0 +1 @@
+console.log('FILE_WITH_SPACES_TO_REQUIRE_WITH_NODE_REQUIRE_OPTION')
diff --git a/test/integration/cli/basic/file with spaces to-require-with-node-require-option.js b/test/integration/cli/basic/file with spaces to-require-with-node-require-option.js
@@ -0,0 +1 @@
+console.log('FILE_WITH_SPACES_TO_REQUIRE_WITH_NODE_REQUIRE_OPTION')
diff --git a/test/integration/cli/test/index.test.js b/test/integration/cli/test/index.test.js
@@ -559,31 +559,65 @@ describe('CLI Usage', () => {
       }
     })
 
-    test("NODE_OPTIONS='--inspect=host:port'", async () => {
+    test("NODE_OPTIONS='--require=file with spaces to-require-with-node-require-option.js'", async () => {
       const port = await findPort()
-      const inspectPort = await findPort()
       let output = ''
       let errOutput = ''
       const app = await runNextCommandDev(
         [dirBasic, '--port', port],
         undefined,
         {
+          cwd: dirBasic,
           onStdout(msg) {
             output += stripAnsi(msg)
           },
           onStderr(msg) {
             errOutput += stripAnsi(msg)
           },
-          env: { NODE_OPTIONS: `--inspect=0.0.0.0:${inspectPort}` },
+          env: {
+            NODE_OPTIONS:
+              '--require "./file with spaces to-require-with-node-require-option.js"',
+          },
         }
       )
       try {
         await check(() => output, new RegExp(`http://localhost:${port}`))
-        await check(() => errOutput, /Debugger listening on/)
-        expect(errOutput).not.toContain('address already in use')
         expect(output).toContain(
-          'the --inspect option was detected, the Next.js router server should be inspected at'
+          'FILE_WITH_SPACES_TO_REQUIRE_WITH_NODE_REQUIRE_OPTION'
+        )
+        expect(errOutput).toBe('')
+      } finally {
+        await killApp(app)
+      }
+    })
+
+    // Checks to make sure that files that look like arguments are not incorrectly parsed out. In this case the file name has `--require` in it.
+    test("NODE_OPTIONS='--require=file with spaces to --require.js'", async () => {
+      const port = await findPort()
+      let output = ''
+      let errOutput = ''
+      const app = await runNextCommandDev(
+        [dirBasic, '--port', port],
+        undefined,
+        {
+          cwd: dirBasic,
+          onStdout(msg) {
+            output += stripAnsi(msg)
+          },
+          onStderr(msg) {
+            errOutput += stripAnsi(msg)
+          },
+          env: {
+            NODE_OPTIONS: '--require "./file with spaces to --require.js"',
+          },
+        }
+      )
+      try {
+        await check(() => output, new RegExp(`http://localhost:${port}`))
+        expect(output).toContain(
+          'FILE_WITH_SPACES_TO_REQUIRE_WITH_NODE_REQUIRE_OPTION'
         )
+        expect(errOutput).toBe('')
       } finally {
         await killApp(app)
       }

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+console.log('FILE_WITH_SPACES_TO_REQUIRE_WITH_NODE_REQUIRE_OPTION')`