no data loss, user already wanted to cancel the operation, not a blocker

Here's the relevant code:

$cmr = MWOAuthDAOAccessControl::wrap(
	MWOAuthConsumer::newFromKey( $dbr, $consumerKey ),
	$this->getContext()
);

$this->getOutput()->addSubtitle( $this->msg( 'mwoauth-desc' )->escaped() );
$this->getOutput()->addWikiMsg(
	'mwoauth-acceptance-cancelled',
	$cmr->get( 'name' )
);

Happens easily when MWOAuthDAOAccessControl::wrap() returns null/false, which is the second condition it can do. $cmr ends up not being an object, which we clearly can't call get() on.

Should probably show an alternative message in that case, or clean up the calling so it won't return null/false

The user should never be presented with an authorization dialog since the URL does not specify a consumer. Yet somehow we present a dialog for a specific consumer (Hello World, which is the first DB record, I think). Let's call this a security issue until we know what's going on (the borked part seems to be MWOAuthServer::getConsumerKey).

tgr@terbium:~$ mwscript eval.php --wiki=mediawikiwiki
> var_dump( $db->selectRow( 'oauth_registered_consumer', [ 'oarc_name' ], [ 'oarc_consumer_key' => false ] ) );
object(stdClass)#221 (1) {
  ["oarc_name"]=>
  string(38) "MWOAuth Hello World (end user version)"
}
> echo $db->selectSQLText( 'oauth_registered_consumer', [ 'oarc_name' ], [ 'oarc_consumer_key' => false ] );
SELECT  oarc_name  FROM `oauth_registered_consumer`    WHERE oarc_consumer_key = 0

Huh.

I guess MySQL does a string => int transform on the consumer key so it will just find the first key which does not start with a number? Crazy stuff. I don't think there is any security impact though.

Apparently MySQL has weird type casting like PHP, such that comparing a string and an integer casts the string to an integer with stupid casting rules. Sigh.

This started happening here thanks to a fix for a different bug causing boolean false to be turned into 0 rather than empty-string by Database. Apparently this code was depending on the former behavior to not screw up when MWOAuthConsumer::newFromKey() gets passed false for $key. :/

In T147414#2694596, @Anomie wrote:

This started happening here thanks to a fix for a different bug causing boolean false to be turned into 0 rather than empty-string by Database.

Should that maybe be changed to '0' to avoid similar issues elsewhere? For int fields that will only result in an unnecessary typecast, for string matches it would behave significantly more sanely.

Anyway,

I took a slightly different approach (and then got distracted waiting for vagrant to update and did not upload it):

I think this task can be unsecuritized (there is no way to use the bug to get around any authorization check) but waiting for a second opinion with that.

+			throw new \ErrorPageError( 'mwoauth-error', 'mwoauth-invalid-consumer-key' );

Just throw an MWOAuthException( 'mwoauth-invalid-consumer-key' ) and let the catch in
SpecialMWOAuth::execute() handle it.

In T147414#2694941, @Anomie wrote:

Just throw an MWOAuthException( 'mwoauth-invalid-consumer-key' ) and let the catch in
SpecialMWOAuth::execute() handle it.

Thanks, fixed.

Filed T147537 about the generic issue.

In T147414#2694886, @Tgr wrote:

I took a slightly different approach (and then got distracted waiting for vagrant to update and did not upload it):

0001-SECURITY-Fix-bugs-with-handling-of-missing-request-p.patch6 KBDownload

Looks good to me.

With T147537 resolved, this can probably be made public, and the patch above merged in Gerrit? (It looks like the original issue here is no longer a problem, but the patch has some nice code quality improvements.)

Change 315346 had a related patch set uploaded (by Gergő Tisza):
Fix bugs with handling of missing request parameters

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

Change 315346 merged by jenkins-bot:
Fix bugs with handling of missing request parameters

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