From b08d50f80db0813be96f9cfbf18b20834abcb558 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 26 2019 11:47:00 +0000
Subject: [PATCH 1/4] check architecture names for mistakes


Fixes: https://pagure.io/koji/issue/1237

---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index bac7203..2591727 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -875,10 +875,11 @@ def _pkglist_remove(tag_id, pkg_id):
 
 def _pkglist_add(tag_id, pkg_id, owner, block, extra_arches):
     #revoke old entry (if present)
-    _pkglist_remove(tag_id, pkg_id)
     data = dslice(locals(), ('tag_id', 'owner', 'extra_arches'))
     data['package_id'] = pkg_id
     data['blocked'] = block
+    validate_arches_string(data['extra_arches'])
+    _pkglist_remove(tag_id, pkg_id)
     insert = InsertProcessor('tag_packages', data=data)
     insert.make_create()  #XXX user_id?
     insert.execute()
@@ -915,6 +916,8 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force,
             assert_policy('package_list', policy_data)
     if not pkg:
         pkg = lookup_package(pkginfo, create=True)
+    # validate arches before running callbacks
+    validate_arches_string(extra_arches)
     koji.plugin.run_callbacks('prePackageListChange', action=action, tag=tag, package=pkg, owner=owner,
                               block=block, extra_arches=extra_arches, force=force, update=update)
     # first check to see if package is:
@@ -2983,6 +2986,16 @@ def lookup_build_target(info, strict=False, create=False):
     return lookup_name('build_target', info, strict, create)
 
 
+def validate_arches_string(arches, strict=True):
+    """run checks against architectures space-separated strings"""
+    if re.match(r'^[a-zA-Z0-9_\- ]+$', arches):
+        return True
+    elif strict:
+        raise koji.GenericError("Architecture can be only [a-zA-Z0-9_-]")
+    else:
+        return False
+
+
 def create_tag(name, parent=None, arches=None, perm=None, locked=False, maven_support=False, maven_include_all=False, extra=None):
     """Create a new tag"""
     context.session.assertPerm('admin')
@@ -2997,6 +3010,9 @@ def _create_tag(name, parent=None, arches=None, perm=None, locked=False, maven_s
         raise koji.GenericError("Tag name %s is too long. Max length is %s characters",
                                 name, max_name_length)
 
+    if arches is not None:
+        validate_arches_string(arches)
+
     if not context.opts.get('EnableMaven') and (maven_support or maven_include_all):
         raise koji.GenericError("Maven support not enabled")
 
@@ -3177,6 +3193,11 @@ SET name = %(name)s
 WHERE id = %(tagID)i"""
         _dml(update, values)
 
+    # sanitize architecture names (space-separated string)
+    arches = kwargs.get('arches')
+    if arches and tag['arches'] != arches:
+        validate_arches_string(arches)
+
     #check for changes
     data = tag.copy()
     changed = False
diff --git a/tests/test_hub/test_create_tag.py b/tests/test_hub/test_create_tag.py
index f483211..95daddb 100644
--- a/tests/test_hub/test_create_tag.py
+++ b/tests/test_hub/test_create_tag.py
@@ -1,8 +1,6 @@
+# coding: utf-8
 from __future__ import absolute_import
-import copy
 import mock
-import shutil
-import tempfile
 try:
     import unittest2 as unittest
 except ImportError:
@@ -67,3 +65,20 @@ class TestCreateTag(unittest.TestCase):
         self.assertEqual(insert.data, values)
         self.assertEqual(insert.rawdata, {})
         insert = self.inserts[0]
+
+    def test_invalid_archs(self):
+        self.get_tag.return_value = None
+        self.get_tag_id.return_value = 99
+        self.context.event_id = 42
+        self.context.session.user_id = 23
+
+        with self.assertRaises(koji.GenericError):
+            kojihub.create_tag('newtag', arches=u'ěšč')
+
+        with self.assertRaises(koji.GenericError):
+            kojihub.create_tag('newtag', arches=u'arch1;arch2')
+
+        with self.assertRaises(koji.GenericError):
+            kojihub.create_tag('newtag', arches=u'arch1,arch2')
+
+        self.assertEqual(len(self.inserts), 0)
diff --git a/tests/test_hub/test_edit_tag.py b/tests/test_hub/test_edit_tag.py
index cb32059..e207074 100644
--- a/tests/test_hub/test_edit_tag.py
+++ b/tests/test_hub/test_edit_tag.py
@@ -1,3 +1,4 @@
+# coding: utf-8
 from __future__ import absolute_import
 import mock
 try:
@@ -197,3 +198,40 @@ WHERE id = %(tagID)i""", {'name': 'newtag', 'tagID': 333})
         self.get_perm_id.assert_called_once()
         self._singleValue.assert_called_once()
         self.assertEqual(cm.exception.args[0], 'Name newtag already taken by tag 2')
+
+    def test_invalid_archs(self):
+        self.get_tag.return_value = {
+            'create_event': 42,
+            'creator_id': 23,
+            'arches': 'arch1 arch2',
+            'locked': True,
+            'maven_include_all': True,
+            'maven_support': True,
+            'perm_id': None,
+            'tag_id': 333,
+            'name': 'newtag',
+            'id': 345,
+        }
+
+        # valid
+        kwargs = {
+            'name': 'newtag',
+            'arches': 'valid_arch',
+        }
+        kojihub._edit_tag('tag', **kwargs)
+
+        # invalid 1
+        kwargs['arches'] = u'ěšč'
+        with self.assertRaises(koji.GenericError):
+            kojihub._edit_tag('tag', **kwargs)
+
+        # invalid 2
+        kwargs['arches'] = u'arch1;arch2'
+        with self.assertRaises(koji.GenericError):
+            kojihub._edit_tag('tag', **kwargs)
+
+        # invalid 2
+        kwargs['arches'] = u'arch1,arch2'
+        with self.assertRaises(koji.GenericError):
+            kojihub._edit_tag('tag', **kwargs)
+
diff --git a/tests/test_hub/test_validate_arches_string.py b/tests/test_hub/test_validate_arches_string.py
new file mode 100644
index 0000000..9261421
--- /dev/null
+++ b/tests/test_hub/test_validate_arches_string.py
@@ -0,0 +1,25 @@
+# coding: utf-8
+from __future__ import absolute_import
+try:
+    import unittest2 as unittest
+except ImportError:
+    import unittest
+import koji
+import kojihub
+
+class TestValidateArchesString(unittest.TestCase):
+    def test_valid_arches(self):
+        kojihub.validate_arches_string('i386')
+        kojihub.validate_arches_string('i386 x86_64')
+        kojihub.validate_arches_string('i386 x86_64   ')
+
+    def test_invalid_arches(self):
+        with self.assertRaises(koji.GenericError):
+            kojihub.validate_arches_string(u'ěšč')
+
+        with self.assertRaises(koji.GenericError):
+            kojihub.validate_arches_string(u'i386;x86_64')
+
+        with self.assertRaises(koji.GenericError):
+            kojihub.validate_arches_string(u'i386,x86_64')
+

From e0969f966ce7d137e9084a7faa4044b3d1cb4c4c Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 26 2019 11:47:56 +0000
Subject: [PATCH 2/4] use same arch parsing for cli and hub


Related: https://pagure.io/koji/issue/1237

---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index 8efe4de..e51c08e 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -195,7 +195,7 @@ def handle_edit_host(options, session, args):
         if val is not None:
             vals[key] = val
     if 'arches' in vals:
-        vals['arches'] = parse_arches(vals['arches'])
+        vals['arches'] = koji.parse_arches(vals['arches'])
 
     session.multicall = True
     for host in args:
@@ -350,7 +350,7 @@ def handle_add_pkg(goptions, session, args):
             continue
         to_add.append(package)
     if options.extra_arches:
-        opts['extra_arches'] = parse_arches(options.extra_arches)
+        opts['extra_arches'] = koji.parse_arches(options.extra_arches)
 
     # add the packages
     print("Adding %i packages to tag %s" % (len(to_add), dsttag['name']))
@@ -472,7 +472,7 @@ def handle_build(options, session, args):
     source = args[1]
     opts = {}
     if build_opts.arch_override:
-        opts['arch_override'] = parse_arches(build_opts.arch_override)
+        opts['arch_override'] = koji.parse_arches(build_opts.arch_override)
     for key in ('skip_tag', 'scratch', 'repo_id', 'fail_fast'):
         val = getattr(build_opts, key)
         if val is not None:
@@ -4823,7 +4823,7 @@ def handle_add_tag(goptions, session, args):
     if options.parent:
         opts['parent'] = options.parent
     if options.arches:
-        opts['arches'] = parse_arches(options.arches)
+        opts['arches'] = koji.parse_arches(options.arches)
     if options.maven_support:
         opts['maven_support'] = True
     if options.include_all:
@@ -4865,7 +4865,7 @@ def handle_edit_tag(goptions, session, args):
     tag = args[0]
     opts = {}
     if options.arches:
-        opts['arches'] = parse_arches(options.arches)
+        opts['arches'] = koji.parse_arches(options.arches)
     if options.no_perm:
         opts['perm_id'] = None
     elif options.perm:
@@ -6237,7 +6237,7 @@ def handle_set_pkg_arches(goptions, session, args):
         parser.error(_("Please specify an archlist, a tag, and at least one package"))
         assert False  # pragma: no cover
     activate_session(session, goptions)
-    arches = parse_arches(args[0])
+    arches = koji.parse_arches(args[0])
     tag = args[1]
     for package in args[2:]:
         #really should implement multicall...
diff --git a/cli/koji_cli/lib.py b/cli/koji_cli/lib.py
index 8e8bf99..5d77954 100644
--- a/cli/koji_cli/lib.py
+++ b/cli/koji_cli/lib.py
@@ -20,6 +20,8 @@ except ImportError:  # pragma: no cover
 
 import koji
 from koji.util import to_list
+# import parse_arches to current namespace for backward compatibility
+from koji import parse_arches
 
 # for compatibility with plugins based on older version of lib
 # Use optparse imports directly in new code.
@@ -132,26 +134,6 @@ def print_task_recurse(task,depth=0):
         print_task_recurse(child,depth+1)
 
 
-def parse_arches(arches, to_list=False):
-    """Normalize user input for a list of arches.
-
-    This method parses a single comma- or space-separated string of arches and
-    returns a space-separated string.
-
-    :param str arches: comma- or space-separated string of arches, eg.
-                       "x86_64,ppc64le", or "x86_64 ppc64le"
-    :param bool to_list: return a list of each arch, instead of a single
-                         string. This is False by default.
-    :returns: a space-separated string like "x86_64 ppc64le", or a list like
-              ['x86_64', 'ppc64le'].
-    """
-    arches = arches.replace(',', ' ').split()
-    if to_list:
-        return arches
-    else:
-        return ' '.join(arches)
-
-
 class TaskWatcher(object):
 
     def __init__(self,task_id,session,level=0,quiet=False):
diff --git a/hub/kojihub.py b/hub/kojihub.py
index 2591727..6477e84 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -878,7 +878,7 @@ def _pkglist_add(tag_id, pkg_id, owner, block, extra_arches):
     data = dslice(locals(), ('tag_id', 'owner', 'extra_arches'))
     data['package_id'] = pkg_id
     data['blocked'] = block
-    validate_arches_string(data['extra_arches'])
+    data['extra_arches'] = koji.parse_arches(data['extra_arches'], strict=True, allow_none=True)
     _pkglist_remove(tag_id, pkg_id)
     insert = InsertProcessor('tag_packages', data=data)
     insert.make_create()  #XXX user_id?
@@ -917,7 +917,7 @@ def _direct_pkglist_add(taginfo, pkginfo, owner, block, extra_arches, force,
     if not pkg:
         pkg = lookup_package(pkginfo, create=True)
     # validate arches before running callbacks
-    validate_arches_string(extra_arches)
+    extra_arches = koji.parse_arches(extra_arches, strict=True, allow_none=True)
     koji.plugin.run_callbacks('prePackageListChange', action=action, tag=tag, package=pkg, owner=owner,
                               block=block, extra_arches=extra_arches, force=force, update=update)
     # first check to see if package is:
@@ -2986,16 +2986,6 @@ def lookup_build_target(info, strict=False, create=False):
     return lookup_name('build_target', info, strict, create)
 
 
-def validate_arches_string(arches, strict=True):
-    """run checks against architectures space-separated strings"""
-    if re.match(r'^[a-zA-Z0-9_\- ]+$', arches):
-        return True
-    elif strict:
-        raise koji.GenericError("Architecture can be only [a-zA-Z0-9_-]")
-    else:
-        return False
-
-
 def create_tag(name, parent=None, arches=None, perm=None, locked=False, maven_support=False, maven_include_all=False, extra=None):
     """Create a new tag"""
     context.session.assertPerm('admin')
@@ -3010,8 +3000,7 @@ def _create_tag(name, parent=None, arches=None, perm=None, locked=False, maven_s
         raise koji.GenericError("Tag name %s is too long. Max length is %s characters",
                                 name, max_name_length)
 
-    if arches is not None:
-        validate_arches_string(arches)
+    arches = koji.parse_arches(arches, strict=True, allow_none=True)
 
     if not context.opts.get('EnableMaven') and (maven_support or maven_include_all):
         raise koji.GenericError("Maven support not enabled")
@@ -3196,7 +3185,7 @@ WHERE id = %(tagID)i"""
     # sanitize architecture names (space-separated string)
     arches = kwargs.get('arches')
     if arches and tag['arches'] != arches:
-        validate_arches_string(arches)
+        kwargs['arches'] = koji.parse_arches(arches, strict=True, allow_none=True)
 
     #check for changes
     data = tag.copy()
diff --git a/koji/__init__.py b/koji/__init__.py
index 6e667ce..0d6713c 100644
--- a/koji/__init__.py
+++ b/koji/__init__.py
@@ -1104,6 +1104,37 @@ def canonArch(arch):
     else:
         return arch
 
+def parse_arches(arches, to_list=False, strict=False, allow_none=False):
+    """Normalize user input for a list of arches.
+
+    This method parses a single comma- or space-separated string of arches and
+    returns a space-separated string.
+
+    Raise an error if arches string contain non-allowed characters. In strict
+    version allow only space-separated strings (db input).
+
+    :param str arches: comma- or space-separated string of arches, eg.
+                       "x86_64,ppc64le", or "x86_64 ppc64le"
+    :param bool to_list: return a list of each arch, instead of a single
+                         string. This is False by default.
+    :param bool allow_none: convert None to ""
+    :returns: a space-separated string like "x86_64 ppc64le", or a list like
+              ['x86_64', 'ppc64le'].
+    """
+    if allow_none and arches is None:
+        arches = ''
+    if not strict:
+        arches = arches.replace(',', ' ')
+    if not re.match(r'^[a-zA-Z0-9_\- ]*$', arches):
+        raise GenericError("Architecture can be only [a-zA-Z0-9_-]")
+
+    arches = arches.split()
+    if to_list:
+        return arches
+    else:
+        return ' '.join(arches)
+
+
 class POMHandler(xml.sax.handler.ContentHandler):
     def __init__(self, values, fields):
         xml.sax.handler.ContentHandler.__init__(self)
diff --git a/tests/test_hub/test_validate_arches_string.py b/tests/test_hub/test_validate_arches_string.py
deleted file mode 100644
index 9261421..0000000
--- a/tests/test_hub/test_validate_arches_string.py
+++ /dev/null
@@ -1,25 +0,0 @@
-# coding: utf-8
-from __future__ import absolute_import
-try:
-    import unittest2 as unittest
-except ImportError:
-    import unittest
-import koji
-import kojihub
-
-class TestValidateArchesString(unittest.TestCase):
-    def test_valid_arches(self):
-        kojihub.validate_arches_string('i386')
-        kojihub.validate_arches_string('i386 x86_64')
-        kojihub.validate_arches_string('i386 x86_64   ')
-
-    def test_invalid_arches(self):
-        with self.assertRaises(koji.GenericError):
-            kojihub.validate_arches_string(u'ěšč')
-
-        with self.assertRaises(koji.GenericError):
-            kojihub.validate_arches_string(u'i386;x86_64')
-
-        with self.assertRaises(koji.GenericError):
-            kojihub.validate_arches_string(u'i386,x86_64')
-
diff --git a/tests/test_lib/test_parse_arches.py b/tests/test_lib/test_parse_arches.py
new file mode 100644
index 0000000..a5aaeac
--- /dev/null
+++ b/tests/test_lib/test_parse_arches.py
@@ -0,0 +1,31 @@
+# coding: utf-8
+from __future__ import absolute_import
+try:
+    import unittest2 as unittest
+except ImportError:
+    import unittest
+import koji
+
+class TestParseArchesString(unittest.TestCase):
+    def test_parse_valid_arches(self):
+        r = koji.parse_arches('i386', to_list=True)
+        self.assertEqual(['i386'], r)
+
+        r = koji.parse_arches('i386 x86_64', to_list=True)
+        self.assertEqual(['i386', 'x86_64'], r)
+
+        r = koji.parse_arches('i386 x86_64   ', to_list=True)
+        self.assertEqual(['i386', 'x86_64'], r)
+
+        r = koji.parse_arches('i386,x86_64', to_list=True)
+        self.assertEqual(['i386', 'x86_64'], r)
+
+    def test_parse_invalid_arches(self):
+        with self.assertRaises(koji.GenericError):
+            koji.parse_arches(u'ěšč')
+
+        with self.assertRaises(koji.GenericError):
+            koji.parse_arches(u'i386;x86_64')
+
+        with self.assertRaises(koji.GenericError):
+            koji.parse_arches(u'i386,x86_64', strict=True)

From cab8a1a1f45e2106aec963edc25aec9d074278a1 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 26 2019 11:52:15 +0000
Subject: [PATCH 3/4] fix test for parse_arches


---

diff --git a/tests/test_hub/test_create_tag.py b/tests/test_hub/test_create_tag.py
index 95daddb..bf79501 100644
--- a/tests/test_hub/test_create_tag.py
+++ b/tests/test_hub/test_create_tag.py
@@ -53,7 +53,7 @@ class TestCreateTag(unittest.TestCase):
         insert = self.inserts[0]
         self.assertEqual(insert.table, 'tag_config')
         values = {
-            'arches': None,
+            'arches': '',
             'create_event': 42,
             'creator_id': 23,
             'locked': False,

From 4ba25ea3e1ac134eb853b66960773f8de7a41438 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Apr 09 2019 12:34:37 +0000
Subject: [PATCH 4/4] validate archs in addHost


---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index 6477e84..7db5b27 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -10886,6 +10886,9 @@ class RootExports(object):
     def addHost(self, hostname, arches, krb_principal=None):
         """Add a host to the database"""
         context.session.assertPerm('admin')
+        # validate arches
+        arches = " ".join(arches)
+        arches = koji.parse_arches(arches, strict=True)
         if get_host(hostname):
             raise koji.GenericError('host already exists: %s' % hostname)
         q = """SELECT id FROM channels WHERE name = 'default'"""
@@ -10903,7 +10906,7 @@ class RootExports(object):
         _dml(insert, dslice(locals(), ('hostID', 'userID', 'hostname')))
 
         insert = InsertProcessor('host_config')
-        insert.set(host_id=hostID, arches=" ".join(arches))
+        insert.set(host_id=hostID, arches=arches)
         insert.make_create()
         insert.execute()