Repository: CTFd/CTFd
Pull request #782 (2018-12-05 11:33:50)

Fix divison-by-zero when adding requirement to dynamic challenge
I noticed that when I wanted to add a pre-requisite challenge to a dynamic challenge, I received a divison-by-zero error.

During a requirement addition, only the new required challenge id is sent. Because the initial, minimum and decay values are not set, the update path in the __init.py set them to a default 0. 
Later on, when a calculation takes place with these values, a divison-by-zero happens before anything gets committed to the database.

I've added a simple check before other calculations to check if data received from client includes a requirements (is not None). If there is data there, it means the request is a requirement addition so we can just commit those changes and return. At the same time, updating values and other data will not include a requirement field so everything else will work as it does currently.

codecov[bot]: # [Codecov](https://codecov.io/gh/CTFd/CTFd/pull/782?src=pr&el=h1) Report
> Merging [#782](https://codecov.io/gh/CTFd/CTFd/pull/782?src=pr&el=desc) into [master](https://codecov.io/gh/CTFd/CTFd/commit/473acdbdc3457f4fd778fbd692ae353c531804ba?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `66.66%`.

[![Impacted file tree graph](https://codecov.io/gh/CTFd/CTFd/pull/782/graphs/tree.svg?width=650&token=IfPqkWkt9A&height=150&src=pr)](https://codecov.io/gh/CTFd/CTFd/pull/782?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
- Coverage   83.61%   83.57%   -0.04%     
==========================================
  Files          91       91              
  Lines        4503     4506       +3     
==========================================
+ Hits         3765     3766       +1     
- Misses        738      740       +2
```


| [Impacted Files](https://codecov.io/gh/CTFd/CTFd/pull/782?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [CTFd/plugins/dynamic\_challenges/\_\_init\_\_.py](https://codecov.io/gh/CTFd/CTFd/pull/782/diff?src=pr&el=tree#diff-Q1RGZC9wbHVnaW5zL2R5bmFtaWNfY2hhbGxlbmdlcy9fX2luaXRfXy5weQ==) | `89.42% <66.66%> (-1.67%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/CTFd/CTFd/pull/782?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/CTFd/CTFd/pull/782?src=pr&el=footer). Last update [473acdb...1ef30ab](https://codecov.io/gh/CTFd/CTFd/pull/782?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ColdHeat: Thanks for reporting this issue. I'll take a look at it when I have the time tomorrow. Feel free to ping me if it slips!
ptr0x01: Thanks, I went ahead and added the unit test for this check to improve coverage.

Patch:
diff --git a/CTFd/plugins/dynamic_challenges/__init__.py b/CTFd/plugins/dynamic_challenges/__init__.py
index 05c76914..a7dce7c9 100644
--- a/CTFd/plugins/dynamic_challenges/__init__.py
+++ b/CTFd/plugins/dynamic_challenges/__init__.py
@@ -87,10 +87,11 @@ class DynamicValueChallenge(BaseChallenge):
         :return:
         """
         data = request.form or request.get_json()
-        data['initial'] = float(data.get('initial', 0))
-        data['minimum'] = float(data.get('minimum', 0))
-        data['decay'] = float(data.get('decay', 0))
+
         for attr, value in data.items():
+            # We need to set these to floats so that the next operations don't operate on strings
+            if attr in ('initial', 'minimum', 'decay'):
+                value = float(value)
             setattr(challenge, attr, value)
 
         Model = get_model()
@@ -228,9 +229,9 @@ class DynamicValueChallenge(BaseChallenge):
 class DynamicChallenge(Challenges):
     __mapper_args__ = {'polymorphic_identity': 'dynamic'}
     id = db.Column(None, db.ForeignKey('challenges.id'), primary_key=True)
-    initial = db.Column(db.Integer)
-    minimum = db.Column(db.Integer)
-    decay = db.Column(db.Integer)
+    initial = db.Column(db.Integer, default=0)
+    minimum = db.Column(db.Integer, default=0)
+    decay = db.Column(db.Integer, default=0)
 
     def __init__(self, *args, **kwargs):
         super(DynamicChallenge, self).__init__(**kwargs)


Test patch:
diff --git a/tests/challenges/test_dynamic.py b/tests/challenges/test_dynamic.py
index f3135fec..b02691e2 100644
--- a/tests/challenges/test_dynamic.py
+++ b/tests/challenges/test_dynamic.py
@@ -85,6 +85,63 @@ def test_can_update_dynamic_challenge():
     destroy_ctfd(app)
 
 
+def test_can_add_requirement_dynamic_challenge():
+    """Test that requirements can be added to dynamic challenges"""
+    app = create_ctfd(enable_plugins=True)
+    with app.app_context():
+        challenge_data = {
+            "name": "name",
+            "category": "category",
+            "description": "description",
+            "value": 100,
+            "decay": 20,
+            "minimum": 1,
+            "state": "hidden",
+            "type": "dynamic"
+        }
+        req = FakeRequest(form=challenge_data)
+        challenge = DynamicValueChallenge.create(req)
+
+        assert challenge.value == 100
+        assert challenge.initial == 100
+        assert challenge.decay == 20
+        assert challenge.minimum == 1
+
+        challenge_data = {
+            "name": "second_name",
+            "category": "category",
+            "description": "new_description",
+            "value": "200",
+            "initial": "200",
+            "decay": "40",
+            "minimum": "5",
+            "max_attempts": "0",
+            "state": "visible"
+        }
+
+        req = FakeRequest(form=challenge_data)
+        challenge = DynamicValueChallenge.create(req)
+
+        assert challenge.name == 'second_name'
+        assert challenge.description == "new_description"
+        assert challenge.value == 200
+        assert challenge.initial == 200
+        assert challenge.decay == 40
+        assert challenge.minimum == 5
+        assert challenge.state == "visible"
+
+        challenge_data = {
+            "requirements": [1]
+        }
+
+        req = FakeRequest(form=challenge_data)
+        challenge = DynamicValueChallenge.update(challenge, req)
+
+        assert challenge.requirements == [1]
+
+    destroy_ctfd(app)
+
+
 def test_can_delete_dynamic_challenge():
     app = create_ctfd(enable_plugins=True)
     with app.app_context():