Add validation of format for 3pid and add validation of 3pid in admin api #7022

dklimpel · 2020-03-02T22:51:51Z

Fix #6398 (Threepid whitespace is not trimmed before inserting to database)
and also add the check to admin api PUT /_synapse/admin/v2/users/<user_id>

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Code style is correct (run the linters)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

clokep

This looks good overall! I believe the adding additional failure modes to the API endpoints will need a proposal for changing the spec.

I believe the referenced issue also talks about trimming whitespace on the input data, seems like that would be pretty straightforward to add here while modifying this code?

clokep · 2020-03-09T19:15:54Z

changelog.d/6398.bugfix

@@ -0,0 +1 @@
+Add validation of format for 3pid.


Contrary to how towncrier is normally used, we've been putting in newsfragments just for the pull request number, so this file can be removed.

Please delete this file.

clokep · 2020-03-09T19:23:24Z

synapse/rest/admin/users.py

+                    # For emails, transform the address to lowercase.
+                    # We store all email addreses as lowercase in the DB.
+                    # (See add_threepid in synapse/handlers/auth.py)
+                    address = threepid["address"].lower()


Just noting that this seems related to #7021.

Yes, it is.

This change needs to wait on the result of that conversation then.

clokep · 2020-03-09T19:26:11Z

synapse/util/threepids.py

+    """
+
+    if medium == "email":
+        regex = r"(^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+$)"


At a glance this looks like it'll hit most valid e-mails, but usually things like e-mails and domains are relatively hard to validate -- where did this regular expression come from?

Source of regex is: https://emailregex.com/

Python
r"(^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+.[a-zA-Z0-9-.]+$)"

We can change the regex. That is the reason why the validation is a new function.

I did not found a good regex for msidns.

The main issue I see here is that this doesn't support international domain names (or Unicode in general, although I'm not sure if Unicode is valid in the local part of an email address).

Taking a look at that site, it seems the Python regular expression is much simpler than some of the others, which I find concerning.

Poking around a bit, some other solutions seem to suggest being generous since it is hard to know the form of an email without trying to actually validate it and use something like: r"^[^@]+@[^@]+\.[^@]+$".

clokep

Sorry for the delay here. I left a few more comments, I'm not sure what we want to do about the lower-casing of the email addresses as that's really covered in #7021. I think the other comments are straightforward though.

This refers to issue #6398, in order to fix that more nicely would it make sense to strip whitespace on the 3pids before validating?

clokep · 2020-03-26T13:03:17Z

synapse/util/threepids.py

+    """
+
+    if medium == "email":
+        regex = r"(^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+$)"


The main issue I see here is that this doesn't support international domain names (or Unicode in general, although I'm not sure if Unicode is valid in the local part of an email address).

Taking a look at that site, it seems the Python regular expression is much simpler than some of the others, which I find concerning.

Poking around a bit, some other solutions seem to suggest being generous since it is hard to know the form of an email without trying to actually validate it and use something like: r"^[^@]+@[^@]+\.[^@]+$".

clokep · 2020-03-26T13:04:59Z

changelog.d/6398.bugfix

@@ -0,0 +1 @@
+Add validation of format for 3pid.


Please delete this file.

clokep · 2020-03-26T13:06:16Z

synapse/rest/admin/users.py

+                    # For emails, transform the address to lowercase.
+                    # We store all email addreses as lowercase in the DB.
+                    # (See add_threepid in synapse/handlers/auth.py)
+                    address = threepid["address"].lower()


This change needs to wait on the result of that conversation then.

synapse/util/threepids.py

clokep · 2020-03-26T13:17:28Z

synapse/util/threepids.py

+    elif medium == "msisdn":
+        return True
+    else:
+        return False


Can you add a comment above this saying that any other medium is not understood and is thus invalid? Thanks!

clokep · 2020-03-26T13:35:55Z

synapse/api/errors.py

@@ -55,6 +55,7 @@ class Codes(object):
    THREEPID_IN_USE = "M_THREEPID_IN_USE"
    THREEPID_NOT_FOUND = "M_THREEPID_NOT_FOUND"
    THREEPID_DENIED = "M_THREEPID_DENIED"
+    INVALID_THREEPID = "M_INVALID_THREEPID"


It is unclear to me if this needs a spec change or not. @matrix-org/synapse-core any opinion?

I did not find an spec for these errors. Only a documenttion https://github.com/matrix-org/matrix-doc/blob/master/specification/client_server_api.rst
I can also remove this error.

You can see the list of error codes at https://matrix.org/docs/spec/client_server/r0.6.0#api-standards, this would need to get added as a separate one following the process at https://matrix.org/docs/spec/proposals

synapse/rest/admin/users.py

clokep · 2020-03-31T11:57:23Z

@dklimpel Thanks for the contribution! After reviewing it again I think this probably needs to get split up into a few different PRs:

A PR which takes the current check_3pid_allowed and adds them to the admin API. (This should be straightforward and not controversial at all!)
A PR which adds the additional check_3pid_valid_format to both the client-server API and the admin API. This will need a spec change to add the additional error code, let me know if I can help with that.
The changes to lower-casing 3PIDs seems like it should be part of Fix inconsistent handling of upper and lower cases of email addresses. #7021, not this PR.

richvdh · 2020-06-05T16:48:40Z

I'm going to close this for now since it needs some substantial changes. please do open new PRs with smaller changes! :)

dklimpel added 5 commits March 2, 2020 23:42

Add validation of format for 3pid

ea2a8de

fixes

a801e8d

fix2

4648933

lint

59e4428

Add validation of 3pid in admin api

7c9da15

dklimpel changed the title ~~Add validation of format for 3pid~~ Add validation of format for 3pid and add validation of 3pid in admin api Mar 3, 2020

dklimpel marked this pull request as ready for review March 3, 2020 19:19

dklimpel added 2 commits March 4, 2020 19:55

sync dev and trigger checks

8ff3522

sync dev and trigger checks

131255d

clokep self-assigned this Mar 9, 2020

clokep self-requested a review March 9, 2020 19:17

clokep removed their assignment Mar 9, 2020

clokep reviewed Mar 9, 2020

View reviewed changes

richvdh assigned clokep Mar 16, 2020

richvdh requested a review from clokep March 16, 2020 22:42

clokep reviewed Mar 26, 2020

View reviewed changes

Not final commit, update of PR after review.

dbaf0a3

dklimpel marked this pull request as draft May 2, 2020 18:03

richvdh closed this Jun 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation of format for 3pid and add validation of 3pid in admin api #7022

Add validation of format for 3pid and add validation of 3pid in admin api #7022

dklimpel commented Mar 2, 2020 •

edited

Loading

clokep left a comment

clokep Mar 9, 2020

dklimpel Mar 9, 2020 •

edited

Loading

clokep Mar 26, 2020

clokep Mar 9, 2020

dklimpel Mar 9, 2020

clokep Mar 26, 2020

clokep Mar 9, 2020

dklimpel Mar 9, 2020

dklimpel Mar 9, 2020

clokep Mar 26, 2020

clokep left a comment

clokep Mar 26, 2020

clokep Mar 26, 2020

clokep Mar 26, 2020

clokep Mar 26, 2020

clokep Mar 26, 2020

dklimpel Mar 30, 2020

clokep Mar 31, 2020

clokep commented Mar 31, 2020

richvdh commented Jun 5, 2020

Add validation of format for 3pid and add validation of 3pid in admin api #7022

Add validation of format for 3pid and add validation of 3pid in admin api #7022

Conversation

dklimpel commented Mar 2, 2020 • edited Loading

Pull Request Checklist

clokep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dklimpel Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep commented Mar 31, 2020

richvdh commented Jun 5, 2020

dklimpel commented Mar 2, 2020 •

edited

Loading

dklimpel Mar 9, 2020 •

edited

Loading