#620 Fix high level bandit findings
Merged 2 years ago by onosek. Opened 2 years ago by onosek.
onosek/rpkg bandit_errors  into  master

file modified
+72 -65
@@ -524,7 +524,7 @@ 

      def load_localarch(self):

          """Get the local arch as defined by rpm"""

  

-         proc = subprocess.Popen(['rpm --eval %{_arch}'], shell=True,

+         proc = subprocess.Popen(('rpm', '--eval', '%{_arch}'),

                                  stdout=subprocess.PIPE,

                                  universal_newlines=True)

          self._localarch = proc.communicate()[0].strip('\n')
@@ -736,16 +736,16 @@ 

  

          if self.uses_rpmautospec and rpmautospec_calculate_release_number:

              release_number = rpmautospec_calculate_release_number(specfile_path)

-             cmd.append("--define '_rpmautospec_release_number %d'" % release_number)

+             cmd.extend(["--define", "_rpmautospec_release_number %d" % release_number])

  

          # We make sure there is a space at the end of our query so that

          # we can split it later.  When there are subpackages, we get a

          # listing for each subpackage.  We only care about the first.

-         cmd.extend(['-q', '--qf', '"??%{NAME} %{EPOCH} %{VERSION} %{RELEASE}??"',

-                     '--specfile', '"%s"' % specfile_path])

+         cmd.extend(["-q", "--qf", "??%{NAME} %{EPOCH} %{VERSION} %{RELEASE}??",

+                     "--specfile", "%s" % specfile_path])

          joined_cmd = ' '.join(cmd)

          try:

-             proc = subprocess.Popen(joined_cmd, shell=True,

+             proc = subprocess.Popen(cmd,

                                      universal_newlines=True,

                                      stdout=subprocess.PIPE,

                                      stderr=subprocess.PIPE)
@@ -789,7 +789,7 @@ 

  

          if self.uses_rpmautospec and rpmautospec_calculate_release_number:

              release_number = rpmautospec_calculate_release_number(specfile_path)

-             rpm_cmd.append("--define '_rpmautospec_release_number %d'" % release_number)

+             rpm_cmd.extend(["--define", "_rpmautospec_release_number %d" % release_number])

  

          # setup the command

          cmd = ['mock']
@@ -812,9 +812,9 @@ 

          # We make sure there is a space at the end of our query so that

          # we can split it later.  When there are subpackages, we get a

          # listing for each subpackage.  We only care about the first.

-         rpm_cmd.extend(['-q', '--qf', '"??%{NAME} %{EPOCH} %{VERSION} %{RELEASE}??"',

-                         '--specfile', '"%s"' % os.path.join(tmp_root, self.spec)])

-         main_cmd = cmd + ['--shell'] + [' '.join(rpm_cmd)] \

+         rpm_cmd.extend(["-q", "--qf", "??%{NAME} %{EPOCH} %{VERSION} %{RELEASE}??",

+                         "--specfile", "%s" % os.path.join(tmp_root, self.spec)])

+         main_cmd = cmd + ['--shell'] + rpm_cmd \

              + ['> ' + os.path.join(tmp_root, 'output')]

  

          copyout_cmd = cmd + ['--copyout', os.path.join(tmp_root, 'output'), tmp_resultdir]
@@ -886,17 +886,17 @@ 

          self._distvar, self._distval = osver.split('-')

          self._distval = self._distval.replace('.', '_')

          self._disttag = 'el%s' % self._distval

-         self._rpmdefines = ["--define '_sourcedir %s'" % self.layout.sourcedir,

-                             "--define '_specdir %s'" % self.layout.specdir,

-                             "--define '_builddir %s'" % self.layout.builddir,

-                             "--define '_srcrpmdir %s'" % self.layout.srcrpmdir,

-                             "--define '_rpmdir %s'" % self.layout.rpmdir,

-                             "--define '_rpmfilename %s'" % self.layout.rpmfilename,

-                             "--define 'dist .%s'" % self._disttag,

-                             "--define '%s %s'" % (self._distvar,

-                                                   self._distval.split('_')[0]),

+         self._rpmdefines = ["--define", "_sourcedir %s" % self.layout.sourcedir,

+                             "--define", "_specdir %s" % self.layout.specdir,

+                             "--define", "_builddir %s" % self.layout.builddir,

+                             "--define", "_srcrpmdir %s" % self.layout.srcrpmdir,

+                             "--define", "_rpmdir %s" % self.layout.rpmdir,

+                             "--define", "_rpmfilename %s" % self.layout.rpmfilename,

+                             "--define", "dist .%s" % self._disttag,

+                             "--define", "%s %s" % (self._distvar,

+                                                    self._distval.split('_')[0]),

                              # int and float this to remove the decimal

-                             "--define '%s 1'" % self._disttag]

+                             "--define", "%s 1" % self._disttag]

  

      @property

      def spec(self):
@@ -1251,6 +1251,10 @@ 

          if shell:

              command = ' '.join(cmd)

              pipecmd = ' '.join(pipe)

+             # TODO: remove shell=True functionality when all shell=True occurences in dependencies

+             # are resolved

+             self.log.warning("Deprecated functionality: shell=True. Command with shell=True "

+                              "is not allowed because of security reasons (bandit).")

  

          if pipe:

              self.log.debug('Running: %s | %s', ' '.join(cmd), ' '.join(pipe))
@@ -1266,17 +1270,17 @@ 

                  # bad thing, but rpmbuild likes to put useful data on

                  # stderr, so....

                  parent_proc = subprocess.Popen(

-                     command, env=environ, shell=shell, cwd=cwd,

+                     command, env=environ, shell=shell, cwd=cwd,  # nosec

                      stdout=subprocess.PIPE, stderr=subprocess.STDOUT)

  

                  proc = subprocess.Popen(

-                     pipecmd, env=environ, shell=shell, cwd=cwd,

+                     pipecmd, env=environ, shell=shell, cwd=cwd,  # nosec

                      stdin=parent_proc.stdout,

                      stdout=proc_stdout, stderr=proc_stderr,

                      universal_newlines=return_text)

              else:

                  proc = subprocess.Popen(

-                     command, env=environ, shell=shell, cwd=cwd,

+                     command, env=environ, shell=shell, cwd=cwd,  # nosec

                      stdout=proc_stdout, stderr=proc_stderr,

                      universal_newlines=return_text)

          except KeyboardInterrupt:
@@ -2504,10 +2508,10 @@ 

          # Command contains workaround (undefines _changelog_trimtime) described at:

          # https://github.com/rpm-software-management/rpm/issues/1301

          # It caused, that older changelog records were not displayed.

-         cmd = ['rpm'] + self.rpmdefines + ['-q', '--qf', '"%{CHANGELOGTEXT}\n"',

-                                            '--undefine', '"_changelog_trimtime"',

-                                            '--specfile', '"%s"' % spec_file]

-         proc = subprocess.Popen(' '.join(cmd), shell=True,

+         cmd = ["rpm"] + self.rpmdefines + ["-q", "--qf", "%{CHANGELOGTEXT}\n",

+                                            "--undefine", "_changelog_trimtime",

+                                            "--specfile", "%s" % spec_file]

+         proc = subprocess.Popen(cmd,

                                  stdout=subprocess.PIPE, stderr=subprocess.PIPE,

                                  universal_newlines=True)

          stdout, stderr = proc.communicate()
@@ -2560,7 +2564,7 @@ 

          cmd.extend(self.rpmdefines)

          if builddir:

              # Tack on a new builddir to the end of the defines

-             cmd.append("--define '_builddir %s'" % os.path.abspath(builddir))

+             cmd.extend(["--define", "_builddir %s" % os.path.abspath(builddir)])

          if arch:

              cmd.extend(['--target', arch])

          if define:
@@ -2576,7 +2580,7 @@ 

              cmd.append('--quiet')

          cmd.extend(['-bc', os.path.join(self.path, self.spec)])

          # Run the command

-         self._run_command(cmd, shell=True)

+         self._run_command(cmd)

  

      def giturl(self):

          """Return the git url that would be used for building"""
@@ -2634,7 +2638,7 @@ 

          cmd.extend(self.rpmdefines)

          if builddir:

              # Tack on a new builddir to the end of the defines

-             cmd.append("--define '_builddir %s'" % os.path.abspath(builddir))

+             cmd.extend(["--define", "_builddir %s" % os.path.abspath(builddir)])

          if arch:

              cmd.extend(['--target', arch])

          if define:
@@ -2649,11 +2653,11 @@ 

          if self.quiet:

              cmd.append('--quiet')

          if buildrootdir:

-             cmd.append("--define '_buildrootdir {0}'".format(

-                 os.path.abspath(buildrootdir)))

+             cmd.extend(["--define", "_buildrootdir {0}".format(

+                 os.path.abspath(buildrootdir))])

          cmd.extend(['-bi', os.path.join(self.path, self.spec)])

          # Run the command

-         self._run_command(cmd, shell=True)

+         self._run_command(cmd)

          return

  

      def lint(self, info=False, rpmlintconf=None):
@@ -2732,7 +2736,7 @@ 

          cmd.append(os.path.join(self.layout.specdir, self.spec))

          cmd.extend(sorted(rpms))

  

-         self._run_command(cmd, shell=True)

+         self._run_command(cmd)

  

      def list_side_tags(self, base_tag=None, user=None):

          return self.kojisession.listSideTags(basetag=base_tag, user=user)
@@ -2769,7 +2773,7 @@ 

          cmd.extend(self.rpmdefines + localargs)

          if builddir:

              # Tack on a new builddir to the end of the defines

-             cmd.append("--define '_builddir %s'" % os.path.abspath(builddir))

+             cmd.extend(["--define", "_builddir %s" % os.path.abspath(builddir)])

          if arch:

              cmd.extend(['--target', arch])

          if define:
@@ -2780,18 +2784,16 @@ 

          if self.quiet:

              cmd.append('--quiet')

          if buildrootdir:

-             cmd.append("--define '_buildrootdir {0}'".format(

-                 os.path.abspath(buildrootdir)))

+             cmd.extend(["--define", "_buildrootdir {0}".format(

+                 os.path.abspath(buildrootdir))])

          # Figure out the hash type to use

          if not hashtype:

              # Try to determine the dist

              hashtype = self._guess_hashtype()

          # This may need to get updated if we ever change our checksum default

          if not hashtype == 'sha256':

-             cmd.extend(["--define '_source_filedigest_algorithm %s'"

-                         % hashtype,

-                         "--define '_binary_filedigest_algorithm %s'"

-                         % hashtype])

+             cmd.extend(["--define", "_source_filedigest_algorithm %s" % hashtype,

+                         "--define", "_binary_filedigest_algorithm %s" % hashtype])

          specpath = os.path.join(self.path, self.spec)

          tmpdir = None

          try:
@@ -2804,16 +2806,23 @@ 

                  cmd.extend(['-ba', tmpspecpath])

              logfile = '.build-%s-%s.log' % (self.ver, self.rel)

  

-             cmd = '%s 2>&1 | tee %s' % (' '.join(cmd), logfile)

+             debug_cmd = '%s 2>&1 | tee %s' % (' '.join(cmd), logfile)

              try:

-                 # Since zsh is a widely used, which is supported by fedpkg

-                 # actually, pipestatus is for checking the first command when zsh

-                 # is used.

-                 subprocess.check_call(

-                     '%s; exit "${PIPESTATUS[0]} ${pipestatus[1]}"' % cmd,

-                     shell=True)

-             except subprocess.CalledProcessError:

-                 raise rpkgError(cmd)

+                 # executes: CMD 2>&1 | tee <logfile>

+                 rpmbuild = subprocess.Popen(

+                     cmd,

+                     stdout=subprocess.PIPE,

+                     stderr=subprocess.STDOUT

+                 )

+                 tee = subprocess.Popen(

+                     ("tee", logfile),

+                     stdin=rpmbuild.stdout,

+                     stdout=subprocess.PIPE)

+                 rpmbuild.stdout.close()

+                 tee.communicate()

+ 

+             except subprocess.SubprocessError:

+                 raise rpkgError(debug_cmd)

          finally:

              self._cleanup_tmp_dir(tmpdir)

  
@@ -3154,7 +3163,7 @@ 

          cmd.extend(self.rpmdefines)

          if builddir:

              # Tack on a new builddir to the end of the defines

-             cmd.append("--define '_builddir %s'" % os.path.abspath(builddir))

+             cmd.extend(["--define", "_builddir %s" % os.path.abspath(builddir)])

          if arch:

              cmd.extend(['--target', arch])

          if define:
@@ -3165,11 +3174,11 @@ 

          if self.quiet:

              cmd.append('--quiet')

          if buildrootdir:

-             cmd.append("--define '_buildrootdir {0}'".format(

-                 os.path.abspath(buildrootdir)))

+             cmd.extend(["--define", "_buildrootdir {0}".format(

+                 os.path.abspath(buildrootdir))])

          cmd.extend(['--nodeps', '-bp', os.path.join(self.path, self.spec)])

          # Run the command

-         self._run_command(cmd, shell=True)

+         self._run_command(cmd)

  

      def srpm(self, hashtype=None, define=None, builddir=None, buildrootdir=None,

               arch=None, extra_args=None):
@@ -3201,7 +3210,7 @@ 

          cmd.extend(self.rpmdefines)

          if builddir:

              # Tack on a new builddir to the end of the defines

-             cmd.append("--define '_builddir %s'" % os.path.abspath(builddir))

+             cmd.extend(["--define", "_builddir %s" % os.path.abspath(builddir)])

          if arch:

              cmd.extend(['--target', arch])

          if define:
@@ -3212,8 +3221,8 @@ 

          if self.quiet:

              cmd.append('--quiet')

          if buildrootdir:

-             cmd.append("--define '_buildrootdir {0}'".format(

-                 os.path.abspath(buildrootdir)))

+             cmd.extend(["--define", "_buildrootdir {0}".format(

+                 os.path.abspath(buildrootdir))])

  

          # Figure out which hashtype to use, if not provided one

          if not hashtype:
@@ -3221,10 +3230,8 @@ 

              hashtype = self._guess_hashtype()

          # This may need to get updated if we ever change our checksum default

          if not hashtype == 'sha256':

-             cmd.extend(["--define '_source_filedigest_algorithm %s'"

-                         % hashtype,

-                         "--define '_binary_filedigest_algorithm %s'"

-                         % hashtype])

+             cmd.extend(["--define", "_source_filedigest_algorithm %s" % hashtype,

+                         "--define", "_binary_filedigest_algorithm %s" % hashtype])

          specpath = os.path.join(self.path, self.spec)

          tmpdir = None

          try:
@@ -3235,7 +3242,7 @@ 

                  tmpspecpath = os.path.join(tmpdir, self.spec)

                  rpmautospec_process_distgit(specpath, tmpspecpath)

                  cmd.extend(['--nodeps', '-bs', tmpspecpath])

-             self._run_command(cmd, shell=True)

+             self._run_command(cmd)

          finally:

              self._cleanup_tmp_dir(tmpdir)

  
@@ -3314,7 +3321,7 @@ 

          cmd.extend(self.rpmdefines)

          if builddir:

              # Tack on a new builddir to the end of the defines

-             cmd.append("--define '_builddir %s'" % os.path.abspath(builddir))

+             cmd.extend(["--define", "_builddir %s" % os.path.abspath(builddir)])

          if define:

              for entry in define:

                  cmd.extend(['--define', entry])
@@ -3323,11 +3330,11 @@ 

          if self.quiet:

              cmd.append('--quiet')

          if buildrootdir:

-             cmd.append("--define '_buildrootdir {0}'".format(

-                 os.path.abspath(buildrootdir)))

+             cmd.extend(["--define", "_buildrootdir {0}".format(

+                 os.path.abspath(buildrootdir))])

          cmd.extend(['-bl', os.path.join(self.path, self.spec)])

          # Run the command

-         self._run_command(cmd, shell=True)

+         self._run_command(cmd)

  

      def _process_koji_task_result(self, task_id):

          """

file modified
+77 -51
@@ -673,35 +673,35 @@ 

  

          expected_cmd = ['rpmbuild'] + cli.cmd.rpmdefines + \

              ['--nodeps', '-bs', os.path.join(cli.cmd.path, cli.cmd.spec)]

-         _run_command.assert_called_once_with(expected_cmd, shell=True)

+         _run_command.assert_called_once_with(expected_cmd)

  

      @patch('pyrpkg.Commands._run_command')

      def test_srpm_with_options(self, _run_command):

          cli_cmd = ['rpkg', '--path', self.cloned_repo_path, '--release', 'rhel-6', 'srpm',

-                    '--define', '"macro1 meansthis"']

+                    '--define', 'macro1 meansthis']

  

          with patch('sys.argv', new=cli_cmd):

              cli = self.new_cli()

              cli.srpm()

  

          expected_cmd = ['rpmbuild'] + cli.cmd.rpmdefines + \

-             ['--define', '"macro1 meansthis"', '--nodeps', '-bs',

+             ['--define', 'macro1 meansthis', '--nodeps', '-bs',

               os.path.join(cli.cmd.path, cli.cmd.spec)]

-         _run_command.assert_called_once_with(expected_cmd, shell=True)

+         _run_command.assert_called_once_with(expected_cmd)

  

      @patch('pyrpkg.Commands._run_command')

      def test_srpm_with_extra_args(self, _run_command):

          cli_cmd = ['rpkg', '--path', self.cloned_repo_path, '--release', 'rhel-6', 'srpm',

-                    '--', '--define', '"name body"', '--undefine', 'python']

+                    '--', '--define', 'name body', '--undefine', 'python']

  

          with patch('sys.argv', new=cli_cmd):

              cli = self.new_cli()

              cli.srpm()

  

          expected_cmd = ['rpmbuild'] + cli.cmd.rpmdefines + \

-             ['--define', shlex.quote('"name body"'), '--undefine', 'python', '--nodeps', '-bs',

+             ['--define', shlex.quote('name body'), '--undefine', 'python', '--nodeps', '-bs',

               os.path.join(cli.cmd.path, cli.cmd.spec)]

-         _run_command.assert_called_once_with(expected_cmd, shell=True)

+         _run_command.assert_called_once_with(expected_cmd)

  

  

  class TestCompile(CliTestCase):
@@ -719,7 +719,7 @@ 

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + ['-bc', spec]

  

-         _run_command.assert_called_once_with(rpmbuild, shell=True)

+         _run_command.assert_called_once_with(rpmbuild)

  

      @patch('pyrpkg.Commands._run_command')

      def test_compile_with_options(self, _run_command):
@@ -735,11 +735,11 @@ 

  

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + \

-             ["--define '_builddir %s'" % builddir, '--target', 'i686',

+             ['--define', '_builddir %s' % builddir, '--target', 'i686',

               '--define', 'macro1 meansthis', '--rmspec', '--short-circuit',

               '--nocheck', '--quiet', '-bc', spec]

  

-         _run_command.assert_called_once_with(rpmbuild, shell=True)

+         _run_command.assert_called_once_with(rpmbuild)

  

  

  class TestPrep(CliTestCase):
@@ -756,7 +756,7 @@ 

  

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + ['--nodeps', '-bp', spec]

-         _run_command.assert_called_once_with(rpmbuild, shell=True)

+         _run_command.assert_called_once_with(rpmbuild)

  

      @patch('pyrpkg.Commands._run_command')

      def test_prep_with_options(self, _run_command):
@@ -775,11 +775,11 @@ 

  

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + [

-             "--define '_builddir %s'" % builddir, '--target', 'i686', '-v',

-             '--quiet', "--define '_buildrootdir %s'" % buildrootdir,

+             '--define', '_builddir %s' % builddir, '--target', 'i686', '-v',

+             '--quiet', '--define', '_buildrootdir %s' % buildrootdir,

              '--nodeps', '-bp', spec

          ]

-         _run_command.assert_called_once_with(rpmbuild, shell=True)

+         _run_command.assert_called_once_with(rpmbuild)

  

      @patch('pyrpkg.Commands._run_command')

      def test_prep_extra_args_with_space(self, _run_command):
@@ -797,7 +797,7 @@ 

                                                          shlex.quote(first_arg_with_space),

                                                          shlex.quote(second_arg_with_space),

                                                          '--nodeps', '-bp', spec]

-         _run_command.assert_called_once_with(rpmbuild, shell=True)

+         _run_command.assert_called_once_with(rpmbuild)

  

  

  class TestInstall(CliTestCase):
@@ -815,7 +815,7 @@ 

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + ['-bi', spec]

  

-         _run_command.assert_called_once_with(rpmbuild, shell=True)

+         _run_command.assert_called_once_with(rpmbuild)

  

      @patch('pyrpkg.Commands._run_command')

      def test_install_with_options(self, _run_command):
@@ -834,37 +834,55 @@ 

  

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + [

-             "--define '_builddir %s'" % builddir, '--target', 'i686',

+             '--define', '_builddir %s' % builddir, '--target', 'i686',

              '--nocheck', '--quiet',

-             "--define '_buildrootdir %s'" % buildrootdir,

+             '--define', '_buildrootdir %s' % buildrootdir,

              '-bi', spec

          ]

  

-         _run_command.assert_called_once_with(rpmbuild, shell=True)

+         _run_command.assert_called_once_with(rpmbuild)

  

  

  class TestLocal(CliTestCase):

  

      create_repo_per_test = False

  

-     @patch('pyrpkg.subprocess.check_call')

-     def test_local(self, check_call):

+     @patch('subprocess.Popen')

+     @patch('pyrpkg.Commands.rel')

+     @patch('pyrpkg.Commands.ver')

+     def test_local(self, ver, rel, popen):

          cli_cmd = ['rpkg', '--path', self.cloned_repo_path, '--release', 'rhel-6', 'local']

  

+         rel.__str__ = Mock()

+         rel.__str__.return_value = '2.el6'

+         ver.__str__ = Mock()

+         ver.__str__.return_value = '1.2'

+ 

          with patch('sys.argv', new=cli_cmd):

              cli = self.new_cli()

              cli.local()

  

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + ['-ba', spec]

-         tee = ['tee', '.build-%s-%s.log' % (cli.cmd.ver, cli.cmd.rel)]

-         cmd = '%s 2>&1 | %s; exit "${PIPESTATUS[0]} ${pipestatus[1]}"' % (

-             ' '.join(rpmbuild), ' '.join(tee)

-         )

-         check_call.assert_called_once_with(cmd, shell=True)

+         tee = ('tee', '.build-%s-%s.log' % (cli.cmd.ver, cli.cmd.rel))

+ 

+         popen.assert_has_calls([

+             # at the beginning of this list, there are other calls from load_nameverrel

+             call(rpmbuild, stdout=-1, stderr=-2),

+             # I can't match this call - stdin=Mock has it's dynamic id. Therefore any_oreder=True

+             # call(tee, stdin=Mock(), stdout=-1),  # check call [-3] separately

+             call().stdout.close(),

+             call().communicate(),

+         ], any_order=True)

  

-     @patch('pyrpkg.subprocess.check_call')

-     def test_local_with_options(self, check_call):

+         tee_call_arg = popen.mock_calls[-3]

+         if 'args' in dir(tee_call_arg):  # doesn't work in <=py36

+             self.assertEqual(tee, tee_call_arg.args[0])

+ 

+     @patch('subprocess.Popen')

+     @patch('pyrpkg.Commands.rel')

+     @patch('pyrpkg.Commands.ver')

+     def test_local_with_options(self, ver, rel, popen):

          builddir = os.path.join(self.cloned_repo_path, 'this-builddir')

          buildrootdir = os.path.join(self.cloned_repo_path, 'this-buildrootdir')

  
@@ -874,6 +892,11 @@ 

              '--with', 'a', '--without', 'b', '--buildrootdir', buildrootdir,

              '--', '--clean']

  

+         rel.__str__ = Mock()

+         rel.__str__.return_value = '2.el6'

+         ver.__str__ = Mock()

+         ver.__str__.return_value = '1.2'

+ 

          with patch('sys.argv', new=cli_cmd):

              cli = self.new_cli()

              cli.local()
@@ -881,18 +904,25 @@ 

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + [

              '--with', 'a', '--without', 'b',

-             "--define '_builddir %s'" % builddir,

+             '--define', '_builddir %s' % builddir,

              '--target', 'i686', '--clean', '--quiet',

-             "--define '_buildrootdir %s'" % buildrootdir,

+             '--define', '_buildrootdir %s' % buildrootdir,

              '-ba', spec

          ]

-         tee = ['tee', '.build-%s-%s.log' % (cli.cmd.ver, cli.cmd.rel)]

+         tee = ('tee', '.build-%s-%s.log' % (cli.cmd.ver, cli.cmd.rel))

  

-         cmd = '%s 2>&1 | %s; exit "${PIPESTATUS[0]} ${pipestatus[1]}"' % (

-             ' '.join(rpmbuild), ' '.join(tee)

-         )

+         popen.assert_has_calls([

+             # at the beginning of this list, there are other calls from load_nameverrel

+             call(rpmbuild, stdout=-1, stderr=-2),

+             # I can't match this call - stdin=Mock has it's dynamic id. Therefore any_oreder=True

+             # call(tee, stdin=Mock(), stdout=-1),  # check call [-3] separately

+             call().stdout.close(),

+             call().communicate(),

+         ], any_order=True)

  

-         check_call.assert_called_once_with(cmd, shell=True)

+         tee_call_arg = popen.mock_calls[-3]

+         if 'args' in dir(tee_call_arg):  # doesn't work in <=py36

+             self.assertEqual(tee, tee_call_arg.args[0])

  

  

  class TestVerifyFiles(CliTestCase):
@@ -909,7 +939,7 @@ 

  

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + ['-bl', spec]

-         _run_command.assert_called_once_with(rpmbuild, shell=True)

+         _run_command.assert_called_once_with(rpmbuild)

  

      @patch('pyrpkg.Commands._run_command')

      def test_verify_files_with_options(self, _run_command):
@@ -928,12 +958,12 @@ 

  

          spec = os.path.join(cli.cmd.path, cli.cmd.spec)

          rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + [

-             "--define '_builddir %s'" % builddir,

+             '--define', '_builddir %s' % builddir,

              '--quiet',

-             "--define '_buildrootdir %s'" % buildrootdir,

+             '--define', '_buildrootdir %s' % buildrootdir,

              '-bl', spec

          ]

-         _run_command.assert_called_once_with(rpmbuild, shell=True)

+         _run_command.assert_called_once_with(rpmbuild)

  

  

  class TestVerrel(CliTestCase):
@@ -1162,7 +1192,7 @@ 

  

          _run_command.assert_has_calls([

              call(['rpmlint', '--version'], return_stdout=True, return_text=True),

-             call(['rpmlint', os.path.join(cli.cmd.path, cli.cmd.spec)], shell=True),

+             call(['rpmlint', os.path.join(cli.cmd.path, cli.cmd.spec)]),

          ])

  

      @patch('pyrpkg.Commands._run_command')
@@ -1179,7 +1209,7 @@ 

  

          _run_command.assert_has_calls([

              call(['rpmlint', '--version'], return_stdout=True, return_text=True),

-             call(['rpmlint', '-i', os.path.join(cli.cmd.path, cli.cmd.spec)], shell=True),

+             call(['rpmlint', '-i', os.path.join(cli.cmd.path, cli.cmd.spec)]),

          ])

  

      @patch('pyrpkg.Commands._run_command')
@@ -1196,7 +1226,7 @@ 

  

          _run_command.assert_has_calls([

              call(['rpmlint', '--version'], return_stdout=True, return_text=True),

-             call(['rpmlint', '--info', os.path.join(cli.cmd.path, cli.cmd.spec)], shell=True),

+             call(['rpmlint', '--info', os.path.join(cli.cmd.path, cli.cmd.spec)]),

          ])

  

      @patch('pyrpkg.Commands._run_command')
@@ -1215,8 +1245,7 @@ 

  

          _run_command.assert_has_calls([

              call(['rpmlint', '--version'], return_stdout=True, return_text=True),

-             call(['rpmlint', '-f', lint_config_path, os.path.join(cli.cmd.path, cli.cmd.spec)],

-                  shell=True),

+             call(['rpmlint', '-f', lint_config_path, os.path.join(cli.cmd.path, cli.cmd.spec)],),

          ])

  

      @patch('pyrpkg.Commands._run_command')
@@ -1235,8 +1264,7 @@ 

  

          _run_command.assert_has_calls([

              call(['rpmlint', '--version'], return_stdout=True, return_text=True),

-             call(['rpmlint', '-r', lint_config_path, os.path.join(cli.cmd.path, cli.cmd.spec)],

-                  shell=True),

+             call(['rpmlint', '-r', lint_config_path, os.path.join(cli.cmd.path, cli.cmd.spec)],),

          ])

  

      @patch('pyrpkg.Commands._run_command')
@@ -1257,8 +1285,7 @@ 

  

          _run_command.assert_has_calls([

              call(['rpmlint', '--version'], return_stdout=True, return_text=True),

-             call(['rpmlint', '-f', lint_config_path, os.path.join(cli.cmd.path, cli.cmd.spec)],

-                  shell=True),

+             call(['rpmlint', '-f', lint_config_path, os.path.join(cli.cmd.path, cli.cmd.spec)],),

          ])

  

      @patch('pyrpkg.Commands._run_command')
@@ -1281,8 +1308,7 @@ 

          _run_command.assert_has_calls([

              call(['rpmlint', '--version'], return_stdout=True, return_text=True),

              call(['rpmlint', '-f', custom_lint_config_path,

-                   os.path.join(cli.cmd.path, cli.cmd.spec)],

-                  shell=True),

+                   os.path.join(cli.cmd.path, cli.cmd.spec)],),

          ])

  

  
@@ -3654,7 +3680,7 @@ 

              self.assert_option_srpm_use()

  

          args, kwargs = _run_command.call_args

-         self.assertEqual({'shell': True}, kwargs)

+         self.assertEqual({}, kwargs)

          rpmbuild_cmd, = args

          self.assertIn('-bs', rpmbuild_cmd)

  

file modified
+16 -15
@@ -187,7 +187,7 @@ 

          self.assertIs(True, self.cmd._uses_rpmautospec)

          self.assertEqual(1, wrapped_popen.call_count)

          args, kwargs = wrapped_popen.call_args

-         self.assertIn("--define '_rpmautospec_release_number %d'" % test_release_number, args[0])

+         self.assertIn("_rpmautospec_release_number %d" % test_release_number, args[0])

  

  

  class LoadBranchMergeTest(CommandTestCase):
@@ -269,9 +269,7 @@ 

          #     '_srcrpmdir': '/path/to/srcrpm-dir',

          #     'dist': 'el7'

          # }

-         defines = dict([item.split(' ') for item in (

-             define.replace("'", '').split(' ', 1)[1] for

-             define in self.cmd._rpmdefines)])

+         defines = dict([item.split(' ') for item in self.cmd._rpmdefines[1::2]])

  

          self.assertEqual(len(expected_defines), len(defines))

          for var, val in expected_defines.items():
@@ -593,7 +591,7 @@ 

          cmd = self.make_commands(path=self.cloned_repo_path)

          # Repos used for running tests locates in local filesyste, refer to

          # self.repo_path and self.cloned_repo_path.

-         cmd.anongiturl = '/tmp/%(module)s'

+         cmd.anongiturl = os.path.join(os.path.dirname(self.cloned_repo_path), "%(module)s")

          cmd.distgit_namespaced = False

  

          self.assertEqual(str(six.next(git.Repo(self.repo_path).iter_commits())),
@@ -820,7 +818,7 @@ 

      def test_raise_error_if_other_non_no_such_file_dir_error(self):

          with patch('shutil.rmtree',

                     side_effect=OSError((errno.EEXIST), 'error message')):

-             self.assertRaises(rpkgError, self.cmd._cleanup_tmp_dir, '/tmp/dir')

+             self.assertRaises(rpkgError, self.cmd._cleanup_tmp_dir, '/tempdir')

  

  

  class TestConfigMockConfigDir(CommandTestCase):
@@ -1020,7 +1018,7 @@ 

                     os.path.join(cmd.path, 'docpkg.spec'),

                     srpm_path,

                     bin_path,

-                    ], shell=True)])

+                    ])])

  

      @patch('glob.glob')

      @patch('os.path.exists')
@@ -1064,7 +1062,7 @@ 

                     os.path.join(cmd.path, 'docpkg.spec'),

                     srpm_path,

                     bin_path,

-                    ], shell=True)])

+                    ])])

  

      @patch('glob.glob')

      @patch('os.path.exists')
@@ -1101,7 +1099,7 @@ 

                     os.path.join(cmd.path, 'docpkg.spec'),

                     srpm_path,

                     bin_path,

-                    ], shell=True)])

+                    ])])

  

  

  class TestRunCommand(CommandTestCase):
@@ -1141,11 +1139,11 @@ 

          Popen.return_value.stdout.read.return_value = 'output'

  

          result = self.cmd._run_command(

-             ['rpmbuild'], shell=True, return_stdout=True)

+             ['rpmbuild'], shell=False, return_stdout=True)

  

          self.assertEqual((0, 'output', None), result)

          Popen.assert_called_once_with(

-             'rpmbuild', env=os.environ, shell=True, cwd=None,

+             ['rpmbuild'], env=os.environ, shell=False, cwd=None,

              stdout=subprocess.PIPE, stderr=None, universal_newlines=False)

  

      @patch('subprocess.Popen')
@@ -1154,11 +1152,11 @@ 

          Popen.return_value.stderr.read.return_value = 'output'

  

          result = self.cmd._run_command(

-             ['rpmbuild'], shell=True, return_stderr=True)

+             ['rpmbuild'], shell=False, return_stderr=True)

  

          self.assertEqual((0, None, 'output'), result)

          Popen.assert_called_once_with(

-             'rpmbuild', env=os.environ, shell=True, cwd=None,

+             ['rpmbuild'], env=os.environ, shell=False, cwd=None,

              stdout=None, stderr=subprocess.PIPE, universal_newlines=False)

  

      @patch('subprocess.Popen')
@@ -1226,12 +1224,15 @@ 

      def test_run_command_in_a_directory(self, Popen):

          Popen.return_value.wait.return_value = 0

  

-         self.cmd._run_command(['rpmbuild'], cwd='/tmp/builddir')

+         tempdir = tempfile.mkdtemp(prefix='rpkg_test_')

+         self.cmd._run_command(['rpmbuild'], cwd=tempdir)

  

          Popen.assert_called_once_with(

-             ['rpmbuild'], env=os.environ, shell=False, cwd='/tmp/builddir',

+             ['rpmbuild'], env=os.environ, shell=False, cwd=tempdir,

              stdout=None, stderr=None, universal_newlines=False)

  

+         shutil.rmtree(tempdir)

+ 

      @patch('subprocess.Popen')

      def test_return_output_as_text(self, Popen):

          Popen.return_value.wait.return_value = 0

file modified
+3 -2
@@ -122,11 +122,12 @@ 

  

      def redirect_cmd_output(self, cmd, shell=False, env=None, pipe=[], cwd=None):

          if shell:

-             cmd = ' '.join(cmd)

+             raise RuntimeError('Command with shell=True is not allowed because '

+                                'of security reasons (bandit).')

          proc_env = os.environ.copy()

          proc_env.update(env or {})

          proc_env['LANG'] = 'en_US.UTF-8'

-         proc = subprocess.Popen(cmd, shell=shell, cwd=cwd, env=proc_env,

+         proc = subprocess.Popen(cmd, cwd=cwd, env=proc_env,

                                  stdout=subprocess.PIPE, stderr=subprocess.PIPE)

          stdout, stderr = proc.communicate()

          sys.stdout.write(stdout)

JIRA: RHELCMP-9241

These changes will probably require similar steps for dependent tools like rhpkg and fedpkg.

Signed-off-by: Ondrej Nosek onosek@redhat.com

If any xpkg tool gets here, it will be basically broken. How about just printing a deprecation warning for some time and removing the argument after other tools have had time to adjust?

debug_cmd is used only for error reporting to the user, right? It may contain improperly quoted arguments, but that may not be a big deal.

If any xpkg tool gets here, it will be basically broken. How about just printing a deprecation warning for some time and removing the argument after other tools have had time to adjust?

I think it is similar to self._rpmdefines. It also has the potential to destroy the workflow of dependent tools. I counted on that there will be changes needed on other sites needed before releasing this code.

But yes, this (change in _run_commmand) is not necessary to proceed and adds risk.

Right, rpmdefines has a different format. Yeah, I don't think that can be backwards compatible.

rebased onto a2c5cbea1a80d66485732797b72bb0b0b7543b3a

2 years ago

rebased onto b8d3c2850852930a5da7dd5b4e8f8b9a35acdeb2

2 years ago

rebased onto 88c2dd8

2 years ago

Pull-Request has been merged by onosek

2 years ago