Repository: matrix-org/sydent (language: python)

Issue:
Email validator incorrectly handling `sendAttempt`
Introduced in 57ba780bbd27fdffbb6fdf6c4044393f35cb4e16

The annotation for `sendAttempt` is incorrect. It is a `str`. Mypy cannot spot this because it is fed something of type `Any` GRRRRRR.

https://sentry.matrix.org/sentry/sydent/issues/234999/?query=is%3Aunresolved
Convert `send_attempt` to an int again
A type annotation was incorrect, and this lead me to introduce a regression in Sydent 2.5.0.

This restores the old behaviour, but converts to int at the rest level rather than within the validator. It does re-introduce a spec bug, but at least it's the old behaviour.

Fixes #460.


Discussion / hints:

Many thanks!
@clokep asks:
>  Did you by any chance check if there are other situations similar to #460?

My response:
> I haven't seen any in Sentry, but it wouldn't surprise me if there are.
> I'll have a quick look in the spec to see if there are other v1 endpoints accepting non-strings.

Method:
- look through all the deprecated /v1 endpoints in https://matrix.org/docs/spec/identity_service/r0.3.0 
- are there any whose request parameters are _not_ a string?
  - https://matrix.org/docs/spec/identity_service/r0.3.0#deprecated-post-matrix-identity-api-v1-bulk-lookup takes a list of string 2-tuples, but must be given a JSON body
  - https://matrix.org/docs/spec/identity_service/r0.3.0#deprecated-post-matrix-identity-api-v1-validate-email-requesttoken is covered by this change
  - https://matrix.org/docs/spec/identity_service/r0.3.0#deprecated-post-matrix-identity-api-v1-validate-msisdn-requesttoken takes an integer `send_attempt` too and might be vulnerable to the same problem.
  - https://matrix.org/docs/spec/identity_service/r0.3.0#deprecated-post-matrix-identity-api-v1-3pid-unbind takes a `3PID` but that's a nested object; must be given as a json body
> [matrix.org/docs/spec/identity_service/r0.3.0#deprecated-post-matrix-identity-api-v1-validate-msisdn-requesttoken](https://matrix.org/docs/spec/identity_service/r0.3.0#deprecated-post-matrix-identity-api-v1-validate-msisdn-requesttoken) takes an integer `send_attempt` too and might be vulnerable to the same problem.

Sounds like we should apply the same fix for this one?

Fix patch:
diff --git a/changelog.d/461.bugfix b/changelog.d/461.bugfix
new file mode 100644
index 00000000..f8fd3eed
--- /dev/null
+++ b/changelog.d/461.bugfix
@@ -0,0 +1,1 @@
+Fix a bug introduced in Sydent 2.5.0 where requests to validate an email or phone number would fail with an HTTP 500 Internal Server Error if arguments were given as a query string or as a www-form-urlencoded body.
\ No newline at end of file
diff --git a/changelog.d/461.misc b/changelog.d/461.misc
deleted file mode 100644
index e4b16d66..00000000
--- a/changelog.d/461.misc
+++ /dev/null
@@ -1,1 +0,0 @@
-Fix a bug introduced in Sydent 2.5.0 where calls to `/_matrix/identity/api/v1/validate/email/requestToken` would fail with an HTTP 500 Internal Server Error if arguments were given as a query string or as a www-form-urlencoded body.
\ No newline at end of file
diff --git a/changelog.d/462.bugfix b/changelog.d/462.bugfix
new file mode 100644
index 00000000..f8fd3eed
--- /dev/null
+++ b/changelog.d/462.bugfix
@@ -0,0 +1,1 @@
+Fix a bug introduced in Sydent 2.5.0 where requests to validate an email or phone number would fail with an HTTP 500 Internal Server Error if arguments were given as a query string or as a www-form-urlencoded body.
\ No newline at end of file
diff --git a/sydent/http/servlets/msisdnservlet.py b/sydent/http/servlets/msisdnservlet.py
index d6ebcf87..435266c9 100644
--- a/sydent/http/servlets/msisdnservlet.py
+++ b/sydent/http/servlets/msisdnservlet.py
@@ -58,7 +58,16 @@ async def render_POST(self, request: Request) -> JsonDict:
 
         raw_phone_number = args["phone_number"]
         country = args["country"]
-        sendAttempt = args["send_attempt"]
+        try:
+            # See the comment handling `send_attempt` in emailservlet.py for
+            # more context.
+            sendAttempt = int(args["send_attempt"])
+        except (TypeError, ValueError):
+            request.setResponseCode(400)
+            return {
+                "errcode": "M_INVALID_PARAM",
+                "error": f"send_attempt should be an integer (got {args['send_attempt']}",
+            }
         clientSecret = args["client_secret"]
 
         if not is_valid_client_secret(clientSecret):


Test patch:
diff --git a/tests/test_msisdn.py b/tests/test_msisdn.py
new file mode 100644
index 00000000..57223d82
--- /dev/null
+++ b/tests/test_msisdn.py
@@ -0,0 +1,78 @@
+#  Copyright 2021 The Matrix.org Foundation C.I.C.
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+import asyncio
+import os.path
+from typing import Awaitable
+from unittest.mock import MagicMock, patch
+
+from twisted.trial import unittest
+from twisted.web.server import Request
+
+from tests.utils import make_request, make_sydent
+
+
+class TestRequestCode(unittest.TestCase):
+    def setUp(self) -> None:
+        # Create a new sydent
+        config = {
+            "general": {
+                "templates.path": os.path.join(
+                    os.path.dirname(os.path.dirname(__file__)), "res"
+                ),
+            },
+        }
+        self.sydent = make_sydent(test_config=config)
+
+    def _render_request(self, request: Request) -> Awaitable[MagicMock]:
+        # Patch out the email sending so we can investigate the resulting email.
+        with patch("sydent.sms.openmarket.OpenMarketSMS.sendTextSMS") as sendTextSMS:
+            # We can't use AsyncMock until Python 3.8. Instead, mock the
+            # function as returning a future.
+            f = asyncio.Future()
+            f.set_result(MagicMock())
+            sendTextSMS.return_value = f
+            request.render(self.sydent.servlets.msisdnRequestCode)
+
+        return sendTextSMS
+
+    def test_request_code(self) -> None:
+        self.sydent.run()
+
+        request, channel = make_request(
+            self.sydent.reactor,
+            "POST",
+            "/_matrix/identity/api/v1/validate/msisdn/requestToken",
+            {
+                "phone_number": "447700900750",
+                "country": "GB",
+                "client_secret": "oursecret",
+                "send_attempt": 0,
+            },
+        )
+        sendSMS_mock = self._render_request(request)
+        sendSMS_mock.assert_called_once()
+        self.assertEqual(channel.code, 200)
+
+    def test_request_code_via_url_query_params(self) -> None:
+        self.sydent.run()
+        url = (
+            "/_matrix/identity/api/v1/validate/msisdn/requestToken?"
+            "phone_number=447700900750"
+            "&country=GB"
+            "&client_secret=oursecret"
+            "&send_attempt=0"
+        )
+        request, channel = make_request(self.sydent.reactor, "POST", url)
+        sendSMS_mock = self._render_request(request)
+        sendSMS_mock.assert_called_once()
+        self.assertEqual(channel.code, 200)