fix(auth): validate the whole whitelist entry, including its CIDR prefix

migrations/container/20260927120000_auth-command_drop_invalid_whitelist_entries_from_acl_files.php

@@ -0,0 +1,116 @@
+<?php
+
+namespace EE\Migration;
+
+use EE;
+use EE\Migration\Base;
+use EE\Model\Whitelist;
+use function EE\Auth\Utils\is_valid_whitelist_ip;
+use function EE\Auth\Utils\normalize_stored_whitelist_ip;
+use function EE\Auth\Utils\warn_invalid_stored_whitelist_ip;
+
+class DropInvalidWhitelistEntriesFromAclFiles extends Base {
+
+	/**
+	 * @var array ACL files to fix, mapped to their new content and the entries removed from them.
+	 */
+	private $files = [];
+
+	/**
+	 * @var array ACL files that couldn't be read.
+	 */
+	private $unreadable = [];
+
+	public function __construct() {
+
+		parent::__construct();
+
+		if ( $this->is_first_execution ) {
+			$this->skip_this_migration = true;
+
+			return;
+		}
+
+		foreach ( glob( EE_ROOT_DIR . '/services/nginx-proxy/vhost.d/*_acl' ) ?: [] as $file ) {
+			$content = file_get_contents( $file );
+			if ( false === $content ) {
+				// It may hold an invalid entry, so up() fails and the migration stays pending.
+				$this->unreadable[] = $file;
+				continue;
+			}
+			$removed = [];
+			$lines   = [];
+			foreach ( explode( "\n", $content ) as $line ) {
+				// nginx also accepts `all` and `unix:`, which EE doesn't write but a hand edit may add.
+				if ( ! preg_match( '/^allow (.*);(\r?)$/', $line, $m ) || is_valid_whitelist_ip( $m[1] ) || in_array( $m[1], [ 'all', 'unix:' ], true ) ) {
+					$lines[] = $line;
+					continue;
+				}
+				// Entries nginx accepts, e.g. with a prefix leading zero, are kept in a valid form.
+				$entry = normalize_stored_whitelist_ip( $m[1] );
+				if ( is_valid_whitelist_ip( $entry ) ) {
+					$lines[] = "allow $entry;$m[2]";
+				} else {
+					$removed[] = $m[1];
+				}
+			}
+			$new_content = implode( "\n", $lines );
+			if ( $new_content !== $content ) {
+				$this->files[ $file ] = [ $new_content, $removed ];
+			}
+		}
+
+		$this->skip_this_migration = empty( $this->files ) && empty( $this->unreadable );
+	}
+
+	/**
+	 * Removes the invalid entries from the `_acl` files: older versions wrote them unchecked, and nginx fails every reload and restart on most of them.
+	 *
+	 * The files are edited in place instead of regenerated, so no new file is written while an older nginx-proxy runs.
+	 *
+	 * @throws \Exception When a file can't be read or rewritten, so the upgrade stops before the image migration recreates the proxy on it.
+	 */
+	public function up() {
+
+		if ( $this->skip_this_migration ) {
+			EE::debug( 'Skipping the whitelist entries check: every _acl file is valid.' );
+
+			return;
+		}
+
+		$removed = [];
+		try {
+			foreach ( $this->files as $file => list( $content, $entries ) ) {
+				$this->fs->dumpFile( $file, $content );
+				if ( $entries ) {
+					$removed[ implode( "', '", $entries ) ][] = basename( $file );
+				} else {
+					EE::debug( "Normalized the whitelist entries of $file" );
+				}
+			}
+			if ( $this->unreadable ) {
+				throw new \Exception( 'Could not read ' . implode( ', ', $this->unreadable ) );
+			}
+		} finally {
+			// Also after a failed write: a retry no longer sees the files already fixed.
+			foreach ( $removed as $entries => $files ) {
+				EE::warning( sprintf( "Removed the invalid whitelist entries '%s' from %s in %s.", $entries, implode( ', ', $files ), EE_ROOT_DIR . '/services/nginx-proxy/vhost.d' ) );
+			}
+		}
+
+		// The rows stay stored; name the command that removes each one.
+		foreach ( Whitelist::all() as $row ) {
+			if ( ! is_valid_whitelist_ip( normalize_stored_whitelist_ip( (string) $row->ip ) ) ) {
+				warn_invalid_stored_whitelist_ip( $row->site_url, $row->ip );
+			}
+		}
+
+		\EE\Site\Utils\reload_global_nginx_proxy();
+	}
+
+	/**
+	 * Not reverted: the removed entries made the file invalid.
+	 */
+	public function down() {
+	}
+}

src/Auth_Command.php

@@ -19,6 +19,7 @@
 use Symfony\Component\Filesystem\Filesystem;
 use function EE\Auth\Utils\generate_site_auth_files;
 use function EE\Auth\Utils\generate_site_whitelist;
+use function EE\Auth\Utils\is_valid_whitelist_ip;
 use function EE\Auth\Utils\verify_htpasswd_is_present;
 use function EE\Auth\Utils\write_htpasswd_file;
 use function EE\Site\Utils\auto_site_name;
@@ -97,26 +98,41 @@ public function create( $args, $assoc_args ) {
 
 	/**
 	 * Cleans and Validate IP addresses
-	 * Converts input separated by comma, spaces and new-lines in array
+	 * Converts input separated by comma, whitespace and new-lines in an array of unique entries
 	 *
-	 * @param string $ips IPs to clean and validate
+	 * @param string $ips      IPs to clean and validate
+	 * @param string $site_url If set, entries already stored for it are accepted, so ones saved by older versions can be deleted.
 	 *
 	 * @return array $user_ips Cleaned IP addresses.
 	 */
-	private function clean_and_validate_ips( string $ips ) {
+	private function clean_and_validate_ips( string $ips, string $site_url = '' ) {
 
-		$user_ips = preg_split( '/[\ \n\,]+/', $ips );
+		// Unique, as a repeated entry would hit the UNIQUE(site_url, ip) constraint.
+		$user_ips = array_values( array_unique( preg_split( '/[\s,]+/', trim( $ips ) ) ) );
 
 		foreach ( $user_ips as $ip ) {
 
-			// Remove subnet from ip if present.
-			if ( preg_match( '~^(.+?)/([^/]+)$~', $ip, $m ) ) {
-				$ip = $m[1];
+			if ( is_valid_whitelist_ip( $ip ) ) {
+				continue;
 			}
 
-			if ( ! filter_var( $ip, FILTER_VALIDATE_IP ) ) {
-				EE::error( 'Please check your list do not have any empty or wrong IP addresses.' );
+			$stored = '' !== $site_url && '' !== $ip && Whitelist::where(
+				[
+					'site_url' => $site_url,
+					'ip'       => $ip,
+				]
+			);
+
+			if ( $stored ) {
+				continue;
 			}
+
+			EE::error(
+				sprintf(
+					'Please check your list do not have any empty or wrong IP addresses. Invalid entry: %s. Use an IPv4 or IPv6 address, optionally with a CIDR prefix of 0-32 (IPv4) or 0-128 (IPv6), e.g. 192.0.2.0/24 or 2001:db8::/32.',
+					'' === $ip ? 'an empty one' : "'$ip'"
+				)
+			);
 		}
 
 		return $user_ips;
@@ -548,7 +564,7 @@ public function delete( $args, $assoc_args ) {
 					$whitelist->delete();
 				}
 			} else {
-				$user_ips = $this->clean_and_validate_ips( $ip );
+				$user_ips = $this->clean_and_validate_ips( $ip, $site_url );
 
 				foreach ( $user_ips as $ip ) {
 					$existing_ips = Whitelist::where(

src/auth-utils.php

@@ -364,9 +364,86 @@ function htpasswd_command( string $flags, string $name, string $username, string
 	);
 }
 
+/**
+ * Checks that a whitelist entry is an IPv4 or IPv6 address, optionally with a CIDR prefix, that nginx's `allow` accepts.
+ *
+ * @param string $entry Whitelist entry.
+ *
+ * @return bool
+ */
+function is_valid_whitelist_ip( string $entry ): bool {
+
+	$parts = explode( '/', $entry );
+
+	if ( count( $parts ) > 2 ) {
+		return false;
+	}
+
+	$is_ipv4 = false !== filter_var( $parts[0], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4 );
+
+	if ( ! $is_ipv4 && false === filter_var( $parts[0], FILTER_VALIDATE_IP, FILTER_FLAG_IPV6 ) ) {
+		return false;
+	}
+
+	// nginx reads 255.255.255.255 as its INADDR_NONE error value, also when embedded in an IPv6 address.
+	if ( preg_match( '/(^|:)255\.255\.255\.255\z/', $parts[0] ) ) {
+		return false;
+	}
+
+	// nginx rejects a `::` that stands for no group, e.g. `1:2:3:4:5:6:7::`.
+	if ( preg_match( '/^(?:[0-9a-f]{1,4}:){7}:\z/i', $parts[0] ) ) {
+		return false;
+	}
+
+	if ( ! isset( $parts[1] ) ) {
+		return true;
+	}
+
+	// Digits only, so no sign, space or `;` reaches the nginx config; leading zeros are refused as ambiguous.
+	return 1 === preg_match( '/^(0|[1-9][0-9]{0,2})\z/', $parts[1] ) && (int) $parts[1] <= ( $is_ipv4 ? 32 : 128 );
+}
+
+/**
+ * Normalizes a whitelist entry stored by an older version that nginx accepts as is, e.g. `10.0.0.0/08` or a trailing CR.
+ *
+ * @param string $entry Stored whitelist entry.
+ *
+ * @return string The entry without surrounding whitespace and prefix leading zeros, or unchanged when it's still invalid.
+ */
+function normalize_stored_whitelist_ip( string $entry ): string {
+
+	// Only the whitespace nginx itself separates on.
+	$normalized = preg_replace( '~/0+(?=[0-9])~', '/', trim( $entry, " \t\r\n" ) );
+
+	return is_valid_whitelist_ip( $normalized ) ? $normalized : $entry;
+}
+
+/**
+ * Warns once per scope and entry about an invalid stored whitelist entry, with the command that removes it.
+ *
+ * @param string $site_url Site URL of the entry, `default` for global.
+ * @param string $ip       Whitelist entry.
+ */
+function warn_invalid_stored_whitelist_ip( string $site_url, string $ip ) {
+
+	static $warned = [];
+
+	$scope = 'default' === $site_url ? 'global' : $site_url;
+	if ( ! empty( $warned[ $scope ][ $ip ] ) ) {
+		return;
+	}
+	$warned[ $scope ][ $ip ] = true;
+
+	// `--ip` splits on whitespace and commas, so such an entry can only go with the whole list.
+	$hint = preg_match( '/[\s,]/', $ip ) ? "`ee auth delete $scope --ip` and add the valid ones again" : sprintf( '`ee auth delete %s --ip=%s`', $scope, escapeshellarg( $ip ) );
+	EE::warning( sprintf( "Skipping the invalid whitelist entry '%s' of %s: nginx would reject it. Remove it with %s.", $ip, $scope, $hint ) );
+}
+
 /**
  * Gets the IPs to whitelist on a site: global and site entries, or none when the site has no own entries.
  *
+ * Invalid entries stored by older versions are skipped with a warning, as nginx would reject the whole file.
+ *
  * @param string $site_url URL of site, `default` for global.
  *
  * @return array
@@ -379,10 +456,18 @@ function get_site_whitelist_ips( string $site_url ): array {
 		return [];
 	}
 
-	return array_column(
-		'default' === $site_url ? $site_ips : array_merge( Whitelist::get_global_ips(), $site_ips ),
-		'ip'
-	);
+	$ips = [];
+	foreach ( 'default' === $site_url ? $site_ips : array_merge( Whitelist::get_global_ips(), $site_ips ) as $row ) {
+		$ip = normalize_stored_whitelist_ip( (string) $row->ip );
+		if ( is_valid_whitelist_ip( $ip ) ) {
+			$ips[] = $ip;
+		} else {
+			// Global entries are merged into every site's file, so this warns once.
+			warn_invalid_stored_whitelist_ip( $row->site_url, $row->ip );
+		}
+	}
+
+	return $ips;
 }
 
 /**