From 88c2dd8a3ff3d4c153ed13dc46ba71546caf8cd2 Mon Sep 17 00:00:00 2001 From: Ondrej Nosek Date: Jul 27 2022 00:25:29 +0000 Subject: Fix high level bandit findings Changes the format of '_rpmdefines' definitions: '--define' items are split into tokens to be safely executed without direct 'shell=True' argument in '_run_command' method. This requires further changes in dependent tools when they contain its '_rpmdefines' structure. JIRA: RHELCMP-9241 Signed-off-by: Ondrej Nosek --- diff --git a/pyrpkg/__init__.py b/pyrpkg/__init__.py index 3d48998..7084bdc 100644 --- a/pyrpkg/__init__.py +++ b/pyrpkg/__init__.py @@ -524,7 +524,7 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): # 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): # 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 @@ class Commands(object): # 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 + 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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 @@ class Commands(object): 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): """ diff --git a/tests/test_cli.py b/tests/test_cli.py index 6a692ac..c532f71 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -673,35 +673,35 @@ class TestSrpm(CliTestCase): 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 @@ class TestCompile(CliTestCase): 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 @@ class TestCompile(CliTestCase): 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 @@ class TestPrep(CliTestCase): 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 @@ class TestPrep(CliTestCase): 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 @@ class TestPrep(CliTestCase): 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 @@ class TestInstall(CliTestCase): 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 @@ class TestInstall(CliTestCase): 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 @@ class TestLocal(CliTestCase): '--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 @@ class TestLocal(CliTestCase): 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 @@ class TestVerifyFiles(CliTestCase): 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 @@ class TestVerifyFiles(CliTestCase): 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 @@ class TestLint(CliTestCase): _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 @@ class TestLint(CliTestCase): _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 @@ class TestLint(CliTestCase): _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 @@ class TestLint(CliTestCase): _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 @@ class TestLint(CliTestCase): _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 @@ class TestLint(CliTestCase): _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 @@ class TestLint(CliTestCase): _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 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): 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) diff --git a/tests/test_commands.py b/tests/test_commands.py index 758dd9a..1c0fdc8 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -187,7 +187,7 @@ class LoadNameVerRelTest(CommandTestCase): 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 @@ class LoadRPMDefinesTest(CommandTestCase): # '_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 @@ class TestGetLatestCommit(CommandTestCase): 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 @@ class TestCleanupTmpDir(CommandTestCase): 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 @@ class TestLint(CommandTestCase): os.path.join(cmd.path, 'docpkg.spec'), srpm_path, bin_path, - ], shell=True)]) + ])]) @patch('glob.glob') @patch('os.path.exists') @@ -1064,7 +1062,7 @@ class TestLint(CommandTestCase): os.path.join(cmd.path, 'docpkg.spec'), srpm_path, bin_path, - ], shell=True)]) + ])]) @patch('glob.glob') @patch('os.path.exists') @@ -1101,7 +1099,7 @@ class TestLint(CommandTestCase): os.path.join(cmd.path, 'docpkg.spec'), srpm_path, bin_path, - ], shell=True)]) + ])]) class TestRunCommand(CommandTestCase): @@ -1141,11 +1139,11 @@ class TestRunCommand(CommandTestCase): 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 @@ class TestRunCommand(CommandTestCase): 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 @@ class TestRunCommand(CommandTestCase): 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 diff --git a/tests/utils.py b/tests/utils.py index fa51625..4c831c6 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -122,11 +122,12 @@ class Utils(object): 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)