Page MenuHomePhabricator

The regex check for invalid domains within `EmailAuthSecondaryAuthenticationProvider::maskDomain` is not currently working properly
Open, Needs TriagePublicBUG REPORT

Description

The following code within EmailAuthSecondaryAuthenticationProvider::maskDomain (added in https://gerrit.wikimedia.org/r/1143874 for T390780: Mask mailaddress during login that triggers EmailAuth) is (AIUI) intended to check whether a provided domain name (e.g., example.com) is valid, and return a 'fallback' domain mask (***.***) if it isn't valid:

		$fallback_domain_mask = '***.***';

		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;
		}

From what I can see, there are a couple of issues with that code as it currently stands:

  • The regex pattern seems like it's one that would return a match for a valid domain-name; however, the inside of the if statement seems like it's code that's intended to be run for invalid domain-names. (It feels [to me] like the condition should maybe be if ( !preg_match( [...], rather than if ( preg_match( [...].)
  • The spaces within {1, 63} & {2, 6} (ie., within the regex pattern itself) are currently causing this block of code to exhibit different behaviour on systems running a version of PCRE below 10.43, & on systems running PCRE 10.43+.

In practical terms, I believe that this means (among other things) that:

  • On systems running PCRE 10.43 or above, the regex will function as (I assume) it was intended to; but the lack of negation in the if statement means that all valid domain names (even those listed in $wgEmailAuthUnmaskedDomains) will be fully masked.
  • On systems running a version of PCRE below 10.43, the preg_match call will return a truthy value if the value of the $domain variable is something like (e.g.) the string literal a{1, 63}.b{2, 6}.
    • Therefore, on versions of PCRE below 10.43, it appears that the two issues listed above will partially cancel each other out -- the if statement is missing a negation operator that would result in invalid (rather than valid) domain-names being fully masked; however, on PCRE <10.43, the regex seems like it wouldn't match valid domain-names in the first place.
  • On systems running either versions of PCRE, most (if not all) invalid domain names (that are also present in $wgEmailAuthUnmaskedDomains) will be displayed unmasked, despite this being (IIUC) what this if statement is intending to prevent.

I put together a quick proof-of-concept test case in order to demonstrate these issues (that should currently also fail on versions of PCRE below 10.43), & I'll upload it to Gerrit shortly.
I'm also boldly tagging this as a PHP 8.4 support task, as FWICS the behaviour here changes when using the version of PCRE that's bundled with PHP 8.4 (10.44), compared to the version of PCRE that's bundled with PHP 8.3 (10.42).


This is currently also causing test failures when running EmailAuth's PHPUnit tests on PHP using PCRE >=10.43 (which FWIW are how I actually ended up coming across these issues), copied below for completeness:

$ php --ri pcre | grep 'Library Version'
PCRE Library Version => 10.47 2025-10-21

$ composer phpunit -- extensions/EmailAuth --filter EmailAuthSecondaryAuthenticationProviderTest
> Composer\Config::disableProcessTimeout
Using PHP 8.5.2
Running with MediaWiki settings because there might be integration tests
PHPUnit 9.6.34 by Sebastian Bergmann and contributors.

........FFFF........FF..                                          24 / 24 (100%)

Time: 00:00.812, Memory: 90.61 MB

There were 6 failures:

1) MediaWiki\Extension\EmailAuth\Tests\EmailAuthSecondaryAuthenticationProviderTest::testMaskDomain with data set #5 ('gmail.com', 'gmail.com')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'gmail.com'
+'***.***'

[...]/extensions/EmailAuth/tests/phpunit/EmailAuthSecondaryAuthenticationProviderTest.php:256

2) MediaWiki\Extension\EmailAuth\Tests\EmailAuthSecondaryAuthenticationProviderTest::testMaskDomain with data set #6 ('googlemail.com', 'googlemail.com')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'googlemail.com'
+'***.***'

[...]/extensions/EmailAuth/tests/phpunit/EmailAuthSecondaryAuthenticationProviderTest.php:256

3) MediaWiki\Extension\EmailAuth\Tests\EmailAuthSecondaryAuthenticationProviderTest::testMaskDomain with data set #7 ('yahoo.com', 'yahoo.com')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'yahoo.com'
+'***.***'

[...]/extensions/EmailAuth/tests/phpunit/EmailAuthSecondaryAuthenticationProviderTest.php:256

4) MediaWiki\Extension\EmailAuth\Tests\EmailAuthSecondaryAuthenticationProviderTest::testMaskDomain with data set #8 ('hotmail.com', 'hotmail.com')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'hotmail.com'
+'***.***'

[...]/extensions/EmailAuth/tests/phpunit/EmailAuthSecondaryAuthenticationProviderTest.php:256

5) MediaWiki\Extension\EmailAuth\Tests\EmailAuthSecondaryAuthenticationProviderTest::testMaskEmail with data set #8 ('t***@gmail.com', 'tester@gmail.com')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'t***@gmail.com'
+'t***@***.***'

[...]/extensions/EmailAuth/tests/phpunit/EmailAuthSecondaryAuthenticationProviderTest.php:280

6) MediaWiki\Extension\EmailAuth\Tests\EmailAuthSecondaryAuthenticationProviderTest::testMaskEmail with data set #9 ('*@gmail.com', 'a@gmail.com')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'*@gmail.com'
+'*@***.***'

[...]/extensions/EmailAuth/tests/phpunit/EmailAuthSecondaryAuthenticationProviderTest.php:280

Event Timeline

Change #1236312 had a related patch set uploaded (by A smart kitten; author: A smart kitten):

[mediawiki/extensions/EmailAuth@master] [POC] Domain-masking test case that should currently fail

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