Add a fix for receiving non-string Config values (#1931)

* Properly receive non-string config values (None, bool, integers, etc) in /api/v1/config
* Closes #1928 
* Fix the response schema for `PATCH /api/v1/configs/<config_key>` in error situations

Overall we weren't particularly strict before and we should try to stay a little lax so we don't break anything.

CTFd/api/v1/config.py

@@ -89,9 +89,6 @@ class ConfigList(Resource):
         response = schema.load(req)
 
         if response.errors:
-            # Inject config key into error
-            config_key = response.data["key"]
-            response.errors["value"][0] = f"{config_key} config is too long"
             return {"success": False, "errors": response.errors}, 400
 
         db.session.add(response.data)
@@ -117,9 +114,6 @@ class ConfigList(Resource):
         for key, value in req.items():
             response = schema.load({"key": key, "value": value})
             if response.errors:
-                # Inject config key into error
-                config_key = response.data["key"]
-                response.errors["value"][0] = f"{config_key} config is too long"
                 return {"success": False, "errors": response.errors}, 400
             set_config(key=key, value=value)
 
@@ -169,11 +163,11 @@ class Config(Resource):
             schema = ConfigSchema()
             data["key"] = config_key
             response = schema.load(data)
-            db.session.add(response.data)
 
         if response.errors:
-            return response.errors, 400
+            return {"success": False, "errors": response.errors}, 400
 
+        db.session.add(response.data)
         db.session.commit()
 
         response = schema.dump(response.data)

CTFd/schemas/config.py

@@ -1,10 +1,28 @@
-from marshmallow import validate
+from marshmallow import fields
+from marshmallow.exceptions import ValidationError
 from marshmallow_sqlalchemy import field_for
 
 from CTFd.models import Configs, ma
 from CTFd.utils import string_types
 
 
+class ConfigValueField(fields.Field):
+    """
+    Custom value field for Configs so that we can perform validation of values
+    """
+
+    def _deserialize(self, value, attr, data, **kwargs):
+        if isinstance(value, str):
+            # 65535 bytes is the size of a TEXT column in MySQL
+            # You may be able to exceed this in other databases
+            # but MySQL is our database of record
+            if len(value) > 65535:
+                raise ValidationError(f'{data["key"]} config is too long')
+            return value
+        else:
+            return value
+
+
 class ConfigSchema(ma.ModelSchema):
     class Meta:
         model = Configs
@@ -13,11 +31,7 @@ class ConfigSchema(ma.ModelSchema):
 
     views = {"admin": ["id", "key", "value"]}
     key = field_for(Configs, "key", required=True)
-    value = field_for(
+    value = ConfigValueField(allow_none=True, required=True)
-        Configs,
-        "value",
-        validate=[validate.Length(max=64000, error="Config is too long")],
-    )
 
     def __init__(self, view=None, *args, **kwargs):
         if view:

tests/api/v1/test_config.py

@@ -105,21 +105,73 @@ def test_api_config_delete_admin():
     destroy_ctfd(app)
 
 
-def test_long_values():
+def test_config_value_types():
-    """Can a config value that is bigger than 64,000 be accepted"""
+    """Test that we properly receive values according to schema"""
     app = create_ctfd()
     with app.app_context():
         with login_as_user(app, "admin") as admin:
-            long_text = "a" * 65000
+            # Test new configs error out if too long
+            long_text = "a" * 65536
             r = admin.post(
-                "/api/v1/configs", json={"key": "ctf_footer", "value": long_text}
+                "/api/v1/configs", json={"key": "new_ctf_config", "value": long_text}
             )
             data = r.get_json()
-            assert data["errors"]["value"][0] == "ctf_footer config is too long"
+            assert data["errors"]["value"][0] == "new_ctf_config config is too long"
+            assert r.status_code == 400
+            r = admin.post(
+                "/api/v1/configs", json={"key": "new_ctf_config", "value": "test"}
+            )
+            assert r.status_code == 200
+            assert get_config("new_ctf_config") == "test"
 
-            r = admin.patch("/api/v1/configs", json={"ctf_theme": long_text})
+            # Test strings too long error out
+            r = admin.patch("/api/v1/configs", json={"ctf_footer": long_text})
             data = r.get_json()
-            assert data["errors"]["value"][0] == "ctf_theme config is too long"
+            assert data["errors"]["value"][0] == "ctf_footer config is too long"
             assert r.status_code == 400
 
+            # Test regular length strings
+            r = admin.patch(
+                "/api/v1/configs", json={"ctf_footer": "// regular length string"},
+            )
+            assert r.status_code == 200
+            assert get_config("ctf_footer") == "// regular length string"
+
+            # Test booleans can be received
+            r = admin.patch("/api/v1/configs", json={"view_after_ctf": True})
+            assert r.status_code == 200
+            assert bool(get_config("view_after_ctf")) == True
+
+            # Test None can be received
+            assert get_config("mail_username") is None
+            r = admin.patch("/api/v1/configs", json={"mail_username": "testusername"})
+            assert r.status_code == 200
+            assert get_config("mail_username") == "testusername"
+            r = admin.patch("/api/v1/configs", json={"mail_username": None})
+            assert r.status_code == 200
+            assert get_config("mail_username") is None
+
+            # Test integers can be received
+            r = admin.patch("/api/v1/configs", json={"mail_port": 12345})
+            assert r.status_code == 200
+            assert get_config("mail_port") == 12345
+
+            # Test specific config key
+            r = admin.patch(
+                "/api/v1/configs/long_config_test", json={"value": long_text}
+            )
+            data = r.get_json()
+            assert data["errors"]["value"][0] == "long_config_test config is too long"
+            assert r.status_code == 400
+            assert get_config("long_config_test") is None
+            r = admin.patch(
+                "/api/v1/configs/config_test", json={"value": "config_value_test"}
+            )
+            assert r.status_code == 200
+            assert get_config("config_test") == "config_value_test"
+            r = admin.patch(
+                "/api/v1/configs/mail_username", json={"value": "testusername"}
+            )
+            assert r.status_code == 200
+            assert get_config("mail_username") == "testusername"
     destroy_ctfd(app)