Details

Reviewers

valerio.bozzolan
speck

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15589: PHP 8.1 "urlencode(null)" exception blocks account registration redirect for custom OAuth provider

Commits

rARCe46025f7a914: Fix PHP 8.1 "urlencode(null)" exception blocking account registration redirect…

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

As an admin, set up custom "MediaWiki" OAuth provider from from https://gitlab.wikimedia.org/-/ide/project/repos/phabricator/extensions/edit/wmf/stable/-/src/oauth/
As an admin, apply D25373
As a user, go to /auth/login/mediawiki:whatever/
Select login button

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 created this revision.Aug 5 2023, 20:28

Owners added a reviewer: O1: Blessed Committers.Aug 5 2023, 20:28

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptAug 5 2023, 20:28

aklapper requested review of this revision.Aug 5 2023, 20:28

Harbormaster completed remote builds in B728: Diff 1206.Aug 5 2023, 20:28

aklapper mentioned this in T15590: PHP 8.1 "strlen(null)" exceptions block account registration with custom OAuth provider after redirect.Aug 5 2023, 20:41

Please fix unit errors

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

In D25374#10923, @Matthew wrote:

Please fix unit errors

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

arc lint
arc unit

Harbormaster completed remote builds in B754: Diff 1255.Aug 15 2023, 09:13

valerio.bozzolan added inline comments.Aug 16 2023, 02:49

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

valerio.bozzolan mentioned this in Wikimania Hackathon 2023 Singapore.Aug 19 2023, 04:47

aklapper added inline comments.Aug 20 2023, 11:21

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

Make code more readable

Harbormaster completed remote builds in B794: Diff 1317.Aug 20 2023, 11:24

Ran arc unit first

Harbormaster completed remote builds in B795: Diff 1318.Aug 20 2023, 11:25

Thaaanks

This seems the exact backward-compatible thing to me

lgtm

fix lint

Harbormaster completed remote builds in B807: Diff 1331.Aug 24 2023, 18:25

In D25374#10923, @Matthew wrote:

Please fix unit errors

Yeah! it seems OK now

valerio.bozzolan mentioned this in Wikimania Hackathon 2023 Singapore.Aug 28 2023, 16:39

Would anyone be willing to give this another review? TIA

This looks like a reasonable chance to me.

In D25374#10923, @Matthew wrote:

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.

pppery unsubscribed.Nov 30 2023, 05:21

avivey removed a reviewer: Matthew.Nov 30 2023, 07:39

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

Closed by commit rARCe46025f7a914: Fix PHP 8.1 "urlencode(null)" exception blocking account registration redirect…. · Explain WhyDec 5 2023, 03:28

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rARCe46025f7a914: Fix PHP 8.1 "urlencode(null)" exception blocking account registration redirect….

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1563

src/future/oauth/PhutilOAuth1Future.php

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1563

src/future/oauth/PhutilOAuth1Future.php

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

Revision Contents
Changeset List