Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config options for how much to obfuscate email addresses in 3rd party invites #311

Merged
merged 6 commits into from
Sep 1, 2020

Conversation

anoadragon453
Copy link
Member

When inviting a user via their email address using Sydent, a third party invite event is injected into the room using an obfuscated version of the invitee's email address (to prevent This PR adds two new config options to sydent:

  • email.third_party_invite_username_obfuscate_characters - for obfuscating the text before the @ sign
  • email.third_party_invite_domain_obfuscate_characters - for obfuscating the text after the @` sign

Instead of only truncating the string, I decided to keep the old behaviour of redacting based on string length (only if the string's length is <= the configured threshold). The old behaviour ensured that a full email address is never shown, even if it is very short (e.g. a@a.co), which is a property I believe we want to uphold.

Reviewable commit-by-commit.

This endpoint is currently only accepting email addresses, which is why I didn't make
it generic (or support msisdn's).
@anoadragon453 anoadragon453 requested a review from a team September 1, 2020 14:28
Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm otherwise

sydent/sydent.py Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 merged commit 2f0d4bb into master Sep 1, 2020
@anoadragon453 anoadragon453 deleted the anoa/3pid_obfuscate_options branch September 1, 2020 17:46
anoadragon453 added a commit that referenced this pull request Sep 2, 2020
* master:
  Add config options for how much to obfuscate email addresses in 3rd party invites (#311)
  Remove : from allowed client_secret chars (#309)
  Riot -> Element (#308)
turt2live pushed a commit that referenced this pull request Sep 11, 2020
…arty invites (#311)

When inviting a user via their email address using Sydent, a third party invite event is injected into the room using an obfuscated version of the invitee's email address (to prevent This PR adds two new config options to sydent:

* `email.third_party_invite_username_obfuscate_characters` - for obfuscating the text before the `@` sign
* `email.third_party_invite_domain_obfuscate_characters - for obfuscating the text after the `@` sign

Instead of only truncating the string, I decided to keep the old behaviour of redacting based on string length (only if the string's length is <= the configured threshold). The old behaviour ensured that a full email address is never shown, even if it is very short (e.g. a@a.co), which is a property I believe we want to uphold.
anoadragon453 added a commit that referenced this pull request Oct 13, 2020
…har (#316)

#311 had a typo in it which can be seen here: https://github.com/matrix-org/sydent/pull/311/files#diff-7d63b60d76737089d460f16b53428930d3b0dedbcf3e26a396155a5f22fee3b3L161-R172

This is the code that obfuscates email addresses when creating a 3PID invite for them in a room. `s[:3]` was changed to `s[3]`, which mean that if we were inviting `abcdef@example.com` and config options were set so that this codepath was run, we'd end up with `d...@example.com` instead of `abc...@example.com`, as we were returning the 4th character (`s[3]`), instead of returning everything in the string up to the 4th character (`s[:3]`).

This PR fixes that typo.
anoadragon453 added a commit that referenced this pull request Oct 14, 2020
…har (#317)

This is a duplicate of #316 for mainline which fixed the same bug on `dinsic`. The bug originally stemmed from #311.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants