GlobalBlocking::getGlobalBlockingBlock() does SELECT *... So, I'm guessing it's returning false

Only in one case does it actually check if $block evaluates to true as PHP reckons... It probably should check in more points

			if ( $block && $this->getParameter( 'modify' ) ) {
				$options[] = 'modify';
			}

I can reproduce this by creating a GlobalBlock of an IP range via the API.

I see in the logs:

[DBQuery] GlobalBlocking::getGlobalBlockingBlock [0s]: SELECT  gb_id,gb_address,gb_by,gb_by_wiki,gb_reason,gb_timestamp,gb_anon_only,gb_expiry,gb_range_start,gb_range_end  FROM `globalblocks`    WHERE (gb_range_start  LIKE '%' ESCAPE '`' ) AND (gb_range_start <= '0') AND (gb_range_end >= '0') AND (gb_expiry > '20190724174208')  LIMIT 1

I do not believe GlobalBlocking::getRangeCondition (which is called by GlobalBlocking::getGlobalBlockingBlock) correctly interprets IP ranges.

In T179741#5368720, @dom_walden wrote:

I do not believe GlobalBlocking::getRangeCondition (which is called by GlobalBlocking::getGlobalBlockingBlock) correctly interprets IP ranges.

reedy@deploy1001:~$ mwscript eval.php enwiki
> $dbr = wfGetDB( DB_REPLICA );

> $hexIp = IP::toHex( '127.0.0.1' );

> $ipPattern = substr( $hexIp, 0, 4 );

> $quotedHex = $dbr->addQuotes( $hexIp );

> $cond = [ 'gb_range_start ' . $dbr->buildLike( $ipPattern, $dbr->anyString() ), 'gb_range_start <= ' . $quotedHex, 'gb_range_end >= ' . $quotedHex, 'gb_expiry > ' . $dbr->addQuotes( $dbr->timestamp( wfTimestampNow() ) ) ];

> var_dump( $cond );
array(4) {
  [0]=>
  string(40) "gb_range_start  LIKE '7F00%' ESCAPE '`' "
  [1]=>
  string(28) "gb_range_start <= '7F000001'"
  [2]=>
  string(26) "gb_range_end >= '7F000001'"
  [3]=>
  string(28) "gb_expiry > '20190726163645'"
}

> $hexIp = IP::toHex( '10.0.0.0/16' );

> $ipPattern = substr( $hexIp, 0, 4 );

> $quotedHex = $dbr->addQuotes( $hexIp );

> $cond = [ 'gb_range_start ' . $dbr->buildLike( $ipPattern, $dbr->anyString() ), 'gb_range_start <= ' . $quotedHex, 'gb_range_end >= ' . $quotedHex, 'gb_expiry > ' . $dbr->addQuotes( $dbr->timestamp( wfTimestampNow() ) ) ];

> var_dump( $cond );
array(4) {
  [0]=>
  string(36) "gb_range_start  LIKE '%' ESCAPE '`' "
  [1]=>
  string(21) "gb_range_start <= '0'"
  [2]=>
  string(19) "gb_range_end >= '0'"
  [3]=>
  string(28) "gb_expiry > '20190726164630'"
}

> 

> $hexIp = IP::toHex( '10.0.0.0-255' );

> $ipPattern = substr( $hexIp, 0, 4 );

> $quotedHex = $dbr->addQuotes( $hexIp );

> $cond = [ 'gb_range_start ' . $dbr->buildLike( $ipPattern, $dbr->anyString() ), 'gb_range_start <= ' . $quotedHex, 'gb_range_end >= ' . $quotedHex, 'gb_expiry > ' . $dbr->addQuotes( $dbr->timestamp( wfTimestampNow() ) ) ];

> var_dump( $cond );
array(4) {
  [0]=>
  string(36) "gb_range_start  LIKE '%' ESCAPE '`' "
  [1]=>
  string(21) "gb_range_start <= '0'"
  [2]=>
  string(19) "gb_range_end >= '0'"
  [3]=>
  string(28) "gb_expiry > '20190726164630'"
}

In T179741#5368720, @dom_walden wrote:

I can reproduce this by creating a GlobalBlock of an IP range via the API.

I initially read this as making a query for global blocks, but it's actually the action of making a block

For the 'target' parameter...

	"apihelp-globalblock-param-target": "The target IP address.",

This suggests it should only accept (single) IPs, not ranges, CIDR or anything else.

I guess, at a minimum, it should error out gracefully if a range is passed... And then potentially a future feature request make it accept ranges and CIDR?

IP::isValid() looks to do the job to make sure it's not a range at least

This doesn't seem to have come about from AHT's work on blocks. We're prioritizing more important tasks over this at the moment.

Change 546506 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/GlobalBlocking@master] Update GlobalBlocking::getRangeCondition to work with IP ranges

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

Change 546510 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/GlobalBlocking@master] Check for block before accessing block's properties in ApiGlobalBlock

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

GlobalBlocking is documented to be able to block IP ranges: https://www.mediawiki.org/wiki/Extension:GlobalBlocking and is certainly used that way, e.g.: https://en.wikipedia.org/wiki/Special:GlobalBlockList

On local testing with the examples given by @Reedy above, the blocks are made successfully, before ApiGlobalBlock::execute errors out. (These examples fail because for IPv4, IP::toHex calls ip2long(), which fails on a range. Confusingly IP::toHex happens to work on an IPv6 range, so perhaps it should be documented that it is not meant for IP ranges.)

GlobalBlocking::getRangeCondition is called on retrieving a block, not inserting one. It may be passed an IP address or an IP range, but is not currently set up to deal with a range.

What should be done:

• GlobalBlocking::getRangeCondition should be updated to use IP::parseRange
• ApiGlobalBlock::execute could be updated to check for a block before accessing properties

Change 546506 merged by jenkins-bot:
[mediawiki/extensions/GlobalBlocking@master] Update GlobalBlocking::getRangeCondition to work with IP ranges

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

Change 546510 merged by jenkins-bot:
[mediawiki/extensions/GlobalBlocking@master] Check for block before accessing block's properties in ApiGlobalBlock

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