Page MenuHomePhorge

Fix PHP 8.1 "urlencode(null)" exception blocking account registration redirect for custom OAuth provider
ClosedPublic

Authored by aklapper on Aug 5 2023, 20:28.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jul 20, 14:34
Unknown Object (File)
Thu, Jul 18, 21:41
Unknown Object (File)
Thu, Jul 18, 12:28
Unknown Object (File)
Thu, Jul 18, 11:30
Unknown Object (File)
Thu, Jul 18, 10:02
Unknown Object (File)
Thu, Jul 18, 07:01
Unknown Object (File)
Thu, Jul 18, 06:22
Unknown Object (File)
Thu, Jul 18, 05:46

Details

Summary

It seems that a tokenSecret is not always passed at this stage, and that PHP's urlencode() does not accept passing a null string since PHP 8.1 (I could not find any upstream note about this but bug reports across the web seem to confirm this).

Thus do not try to urlencode($this->tokenSecret) if it is null.

EXCEPTION: (RuntimeException) urlencode(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(), ava(), phorge(), wmf-ext-misc()
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
  #1 <#2> urlencode(NULL) called at [<arcanist>/src/future/oauth/PhutilOAuth1Future.php:232]

Closes T15589

Test Plan

Redirect now works as expected: The URL redirect to allow access on
http://mediawiki.localhost/index.php?title=Special%3AOAuth%2Fauthorize&oauth_token=1234567890abcdef1234567890abcdef&oauth_consumer_key=1234567890abcdef1234567890abcdef works as expected, instead of showing a raw error page about urlencode() not accepting passing null. (After allowing authorization there are more issues in Phorge code but they are out of scope for this Arcanist patch.)

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

aklapper requested review of this revision.Aug 5 2023, 20:28
Matthew requested changes to this revision.Aug 7 2023, 03:12

Please fix unit errors

This revision now requires changes to proceed.Aug 7 2023, 03:12

Please fix unit errors

I've looked at this for five minutes and unfortunately I'm afraid that I do not know.

src/future/oauth/PhutilOAuth1Future.php
232–236

Thanks. Does this work as well? (Also reported below to make it more visible)

$key = urlencode($consumer_secret);
if ($this->tokenSecret !== null) {
  $key .= '&' . urlencode($this->tokenSecret);
}
src/future/oauth/PhutilOAuth1Future.php
232–236

No, this does not work as well. In that case I get an Expected 'oauth_callback_confirmed' to be 'true'! error. But your format is more readable, so I should adjust my proposal.

aklapper marked an inline comment as done and an inline comment as not done.Aug 20 2023, 11:22

Thaaanks

This seems the exact backward-compatible thing to me

lgtm

Please fix unit errors

Yeah! it seems OK now

Would anyone be willing to give this another review? TIA

This looks like a reasonable chance to me.

Please fix unit errors

I think they are fixed now

As I just ran into this again spinning up another machine and had to look this up again, I'd really appreciate if a Blessed Committer could give an "Accepted" state. Thanks in advance!

This already has been accepted. I think @Matthew 's old request for changes is what is keeping this from being landable right now.

This revision is now accepted and ready to land.Nov 30 2023, 07:39