Discourse PR #3 — Live Benchmark — FriendlyReviewer

Golden Comments (1/2 found + 1 partial)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	`BlockedEmail.should_block_email?` has side effects during a read operation — updates stats even when just checking. Race conditions possible, misleading method name.	MEDIUM	✓ Found	FriendlyReviewer found both aspects: 🔴 non-atomic read-increment-write race condition (`blocked_email.rb` L12) and 🟡 side-effect mutation in a query method (`blocked_email.rb` L16). Suggests `increment!` for atomicity and method renaming.
#2	The regex `@(#\{domains\})` only matches domain suffixes, not full names. `evil.example.com` would pass the whitelist if `example.com` is whitelisted.	MEDIUM	⚠ Partial	FriendlyReviewer found incomplete regex escaping — only dots are escaped, other metacharacters are not (🔴 RED, `email_validator.rb` L19). Suggests `Regexp.escape`. Did not specifically identify the whitelist bypass via subdomain suffix matching.

Supplementary Findings (6 findings)

#	Finding	File	Severity	Legitimate?
1	Incomplete regex escaping: only dots are escaped, other metacharacters are not	email_validator.rb:19	HIGH	⚠ Golden #2 partial
2	Singleton state leak: `rejectedEmails` persists across modal sessions	create_account_controller.js:17	HIGH	✓ Data leak — legitimate security bug
3	Case-sensitive email comparison — use lowercase for case-insensitive matching	create_account_controller.js:69	MEDIUM	✓ Legitimate bug
4	`rejectedEmails` not reset after successful account creation, causing state leak across registration sessions	create_account_controller.js:271	MEDIUM	✓ Legitimate bug
5	Inconsistent error response schema between rescue blocks	users_controller.rb:211	MEDIUM	✓ Legitimate bug
6	Translation key `blocked` is too terse, semantically overlapping with `not_allowed`	server.en.yml:734	LOW	✓ Acceptable noise

Honest Analysis

FriendlyReviewer found golden #1 with both dimensions — the race condition and the side-effect mutation in a query method. The diagnosis is more complete than the golden comment itself: it classifies the race condition as high (the golden marked it medium, which is arguably too conservative for non-atomic read-increment-write) and provides a concrete fix (increment!). The side-effect finding on line 16 reinforces the same method-level concern from a different angle.

Golden #2 is a partial match. FriendlyReviewer correctly identified that the regex escaping is incomplete (only dots are escaped, suggesting Regexp.escape), but missed the specific subdomain bypass scenario: evil.example.com would still pass a whitelist containing example.com because the regex matches suffixes, not full domain names. This is a nuanced distinction between two different bugs in the same line — escaping is one problem, suffix matching semantics is another. FriendlyReviewer caught the first but not the second.

The singleton state leak finding (rejectedEmails persisting across modal sessions) is a legitimate security concern. A blocked user could be permanently prevented from registering even after correction, or conversely, a previously rejected email could be silently accepted on a subsequent attempt if the modal state is not cleared. This is a genuine authentication-adjacent bug that the golden comments did not flag.

The case-sensitive comparison finding and the inconsistent error schema finding are both valid — email addresses are case-insensitive per RFC 5321, and inconsistent error responses make client-side error handling fragile. These are the kinds of edge cases that systematic review catches reliably.

One finding is clearly noise: the translation key being "too terse" and overlapping with not_allowed. This is a minor semantic preference, not a functional issue. It is included here because the bar for inclusion is low, and a small amount of noise is the trade-off for not missing real issues. This is acceptable.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
3 high, 4 medium, 1 low

The implementation introduces a Blocked Email feature with domain whitelist/blacklist validation and client-side rejected email tracking. While architecturally sound, several correctness, consistency, and quality issues must be addressed. Key concerns include a race condition in should_block?, incomplete regex escaping in the email validator, a client-side singleton state leak that can permanently block legitimate emails, inconsistent error response schemas, and insufficient test coverage. Additionally, the new translation key is too terse and semantically overlapping with an existing key. These issues should be resolved before merging.

These might need a close look

🔴 app/models/blocked_email.rb (L12)
Race condition: should_block? performs non-atomic read-increment-write. Use record.increment!(:match_count, 1) or similar atomic operation.
🔴 lib/validators/email_validator.rb (L19)
Incomplete regex escaping: only dots are escaped; other metacharacters are not. Use Regexp.escape(domain) for each domain segment.
🔴 app/assets/javascripts/discourse/controllers/create_account_controller.js (L17)
Singleton state leak: rejectedEmails persists across modal sessions. Reset it when the modal is opened or on successful account creation.

Worth checking

🟡 app/models/blocked_email.rb (L16)
Side-effect mutation in query method: should_block? updates match_count even when action_type is :do_nothing. Consider renaming or moving the update.
🟡 app/assets/javascripts/discourse/controllers/create_account_controller.js (L69)
Case-sensitive email comparison: use lowercase comparison to match case-insensitive emails.
🟡 app/assets/javascripts/discourse/controllers/create_account_controller.js (L271)
rejectedEmails not reset on successful account creation, causing state to leak across registration sessions.
🟡 app/controllers/users_controller.rb (L211)
Inconsistent error response schema: rescue blocks return different JSON structures. Include errors: {} and values: {} in all error responses.

Small things (take or leave)

🔵 config/locales/server.en.yml (L734)
The blocked translation key is too terse and semantically overlaps with not_allowed. Improve the message and consider renaming for clarity.

Add comprehensive email validation for blocked users