Remove shell=True for node subprocesses

shell=True is a security hazard per the Python documentation:
https://docs.python.org/2/library/subprocess.html#frequently-used-arguments

This has 2 effects that required changes in scripts:
1. The shell removes extraneous quotes from arguments, which we now should no
longer include in any argument passed in
2. The shell now requires an array of arguments, rather than a string

Next to that, removal of shell=True is required if we later want to call
the script as part of a GN action.

R=tapted@chromium.org,dpapad@chromium.org

Bug: 1096473
Change-Id: I77e85048e5a6db33035ca774434857170aeaf91e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264365
Reviewed-by: dpapad <dpapad@chromium.org>
Commit-Queue: Tim van der Lippe <tvanderlippe@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782918}

chrome/browser/resources/optimize_webui.py

@@ -105,7 +105,7 @@
 
 
 _VULCANIZE_REDIRECT_ARGS = list(itertools.chain.from_iterable(map(
-    lambda m: ['--redirect', '"%s|%s"' % (m[0], m[1])], _URL_MAPPINGS)))
+    lambda m: ['--redirect', '%s|%s' % (m[0], m[1])], _URL_MAPPINGS)))
 
 
 def _undo_mapping(mappings, url):
@@ -272,7 +272,7 @@ def _bundle_v2(tmp_out_dir, in_path, out_path, manifest_out_path, args,
       [
        '--manifest-out', manifest_out_path,
        '--root', in_path,
-       '--redirect', '"chrome://%s/|%s"' % (args.host, in_path + '/'),
+       '--redirect', 'chrome://%s/|%s' % (args.host, in_path + '/'),
        '--out-dir', os.path.relpath(tmp_out_dir, _CWD).replace('\\', '/'),
        '--shell', args.html_in_files[0],
       ] + in_html_args)
@@ -350,7 +350,7 @@ def _optimize(in_folder, args):
     for index, js_out_file in enumerate(args.js_out_files):
       node.RunNode([node_modules.PathToUglify(),
                     os.path.join(tmp_out_dir, js_out_file),
-                    '--comments', '"/Copyright|license|LICENSE|\<\/?if/"',
+                    '--comments', '/Copyright|license|LICENSE|\<\/?if/',
                     '--output', os.path.join(out_path, js_out_file)])
   finally:
     shutil.rmtree(tmp_out_dir)

third_party/node/node.py

@@ -18,11 +18,22 @@ def GetBinaryPath():
 
 
 def RunNode(cmd_parts, stdout=None):
-  cmd = " ".join([GetBinaryPath()] + cmd_parts)
+  cmd = [GetBinaryPath()] + cmd_parts
   process = subprocess.Popen(
-      cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True)
+      cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
   stdout, stderr = process.communicate()
 
+  # TODO(crbug.com/1098074): Properly handle the returncode of
+  # process defined above. Right now, if the process would exit
+  # with a return code of non-zero, but the stderr is empty,
+  # we would still pass.
+  #
+  # However, we can't make this change here yet, as there are
+  # various presubmit scripts that rely on the runtime error
+  # and are unable to handle a `os.exit` call in this branch.
+  # These presubmit scripts need to spawn `subprocesses`
+  # themselves to handle the exitcode, before we can make the
+  # change here.
   if stderr:
     raise RuntimeError('%s failed: %s' % (cmd, stderr))
 

tools/web_dev_style/js_checker.py

@@ -70,7 +70,7 @@ def RunEsLintChecks(self, affected_js_files, format="stylish"):
 
     from os import isatty as os_isatty
     args = ["--color"] if os_isatty(self.input_api.sys.stdout.fileno()) else []
-    args += ["--format", format, "--ignore-pattern '!.eslintrc.js'"]
+    args += ["--format", format, "--ignore-pattern", "!.eslintrc.js"]
     args += affected_js_files_paths
 
     import eslint