Page MenuHomePhabricator

EmailAuth: Fix broken regex and inverted logic in maskDomain()
Closed, ResolvedPublic

Description

maskDomain() in EmailAuthSecondaryAuthenticationProvider has two bugs that cancel each other out, producing correct behavior by accident:

Bug 1 — Spaces in regex quantifiers:
The regex uses {1, 63} and {2, 6} (with spaces). PCRE treats these as literal characters rather than quantifiers, so the regex never matches any domain.

Bug 2 — Inverted condition:
The guard returns the fallback mask when preg_match succeeds (valid domain), but the comment and intent is to return it when the domain is invalid. The condition should be negated.

// Before (broken):
if ( preg_match( "/^((?!-)[A-Za-z0-9-]{1, 63}(?<!-)\\.)+[A-Za-z]{2, 6}$/", $domain ) ) {                                                                                                                                     
      // error on the side of caution if invalid domain                                                                                                                                                                        
      return $fallback_domain_mask;                         
}
// After (fixed):
  if ( !preg_match( '/^((?!-)[A-Za-z0-9-]{1,63}(?<!-)\.)+[A-Za-z]{2,6}$/', $domain ) ) {                                                                                                                                       
      // error on the side of caution if invalid domain     
      return $fallback_domain_mask;                                                                                                                                                                                            
  }

Why this hasn't caused issues:
The broken regex never matches, so the guard is dead code. All domains fall through to the EmailAuthUnmaskedDomains config check, which happens to produce the correct result: listed domains are shown, everything else is masked.

Why this matters:
If someone fixes only one of the two bugs (e.g. removes the spaces from the quantifiers without also negating the condition), all valid domains — including those in the unmasked list like gmail.com — would be masked before reaching the unmasked-domains check.

Testing:
Existing tests in EmailAuthSecondaryAuthenticationProviderTest::provideMaskDomainData continue to pass since the observable behavior is unchanged.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change #1266976 had a related patch set uploaded (by Arendpieter; author: Arendpieter):

[mediawiki/extensions/EmailAuth@master] Fix broken regex and inverted logic in maskDomain()

https://gerrit.wikimedia.org/r/1266976

Change #1266976 merged by jenkins-bot:

[mediawiki/extensions/EmailAuth@master] Fix broken regex and inverted logic in maskDomain()

https://gerrit.wikimedia.org/r/1266976

Dreamy_Jazz subscribed.

Seems fixed? Resolving, but feel free to reopen if not