Page MenuHomePhorge

Enhance protections in signup in two ways;
AbandonedPublic

Authored by vanmeeuwen on Dec 22 2021, 10:14 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 1:16 AM
Unknown Object (File)
Mon, Mar 25, 1:16 AM
Unknown Object (File)
Sun, Mar 24, 11:22 AM
Unknown Object (File)
Sun, Mar 24, 11:09 AM
Unknown Object (File)
Sat, Mar 23, 9:29 AM
Unknown Object (File)
Tue, Mar 5, 5:54 PM
Unknown Object (File)
Tue, Mar 5, 5:50 PM
Unknown Object (File)
Tue, Mar 5, 5:40 PM
Subscribers
Restricted Project

Details

Reviewers
machniak
Group Reviewers
Restricted Project
Summary
  • Ensure the short code is unique among all short codes that have not yet expired.
  • Allow protecting the signup controller against duplicate external email addresses, and duplicate source IP addresses.
Test Plan

None

Diff Detail

Repository
rK kolab
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 37439
Build 15017: arc lint + arc unit

Event Timeline

vanmeeuwen created this revision.
mollekopf added inline comments.
src/app/SignupCode.php
107

Doesn't this check mean that if you accidentally generate a collision with an expired code it's reused?
So it seems to me you shouldn't check for isExpired and always generate again if there is a collision.

src/app/Rules/ExternalEmail.php
46

Perhaps add a comment that this is the same message on purpose, otherwise we're bound to "fix" this.

machniak subscribed.
machniak added inline comments.
src/app/Rules/ExternalEmail.php
41

ExternalEmail rule is also used to validate group (distlist) members. And is not, but actually should be used to validate user external email. So, doing the signup related checks here is not right.

This revision now requires changes to proceed.Dec 22 2021, 11:07 AM
vanmeeuwen marked an inline comment as done.
vanmeeuwen added inline comments.
src/app/Rules/ExternalEmail.php
41

Then where should they be?

46

This is apparently moving anyway.

src/app/SignupCode.php
107

No, but it at least prevents this;

MariaDB [kolabdev]> select count(*) as count, short_code from signup_codes where expires_at > NOW() group by short_code order by count desc limit 0,10;
+-------+------------+
| count | short_code |
+-------+------------+
|     3 | *****      |
|     3 | *****      |
|     3 | *****      |
|     2 | *****      |
(...)
10 rows in set (0.077 sec)
src/app/SignupCode.php
107

To me this code reads like:

  • Generate a supposedly unique shortcode
  • Check if it's unique
  • If it isn't unique and it's expired reuse it
  • If it isn't unique and not expired, generate a new one.

Maybe it's okay to reuse expired shortcodes because they're about to be purged anyways? In either case you shouldn't end up with duplicates, so at best this is an optimization (that may or may not be necessary) and at worst it's masking an error in the uniqueness guarantee.

src/app/Rules/ExternalEmail.php
41

Either in another rule class or in the SignupController.

46

emailinvalid does not make sense in this case, indeed.

vanmeeuwen marked an inline comment as done.