Enhance protections in signup in two ways;
AbandonedPublic
Actions

Authored by vanmeeuwen on Dec 22 2021, 10:14 AM.

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

Repository

rK kolab

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 37439
Build 15017: arc lint + arc unit

vanmeeuwen requested review of this revision.Dec 22 2021, 10:14 AM

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 requested changes to this revision.Dec 22 2021, 11:07 AM

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 requested review of this revision.Dec 22 2021, 9:02 PM

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.