From b55afb5991589cf43bce56ee70517610251f3cc9 Mon Sep 17 00:00:00 2001
From: dekehn <dekehn@gmail.com>
Date: Thu, 20 Jan 2022 20:38:06 +0000
Subject: Checks for invalid denylist regex patterns

Adds new field check method DenyListFields to validate
the pattern string. in addition, check for a zero
length string as well.

Closes-Bug: #1934252
Change-Id: I2b69025fc11125bb73a4e0f8c0dedad951399cbf
---
 designate/objects/blacklist.py                     |  4 +-
 designate/objects/fields.py                        | 22 +++++++++++
 .../tests/test_api/test_v2/test_blacklists.py      | 45 ++++++++++++++++++++++
 ...atterns-not-being-checked-ec1f1316ccc6cb1d.yaml | 16 ++++++++
 4 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 releasenotes/notes/Fix-to-address-denylist-invalid-patterns-not-being-checked-ec1f1316ccc6cb1d.yaml

diff --git a/designate/objects/blacklist.py b/designate/objects/blacklist.py
index 1a5eb388..a0dd4fcf 100644
--- a/designate/objects/blacklist.py
+++ b/designate/objects/blacklist.py
@@ -20,8 +20,8 @@ from designate.objects import fields
 class Blacklist(base.DictObjectMixin, base.PersistentObjectMixin,
                 base.DesignateObject):
     fields = {
-        'pattern': fields.StringFields(maxLength=255),
-        'description': fields.StringFields(maxLength=160, nullable=True),
+        'pattern': fields.DenylistFields(maxLength=255),
+        'description': fields.DenylistFields(maxLength=160, nullable=True),
     }
 
     STRING_KEYS = [
diff --git a/designate/objects/fields.py b/designate/objects/fields.py
index 9b533438..dd3268c8 100644
--- a/designate/objects/fields.py
+++ b/designate/objects/fields.py
@@ -451,3 +451,25 @@ class IPOrHost(IPV4AndV6AddressField):
             if not re.match(StringFields.RE_ZONENAME, value):
                 raise ValueError("%s is not IP address or host name" % value)
         return value
+
+
+class DenylistFields(StringFields):
+    def __init__(self, **kwargs):
+        super(DenylistFields, self).__init__(**kwargs)
+
+    def coerce(self, obj, attr, value):
+        value = super(DenylistFields, self).coerce(obj, attr, value)
+
+        if value is None:
+            return self._null(obj, attr)
+
+        # determine the validity if a regex expression filter has been used.
+        msg = ("%s is not a valid regular expression" % value)
+        if not len(value):
+            raise ValueError(msg)
+        try:
+            re.compile(value)
+        except Exception:
+            raise ValueError(msg)
+
+        return value
diff --git a/designate/tests/test_api/test_v2/test_blacklists.py b/designate/tests/test_api/test_v2/test_blacklists.py
index 2dff9b8c..0677b7e1 100644
--- a/designate/tests/test_api/test_v2/test_blacklists.py
+++ b/designate/tests/test_api/test_v2/test_blacklists.py
@@ -165,3 +165,48 @@ class ApiV2BlacklistsTest(ApiV2TestCase):
         url = '/blacklists?description=test'
         self.policy({'find_blacklists': '@'})
         self._assert_exception('bad_request', 400, self.client.get, url)
+
+    def test_create_invalid_denylist_pattern(self):
+        self.policy({'create_blacklist': '@'})
+        body = {
+            'description': u'This is the description.'
+        }
+
+        url = '/blacklists/'
+
+        # doing each pattern individually so upon error one can trace
+        # back to the exact line number
+        body['pattern'] = ''
+        self._assert_exception(
+            'invalid_object', 400, self.client.post_json, url, body)
+
+        body['pattern'] = '#(*&^%$%$#@$'
+        self._assert_exception(
+            'invalid_object', 400, self.client.post_json, url, body)
+
+        body['pattern'] = 'a' * 1000
+        self._assert_exception(
+            'invalid_object', 400, self.client.post_json, url, body)
+
+    def test_update_invalid_denylist_pattern(self):
+        blacklist = self.create_blacklist(fixture=0)
+        self.policy({'update_blacklist': '@'})
+
+        url = ('/blacklists/%s' % blacklist['id'])
+
+        # doing each pattern individually so upon error one can trace
+        # back to the exact line number
+        body = {'pattern': ''}
+        self._assert_exception(
+            'invalid_object', 400, self.client.patch_json, url, body,
+            status=400)
+
+        body = {'pattern': '#(*&^%$%$#@$'}
+        self._assert_exception(
+            'invalid_object', 400, self.client.patch_json, url, body,
+            status=400)
+
+        body = {'pattern': 'a' * 1000}
+        self._assert_exception(
+            'invalid_object', 400, self.client.patch_json, url, body,
+            status=400)
diff --git a/releasenotes/notes/Fix-to-address-denylist-invalid-patterns-not-being-checked-ec1f1316ccc6cb1d.yaml b/releasenotes/notes/Fix-to-address-denylist-invalid-patterns-not-being-checked-ec1f1316ccc6cb1d.yaml
new file mode 100644
index 00000000..43a3ca94
--- /dev/null
+++ b/releasenotes/notes/Fix-to-address-denylist-invalid-patterns-not-being-checked-ec1f1316ccc6cb1d.yaml
@@ -0,0 +1,16 @@
+---
+fixes:
+  - |
+    Fixes `bug 1934252`_ which ignored invalid denylist patterns. The fix
+    entailed checking the pattern string via regular expression compiler and
+    testing for zero length.
+
+    Previously you could create blacklist/denylist using string that cannot
+    be used either as a regex or as a zone name, for example:
+    patterns = ['', ``'#(*&^%$%$#@$']``
+
+    In addition, the server will return a 400 BadRequest response to an
+    invalid pattern.
+
+    .. _Bug 1934252: https://bugs.launchpad.net/designate/+bug/1934252
+
-- 
cgit v1.2.1