fix: run all commands in a shell and quote all user provided parameters

The other approach proved problematic as there were different commands still requiring a shell. Now we always use the default shell but this has the drawback of not having quotes causing parsing issues in the container. There are probably still issues with escape sequences in user provided parameters, but that's for another day.
anacierdem · Oct 20, 2024 · 9383222 · 9383222
1 parent 5b94805
commit 9383222
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 41 deletions.
diff --git a/modules/actions/exec.js b/modules/actions/exec.js
@@ -66,22 +66,24 @@ const exec = async (info) => {
   const stdin = new PassThrough();
 
   /** @type {string[]} */
-  const paramsWithConvertedPaths = await Promise.all(
-    parameters.map(async (item) => {
-      if (item.startsWith('-')) {
+  const paramsWithConvertedPaths = (
+    await Promise.all(
+      parameters.map(async (item) => {
+        if (item.startsWith('-')) {
+          return item;
+        }
+        if (
+          item.includes(path.sep) &&
+          ((await fileExists(item)) || (await dirExists(item)))
+        ) {
+          return toPosixPath(
+            path.isAbsolute(item) ? path.relative(process.cwd(), item) : item
+          );
+        }
         return item;
-      }
-      if (
-        item.includes(path.sep) &&
-        ((await fileExists(item)) || (await dirExists(item)))
-      ) {
-        return toPosixPath(
-          path.isAbsolute(item) ? path.relative(process.cwd(), item) : item
-        );
-      }
-      return item;
-    })
-  );
+      })
+    )
+  ).map((param) => `"${param}"`);
 
   /**
    *

diff --git a/modules/actions/start.js b/modules/actions/start.js
@@ -34,12 +34,13 @@ const initContainer = async (libdragonInfo) => {
         'run',
         '-d', // Detached
         '--mount',
-        'type=bind,source=' +
+        '"type=bind,source=' +
           libdragonInfo.root +
           ',target=' +
-          CONTAINER_TARGET_PATH, // Mount files
-        '-w=' + CONTAINER_TARGET_PATH, // Set working directory
-        libdragonInfo.imageName,
+          CONTAINER_TARGET_PATH +
+          '"', // Mount files
+        '"-w=' + CONTAINER_TARGET_PATH + '"', // Set working directory
+        '"' + libdragonInfo.imageName + '"',
         'tail',
         '-f',
         '/dev/null',
@@ -58,16 +59,7 @@ const initContainer = async (libdragonInfo) => {
         '-R',
         `${uid >= 0 ? uid : ''}:${gid >= 0 ? gid : ''}`,
         '/n64_toolchain',
-      ],
-      {
-        userCommand: false,
-        inheritStdin: true,
-        inheritStdout: false,
-        inheritStderr: false,
-        spawnOptions: {
-          shell: true,
-        },
-      }
+      ]
     );
   } catch (e) {
     // Dispose the invalid container, clean and exit

diff --git a/modules/helpers.js b/modules/helpers.js
@@ -188,6 +188,12 @@ function spawnProcess(
         // Prevent the annoying docker "What's next?" message. It messes up everything.
         DOCKER_CLI_HINTS: 'false',
       },
+      // On macos, we need to run the command in a shell for some docker commands
+      // to work properly. The ones with paths in them probably not working on
+      // macOS. No need to do this on Windows, as it'd now require additional
+      // escaping for the paths.
+      // shell: process.platform === 'darwin',
+      shell: true,
       ...spawnOptions,
     });
 

diff --git a/modules/project-info.js b/modules/project-info.js
@@ -91,7 +91,7 @@ async function findContainerId(libdragonInfo) {
       '--format',
       '{{.}}{{.ID}}',
       '-f',
-      'volume=' + CONTAINER_TARGET_PATH,
+      '"volume=' + CONTAINER_TARGET_PATH + '"',
     ])
   )
     .split('\n')

diff --git a/modules/utils.js b/modules/utils.js
@@ -40,16 +40,7 @@ const installDependencies = async (libdragonInfo) => {
       CONTAINER_TARGET_PATH + '/' + libdragonInfo.vendorDirectory,
       ...dockerHostUserParams(libdragonInfo),
     ],
-    ['/bin/bash', './build.sh'],
-    {
-      userCommand: false,
-      inheritStdin: true,
-      inheritStdout: false,
-      inheritStderr: false,
-      spawnOptions: {
-        shell: true,
-      },
-    }
+    ['/bin/bash', './build.sh']
   );
 };
 
@@ -142,7 +133,7 @@ async function runGit(libdragonInfo, params, options = {}) {
 
   return await spawnProcess(
     'git',
-    ['-C', libdragonInfo.root, ...params],
+    ['-C', '"' + libdragonInfo.root + '"', ...params],
     // Windows git is breaking the TTY somehow - disable TTY for now
     // We are not able to display progress for the initial clone b/c of this
     // Enable progress otherwise.